diff --git a/src/test-timeout-policy.test.ts b/src/test-timeout-policy.test.ts index 1378507..e84394b 100644 --- a/src/test-timeout-policy.test.ts +++ b/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 diff --git a/src/test-timeout-policy.ts b/src/test-timeout-policy.ts index 285eb5a..a1e8c2d 100644 --- a/src/test-timeout-policy.ts +++ b/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)) { + 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; +}