Merge remote-tracking branch 'origin/openspec/add-vale-rule-engine-5-integration' into openspec/agent-command-and-vale-authoring

Author: thecodedrift

.agents/skills/iterate-pr/SKILL.md

@@ -246,11 +246,16 @@ exactly once, at the END of the work — so a PR that still carries an in-progre
 change directory will fail this check. Archiving on an intermediate PR is wrong:
 it would remove the change docs before the implementation PRs above it merge.
 
-The workflow's trigger is `pull_request.branches: [main]`, but do NOT read that
-as "it only runs on PRs whose base is `main`." Under GitHub's stacked-PR support
-a stacked PR targets `main` eventually and the filter matches that eventual
-target, so this workflow runs on mid-stack PRs as well. Expect to see the check
-on every PR in a stack and decide from stack position, not from the `on:` block.
+This workflow carries **no `branches:` filter** — that is why it runs on every
+PR in a stack, and it is the reliable way to get that behavior. Expect to see
+the check on every PR in a stack and decide from stack position, not from the
+`on:` block.
+
+Do not generalize from workflows that DO filter on `branches: [main]`. GitHub
+sometimes resolves a stacked PR's eventual target and matches on that, so such a
+workflow may appear on mid-stack PRs — but it stops without warning (see
+"two other failures that are structural" below). A filter-less trigger is the
+only dependable way to run everywhere.
 
 The archive job is also skipped while a PR is a **draft**. A spec-only proposal
 is its own tip until its implementation is stacked on top, so the gate would
@@ -300,13 +305,21 @@ Mid-stack PRs bypass the check on their base ref. If you see one failing it,
 look at that guard rather than reaching for the label, which would wrongly
 record the change as shipping no release note.
 
-**Do not read `on: pull_request: branches: [main]` as "this only runs on the
-bottom PR."** Under GitHub's stacked-PR support a stacked PR targets `main`
-eventually, and the filter matches that eventual target — so these workflows run
-on mid-stack PRs too. A workflow that must act only on the PR merging to `main`
-has to establish that from the base ref or its stack position. When judging
-whether a check "should even be running here," check the stack rather than the
-`on:` block.
+**`on: pull_request: branches: [main]` tells you nothing dependable about where
+a workflow runs.** GitHub sometimes resolves a stacked PR's eventual target and
+matches on that, so such a workflow may run on mid-stack PRs — and may also
+silently stop. Measured on the #71→#106 stack: every PR up to #102 got a
+`Validate` run, #103 and #106 got none, across 16 `pull_request` events that
+filter-less workflows handled fine. #103 reached "ready for review" as a
+~93-file change never linted, typechecked, or tested in CI.
+
+So: a workflow that must run **everywhere** carries no `branches:` filter (and
+names `ready_for_review` in `types:`, which is not in the default set). A
+workflow that must act only on the PR merging to `main` establishes that from
+the base ref or its stack position, inside the job. Either way, when judging
+whether a check "should even be running here," check the stack — and if a check
+you expected is simply **absent**, suspect the filter before assuming the PR is
+fine. An absent check reads like a passing one.
 
 ### Never leave a PR on red
 

.github/workflows/ci.yml

@@ -3,8 +3,24 @@ name: CI
 on:
   push:
     branches: [main]
+  # No `branches:` filter, deliberately. Lint, typecheck, and tests have no
+  # interest in where a PR eventually merges, and filtering on `main` silently
+  # skipped this workflow on stacked PRs.
+  #
+  # The filter matches the PR's base ref, but GitHub also resolves a stacked
+  # PR's *eventual* target and matches on that — so `branches: [main]` did run
+  # on PRs based on another branch, until it stopped. Measured on the
+  # #71→#93→#94→#95→#100→#102→#103→#106 stack: every PR up to #102 got a
+  # `Validate` run, while #103 and #106 got none, across 16 `pull_request`
+  # events that other workflows handled fine. A filter that works for six PRs
+  # and quietly fails on the seventh is worse than one that never worked,
+  # because nobody re-checks it.
+  #
+  # `ready_for_review` is NOT in the default set (opened/synchronize/reopened)
+  # and must be named: without it a draft marked ready gets no fresh run until
+  # something happens to push again, which is exactly the state #103 sat in.
   pull_request:
-    branches: [main]
+    types: [opened, synchronize, reopened, ready_for_review]
 
 permissions:
   contents: read

.github/workflows/require-changeset.yml

@@ -54,7 +54,15 @@ jobs:
           # `Require a changeset` check run (e.g. run 30786954198, event
           # `pull_request`, head `openspec/partition-engine-2-dispatch`).
           #
-          # So `branches:` no longer scopes a workflow to the bottom of a stack.
+          # That resolution is undocumented and it also stops without warning:
+          # on the #71→#106 stack this workflow ran up to #102 and then produced
+          # no check run at all on #103 or #106. So `branches:` is unreliable in
+          # BOTH directions — it neither scopes a workflow to the bottom of a
+          # stack nor guarantees it reaches every PR. This guard stays because
+          # the first failure mode is the dangerous one here; a workflow that
+          # must reach every PR should instead carry no `branches:` filter, as
+          # `ci.yml` and `pr-check-openspec.yml` do.
+          #
           # Any job whose correctness depends on "is this the PR that merges to
           # main" has to establish that itself, as this one does.
           if [ "$BASE_REF" != "main" ]; then

CLAUDE.md

@@ -110,9 +110,16 @@ Note how this interacts with the archive gate (see the OpenSpec archive check be
 - **The changeset belongs on the bottom PR**, the one that targets `main`. That is the only place it can live: a changeset added on the tip is invisible to the bottom PR's diff, so the check would fail on the PR that actually merges.
 - **Mid-stack PRs bypass the check**, because the base branch is not `main`. They inherit the base's changeset rather than adding one, so there is nothing for the check to find. **Do not label them `skip-changeset`** — the label records a deliberate "this change ships no release note," which is false here, and the bypass already handles it.
 
-The bypass is an in-step check on the base ref, and it is load-bearing. **`branches: [main]` no longer means "only the PR whose base is `main`."** Under GitHub's stacked-PR support, a PR in a stack is understood to target `main` eventually, so the filter matches on that eventual target and the workflow runs on mid-stack PRs as well — observed here on #73, #80, and #81, all with `openspec/partition-engine-*` bases.
+The bypass is an in-step check on the base ref, and it is load-bearing. **`branches: [main]` does not reliably mean either "only the PR whose base is `main`" or "every PR in the stack."** The filter matches the PR's base ref, but GitHub also resolves a stacked PR's _eventual_ target and sometimes matches on that instead, so the workflow runs on mid-stack PRs — observed on #73, #80, and #81, all with `openspec/partition-engine-*` bases.
 
-The general rule that follows: **any workflow whose correctness depends on "is this the PR that merges to `main`" must determine that itself** — from the base ref, or by resolving stack position — and cannot lean on the `on:` filter to scope it. If you see a mid-stack PR failing this check, look at that guard rather than reaching for the label.
+**Do not depend on that resolution. It is undocumented and it stops without warning.** On the #71→#93→#94→#95→#100→#102→#103→#106 stack, every PR up to #102 got a `Validate` run and **#103 and #106 got none** — across 16 `pull_request` events that filter-less workflows handled fine. #103 was a ~93-file change that reached "ready for review" having never been linted, typechecked, or tested in CI. Depth correlates (#102 is six hops from `main`, #103 seven) but nothing confirms a cap, and it was not a date cutoff: #102 kept getting runs after #103 had already stopped. A filter that works for six PRs and quietly fails on the seventh is worse than one that never worked, because nobody re-checks it.
+
+Two rules follow, and they pull in opposite directions:
+
+- **A workflow that must run everywhere carries no `branches:` filter at all.** Lint, typecheck, and tests have no interest in where a PR eventually merges. `ci.yml` dropped its filter for exactly this reason; `pr-check-openspec.yml` and `stack-breadcrumb.yml` never had one, which is why they kept running on #103. If you add such a workflow, also name `ready_for_review` in `types:` — it is not in the default set (`opened`/`synchronize`/`reopened`), so without it a draft marked ready gets no fresh run until someone happens to push again.
+- **A workflow whose correctness depends on "is this the PR that merges to `main`" must determine that itself** — from the base ref, or by resolving stack position — and cannot lean on the `on:` filter to scope it. If you see a mid-stack PR failing the changeset check, look at that guard rather than reaching for the label.
+
+The shared point: the `on:` filter is not a reliable answer to "where does this PR land." Let the workflow run, and decide inside it.
 
 Put the changeset at the base and every branch above inherits it, since a child contains its ancestors' commits.
 

openspec/changes/archive/2026-08-11-add-vale-rule-engine/specs/cli-check/spec.md

@@ -13,5 +13,3 @@
 
 - **WHEN** the `vale` binary is unavailable but `.taskless/sg/` has rules
 - **THEN** the CLI reports the Vale engine as unavailable and still returns ast-grep results
-
-## MODIFIED Requirements

packages/cli/src/commands/check.ts

@@ -282,7 +282,10 @@ export const checkCommand = defineCommand({
     const telemetry = await getTelemetry(cwd);
 
     // Warnings/notices are advisory human output; suppress them under --json so
-    // the machine output stays the { success, results, skipped? } shape.
+    // the machine output stays the
+    // { success, results, skipped?, failures?, notices? } shape. Engine
+    // failures and notices are carried in that envelope instead, since a
+    // machine consumer cannot read stderr prose.
     const warn = (message: string) => {
       if (!args.json) console.error(message);
     };
@@ -422,6 +425,12 @@ export const checkCommand = defineCommand({
             success: exitCode === 0,
             results,
             ...(plan.skipped.length > 0 ? { skipped: plan.skipped } : {}),
+            ...(dispatched.failures.length > 0
+              ? { failures: dispatched.failures }
+              : {}),
+            ...(dispatched.notices.length > 0
+              ? { notices: dispatched.notices }
+              : {}),
           });
           console.log(JSON.stringify(output));
         } else {

packages/cli/src/rules/dispatch.ts

@@ -3,14 +3,12 @@ import { join } from "node:path";
 
 import type { CheckResult } from "../types/check";
 import { dedupeFindings, ENGINE_LAYOUTS, type EngineName } from "./engines";
+import { isMissingDirectory } from "./errno";
 import { executeRuntimeRules } from "./runtime/harness";
 import type { RuntimeRule } from "./runtime/discover";
 import { runAstGrepScan } from "./scan";
 import { runVale } from "./vale/run";
 
-/** Errno values that mean "the directory is not there", and nothing worse. */
-const ABSENT_DIRECTORY_CODES = new Set(["ENOENT", "ENOTDIR"]);
-
 /**
  * Whether `.taskless/vale/rules/` holds anything to run.
  *
@@ -34,8 +32,7 @@ export async function hasValeRules(cwd: string): Promise<boolean> {
     );
     return entries.some((entry) => entry.endsWith(".yml"));
   } catch (error) {
-    const code = (error as NodeJS.ErrnoException).code;
-    if (code !== undefined && ABSENT_DIRECTORY_CODES.has(code)) return false;
+    if (isMissingDirectory(error)) return false;
     throw error;
   }
 }
@@ -115,17 +112,22 @@ export interface DispatchResult {
  * `sg/rules/` and the legacy `.taskless/rules/` are scanned separately, so a
  * rule present in both reports twice; the finding is its own identity, so
  * identical matches collapse.
+ *
+ * The configs are scanned concurrently, for the same reason the engines are:
+ * each is an independent subprocess over the same paths, and a project holding
+ * both `sg/rules/` and the legacy `rules/` should not pay their latencies in
+ * series. `Promise.all` preserves input order in its output, so the flattened
+ * results are ordered by config exactly as the sequential loop left them.
  */
 async function runAstGrepEngine(
   options: DispatchOptions
 ): Promise<EngineOutcome> {
-  const results: CheckResult[] = [];
-  for (const configPath of options.astGrepConfigPaths) {
-    const scan = await runAstGrepScan(options.cwd, options.paths, {
-      configPath,
-    });
-    results.push(...scan.results);
-  }
+  const scans = await Promise.all(
+    options.astGrepConfigPaths.map((configPath) =>
+      runAstGrepScan(options.cwd, options.paths, { configPath })
+    )
+  );
+  const results: CheckResult[] = scans.flatMap((scan) => scan.results);
   return { engine: "sg", results: dedupeFindings(results) };
 }
 

packages/cli/src/rules/errno.ts

@@ -0,0 +1,18 @@
+/**
+ * Whether an `fs` failure genuinely means "that directory is not there".
+ *
+ * `ENOENT` is the path not existing; `ENOTDIR` is a path that exists but is a
+ * file, or that has a file for an ancestor. Every other code — `EACCES` above
+ * all — is a real IO problem, and reading it as "nothing here" is what makes an
+ * unreadable bucket indistinguishable from an unwritten one.
+ *
+ * One definition, so no caller can accidentally answer that question
+ * differently: two spellings of "absent" drift, and the drift is silent.
+ */
+export function isMissingDirectory(error: unknown): boolean {
+  if (error === null || typeof error !== "object" || !("code" in error)) {
+    return false;
+  }
+  const { code } = error as NodeJS.ErrnoException;
+  return code === "ENOENT" || code === "ENOTDIR";
+}

packages/cli/src/rules/vale/binary.ts

@@ -1,4 +1,5 @@
 import {
+  pathCommandName,
   resolvePlatformBinary,
   type PlatformBinaryResolution,
   type PlatformBinarySpec,
@@ -54,11 +55,17 @@ export function resetValeBinaryCache(): void {
   cached = undefined;
 }
 
-/** An actionable message naming where we looked. */
+/**
+ * An actionable message naming where we looked.
+ *
+ * The PATH advice is spelled for the platform — `vale.exe` on Windows — rather
+ * than hardcoded, so a Windows user is not told to install a name that the
+ * resolver would not find there.
+ */
 export function valeUnavailableMessage(tried: string[]): string {
   return (
     `Vale binary not found. Looked in: ${tried.join(", ")}. Install a ` +
-    `supported platform build, or put \`vale\` on your PATH. Other engines ` +
-    `still ran.`
+    `supported platform build, or put \`${pathCommandName(VALE_BINARY)}\` on ` +
+    `your PATH. Other engines still ran.`
   );
 }

packages/cli/src/rules/vale/verify.ts

@@ -4,6 +4,7 @@ import { tmpdir } from "node:os";
 import { join, posix, relative, resolve, sep } from "node:path";
 
 import { ENGINE_LAYOUTS } from "../engines";
+import { isMissingDirectory } from "../errno";
 import { runVale, type ValeRunOutcome } from "./run";
 
 /** Where a rule's fixtures live, relative to the project root. */
@@ -49,22 +50,6 @@ export function buildIsolatingConfig(cwd: string, ruleId: string): string {
   ].join("\n");
 }
 
-/**
- * Whether a `readdir` failure genuinely means "that directory is not there".
- *
- * `ENOENT` is the path not existing; `ENOTDIR` is a path that exists but is a
- * file, or that has a file for an ancestor. Every other code — `EACCES` above
- * all — is a real IO problem, and reading it as "nothing here" is what makes an
- * unreadable bucket indistinguishable from an unwritten one.
- */
-function isMissingDirectory(error: unknown): boolean {
-  if (error === null || typeof error !== "object" || !("code" in error)) {
-    return false;
-  }
-  const { code } = error as NodeJS.ErrnoException;
-  return code === "ENOENT" || code === "ENOTDIR";
-}
-
 /**
  * Directory entries, with a directory that is not there reading as an empty one.
  *
@@ -254,9 +239,10 @@ export async function verifyValeRule(
 
   const configDirectory = mkdtempSync(join(tmpdir(), `vale-verify-${ruleId}-`));
   const configPath = join(configDirectory, ".vale.ini");
-  writeFileSync(configPath, buildIsolatingConfig(cwd, ruleId));
 
   try {
+    writeFileSync(configPath, buildIsolatingConfig(cwd, ruleId));
+
     const outcome = await runVale({
       cwd,
       configPath,