fix(test-policy): the workflow scan must read code, not comments

src/test-timeout-policy.test.ts

@@ -22,6 +22,8 @@ import {
   BUN_DEFAULT_TIMEOUT_MS,
   CLI_SPAWN_TIMEOUT_MS,
   SUITE_TIMEOUT_MS,
+  findBunTestInvocations,
+  stripShellComments,
 } from "./test-timeout-policy.js";
 
 const repoRoot = join(dirname(fileURLToPath(import.meta.url)), "..");
@@ -135,30 +137,166 @@ describe("test timeout policy", () => {
     const files = readdirSync(workflowDir).filter((name) => /\.ya?ml$/.test(name));
     expect(files.length).toBeGreaterThan(0);
 
-    const invocations: string[] = [];
-    for (const name of files) {
-      for (const line of readFileSync(join(workflowDir, name), "utf8").split("\n")) {
-        if (!/\bbun\s+test\b/.test(line)) continue;
-        invocations.push(`${name}: ${line.trim()}`);
-
-        // `bun run test` resolves to the `test` script, which the assertion
-        // above already proves carries the budget.
-        if (/\bbun\s+run\s+test\b/.test(line)) continue;
-
-        const match = /\bbun\s+test\b[^&|]*?--timeout\s+(\d+)/.exec(line);
-        expect(
-          match,
-          `${name} must use \`bun run test\` or pass --timeout explicitly — got: ${line.trim()}`,
-        ).not.toBeNull();
-        expect(Number(match![1])).toBeGreaterThan(BUN_DEFAULT_TIMEOUT_MS);
-      }
+    const invocations = files.flatMap((name) =>
+      findBunTestInvocations(name, readFileSync(join(workflowDir, name), "utf8")),
+    );
+
+    for (const invocation of invocations) {
+      // `bun run test` resolves to the `test` script, which the assertion
+      // above already proves carries the budget.
+      if (invocation.viaTestScript) continue;
+
+      expect(
+        invocation.timeoutMs,
+        `${invocation.workflow} must use \`bun run test\` or pass --timeout explicitly — got: ${invocation.command}`,
+      ).not.toBeNull();
+      expect(invocation.timeoutMs!).toBeGreaterThan(BUN_DEFAULT_TIMEOUT_MS);
     }
 
     // Positive control: if this ever reads zero, the scan found nothing and
-    // the assertions above were vacuous.
+    // the assertions above were vacuous. Counted over executable content only,
+    // so a comment mentioning `bun test` can no longer pad it.
     expect(invocations.length).toBeGreaterThan(0);
   });
 
+  /**
+   * The scan above is only worth anything if it reads code and not prose, and
+   * the parser it replaced failed that in both directions at once — see
+   * test-timeout-policy.ts for the two live instances. Each case below states
+   * the passing state and the failing state, so none of them can quietly
+   * become an assertion that always holds.
+   */
+  describe("the workflow scan reads executable content, not prose", () => {
+    const workflow = (steps: string) =>
+      ["name: probe", "on: push", "jobs:", "  probe:", "    steps:", steps, ""].join("\n");
+
+    // THE control that matters: the scan must still catch the real violation.
+    test("a genuine bare `bun test` in a run step is reported with no budget", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(["      - name: Test", "        run: bun test"].join("\n")),
+      );
+
+      expect(found).toHaveLength(1);
+      expect(found[0]!.command).toBe("bun test");
+      expect(found[0]!.viaTestScript).toBe(false);
+      expect(found[0]!.timeoutMs).toBeNull();
+    });
+
+    test("a compliant invocation reports its budget", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(["      - name: Test", "        run: bun test --timeout 120000"].join("\n")),
+      );
+
+      expect(found).toHaveLength(1);
+      expect(found[0]!.timeoutMs).toBe(SUITE_TIMEOUT_MS);
+    });
+
+    // The false positive that failed CI on this PR: prose above a real command.
+    test("a YAML comment naming bare `bun test` is not an invocation", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(
+          [
+            "      # `bun test` does not invoke tsc in this repository, so typecheck",
+            "      # is its own step rather than something the suite implies.",
+            "      - name: Test",
+            "        run: bun test --timeout 120000",
+          ].join("\n"),
+        ),
+      );
+
+      expect(found).toHaveLength(1);
+      expect(found[0]!.command).toBe("bun test --timeout 120000");
+    });
+
+    // The false negative: prose that named both forms took the `bun run test`
+    // early-exit and was counted as a compliant invocation.
+    test("a comment naming both forms is not counted as a compliant invocation", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(
+          [
+            "      - name: Test",
+            "        # `bun run test`, not bare `bun test`: the budget lives on the script.",
+            "        run: bun run test",
+          ].join("\n"),
+        ),
+      );
+
+      expect(found).toHaveLength(1);
+      expect(found[0]!.command).toBe("bun run test");
+      expect(found[0]!.viaTestScript).toBe(true);
+    });
+
+    test("a shell comment inside a `run:` block is not an invocation", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(
+          [
+            "      - name: Test",
+            "        run: |",
+            "          # bun test is deliberately not run here",
+            "          bun run build",
+          ].join("\n"),
+        ),
+      );
+
+      expect(found).toEqual([]);
+    });
+
+    // The under-blocking direction, and the reason comment stripping is
+    // quote-aware: cutting at the first `#` would discard the real invocation
+    // along with the quoted one and report this workflow clean.
+    //
+    // A BLOCK scalar is the shape that can express this, and measuring that
+    // was worth more than assuming it. In a PLAIN scalar YAML itself ends the
+    // value at ` #`, so `run: echo "a # b" && bun test` carries no invocation
+    // for anything to hide — GitHub Actions would run `echo "a` and nothing
+    // else. Inside `run: |` the `#` is literal and shell rules take over,
+    // which is where a naive cut would lose the command that follows.
+    test("a `#` inside a quoted string cannot hide a real invocation", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(
+          [
+            "      - name: Test",
+            "        run: |",
+            '          echo "a # b" && bun test',
+          ].join("\n"),
+        ),
+      );
+
+      expect(found).toHaveLength(1);
+      expect(found[0]!.timeoutMs).toBeNull();
+      expect(found[0]!.viaTestScript).toBe(false);
+    });
+
+    test("a later command's --timeout does not excuse an earlier bare invocation", () => {
+      const found = findBunTestInvocations(
+        "probe.yml",
+        workflow(
+          [
+            "      - name: Test",
+            "        run: bun test && bun test --timeout 120000",
+          ].join("\n"),
+        ),
+      );
+
+      expect(found).toHaveLength(2);
+      expect(found[0]!.timeoutMs).toBeNull();
+      expect(found[1]!.timeoutMs).toBe(SUITE_TIMEOUT_MS);
+    });
+
+    test("stripShellComments keeps quoted `#` and drops unquoted ones", () => {
+      expect(stripShellComments('echo "a # b" # trailing').trim()).toBe('echo "a # b"');
+      expect(stripShellComments("echo 'a # b'").trim()).toBe("echo 'a # b'");
+      // A `#` that does not start a word is part of the word, not a comment.
+      expect(stripShellComments("echo abc#def").trim()).toBe("echo abc#def");
+    });
+  });
+
   /**
    * The budget above is only safe to raise because hang detection moved to the
    * spawn boundary. If that guard is finite and enforced, a raised per-test

src/test-timeout-policy.ts

@@ -87,3 +87,149 @@ export const SUITE_TIMEOUT_MS = 120_000;
  * it without re-registering that file's test cases.
  */
 export const CLI_SPAWN_TIMEOUT_MS = 60_000;
+
+/**
+ * WHY THE WORKFLOW SCAN BELOW PARSES INSTEAD OF GREPPING
+ *
+ * The policy test that guards `.github/workflows` used to read each file as
+ * raw lines and treat any line matching `bun test` as an invocation. A comment
+ * is not an invocation, and that parser was measurably wrong in BOTH
+ * directions in this repository at the same time:
+ *
+ *   - FALSE POSITIVE. release.yml carries the comment "`bun test` does not
+ *     invoke tsc in this repository", which has no `--timeout` because it is
+ *     prose. The scan reported it as a bare invocation and failed CI on both
+ *     runners, while the workflow's real command two lines below was the
+ *     compliant `bun test --timeout 120000`.
+ *   - FALSE NEGATIVE. ci.yml carries the comment "`bun run test`, not bare
+ *     `bun test`". The scan matched it as an invocation, then took the
+ *     `bun run test` early-exit and waved it through — so a comment was
+ *     silently counted as a compliant invocation, and it padded the very
+ *     positive control that is supposed to prove the scan found real commands.
+ *
+ * Both faults are the same root: prose was being read as code. So the scan
+ * parses the workflow and inspects only the executable content of `run:`
+ * steps, and strips shell comments inside those steps as well, because a `#`
+ * line inside a `run: |` block is prose too.
+ *
+ * Comment stripping is quote-aware rather than a cut at the first `#`. That
+ * matters in the under-blocking direction, which is the dangerous one: a naive
+ * cut on `echo "a # b" && bun test` would discard the real bare invocation
+ * along with the quoted `#` and report the workflow clean.
+ */
+
+/** One `bun test` invocation found in a workflow's executable content. */
+export interface WorkflowBunTestInvocation {
+  /** The workflow file it was found in, e.g. `release.yml`. */
+  workflow: string;
+  /** The single command it appeared in, with comments already removed. */
+  command: string;
+  /** The explicit `--timeout` budget in ms, or null when none is present. */
+  timeoutMs: number | null;
+  /**
+   * True only for `bun run test`, which resolves to the `test` script and so
+   * inherits the budget asserted separately against package.json. Set false
+   * whenever a bare `bun test` is also present in the same command, so a
+   * command carrying both is judged on the bare one rather than excused by the
+   * other — that excuse is exactly the false negative described above.
+   */
+  viaTestScript: boolean;
+}
+
+/**
+ * Remove shell comments from a script, honouring single and double quotes so a
+ * `#` inside a string cannot truncate the command that follows it.
+ *
+ * Quote state is tracked across the whole script rather than per line, because
+ * a shell string may legitimately span lines. Newlines are preserved so that
+ * command boundaries survive the strip.
+ */
+export function stripShellComments(script: string): string {
+  let out = "";
+  let inSingle = false;
+  let inDouble = false;
+
+  for (let i = 0; i < script.length; i += 1) {
+    const ch = script[i]!;
+
+    // A backslash escapes the next character everywhere except inside single
+    // quotes, where it is literal.
+    if (ch === "\\" && !inSingle && i + 1 < script.length) {
+      out += ch + script[i + 1];
+      i += 1;
+      continue;
+    }
+    if (ch === "'" && !inDouble) {
+      inSingle = !inSingle;
+      out += ch;
+      continue;
+    }
+    if (ch === '"' && !inSingle) {
+      inDouble = !inDouble;
+      out += ch;
+      continue;
+    }
+
+    // `#` opens a comment only when it starts a word and is unquoted.
+    if (ch === "#" && !inSingle && !inDouble && /\s/.test(i === 0 ? "\n" : script[i - 1]!)) {
+      while (i < script.length && script[i] !== "\n") i += 1;
+      if (i < script.length) out += "\n";
+      continue;
+    }
+
+    out += ch;
+  }
+
+  return out;
+}
+
+/** Every `run:` script in a parsed workflow, in document order. */
+function runScripts(workflowSource: string): string[] {
+  const scripts: string[] = [];
+
+  const walk = (node: unknown): void => {
+    if (Array.isArray(node)) {
+      for (const entry of node) walk(entry);
+      return;
+    }
+    if (node === null || typeof node !== "object") return;
+    for (const [key, value] of Object.entries(node as Record<string, unknown>)) {
+      if (key === "run" && typeof value === "string") scripts.push(value);
+      walk(value);
+    }
+  };
+
+  walk(Bun.YAML.parse(workflowSource));
+  return scripts;
+}
+
+/**
+ * Every `bun test` invocation in a workflow's executable content.
+ *
+ * Commands are separated on `&&`, `||`, `;`, `|` and newlines so that a
+ * `--timeout` belonging to a later command cannot be read as covering an
+ * earlier bare one.
+ */
+export function findBunTestInvocations(
+  workflow: string,
+  workflowSource: string,
+): WorkflowBunTestInvocation[] {
+  const invocations: WorkflowBunTestInvocation[] = [];
+
+  for (const script of runScripts(workflowSource)) {
+    for (const command of stripShellComments(script).split(/&&|\|\||;|\||\n/)) {
+      if (!/\bbun\s+(?:run\s+)?test\b/.test(command)) continue;
+
+      const bare = /\bbun\s+test\b/.test(command);
+      const match = /\bbun\s+test\b.*?--timeout\s+(\d+)/.exec(command);
+      invocations.push({
+        workflow,
+        command: command.trim(),
+        timeoutMs: match ? Number(match[1]) : null,
+        viaTestScript: !bare && /\bbun\s+run\s+test\b/.test(command),
+      });
+    }
+  }
+
+  return invocations;
+}