feat(review): script-lint — run linters over a diff's executable scripts, as a required step

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',

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: () => {

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',

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');
 }
 

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',

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,

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