diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index f65e0ae25db..f8150cc0ef0 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -48,6 +48,7 @@ describe('reviewCommand', () => { 'load-rules', 'agent-prompt', 'build-test', + 'script-lint', 'resolve-anchors', 'check-coverage', 'presubmit', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index a42257b772f..03f8091a9c7 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -22,6 +22,7 @@ import { resolveAnchorsCommand } from './review/resolve-anchors.js'; import { checkCoverageCommand } from './review/check-coverage.js'; import { agentPromptCommand } from './review/agent-prompt.js'; import { buildTestCommand } from './review/build-test.js'; +import { scriptLintCommand } from './review/script-lint.js'; import { submitCommand } from './review/submit.js'; import { testEfficacyCommand } from './review/test-efficacy.js'; import { cleanupCommand } from './review/cleanup.js'; @@ -41,6 +42,7 @@ export const reviewCommand: CommandModule = { .command(loadRulesCommand) .command(agentPromptCommand) .command(buildTestCommand) + .command(scriptLintCommand) .command(resolveAnchorsCommand) .command(checkCoverageCommand) .command(presubmitCommand) @@ -50,7 +52,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', + 'Specify a subcommand: parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 35d79c4f151..ce5df125d0a 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1887,6 +1887,48 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { expect(p).toContain('timeout: 600000'); }); + it('gives Script Lint no diff — its evidence is what the linters say', () => { + const p = buildRoleBrief(PR_PLAN, 'script-lint'); + expect(p).not.toContain(PLAN.diffPathAbsolute); + expect(p).toContain('Source: [build]'); + }); + + it('hands Script Lint the script-lint command with absolute --plan/--worktree/--out', () => { + const p = buildRoleBrief(PR_PLAN, 'script-lint', { + planPath: '/abs/tmp/plan.json', + }); + expect(p).toContain('"${QWEN_CODE_CLI:-qwen}" review script-lint'); + // Paths are shell-quoted so a worktree with a space cannot word-split. + expect(p).toContain("--plan '/abs/tmp/plan.json'"); + expect(p).toMatch(/--worktree '\/[^\s']*review-pr-6766'/); + expect(p).not.toMatch(/--plan '?\.qwen/); + expect(p).toContain( + "--out '/abs/tmp/qwen-review-pr-6766-script-lint.json'", + ); + // Same PATH-skew guard as build-test: no bare executable `qwen`. + expect(p).not.toMatch(/^qwen review /m); + }); + + it('never emits a literal "undefined" in the script-lint --out filename', () => { + const noPr = { ...PR_PLAN }; + delete (noPr as { prNumber?: unknown }).prNumber; + const p = buildRoleBrief(noPr, 'script-lint', { + planPath: '/abs/tmp/plan.json', + }); + expect(p).not.toContain('undefined'); + expect(p).toContain("--out '/abs/tmp/qwen-review-script-lint.json'"); + }); + + it('emits NO script-lint block in PR mode when the worktree is missing', () => { + // Same tree-safety rule as build-test: a PR-mode report with no worktree must + // not fall back to the user's own checkout. + const prNoWt = { ...PLAN, prNumber: '42', ownerRepo: 'o/r' }; + const p = buildRoleBrief(prNoWt, 'script-lint', { + planPath: '/abs/tmp/plan.json', + }); + expect(p).not.toMatch(/review script-lint \\/); + }); + it('welds the PR into Agent 0 — a bare `gh pr view` judges the wrong issue', () => { const p = buildRoleBrief(PR_PLAN, '0', { planPath: '/x/qwen-review-pr-6766-fetch.json', diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 768ae8ab79b..20b96efb99d 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -50,6 +50,7 @@ import { import { recordPrompt, writeBrief } from './lib/prompt-record.js'; import { BRIEFS, type RoleId } from './lib/agent-briefs.js'; import { pathRulesFor } from './lib/path-rules.js'; +import { shellQuotePath } from './lib/shell-quote.js'; import { requiredAgents, reviewMode, @@ -881,6 +882,52 @@ export function buildRoleBrief( } } + // Script Lint runs a command over the changed executable files, and — like + // Agent 7 — needs a tree to read them from and the plan to know which they are. + // Same tree/fallback rule: a worktree when there is one, the cwd only in local + // mode, never the user's own checkout in PR mode that unexpectedly lacks one. + if (role === 'script-lint') { + const wt = report.worktreePath; + if (typeof wt === 'string' && wt) { + parts.push( + '', + `**Run everything in the PR worktree** — your working directory is already ` + + `\`${wt}\`. Do not \`cd\` elsewhere.`, + ); + } + const pr = report.prNumber; + const lintTree = + typeof wt === 'string' && wt + ? resolve(wt) + : pr === undefined && opts.planPath + ? '.' + : null; + if (lintTree && opts.planPath) { + // Guard `pr` before interpolating, exactly as the build-test block does: an + // absent number must not write `qwen-review-pr-undefined-script-lint.json`. + const outName = + pr !== undefined + ? `qwen-review-pr-${pr}-script-lint.json` + : 'qwen-review-script-lint.json'; + parts.push( + '', + '**Lint the executable scripts the diff changed.** One call — it dispatches ' + + '`shellcheck` / `actionlint` / `hadolint` by file type and reports what they ' + + 'say. Run it as given (a bare `qwen` re-creates the PATH skew this prefix ' + + 'exists to avoid, and `script-lint` is new enough that an old global lacks it):', + '', + '```bash', + // Quote every interpolated path: a worktree like `/home/a/My Project` + // would otherwise word-split and the command would never run. + `"\${QWEN_CODE_CLI:-qwen}" review script-lint \\`, + ` --plan ${shellQuotePath(resolve(opts.planPath))} \\`, + ` --worktree ${shellQuotePath(resolve(lintTree))} \\`, + ` --out ${shellQuotePath(resolve(dirname(opts.planPath), outName))}`, + '```', + ); + } + } + // The checklists that attach to a path rather than to a dimension. A whole-diff // agent sees every file, so it gets every rule the diff triggers — but only the // agents that review *code* get them at all: Build & Test runs commands and Issue @@ -909,10 +956,16 @@ export function buildRoleBrief( } // SKILL.md is explicit: "Do NOT inject review rules into Agent 7 (Build & - // Test) — it runs deterministic commands, not code review." The roster path - // hands the same --rules to every role, so the exclusion lives here, where - // both the single-role and roster builds pass through. - parts.push(...tail(role === '7' ? undefined : opts.rules, brief.output)); + // Test) — it runs deterministic commands, not code review." Script Lint is the + // same shape — a command's verdict, not a read — so it is excluded too. The + // roster path hands the same --rules to every role, so the exclusion lives here, + // where both the single-role and roster builds pass through. + parts.push( + ...tail( + role === '7' || role === 'script-lint' ? undefined : opts.rules, + brief.output, + ), + ); return parts.join('\n'); } diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 922cdfbf240..d46641f1b13 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -46,6 +46,7 @@ export type RoleId = | '6b' | '6c' | '7' + | 'script-lint' | 'test-matrix' | 'invariant-a' | 'invariant-b' @@ -388,6 +389,24 @@ Read the JSON it prints: Use \`Source: [build]\` or \`Source: [test]\`, never \`[review]\`.`, }, + 'script-lint': { + label: 'Script Lint: shell / workflow / Dockerfile static analysis', + publicLabel: 'the executable-script lint', + publicLabelZh: '可执行脚本静态检查', + readsDiff: false, + brief: `You are **Script Lint**. You do not read the diff line by line — you run the project's deterministic linters over the executable code it changed (shell scripts, GitHub Actions \`run:\` steps, Dockerfiles) and report what they say. Your evidence is **the command you ran and its JSON**; a return that names no command has not done this job. + +**Run \`qwen review script-lint\` (the exact command, with its \`--plan\` and \`--worktree\`, is below).** It dispatches \`shellcheck\` / \`actionlint\` / \`hadolint\` by file type, runs each over the post-change file, and marks every finding with whether its line is one the diff changed. A shell bug — an unquoted \`$x\` that word-splits on a path with a space, a \`\${PIPESTATUS[1]}\` read after the array was already reset, a \`[ ]\` where \`[[ ]]\` was meant — is exactly the class that hides from a read of a long YAML and is caught by the checker. Do **not** hand-read the YAML instead: measured, that misses this class (a model told in prose to run the step scripts read them 4 times out of 4 and ran them 0). + +Read the JSON it prints: + +- \`checked[]\` — each file a linter ran on, with \`findings[]\`. A finding with \`inDiff: true\` is on a line **this PR changed**: report it. Key severity on the outcome, not the rule number — word-splitting (\`SC2086\`/\`SC2046\`) on a destructive or security-sensitive command, a silently dropped exit status, an injection, is a **Critical**; a finding with no nameable wrong outcome is a **Suggestion**. A finding with \`inDiff: false\` is **pre-existing** — real, but not this PR's to answer for: say so, do not file it against this diff. +- \`skipped[]\` — an executable file whose linter is **not installed** on this machine. Report each under a "Not reviewed" note: an unrun checker is not a clean file, and you must not certify one as passing. +- \`ok: true\` with an empty \`skipped[]\` and no \`inDiff\` findings → name the files linted; a return that names no command is a whiff. + +Use \`Source: [build]\`, never \`[review]\` — this is a tool's verdict, not a read of the diff.`, + }, + 'test-matrix': { label: 'Test coverage matrix (whole-diff)', publicLabel: 'the whole-diff test-coverage check', diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index 9b123d29e45..2f14ea06af5 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -160,6 +160,55 @@ describe('requiredAgents — Step 3A', () => { }); }); +describe('requiredAgents — the executable-script lint', () => { + // The requirement is scoped to a diff that actually carries a script a linter + // owns, detected by path — otherwise a pure-TS PR would exit-3 over an agent + // with nothing to check. It is the same `pathTool` the command dispatches on, so + // the roster and the command cannot disagree about what counts. + it.each([ + ['deploy.sh', true], + ['scripts/build.bash', true], + ['.github/workflows/ci.yml', true], + ['Dockerfile', true], + ['docker/api.Dockerfile', true], + ['src/pay.ts', false], // production TS: nothing a shell linter owns + ['README.md', false], + ['config.yml', false], // yaml, but not a workflow + ])('a diff touching %s requires script-lint: %s', (path, required) => { + const plan = { + ...PR, + files: [{ path, kind: 'source', removedLines: 0, heavy: false }], + }; + expect(keys(plan).includes('script-lint')).toBe(required); + }); + + it('requires it when any one file among many is an executable script', () => { + const plan = { + ...PR, + files: [ + { path: 'src/a.ts', kind: 'source' }, + { path: 'src/b.ts', kind: 'source' }, + { path: '.husky/pre-commit.sh', kind: 'source' }, + ], + }; + expect(keys(plan)).toContain('script-lint'); + }); + + it('does NOT require it on a diff-only review — there is no tree to lint', () => { + // Like Build & Test, it reads the changed files from a worktree; a cross-repo + // lightweight review has none, so requiring it would fail a review for not + // doing something it cannot. + const light = { + ...PR, + worktreePath: undefined, + prNumber: undefined, + files: [{ path: 'deploy.sh', kind: 'source' }], + }; + expect(reviewMode(light)).toBe('diff-only'); + expect(keys(light)).not.toContain('script-lint'); + }); +}); + describe('requiredAgents — Step 3B', () => { const BIG = { ...PR, diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index 0f39b782e81..f22ebad1216 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -25,6 +25,7 @@ // roster that gets shrunk. import type { RoleId } from './agent-briefs.js'; +import { pathTool } from '../script-lint.js'; /** * How this review's diff was captured — which decides what can be asked of it. @@ -51,6 +52,7 @@ export interface RosterPlan { path?: unknown; kind?: unknown; heavy?: unknown; + addedLines?: unknown; removedLines?: unknown; }>; srcDiffLines?: unknown; @@ -123,6 +125,27 @@ function isPositivePrNumber(value: unknown): boolean { return false; } +/** + * Does the diff touch a file a linter owns by path — a shell script, a workflow, + * a Dockerfile? Detected by path alone (`pathTool`), the same detector the command + * uses, because here only the plan's file paths are in hand, not the files. A + * shebang-only extensionless script does not trip this — the roster cannot read it + * — but if any script-lint agent runs, the command still lints it; the roster only + * decides whether to *require* the agent, and it requires it whenever the diff + * carries something a lint would name. + */ +function hasExecutableScript(plan: RosterPlan): boolean { + const files = Array.isArray(plan.files) ? plan.files : []; + return files.some((f) => { + if (typeof f?.path !== 'string' || pathTool(f.path) === null) return false; + // A pure deletion has nothing on the new side to lint, so requiring the agent + // for it launches a mandatory no-op. Exclude files with zero added lines; + // `addedLines` absent (a plan that never recorded it) fails safe to "require". + const added = f.addedLines; + return added === undefined || Number(added) > 0; + }); +} + /** Source files rewritten heavily enough that the diff is the wrong frame. */ function heavyFiles(plan: RosterPlan): string[] { const files = Array.isArray(plan.files) ? plan.files : []; @@ -207,6 +230,14 @@ export function requiredAgents(plan: RosterPlan): RequiredAgent[] { if (mode !== 'diff-only') { add('1c'); add('7'); + // Script Lint reads the changed files from the tree, so — like Build & Test — + // it needs one: a `diff-only` review has no worktree to lint. And it is only + // required when the diff actually carries an executable script; a pure-TS PR + // has nothing for it to check, and requiring it there would exit-3 every such + // review over an agent with no job. The command still runs harmlessly on a + // diff with no scripts (it reports "nothing to lint"), but the *requirement* + // is scoped to when there is something to find. + if (hasExecutableScript(plan)) add('script-lint'); } // A largely-rewritten file is not reviewable as a diff: the two ends of an diff --git a/packages/cli/src/commands/review/script-lint.mock.test.ts b/packages/cli/src/commands/review/script-lint.mock.test.ts new file mode 100644 index 00000000000..344a6436ccf --- /dev/null +++ b/packages/cli/src/commands/review/script-lint.mock.test.ts @@ -0,0 +1,215 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The gated tests in script-lint.test.ts need a real `shellcheck` and never run +// actionlint/hadolint (not installed in CI). These inject a fake tool runner so +// all three linters' JSON normalisation, the fail-closed paths (a checker that +// errors is not a clean file), and the context-line classification are pinned +// with no binary present. + +import { describe, it, expect } from 'vitest'; +import { + mkdtempSync, + mkdirSync, + writeFileSync, + readFileSync, + rmSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + runScriptLint, + type ToolRun, + type ToolRunner, + type LintTool, +} from './script-lint.js'; + +let dir: string; +function fresh() { + dir = mkdtempSync(join(tmpdir(), 'script-lint-mock-')); +} +function clean() { + rmSync(dir, { recursive: true, force: true }); +} + +/** A runner that returns the same canned result for whichever tool is asked. */ +function fixedRunner(res: ToolRun): ToolRunner { + return () => res; +} +/** A runner that returns shellcheck-style json1 findings on the given lines. */ +function shellcheckRunner( + comments: Array<{ line: number; code: number; level: string }>, +): ToolRunner { + return (tool: LintTool): ToolRun => + tool === 'shellcheck' + ? { + kind: 'ok', + stdout: JSON.stringify({ + comments: comments.map((c) => ({ ...c, message: 'msg' })), + }), + } + : { kind: 'missing' }; +} + +/** Write a worktree file + a plan pointing at it with the given plan fields. */ +function setup( + path: string, + content: string, + extra: Record = {}, +): { plan: string; worktree: string } { + const abs = join(dir, path); + mkdirSync(join(abs, '..'), { recursive: true }); + writeFileSync(abs, content); + const planPath = join(dir, 'plan.json'); + writeFileSync( + planPath, + JSON.stringify({ files: [{ path, kind: 'source', ...extra }] }), + ); + return { plan: planPath, worktree: dir }; +} + +describe('runScriptLint — tool JSON normalisation (injected runner)', () => { + it('normalises actionlint output and blocks on a changed-line finding', () => { + fresh(); + const runner = fixedRunner({ + kind: 'ok', + stdout: JSON.stringify([ + { + message: 'shellcheck SC2086', + line: 8, + column: 9, + kind: 'shellcheck', + }, + ]), + }); + const { plan, worktree } = setup( + '.github/workflows/ci.yml', + 'name: CI\non: push\njobs: {}\n', + { hunks: [{ newStart: 8, newEnd: 8 }] }, + ); + const r = runScriptLint({ plan, worktree }, runner); + expect(r.checked[0].tool).toBe('actionlint'); + expect(r.checked[0].findings[0]).toMatchObject({ line: 8, inDiff: true }); + expect(r.ok).toBe(false); + clean(); + }); + + it('normalises hadolint output (code + level preserved)', () => { + fresh(); + const runner = fixedRunner({ + kind: 'ok', + stdout: JSON.stringify([ + { line: 3, code: 'DL3006', level: 'warning', message: 'tag the image' }, + ]), + }); + const { plan, worktree } = setup( + 'Dockerfile', + 'FROM alpine\nRUN echo hi\n', + { + hunks: [{ newStart: 3, newEnd: 3 }], + }, + ); + const r = runScriptLint({ plan, worktree }, runner); + expect(r.checked[0].tool).toBe('hadolint'); + expect(r.checked[0].findings[0]).toMatchObject({ + code: 'DL3006', + level: 'warning', + line: 3, + inDiff: true, + }); + expect(r.ok).toBe(false); + clean(); + }); + + it('normalises shellcheck json1 (SC-prefixed code, info blocks)', () => { + fresh(); + const { plan, worktree } = setup('x.sh', '#!/bin/bash\nrm $X\n', { + hunks: [{ newStart: 2, newEnd: 2 }], + }); + const r = runScriptLint( + { plan, worktree }, + shellcheckRunner([{ line: 2, code: 2086, level: 'info' }]), + ); + expect(r.checked[0].findings[0]).toMatchObject({ + code: 'SC2086', + level: 'info', + inDiff: true, + }); + expect(r.ok).toBe(false); + clean(); + }); +}); + +describe('runScriptLint — fail closed (a crashed checker is not clean)', () => { + it.each([ + [ + 'a spawn error (EACCES)', + { kind: 'error', reason: 'shellcheck failed to run: EACCES' }, + ], + ['a signal', { kind: 'error', reason: 'shellcheck was killed by SIGKILL' }], + [ + 'an unexpected status', + { kind: 'error', reason: 'shellcheck exited 2: boom' }, + ], + ] as Array<[string, ToolRun]>)( + 'reports %s as errored, not ok', + (_label, res) => { + fresh(); + const { plan, worktree } = setup('x.sh', '#!/bin/bash\nrm $X\n', { + hunks: [{ newStart: 2, newEnd: 2 }], + }); + const r = runScriptLint({ plan, worktree }, fixedRunner(res)); + expect(r.checked).toEqual([]); + expect(r.errored).toHaveLength(1); + expect(r.errored[0].tool).toBe('shellcheck'); + expect(r.ok).toBe(false); + expect(r.note).toContain('failed to lint'); + clean(); + }, + ); +}); + +describe('runScriptLint — inDiff uses added lines, not hunk context', () => { + it('does NOT block on a finding that lands on a context line', () => { + fresh(); + // The diff ADDS line 4 (`echo new`); line 3 (`rm $X`) is unchanged context + // inside the same hunk. A pre-existing SC2086 on line 3 must not be this PR's. + const diff = [ + 'diff --git a/x.sh b/x.sh', + 'index 1111111..2222222 100644', + '--- a/x.sh', + '+++ b/x.sh', + '@@ -1,4 +1,5 @@', + ' #!/bin/bash', + ' set -e', + ' rm $X', + '+echo new', + ' echo done', + '', + ].join('\n'); + const diffPath = join(dir, 'pr.diff'); + writeFileSync(diffPath, diff); + const { plan, worktree } = setup( + 'x.sh', + '#!/bin/bash\nset -e\nrm $X\necho new\necho done\n', + { hunks: [{ newStart: 1, newEnd: 5 }] }, // context-inclusive hunk + ); + const planObj = JSON.parse(readFileSync(plan, 'utf8')); + planObj.diffPathAbsolute = diffPath; + writeFileSync(plan, JSON.stringify(planObj)); + + const r = runScriptLint( + { plan, worktree }, + shellcheckRunner([{ line: 3, code: 2086, level: 'info' }]), + ); + const sc = r.checked[0].findings.find((f) => f.code === 'SC2086'); + expect(sc).toBeDefined(); + expect(sc!.line).toBe(3); + expect(sc!.inDiff).toBe(false); // line 3 is context, not an added line + expect(r.ok).toBe(true); + clean(); + }); +}); diff --git a/packages/cli/src/commands/review/script-lint.test.ts b/packages/cli/src/commands/review/script-lint.test.ts new file mode 100644 index 00000000000..b1f625b1276 --- /dev/null +++ b/packages/cli/src/commands/review/script-lint.test.ts @@ -0,0 +1,159 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The point of this command is that a shell bug in a diff is caught by *running* +// the checker, not by asking a model to read the YAML — measured, a model told +// in prose to "run the workflow scripts" reads instead (0/4 executed). So the +// engine is deterministic, and these tests pin it: shellcheck's finding on a +// changed line is reported and blocks; the same finding on an unchanged line is +// disclosed but does not; a linter that is not installed is skipped, not clean. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { runScriptLint, toolFor } from './script-lint.js'; + +const hasShellcheck = (() => { + try { + execFileSync('shellcheck', ['--version'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } +})(); + +let dir: string; +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'script-lint-')); +}); +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +/** Write the worktree file and a plan pointing at it with the given hunk ranges. */ +function setup( + path: string, + content: string, + hunks: Array<{ newStart: number; newEnd: number }>, +): { plan: string; worktree: string } { + const abs = join(dir, path); + mkdirSync(join(abs, '..'), { recursive: true }); + writeFileSync(abs, content); + const planPath = join(dir, 'plan.json'); + writeFileSync( + planPath, + JSON.stringify({ files: [{ path, kind: 'source', hunks }] }), + ); + return { plan: planPath, worktree: dir }; +} + +describe('toolFor — dispatch by file type, not by GitHub', () => { + it.each([ + ['.github/workflows/ci.yml', '', 'actionlint'], + ['deploy.sh', '', 'shellcheck'], + ['scripts/build.bash', '', 'shellcheck'], + ['Dockerfile', '', 'hadolint'], + ['docker/api.Dockerfile', '', 'hadolint'], + // Extensionless script, decided by its shebang — a git hook, a CI helper. + ['.husky/pre-commit', '#!/usr/bin/env bash', 'shellcheck'], + ['hooks/prepush', '#!/bin/sh', 'shellcheck'], + ] as const)('%s -> %s', (path, firstLine, tool) => { + expect(toolFor(path, firstLine)).toBe(tool); + }); + + it('leaves non-executable files alone', () => { + expect(toolFor('src/index.ts', 'export const x = 1;')).toBeNull(); + expect(toolFor('README.md', '# Title')).toBeNull(); + expect(toolFor('config.yml', 'key: value')).toBeNull(); // yaml, but not a workflow + }); +}); + +describe.skipIf(!hasShellcheck)( + 'runScriptLint — shellcheck on a changed line', + () => { + // A shell script with an SC2086 (unquoted `$X` word-splits) on line 3. + const SCRIPT = [ + '#!/usr/bin/env bash', + 'set -e', + 'rm $TARGET', + 'echo finished', + '', + ].join('\n'); + + it('reports the finding on a changed line and blocks (ok=false)', () => { + const { plan, worktree } = setup('clean.sh', SCRIPT, [ + { newStart: 3, newEnd: 3 }, // the `rm $TARGET` line is in the diff + ]); + const r = runScriptLint({ plan, worktree }); + expect(r.checked).toHaveLength(1); + expect(r.checked[0].tool).toBe('shellcheck'); + const sc2086 = r.checked[0].findings.find((f) => f.code === 'SC2086'); + expect(sc2086).toBeDefined(); + expect(sc2086!.line).toBe(3); + expect(sc2086!.inDiff).toBe(true); + expect(r.ok).toBe(false); + }); + + it('discloses the same finding on an unchanged line but does NOT block', () => { + // The buggy line is line 3, but the diff only touched line 4. + const { plan, worktree } = setup('clean.sh', SCRIPT, [ + { newStart: 4, newEnd: 4 }, + ]); + const r = runScriptLint({ plan, worktree }); + const sc2086 = r.checked[0].findings.find((f) => f.code === 'SC2086'); + expect(sc2086).toBeDefined(); + expect(sc2086!.inDiff).toBe(false); // pre-existing — not this PR's fault + expect(r.ok).toBe(true); + }); + + it('is clean on a well-quoted script', () => { + const good = ['#!/usr/bin/env bash', 'set -e', 'rm "$TARGET"', ''].join( + '\n', + ); + const { plan, worktree } = setup('ok.sh', good, [ + { newStart: 3, newEnd: 3 }, + ]); + const r = runScriptLint({ plan, worktree }); + expect(r.checked[0].findings.filter((f) => f.inDiff)).toEqual([]); + expect(r.ok).toBe(true); + }); + }, +); + +describe('runScriptLint — graceful degradation and scoping', () => { + it('skips a file whose linter is not installed, and says so (not clean)', () => { + // actionlint / hadolint are not installed in CI here; a workflow file must be + // reported as skipped, never as a clean pass. + const { plan, worktree } = setup( + '.github/workflows/ci.yml', + 'name: CI\non: push\njobs: {}\n', + [{ newStart: 1, newEnd: 3 }], + ); + const r = runScriptLint({ plan, worktree }); + if (r.checked.some((c) => c.tool === 'actionlint')) { + // actionlint IS installed on this machine — then it was checked, fine. + expect(r.skipped).toEqual([]); + } else { + expect(r.skipped).toHaveLength(1); + expect(r.skipped[0].tool).toBe('actionlint'); + expect(r.skipped[0].reason).toContain('not installed'); + expect(r.note).toContain('not installed'); + } + }); + + it('checks nothing when no executable file changed', () => { + const { plan, worktree } = setup('src/a.ts', 'const x = 1;\n', [ + { newStart: 1, newEnd: 1 }, + ]); + const r = runScriptLint({ plan, worktree }); + expect(r.checked).toEqual([]); + expect(r.skipped).toEqual([]); + expect(r.ok).toBe(true); + expect(r.note).toContain('No executable scripts'); + }); +}); diff --git a/packages/cli/src/commands/review/script-lint.ts b/packages/cli/src/commands/review/script-lint.ts new file mode 100644 index 00000000000..d86288e1763 --- /dev/null +++ b/packages/cli/src/commands/review/script-lint.ts @@ -0,0 +1,492 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review script-lint`: run the deterministic linters over the executable +// code a diff adds or changes, and report what they say. +// +// A diff's shell — a `.sh`/`.bash` file, a Dockerfile `RUN`, a GitHub Actions +// `run:` block — is code, and its bugs (an unquoted `$x` that +// word-splits, a `${PIPESTATUS[1]}` read after the array was already reset, a +// `[ ]` where `[[ ]]` was meant) are exactly the class a reviewer misses by +// *reading* a 3000-line YAML and catches by *running* the checker. Measured: +// a model told in prose to "run the workflow scripts" does not — it reads and +// reasons instead (0 of 4 runs executed anything). So the execution is a +// command, not a request: `shellcheck`/`actionlint`/`hadolint` do the work, an +// agent reads this report, and coverage requires the agent ran. +// +// It is not GitHub-specific. `shellcheck` is the workhorse and applies to shell +// wherever it appears; `actionlint` and `hadolint` are format front-ends for the +// two embeds worth special-casing. A linter that is not installed is disclosed +// as skipped, never treated as a clean bill of health. + +import type { CommandModule } from 'yargs'; +import { spawnSync } from 'node:child_process'; +import { + readFileSync, + writeFileSync, + mkdirSync, + lstatSync, + openSync, + readSync, + closeSync, +} from 'node:fs'; +import { dirname, join, resolve, basename } from 'node:path'; +import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +import { parseDiff } from './lib/diff-plan.js'; + +/** The deterministic checkers this command dispatches. */ +export type LintTool = 'shellcheck' | 'actionlint' | 'hadolint'; + +/** One diagnostic, normalised across the three tools. */ +export interface LintFinding { + /** New-side line in the post-change file. */ + line: number; + /** The tool's own rule id — `SC2086`, `DL3006`, or the actionlint kind. */ + code: string; + /** `error` | `warning` | `info` | `style`. */ + level: string; + message: string; + /** + * Whether `line` falls inside a hunk this diff changed. A lint finding on an + * unchanged line is pre-existing — real, but not this PR's to answer for — so + * the agent keys severity on this, exactly as Build & Test keys it on whether + * the failing file was changed. + */ + inDiff: boolean; +} + +/** One executable file that had an applicable linter, and what it said. */ +export interface FileLint { + path: string; + tool: LintTool; + findings: LintFinding[]; +} + +export interface ScriptLintReport { + /** Files an installed linter actually checked. */ + checked: FileLint[]; + /** + * Executable files whose linter is **not installed** — checked by nothing, and + * said so. Never silently dropped: an unrun checker is not a clean file. + */ + skipped: Array<{ path: string; tool: LintTool; reason: string }>; + /** + * Files whose linter **ran but failed** — a spawn error, a signal, an + * unexpected exit status, a `maxBuffer` overflow. Distinct from `skipped` (not + * installed): a checker that crashed reviewed nothing, so we fail closed — an + * errored file forces `ok` false, it is never a clean pass on the tool's silence. + */ + errored: Array<{ path: string; tool: LintTool; reason: string }>; + /** + * True when every applicable linter ran cleanly **and** no finding on a changed + * line is above `style` — `info`/`warning`/`error` all count against it (the + * SC2086 word-split is `info`, and it blocks). A run error (`errored[]` + * non-empty) also makes this false. An uninstalled linter (`skipped[]`) does + * not flip `ok`, but is disclosed for the agent to report as unreviewed. + */ + ok: boolean; + /** One line for the agent's report. */ + note: string; +} + +interface ScriptLintArgs { + plan: string; + worktree: string; + out?: string; +} + +interface PlanFile { + path?: unknown; + hunks?: Array<{ newStart?: unknown; newEnd?: unknown }>; +} + +/** + * Which linter owns a path by its **name alone** — no file contents needed. + * + * Split out from `toolFor` because the roster (`lib/roster.ts`) must decide + * whether to require the script-lint agent knowing only the plan's file paths, + * not the files themselves. One detector, so the roster and the command cannot + * disagree about what counts as an executable script. + */ +export function pathTool(path: string): LintTool | null { + const p = path.toLowerCase(); + const base = basename(p); + if (/(^|\/)\.github\/workflows\/.+\.ya?ml$/.test(p)) { + return 'actionlint'; + } + if ( + base === 'dockerfile' || + p.endsWith('.dockerfile') || + base.startsWith('dockerfile.') + ) { + return 'hadolint'; + } + if (p.endsWith('.sh') || p.endsWith('.bash')) return 'shellcheck'; + return null; +} + +/** Which linter owns a path, or null when it is not executable code we check. + * A name match wins; otherwise an extensionless script is decided by its shebang + * (a git hook, a CI helper) — which is why this one needs the file's first line. */ +export function toolFor(path: string, firstLine: string): LintTool | null { + const byPath = pathTool(path); + if (byPath) return byPath; + if (/^#!.*\b(sh|bash|dash|ksh)\b/.test(firstLine)) return 'shellcheck'; + return null; +} + +/** New-side hunk ranges from the plan, as `[start, end]` pairs (empty if none). */ +function hunksOf(file: PlanFile): Array<[number, number]> { + const hs = Array.isArray(file.hunks) ? file.hunks : []; + const out: Array<[number, number]> = []; + for (const h of hs) { + const s = Number(h?.newStart); + const e = Number(h?.newEnd); + if (Number.isInteger(s) && Number.isInteger(e) && e >= s) out.push([s, e]); + } + return out; +} + +function inAnyHunk(line: number, ranges: Array<[number, number]>): boolean { + return ranges.some(([s, e]) => line >= s && line <= e); +} + +/** + * Added-line ranges per path, parsed from the unified diff — the lines this PR + * actually **added or changed**, with the three context lines git prints around + * each hunk EXCLUDED. The plan's `hunks` include that context (see report.ts), so + * keying `inDiff` off them marks a pre-existing diagnostic three lines from a real + * change as this PR's and blocks on someone else's bug. `addedRanges` is populated + * only for heavy files in the plan, so we parse the diff, which carries it for + * every file. If the diff cannot be read we fall back to the (context-inclusive) + * plan hunks — over-inclusive, but fail-closed, never fail-open to "nothing changed". + */ +function addedRangesByPath( + diffPath: string, +): Map> { + const map = new Map>(); + let text: string; + try { + text = readFileSync(diffPath, 'utf8'); + } catch { + return map; + } + let parsed: ReturnType; + try { + parsed = parseDiff(text); + } catch { + return map; + } + for (const f of parsed.files) { + map.set( + f.path, + f.addedRanges.map((r) => [r.start, r.end] as [number, number]), + ); + } + return map; +} + +/** + * The file's first line, read safely for shebang detection — or `null` if the + * path is not a regular file. A PR is untrusted: a changed `hang.sh` symlinked to + * `/dev/zero` would hang a whole-file read, and a fifo would block. `lstat` does + * not follow the link, so a non-regular file is skipped entirely — the linter is + * never pointed at it either. The read is bounded to one block, not the whole file. + */ +function firstLineOf(abs: string): string | null { + let st; + try { + st = lstatSync(abs); + } catch { + return null; + } + if (!st.isFile()) return null; + let fd: number | undefined; + try { + fd = openSync(abs, 'r'); + const buf = Buffer.alloc(4096); + const n = readSync(fd, buf, 0, buf.length, 0); + const text = buf.toString('utf8', 0, n); + const nl = text.indexOf('\n'); + return nl >= 0 ? text.slice(0, nl) : text; + } catch { + return null; + } finally { + if (fd !== undefined) closeSync(fd); + } +} + +/** The outcome of pointing a linter at one file. */ +export type ToolRun = + | { kind: 'ok'; stdout: string } + | { kind: 'missing' } + | { kind: 'error'; reason: string }; + +/** + * How `runScriptLint` invokes a linter. Injectable so a test can feed canned + * output for all three tools — and exercise the fail-closed paths — without the + * binaries installed; the default is the real `spawnSync`-backed runner. + */ +export type ToolRunner = (tool: LintTool, absPath: string) => ToolRun; + +/** + * Run a linter over one file. Fails **closed**: only a clean exit (0) or a + * findings exit (1) yields output to parse; a spawn error (`EACCES`), a signal, + * a `maxBuffer` overflow, or any other status is an `error` the caller must not + * read as a clean file. `ENOENT` alone is `missing` (the binary is not installed). + */ +function runTool(tool: LintTool, absPath: string): ToolRun { + // The three tools all take a file path and print machine-readable diagnostics. + // `shellcheck --norc` ignores a PR-controlled `.shellcheckrc` in the worktree + // (which could disable SC2086), and the sanitized env drops `SHELLCHECK_OPTS` + // for the same reason: a checker's configuration must come from us, not the diff. + const argv: Record = { + shellcheck: ['--norc', '--format=json1', '--severity=style', absPath], + actionlint: ['-format', '{{json .}}', '-no-color', absPath], + hadolint: ['--format', 'json', absPath], + }; + const env = { ...process.env }; + delete env['SHELLCHECK_OPTS']; + const r = spawnSync(tool, argv[tool], { + encoding: 'utf8', + maxBuffer: 32 * 1024 * 1024, + stdio: ['ignore', 'pipe', 'pipe'], + env, + }); + const err = r.error as NodeJS.ErrnoException | undefined; + if (err?.code === 'ENOENT') return { kind: 'missing' }; + if (err) { + return { kind: 'error', reason: `${tool} failed to run: ${err.message}` }; + } + if (r.signal) { + return { kind: 'error', reason: `${tool} was killed by ${r.signal}` }; + } + // All three exit 0 (clean) or 1 (found something) on a normal run. Any other + // status — a parse/usage error, a crash — is not "no findings"; fail closed. + if (r.status !== 0 && r.status !== 1) { + const detail = `${r.stderr ?? ''}`.trim().split('\n')[0] ?? ''; + return { + kind: 'error', + reason: `${tool} exited ${r.status ?? 'null'}${detail ? `: ${detail}` : ''}`, + }; + } + return { kind: 'ok', stdout: `${r.stdout ?? ''}` }; +} + +/** Normalise each tool's JSON into `LintFinding[]` (line/code/level/message). */ +function parseFindings(tool: LintTool, raw: string): LintFinding[] { + if (!raw.trim()) return []; + let json: unknown; + try { + json = JSON.parse(raw); + } catch { + return []; + } + const mk = ( + line: unknown, + code: string, + level: string, + message: unknown, + ): LintFinding | null => { + const l = Number(line); + if (!Number.isInteger(l) || l < 1) return null; + return { + line: l, + code, + level, + message: String(message ?? ''), + inDiff: false, + }; + }; + if (tool === 'shellcheck') { + const comments = (json as { comments?: unknown[] })?.comments ?? []; + return (Array.isArray(comments) ? comments : []) + .map((c) => { + const o = c as { + line?: unknown; + code?: unknown; + level?: unknown; + message?: unknown; + }; + return mk( + o.line, + `SC${o.code}`, + String(o.level ?? 'warning'), + o.message, + ); + }) + .filter((x): x is LintFinding => x !== null); + } + if (tool === 'hadolint') { + return (Array.isArray(json) ? json : []) + .map((c) => { + const o = c as { + line?: unknown; + code?: unknown; + level?: unknown; + message?: unknown; + }; + return mk( + o.line, + String(o.code ?? 'DL'), + String(o.level ?? 'warning'), + o.message, + ); + }) + .filter((x): x is LintFinding => x !== null); + } + // actionlint: an array of { message, line, column, kind, ... } + return (Array.isArray(json) ? json : []) + .map((c) => { + const o = c as { line?: unknown; kind?: unknown; message?: unknown }; + return mk(o.line, String(o.kind ?? 'actionlint'), 'error', o.message); + }) + .filter((x): x is LintFinding => x !== null); +} + +export function runScriptLint( + args: ScriptLintArgs, + runner: ToolRunner = runTool, +): ScriptLintReport { + let plan: { files?: PlanFile[]; diffPathAbsolute?: unknown }; + try { + plan = JSON.parse(readFileSync(args.plan, 'utf8')); + } catch (err) { + throw new Error( + `script-lint: cannot read the plan ${args.plan}: ${(err as Error).message}`, + ); + } + const files = Array.isArray(plan.files) ? plan.files : []; + // The lines this PR actually added or changed, context excluded — keyed off the + // diff, not the plan's context-inclusive hunks. Empty when the diff is absent, + // in which case each file falls back to its (over-inclusive) plan hunks below. + const addedRanges = + typeof plan.diffPathAbsolute === 'string' + ? addedRangesByPath(plan.diffPathAbsolute) + : new Map>(); + + const checked: FileLint[] = []; + const skipped: ScriptLintReport['skipped'] = []; + const errored: ScriptLintReport['errored'] = []; + const missing = new Set(); + + for (const f of files) { + const path = typeof f?.path === 'string' ? f.path : ''; + if (!path) continue; + const abs = resolve(join(args.worktree, path)); + // A file the diff deleted, or a symlink / fifo we will not follow, has nothing + // safe to lint on the new side — `firstLineOf` returns null for both. + const firstLine = firstLineOf(abs); + if (firstLine === null) continue; + const tool = toolFor(path, firstLine); + if (!tool) continue; + + if (missing.has(tool)) { + skipped.push({ path, tool, reason: `${tool} is not installed` }); + continue; + } + const res = runner(tool, abs); + if (res.kind === 'missing') { + missing.add(tool); + skipped.push({ path, tool, reason: `${tool} is not installed` }); + continue; + } + if (res.kind === 'error') { + // Fail closed: a checker that crashed reviewed nothing, so this file is not + // a clean pass — it is surfaced as errored and forces `ok` false below. + errored.push({ path, tool, reason: res.reason }); + continue; + } + // Prefer the diff's added-line ranges (context excluded); fall back to the + // plan's context-inclusive hunks only when the diff was unavailable. A file + // present in the diff with no added lines yields `[]` — correctly nothing. + const ranges = addedRanges.get(path) ?? hunksOf(f); + const findings = parseFindings(tool, res.stdout).map((x) => ({ + ...x, + inDiff: inAnyHunk(x.line, ranges), + })); + checked.push({ path, tool, findings }); + } + + // `style` is cosmetic (SC2006 backticks, SC2250 brace-your-vars); everything + // else shellcheck reports — including the `info`-rated SC2086 word-splitting + // and SC2046 — is a real correctness/quoting bug worth the agent's eyes. So a + // changed-line finding at any level except `style` counts against `ok`. + const blocking = checked + .flatMap((c) => c.findings) + .filter((x) => x.inDiff && x.level !== 'style'); + // Fail closed: a linter that errored on a file also blocks — that file is not + // clean, and `ok: true` on a crashed checker's silence is the trap we avoid. + const ok = blocking.length === 0 && errored.length === 0; + const note = buildNote(checked, skipped, errored, blocking.length); + return { checked, skipped, errored, ok, note }; +} + +function buildNote( + checked: FileLint[], + skipped: ScriptLintReport['skipped'], + errored: ScriptLintReport['errored'], + blocking: number, +): string { + if (checked.length === 0 && skipped.length === 0 && errored.length === 0) { + return 'No executable scripts changed — nothing to lint.'; + } + const parts: string[] = []; + parts.push( + `Linted ${checked.length} file(s); ${blocking} finding(s) on changed lines.`, + ); + if (errored.length > 0) { + const tools = [...new Set(errored.map((e) => e.tool))].join(', '); + parts.push( + `${errored.length} file(s) failed to lint — ${tools} errored (fail closed: not clean).`, + ); + } + if (skipped.length > 0) { + const tools = [...new Set(skipped.map((s) => s.tool))].join(', '); + parts.push( + `${skipped.length} file(s) not checked — ${tools} not installed (report as unreviewed, not clean).`, + ); + } + return parts.join(' '); +} + +export const scriptLintCommand: CommandModule = { + command: 'script-lint', + describe: + 'Run shellcheck/actionlint/hadolint over the executable scripts a diff ' + + 'changed, filtered to the changed lines; the evidence is what the linters say', + builder: (yargs) => + yargs + .option('plan', { + type: 'string', + demandOption: true, + describe: 'Path to the plan report from Step 1', + }) + .option('worktree', { + type: 'string', + demandOption: true, + describe: 'Path to the checkout whose files are linted', + }) + .option('out', { + type: 'string', + describe: 'Also write the report JSON to this path', + }), + handler: (argv) => { + const args = argv as unknown as ScriptLintArgs; + const report = runScriptLint(args); + const json = JSON.stringify(report, null, 2); + // Write the file when asked AND always print the JSON — the agent's brief + // says "read the JSON it prints", and the roster's generated command passes + // `--out`, so an `--out`-only "Wrote ..." line would leave the agent with no + // findings to read. Build & Test does exactly this (writes then prints). + if (args.out) { + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + writeFileSync(args.out, json); + } + writeStdoutLine(json); + writeStderrLine(report.note); + }, +}; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c47b5fbeb26..3988a13517c 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -328,7 +328,7 @@ Everything below still governs what the agent is asked to do; the command builds **Whole-diff agents — launched alongside the chunk agents, in the same response.** -**Their blocks are already in the `--roster` output above — you have them.** Roles there: `0` (PR reviews), `1b` (when the diff removes anything), `1c`, `test-matrix`, `7` (same-repo), and for a **heavy** file three more, one per checklist slice (their blocks are labelled `Invariant agent A|B|C: … — `). Pass each **verbatim**. To rebuild one for a relaunch: `--role ` (an invariant agent adds `--file `). `check-coverage` derives the same list from the plan and will name any role that did not run. +**Their blocks are already in the `--roster` output above — you have them.** Roles there: `0` (PR reviews), `1b` (when the diff removes anything), `1c`, `test-matrix`, `7` (same-repo), `script-lint` (same-repo, when the diff changes an executable script), and for a **heavy** file three more, one per checklist slice (their blocks are labelled `Invariant agent A|B|C: … — `). Pass each **verbatim**. To rebuild one for a relaunch: `--role ` (an invariant agent adds `--file `). `check-coverage` derives the same list from the plan and will name any role that did not run. Why: **the chunk agents got the diff and these did not.** Measured against the harness's record of one real 3B run, all three whole-diff agents — cross-file tracer, test-coverage matrix, build & test — were launched with a prompt that named **no diff file at all**. The test-coverage matrix was told, in prose, to "Read the diff chunks and the test files", and given no path to read them from. It went and read the post-change source instead, and on a diff with deletions that shows an agent precisely nothing: the removed line is not in that file, and nothing marks where it was. These are the agents that own the classes a chunk agent is structurally blind to — the cross-file trace, the cross-chunk removed-behaviour pairing, the test matrix. The review's only coverage of all three was done by agents that never opened the diff, and the coverage check could not see it, because it only ever asked that question of agents whose prompt said `chunk N of M`. @@ -336,6 +336,7 @@ The sections below say what each agent is _for_. They are no longer what it is _ - **Agent 0 (Issue Fidelity)** — PR reviews only. Unchanged. - **Agent 7 (Build & Test)** — same-repo reviews only. Unchanged. +- **Script Lint** — same-repo reviews, **when the diff changes an executable script** (a `.sh`/`.bash` file, a `.github/workflows/*` file, a Dockerfile — the roster decides by path). Like Build & Test it reviews no diff: it runs `qwen review script-lint`, which dispatches `shellcheck`/`actionlint`/`hadolint` by file type over the changed files and reports what they say, each finding marked `inDiff` when it lands on a line the diff changed. A shell bug — an unquoted `$x` that word-splits, a `${PIPESTATUS[1]}` read after the array was reset — is the class that hides from a read of a long YAML and is caught by running the checker; measured, a model told in prose to run the step scripts read them and did not run them. A linter that is not installed on the runner is reported as **skipped** (unreviewed), never as clean. The roster requires this agent whenever the diff carries such a file, and `check-coverage` exit-3s if it did not run — so it is not a step you can forget. Do NOT inject review rules into it, same as Agent 7. - **Agent 1b (Removed-behavior audit)** — run once over the whole diff, **in addition to** each chunk agent's audit of its own deleted lines. A chunk agent can only ask "was this deletion re-established _here_"; the answer usually lives somewhere else. The whole-diff 1b owns the class no territory can see: a **removed or renamed exported symbol whose replacement lives in another chunk or another file**. For each, find the replacement anywhere in the diff and compare **semantics, not existence** — a default that flipped (`includeSubdirs: true` → an exact-match override), a scope that narrowed, an error that used to propagate and is now logged — and then check the **consumers the diff never touches**: does the replacement still mean the same thing to them? This is the pairing a chunk agent is structurally blind to, and the reason it is a whole-diff agent rather than a per-territory duty. - **Agent 1c (Cross-file tracer)** — run once over the whole diff rather than repeated by every chunk agent (a chunk agent cannot see a caller that lives in another chunk). Note the division of labour with 1b, which is by **task**, not by symbol — both agents care about a removed export, and both have its old name (it is right there in the diff's deleted lines). **1c owns caller compatibility**: grep the old name, find every call site, check each one against whatever the diff leaves it calling. **1b owns the pairing**: find the _replacement_ and compare its **semantics** to what was deleted (a default that flipped, a scope that narrowed, an error that stopped propagating). Neither subsumes the other — a replacement can leave every call site compiling, which is all 1c can see, while meaning something different at every one of them, which only 1b goes looking for. - **Test coverage matrix** — does each behavioural change in the diff have a corresponding test? A chunk agent sees either the implementation or the test, rarely both. @@ -400,7 +401,7 @@ Agent 2 (Security) — WHIFF (returned "No issues found." with no evidence A check you perform silently is a check you skip, and this one has been skipped: dogfooded against this skill's own PR, Agent 0 returned in **6 seconds** having made **one tool call**, and the review went on to print "All chunks were successfully reviewed and covered" and **Approve**. The roll-call is what makes that impossible to miss — you cannot write the artifact line for an agent that named no artifact, and a `WHIFF` line you have written is a `WHIFF` you must then act on (relaunch once; on a second bare return, record the dimension in `unreviewedDimensions`, which forbids the Approve). -**The whole-diff agents have no receipt, so this is the only check they get: an agent that returns near-instantly with almost no output did not do its job, and its silence is indistinguishable from "found nothing".** This is not hypothetical — in dogfooding an invariant agent on a heavy file returned in 11 seconds having emitted a few hundred tokens, while its sibling agents ran for minutes; the whiffing agent happened to own the checklist half that held the run's most serious defect, and nothing flagged the miss. Apply the check to **every agent that owes no receipt** — in 3B, the whole-diff agents (Agent 0, **1b**, 1c, Agent 7, the invariant agents, the test-coverage matrix, Agent 8); in 3A, **all of them**, since no 3A agent emits a receipt (Agents 0, 1a, 1b, 1c, 2, 3, 4, 5, 6a, 6b, 6c, 7, and Agent 8 if launched). A whiffing 3A dimension agent is exactly as invisible as a whiffing invariant agent, and the same one-line fix applies. For each such agent, sanity-check that its return is substantive: it names the specific fields/callers/lines it walked, or it explicitly says "No issues found" **after** describing what it examined. For **Agent 7** the evidence is the build/test **commands it ran and their outcomes** — a Build & Test return that names no command whiffed even if it says "build passed", and after its second whiff record `build-and-test` in `unreviewedDimensions` like any other dimension: a zero-finding run whose deterministic verification never actually ran must not certify on its silence. A legitimately empty scope also passes — Agent 0 on a feature PR with no linked issue returns "No issues found — scope empty" plus the evidence it checked (empty `closingIssuesReferences`, no referenced issue, not a bugfix), and that is a complete answer, not a whiff; do not relaunch it. What fails the check is a bare "No issues found" with no evidence of any walk or scope determination, or a response conspicuously shorter and faster than its peers — relaunch that one agent before Step 4, **once**. The relaunch is capped at one attempt per agent: if the second return is also bare, do not spin — take it, and record that agent's dimension in an **`unreviewedDimensions`** list. (The finding format tells every agent to return `No issues found — `; an agent that ignores that twice is not going to comply on the third ask.) A silent whole-diff agent is the Step-3A/3B equivalent of a chunk with no receipt — **and it is treated like one**: `unreviewedDimensions` is carried into Step 6's "Not reviewed" section, it **forbids an Approve** (a dimension nobody reviewed cannot be certified clean, exactly as an uncoverable chunk cannot), and Step 7 serializes it in the review body (compose-review's `unreviewedDimensions` input), named alongside any uncoverable chunks. A run that silently drops Security or the cross-chunk removed-behavior audit and then posts LGTM is the failure this whole check exists to prevent; noting the gap in the terminal and approving anyway would only move it. +**The whole-diff agents have no receipt, so this is the only check they get: an agent that returns near-instantly with almost no output did not do its job, and its silence is indistinguishable from "found nothing".** This is not hypothetical — in dogfooding an invariant agent on a heavy file returned in 11 seconds having emitted a few hundred tokens, while its sibling agents ran for minutes; the whiffing agent happened to own the checklist half that held the run's most serious defect, and nothing flagged the miss. Apply the check to **every agent that owes no receipt** — in 3B, the whole-diff agents (Agent 0, **1b**, 1c, Agent 7, Script Lint, the invariant agents, the test-coverage matrix, Agent 8); in 3A, **all of them**, since no 3A agent emits a receipt (Agents 0, 1a, 1b, 1c, 2, 3, 4, 5, 6a, 6b, 6c, 7, Script Lint if the diff carries an executable script, and Agent 8 if launched). A whiffing 3A dimension agent is exactly as invisible as a whiffing invariant agent, and the same one-line fix applies. For each such agent, sanity-check that its return is substantive: it names the specific fields/callers/lines it walked, or it explicitly says "No issues found" **after** describing what it examined. For **Agent 7** the evidence is the build/test **commands it ran and their outcomes** — a Build & Test return that names no command whiffed even if it says "build passed", and after its second whiff record `build-and-test` in `unreviewedDimensions` like any other dimension: a zero-finding run whose deterministic verification never actually ran must not certify on its silence. A legitimately empty scope also passes — Agent 0 on a feature PR with no linked issue returns "No issues found — scope empty" plus the evidence it checked (empty `closingIssuesReferences`, no referenced issue, not a bugfix), and that is a complete answer, not a whiff; do not relaunch it. What fails the check is a bare "No issues found" with no evidence of any walk or scope determination, or a response conspicuously shorter and faster than its peers — relaunch that one agent before Step 4, **once**. The relaunch is capped at one attempt per agent: if the second return is also bare, do not spin — take it, and record that agent's dimension in an **`unreviewedDimensions`** list. (The finding format tells every agent to return `No issues found — `; an agent that ignores that twice is not going to comply on the third ask.) A silent whole-diff agent is the Step-3A/3B equivalent of a chunk with no receipt — **and it is treated like one**: `unreviewedDimensions` is carried into Step 6's "Not reviewed" section, it **forbids an Approve** (a dimension nobody reviewed cannot be certified clean, exactly as an uncoverable chunk cannot), and Step 7 serializes it in the review body (compose-review's `unreviewedDimensions` input), named alongside any uncoverable chunks. A run that silently drops Security or the cross-chunk removed-behavior audit and then posts LGTM is the failure this whole check exists to prevent; noting the gap in the terminal and approving anyway would only move it. **Step 3A has no receipts, and must not.** There every dimension agent walks every chunk, so "exactly one receipt per chunk" would demand either none or one per diff-reading agent — eleven, or up to thirteen when Agent 8 launches (every agent except Build & Test reads the diff). Territory ownership is a Step 3B idea. **What Step 3A does not lack is coverage** — that is Step 3D's job on both paths, and it needs no receipt from anyone: it reads the lines each agent was pointed at out of the prompt the CLI built, and the diff reads out of the harness's transcript. A receipt was only ever a sentence the agent typed. (For a while the two were confused, and 3A reviews were told nobody had read them. See Step 3D.) What Step 3A shares is the uncoverable rule, and that needs no agent at all: **a chunk is uncoverable iff its `maxLineChars` exceeds ~25 000**, which the orchestrator reads straight out of the plan before launching anything. Compute that list up front on both paths, carry it into Step 6, and let a Step 3B agent's `Uncoverable` receipt add to it rather than be the only source of it.