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

[wenshao]

What & why

A diff's shell is code, and its bugs are a class code review reliably misses. 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 — these hide from a human (or a model) reading a long workflow YAML, and are caught in a second by running the checker. Measured against this skill's own agents: a reviewer told in prose to "run the changed step scripts" reads and reasons instead — it does not run them.

So the running is a command, not a request. /review now has a script-lint step that dispatches the deterministic linters over the executable files a diff changes, and — like Build & Test — it is a required, coverage-gated step, not a lens an agent has to remember to apply.

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 workflow's run: blocks, a Dockerfile). A .sh helper, a git hook — all in scope.

How it works

• qwen review script-lint (new deterministic command) reads the plan, dispatches shellcheck / actionlint / hadolint by file type over each changed executable file, and marks every finding with whether its line is one the diff changed (inDiff). A finding on a changed line is the PR's to answer for; one on an unchanged line is pre-existing and disclosed as such — the same "changed file vs. not" calibration Build & Test already uses. A linter that is not installed on the runner is reported as skipped (unreviewed), never treated as a clean bill of health.
• A script-lint agent role reads no diff — it runs the command and reports from its JSON. agent-prompt welds the exact invocation (absolute --plan / --worktree / --out) into the agent's brief, beside the build-test block and with the same PATH-skew and absent-PR-number guards.
• The roster requires the agent whenever the diff carries a file a linter owns by path (a .sh/.bash, a .github/workflows/*, a Dockerfile) and the review has a worktree to lint in. The requirement uses the command's own pathTool, so the roster and the command cannot disagree about what counts. A pure-TypeScript diff does not require it; a diff-only (cross-repo lightweight) review has no tree to lint, so it does not either.
• Coverage needs no new code. It derives missing roles from the roster generically, so a required script-lint agent that did not run exit-3s check-coverage exactly like any other missing dimension. The role carries its label / publicLabel / publicLabelZh for that machinery.

Validation

• Unit tests pin each deterministic link: toolFor/pathTool dispatch, runScriptLint (an SC2086 on a changed line blocks; the same finding on an unchanged line is disclosed but does not; a clean script passes; an uninstalled linter is skipped, not clean), the roster requirement (required for a .sh/workflow/Dockerfile diff, not for pure TS, not for diff-only), and the brief weld.
• End-to-end on a crafted diff (a workflow plus a scripts/deploy.sh containing rm -rf $TARGET): the roster requires script-lint, and the command blocks on the SC2086 (word-splitting on a destructive command) on the changed line, while disclosing the workflow as skipped where actionlint is absent.
• Full /review command suite green (932 tests); ESLint clean at --max-warnings 0.

中文说明

背景与动机

diff 里的 shell 也是代码，而它的 bug 恰恰是代码评审最容易漏掉的一类：一个没加引号、会在带空格的路径上发生词拆分的 $x，一个在数组已被重置之后才读取的 ${PIPESTATUS[1]}，一个本该用 [[ ]] 却写成 [ ] 的判断——这些在人（或模型）阅读一大段 workflow YAML 时几乎看不出来，而运行一下检查器就能瞬间发现。针对本 skill 自己的 agent 做过测量：仅在提示词里告诉评审者"去运行改动的 step 脚本"，它只会去读、去推理，并不会真的运行。

所以"运行"应当是一条命令，而不是一句请求。/review 现在有了一个 script-lint 步骤：对 diff 改动到的可执行文件跑确定性的 linter；并且和 Build & Test 一样，它是一个必需的、由 coverage 把关的步骤，而不是一个需要 agent 记得去用的"视角"。

它不局限于 GitHub。shellcheck 是主力，适用于任何地方出现的 shell；actionlint 和 hadolint 只是针对两类值得特殊处理的嵌入格式（workflow 的 run: 块、Dockerfile）的前端。.sh 辅助脚本、git hook——都在覆盖范围内。

实现方式

• qwen review script-lint（新的确定性命令）读取 plan，按文件类型对每个改动的可执行文件分发 shellcheck / actionlint / hadolint，并为每条发现标注它所在的行是否被本次 diff 改动（inDiff）。落在改动行上的发现由本 PR 负责；落在未改动行上的是既有问题，会如实披露——这与 Build & Test 已有的"改动文件 vs. 未改动文件"的定级方式一致。运行机器上未安装的 linter 会被报告为 skipped（未评审），绝不当作"干净"。
• 一个 script-lint agent 角色不读 diff——它运行命令，并根据其 JSON 汇报。agent-prompt 会把确切的调用（绝对路径的 --plan / --worktree / --out）焊进该 agent 的 brief，紧挨 build-test 块，并带有同样的 PATH 偏移与"PR 号缺失"防护。
• roster 会要求该 agent：当 diff 带有 linter 按路径识别的文件（.sh/.bash、.github/workflows/*、Dockerfile）且评审有可供检查的 worktree 时。该要求复用命令自己的 pathTool，因此 roster 与命令对"什么算可执行脚本"不会产生分歧。纯 TypeScript 的 diff 不会触发；diff-only（跨仓轻量）评审没有可检查的代码树，因此也不会触发。
• coverage 无需新增代码。它以通用方式从 roster 推导缺失角色，因此一个被要求却没有运行的 script-lint agent 会像任何其他缺失维度一样让 check-coverage 以 exit 3 失败。该角色为此机制带上了 label / publicLabel / publicLabelZh。

验证

• 单元测试固定了每一个确定性环节：toolFor/pathTool 分发、runScriptLint（改动行上的 SC2086 会拦截；同样的发现落在未改动行则披露但不拦截；干净脚本通过；未安装的 linter 记为 skipped 而非干净）、roster 要求（对 .sh/workflow/Dockerfile 的 diff 要求、对纯 TS 不要求、对 diff-only 不要求）、以及 brief 焊接。
• 在构造的 diff 上做端到端验证（一个 workflow 加一个含 rm -rf $TARGET 的 scripts/deploy.sh）：roster 要求 script-lint，命令对改动行上的 SC2086（在破坏性命令上的词拆分）拦截，同时在 actionlint 缺失处将 workflow 披露为 skipped。
• /review 命令全量测试通过（932 项）；ESLint 在 --max-warnings 0 下无告警。

[qwen-code-ci-bot]

✅ Qwen Triage finished — view run. See the stage comments in this thread for the result.

✅ Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

[gwinthis]

Independent local verification report (Linux, real bundled CLI + real shellcheck)

Verdict: verified working end-to-end. The blocking/disclosure/skipped triage behaves exactly as designed, including the shebang-detected extensionless script. Two non-blocking observations: unparseable linter output silently reads as clean, and inDiff is hunk-granular (includes context lines).

Method

Built this branch (npm run build && npm run bundle), placed a real shellcheck 0.10.0 static binary on PATH (with actionlint/hadolint deliberately absent), and drove the real review script-lint subcommand in tmux against crafted worktrees and plans. Baseline: qwen 0.20.1 does not know the subcommand (prints the qwen review usage) — the command is new.

1. Scenario matrix (real binary, real linter)

#	Fixture	Result
A	deploy.sh with rm -rf $TARGET, hunk covers it; workflow file, no actionlint	ok: false; SC2086 line 3 inDiff: true (blocks); workflow in skipped with "report as unreviewed, not clean" in the note
B	Same script, hunk covers only line 4	SC2086 line 3 inDiff: false — disclosed, does not count against ok on its own
C	Clean set -euo pipefail script	ok: true, 0 findings
D	Extensionless hook with #!/bin/bash + rm -rf $1	Detected via shebang, linted, SC2086 in-diff blocks — the git-hook case works

Unit tests: the three PR-touched files (script-lint, roster, agent-prompt) 192 passed / 3 skipped. The roster-requirement and brief-weld logic is covered there; my E2E certifies the command layer those sit on.

2. Design points that check out in code

• pathTool is the single detector shared by the roster and the command — the "roster and command cannot disagree" claim is structural, not aspirational.
• A missing linter becomes skipped + an explicit unreviewed note (fail-disclosed, not fail-silent) — consistent with the repo's cannot-tell-forbids-approve discipline.
• style-level findings don't block but info-level SC2086/SC2046 do — the severity cut is placed at correctness, not cosmetics, with the reasoning in a comment.

3. Observations (non-blocking)

1. Unparseable linter output reads as clean. parseFindings returns [] on JSON parse failure, and the file still lands in checked. A shellcheck too old for --format=json1 (pre-0.7) exits non-zero with usage text on stdout — that file would be reported as checked-and-clean rather than skipped. Cheap hardening: when the tool ran, exited non-zero, and produced unparseable output, classify as skipped with a "tool output not understood" reason.
2. inDiff is hunk-granular. Plan hunks include git's context lines, so a finding on an unchanged context line inside a hunk counts as in-diff (scenario B's line-4 warning demonstrated this). The error is in the conservative direction (over-blocks, never under-blocks), so this is a note, not a defect.

Conclusion

Thesis: "running the checker" became a required, coverage-gated command precisely because prose instructions to run things don't get executed — and the command's three-way triage (changed-line finding blocks / pre-existing discloses / missing tool is unreviewed) is the right severity calibration, now verified against a real linter on real files. Evidence: the four-scenario matrix with a real shellcheck (§1), the structural single-detector and fail-disclosed properties confirmed in code (§2), and a subcommand-absence baseline. LGTM with §3 as cheap follow-ups.

中文摘要

Linux 真机验证（构建本分支 + 真实 shellcheck 0.10.0，actionlint/hadolint 刻意缺席）：四场景矩阵全过——改动行 SC2086 拦截（ok:false）；未改动行同发现 inDiff:false 仅披露；干净脚本 ok:true；无扩展名 hook 经 shebang 识别并拦截；缺失的 actionlint 记为 skipped 且注明"按未评审报告，不当作干净"。基线 0.20.1 无此子命令。单测 192 过/3 跳。两条非阻塞观察：(1) linter 输出无法解析时静默按"干净"处理（老版 shellcheck 无 json1 会踩中），建议归入 skipped；(2) inDiff 为 hunk 粒度（含上下文行），方向保守（多拦不漏拦），仅记录。

— independent review loop, real-linter verification on Linux

[wenshao]

Review — feat(review): script-lint (#7749)

Verdict: request changes. The design is right and the wiring (roster → brief → coverage) is faithful to the Build & Test precedent. But the inDiff filter — the one thing that decides whether a finding blocks — does not mean what the brief tells the agent it means, and for GitHub workflows it inverts on exactly the case the PR was written for. Two more holes let an un-run checker certify a file as clean.

Everything below was verified by running the code in a worktree at 0633344, with shellcheck 0.11.0 and actionlint 1.7.12 installed — which the PR's own e2e was not ("disclosing the workflow as skipped where actionlint is absent"). That absence is what let C1 through.

What I ran green: 927/927 src/commands/review/ tests, eslint --max-warnings 0 clean, prettier --check clean, tsc --noEmit clean for commands/review/** (remaining TS6305s are unbuilt sibling packages in my worktree).

---

C1 — Critical: a shell bug added inside a workflow run: block is stamped inDiff: false and disclosed as pre-existing

actionlint anchors every shellcheck diagnostic at the line of the - run: key, not at the offending line inside the block. parseFindings reads that line (script-lint.ts:222) and inAnyHunk compares it to the diff's hunks. So for the PR's own flagship example — an unquoted $x added to a run: block more than 3 lines below the run: key — the finding lands outside the hunk.

Repro (workflow with - run: | at line 7, the PR adds rm -rf $TARGET at line 20, hunk 17–21):

ok = true
note = Linted 1 file(s); 0 finding(s) on changed lines.
findings = [
  { line: 7, code: "shellcheck", level: "error", inDiff: false,
    message: "shellcheck reported issue in this script: SC2086:info:13:8: Double quote to prevent globbing and word splitting" },
  { line: 7, code: "shellcheck", level: "error", inDiff: false,
    message: "... SC1010:warning:14:6: Use semicolon or linefeed before 'done' ..." }
]

The brief then instructs the agent: "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." The command reports "0 finding(s) on changed lines", ok: true. The word-splitting bug the PR just introduced is actively suppressed.

It fails the other way too: touch the - run: line itself and every shellcheck finding in that entire block — however old — becomes inDiff: true and blocks.

Fix: for kind: "shellcheck" findings, actionlint embeds the real position in the message (SC2086:info:<line>:<col>:, script-relative). Parse that offset and add it to the run: block's script start to recover the workflow line; or drop inDiff gating for actionlint's shellcheck kind rather than mis-attribute it.

C2 — Critical: a linter that exists but fails is reported as checked-and-clean

runTool (script-lint.ts:153) only treats ENOENT as "not installed". Any other failure returns ${r.stdout ?? ''}, and parseFindings swallows a JSON parse error and returns [] — so the file lands in checked[] with zero findings and ok: true.

Repro, with a shellcheck on PATH that exits 1 with an error on stderr (i.e. a version that lacks --format=json1 — that flag needs shellcheck ≥ 0.7.0; Ubuntu 18.04 ships 0.4.6, Debian buster 0.5.0):

ok = true | note = Linted 1 file(s); 0 finding(s) on changed lines.
checked = [{"path":"deploy.sh","tool":"shellcheck","findings":[]}] | skipped = []

This is the exact failure the design says it prevents — "an unrun checker is not a clean file" — implemented for one of its causes. EACCES, a wrapper script, an OOM kill and a flag-version skew all produce a clean bill. Fix: if stdout does not parse into the tool's expected JSON shape, route to skipped[] with stderr as the reason, never to checked[].

H1 — High: inDiff is computed against hunks, which include git's 3 context lines

hunksOf reads plan.files[].hunks. lib/report.ts:20-33 says of that field, in as many words: "These are hunk ranges, which include the three context lines git prints around every change. For 'which lines did this PR write', use addedRanges." The brief tells the agent the opposite: "A finding with inDiff: true is on a line this PR changed: report it", and to rate SC2086 on a destructive command a Critical.

Repro — the PR writes only line 5; a pre-existing rm $PREEXISTING sits at line 3, inside the hunk's leading context:

ok = false
findings = [{ line: 3, code: "SC2086", level: "info", inDiff: true, ... }]

A Critical filed against a PR for a line it never touched. On a one-line edit to a .sh file, 6 of the 7 lines that can trip inDiff are lines the PR did not write. addedRanges is emitted only for heavy files today, so this needs plumbing — but at minimum the brief must stop claiming line-level precision it does not have.

H2 — High: every changed file is read whole before dispatch; a submodule bump crashes the required step

script-lint.ts:244-249 does readFileSync(abs, 'utf8') on every file in the plan, then calls toolFor. Two consequences:

1. Crash. A submodule pointer change parses into files[] as a bare directory path (verified against parseDiff: [{path: "vendor/lib", hunks: 1}]), existsSync says true, and the read throws:

   THREW -> EISDIR: illegal operation on a directory, read
   

   And unlike build-test, scriptLintCommand.handler (script-lint.ts:325) has no try/catch — build-test wraps runBuildTest deliberately, with a comment: "rather than letting a raw stack trace reach the agent as the whole of Agent 7's result." So a diff carrying a submodule bump and a .sh file takes the whole required, coverage-gated step down with a stack trace, before it lints anything.
2. Waste. A 12 MB package-lock.json or an added PNG is read into a JS string to obtain one line.

Both fixes are the same line: pathTool(path) already answers for .sh/.bash/workflow/Dockerfile with no I/O — only fall through to the read when it returns null, and wrap that read in a try/catch.

---

Suggestions

• script-lint.ts:222 — actionlint severity and rule id are discarded. level is hardcoded 'error' and code becomes the actionlint kind ("shellcheck"), so for every workflow finding the agent sees code: "shellcheck". The brief's rule — "word-splitting (SC2086/SC2046) on a destructive command … is a Critical" — cannot fire, because SC2086 is buried in the message string. Parse it out.
• script-lint.ts:67 — the ok docstring is wrong. It says "no finding … is an error or warning"; line 275 blocks on anything except style, which includes info. That is the right behaviour (SC2086 is info), so fix the comment, not the code.
• script-lint.ts:10 — Makefile is claimed and not implemented. The header says the class covered includes "a Makefile recipe"; the PR body says "A .sh helper, a git hook, a Makefile recipe's shell — all in scope." Verified: pathTool('Makefile') and toolFor('Makefile', '.PHONY: all') both return null. Nothing dispatches on a Makefile. Either drop the claim or add it (checkmake, or extract recipe bodies for shellcheck). SKILL.md gets this right — only the header, the commit message and the PR body overclaim.
• roster.ts:290 — a git hook never requires the agent. hasExecutableScript uses pathTool, which is path-only, so a real husky hook (.husky/pre-commit, extensionless) does not trip the requirement even though toolFor lints it happily. The roster test uses .husky/pre-commit.sh, which is not what husky writes. The tradeoff is documented in the code — but "a git hook … in scope" in the PR body isn't true of the gate.
• roster.ts:28 — layering inversion. import { pathTool } from '../script-lint.js' is the only lib/ → ../<command>.ts import in the review tree (everything else imports downward). It makes coverage.ts, agent-prompt.ts and compose-review.ts all transitively load a yargs command module and node:child_process, and puts a cycle one import away. Move pathTool/LintTool into lib/ and have both sides import it — the "one detector" property is preserved, the direction is fixed.

Test coverage

The unit tests are well-aimed at what they cover, but the gaps line up exactly with the defects above:

• parseFindings for actionlint and hadolint has no test at all. Only the shellcheck branch is exercised. C1 lives in the untested actionlint branch — and the one workflow test asserts on skipped[], i.e. it only runs its assertions when actionlint is absent. A test with actionlint installed would have caught it.
• The blocking/non-blocking core is describe.skipIf(!hasShellcheck). CI runs macos-latest, windows-2022 and a self-hosted ECS runner alongside ubuntu-latest; on any lane without shellcheck those three tests silently vanish. Consider asserting on a stubbed tool so the inDiff/ok logic is pinned everywhere, and keep the real-shellcheck tests as the integration layer.
• "Coverage needs no new code" has no test. I read the path and it holds structurally — roleLabel/publicRoleLabel/selectorOf all index BRIEFS[req.role], and the new role supplies all three labels — but the headline claim of the second commit is the one thing not pinned. One check-coverage test asserting exit 3 for a required-but-absent script-lint would lock it.
• No test covers scriptLintCommand.handler (H2's missing try/catch), or a runScriptLint that throws.

What's good

• Splitting pathTool out of toolFor so the roster and the command cannot disagree about what counts is the right call, and the comment explaining why the roster is path-only is honest about its own limit.
• The --out/--plan/--worktree weld reuses build-test's absolute-path and ${QWEN_CODE_CLI:-qwen} treatment, including the prNumber-undefined guard, and the tests pin all three traps.
• Excluding the role from --rules injection alongside Agent 7 is consistent and correctly justified.
• skipped[] as a first-class output rather than a silent drop is the right shape — C2 is a hole in its coverage, not in the idea.
• No lockfile, dependency or CI-surface change; the command is additive and inert on a diff with no scripts.

---

中文说明

结论：建议修改

设计方向是对的，roster → brief → coverage 的接线也忠实照搬了 Build & Test 的先例。但决定一条发现是否拦截的 inDiff 过滤，含义与 brief 告诉 agent 的并不一致；对 GitHub workflow 而言，它在本 PR 最核心的场景上恰好是反的。另有两个漏洞会让一个根本没跑成功的检查器给出"干净"结论。

以下结论均在 0633344 的 worktree 中实际运行验证，环境同时装有 shellcheck 0.11.0 和 actionlint 1.7.12——而本 PR 自己的端到端验证并没有装 actionlint（PR 描述："在 actionlint 缺失处将 workflow 披露为 skipped"）。正是这个缺失让 C1 漏了过去。

我跑绿的： src/commands/review/ 927/927 测试通过；eslint --max-warnings 0 无告警；prettier --check 通过；tsc --noEmit 在 commands/review/** 下无错误（其余 TS6305 是我 worktree 里兄弟包未构建所致）。

C1 — 严重：加进 workflow run: 块里的 shell bug 被标成 inDiff: false，并被当作"既有问题"披露

actionlint 把每一条 shellcheck 诊断都锚定在 - run: 这一行，而不是块内真正出问题的那一行。parseFindings（script-lint.ts:222）读的就是这个 line。于是当 PR 在距 run: 键 3 行以外的位置加入一个未加引号的 $x——正是本 PR 的旗舰例子——该发现落在 hunk 之外：

ok = true
note = Linted 1 file(s); 0 finding(s) on changed lines.
findings = [{ line: 7, code: "shellcheck", inDiff: false,
              message: "... SC2086:info:13:8: Double quote to prevent globbing and word splitting" }, ...]

而 brief 明确告诉 agent："inDiff: false 是既有问题……不要把它记在本 diff 头上。" 结果：本 PR 刚引入的词拆分 bug 被主动压制。

反方向同样错：只要改动碰到 - run: 那一行，整个块里所有 shellcheck 发现——不论多老——都会变成 inDiff: true 并拦截。

修法：actionlint 的 shellcheck 类发现，其真实位置以 SC2086:info:<行>:<列>: 形式嵌在 message 里（相对脚本），解析出来加上 run 块起始行即可还原 workflow 行号；或干脆对该类发现不做 inDiff 归因，而不是错误归因。

C2 — 严重：装了但运行失败的 linter，被报成"已检查且干净"

runTool（script-lint.ts:153）只把 ENOENT 当作"未安装"。其他任何失败都返回空 stdout，parseFindings 又吞掉 JSON 解析错误返回 []，于是文件进入 checked[]、零发现、ok: true。

用一个"存在但退出码非 0"的 shellcheck 垫片复现（即缺少 --format=json1 的老版本；该 flag 需要 shellcheck ≥ 0.7.0，Ubuntu 18.04 是 0.4.6，Debian buster 是 0.5.0）：

ok = true | checked = [{"path":"deploy.sh","tool":"shellcheck","findings":[]}] | skipped = []

这正是设计声称要防住的失败——"没跑过的检查器不等于文件干净"——却只对其中一种成因做了实现。EACCES、包装脚本、被 OOM 杀掉、flag 版本偏移，都会给出干净结论。修法：stdout 无法解析成该工具预期的 JSON 结构时，一律进 skipped[] 并把 stderr 作为原因，绝不进 checked[]。

H1 — 高：inDiff 基于 hunks 计算，而 hunks 包含 git 的 3 行上下文

lib/report.ts:20-33 对该字段写得很清楚："这些是 hunk 范围，包含 git 在每处改动前后打印的三行上下文。要知道'本 PR 写了哪些行'，请用 addedRanges。" 而 brief 说的正相反："inDiff: true 表示这一行是本 PR 改过的：报告它"，并要求把破坏性命令上的 SC2086 定为 Critical。

复现——PR 只写了第 5 行，既有的 rm $PREEXISTING 在第 3 行、落在 hunk 的前置上下文里：

ok = false | findings = [{ line: 3, code: "SC2086", inDiff: true }]

一个 PR 因为自己从未碰过的行被记了一条 Critical。对 .sh 文件的单行改动，能触发 inDiff 的 7 行里有 6 行不是 PR 写的。addedRanges 目前只对 heavy 文件输出，所以彻底修需要打通管线；但至少 brief 不该再宣称它拥有的行级精度。

H2 — 高：分发前把每个改动文件整体读入；submodule 变更会让这个必需步骤崩溃

script-lint.ts:244-249 对 plan 里每一个文件先 readFileSync(abs, 'utf8')，再调 toolFor。两个后果：

1. 崩溃。 submodule 指针变更会被解析成 files[] 里的一个目录路径（已用 parseDiff 验证：[{path: "vendor/lib", hunks: 1}]），existsSync 为真，读取抛出 EISDIR: illegal operation on a directory, read。而 scriptLintCommand.handler（script-lint.ts:325）没有 try/catch——build-test 是特意加的，注释写着*"以免原始堆栈成为 Agent 7 结果的全部"*。于是一个同时含 submodule bump 和 .sh 的 diff，会让整个必需且受 coverage 把关的步骤在 lint 任何东西之前带着堆栈崩掉。
2. 浪费。 12 MB 的 package-lock.json、新增的 PNG，都会被整体读成 JS 字符串，只为取一行。

两者同一个修法：pathTool(path) 本就能在零 I/O 下回答 .sh/.bash/workflow/Dockerfile，只在它返回 null 时再读首行，并给该读取加 try/catch。

其他建议

• script-lint.ts:222：actionlint 的严重级别与规则号被丢弃。 level 被硬编码为 'error'，code 变成 actionlint 的 kind（"shellcheck"）。于是 brief 里*"SC2086/SC2046 落在破坏性命令上是 Critical"*这条规则对 workflow 根本无法生效——SC2086 埋在 message 字符串里。
• script-lint.ts:67：ok 的文档注释写错了。 注释说"error 或 warning"，第 275 行实际是"除 style 外一律拦截"（含 info）。行为是对的（SC2086 就是 info），该改的是注释。
• script-lint.ts:10：Makefile 只在文案里，没有实现。 已验证 pathTool('Makefile') 与 toolFor('Makefile', '.PHONY: all') 均返回 null。SKILL.md 写得准确，只有文件头注释、commit message 和 PR 描述过度宣称。
• roster.ts:290：git hook 永远不会触发要求。 hasExecutableScript 用的是纯路径的 pathTool，真实的 husky hook（.husky/pre-commit，无扩展名）不会触发要求，尽管 toolFor 能顺利 lint 它。roster 测试用的 .husky/pre-commit.sh 并不是 husky 实际写出的文件名。
• roster.ts:28：分层反转。 这是 review 目录下唯一一处 lib/ → ../<command>.ts 的导入，使 coverage.ts、agent-prompt.ts、compose-review.ts 都间接加载一个 yargs 命令模块和 node:child_process，离形成环只差一次 import。把 pathTool/LintTool 移进 lib/，两边都从那里导入，"单一检测器"的性质不变，方向也正了。

测试覆盖

• parseFindings 的 actionlint 与 hadolint 分支完全没有测试。 C1 就住在没测的 actionlint 分支里——而唯一那条 workflow 测试的断言只在 actionlint 缺失时才成立。
• 拦截/不拦截的核心逻辑被 describe.skipIf(!hasShellcheck) 包着。CI 除 ubuntu-latest 外还有 macos-latest、windows-2022 和自建 ECS runner，没装 shellcheck 的 lane 上这三条测试会静默消失。建议用打桩工具把 inDiff/ok 逻辑在所有平台钉住，真 shellcheck 的用例留作集成层。
• "coverage 无需新增代码"没有测试。 我读过路径，结论成立（roleLabel/publicRoleLabel/selectorOf 都索引 BRIEFS[req.role]，新角色三个 label 齐备），但第二个 commit 的头号主张恰恰是唯一没被钉住的。补一条"required 但缺席的 script-lint 让 check-coverage exit 3"的测试即可。
• scriptLintCommand.handler 无测试（H2 缺 try/catch 即在此）。

做得好的地方

• 把 pathTool 从 toolFor 里拆出来，让 roster 与命令对"什么算可执行脚本"无法产生分歧，是正确的做法；解释 roster 为何只看路径的注释也如实交代了自身的局限。
• --out/--plan/--worktree 的焊接完整复用了 build-test 的绝对路径与 ${QWEN_CODE_CLI:-qwen} 处理（含 prNumber 缺失的守卫），三个陷阱都有测试钉住。
• 与 Agent 7 一起排除 --rules 注入，一致且理由充分。
• 把 skipped[] 作为一等输出而非静默丢弃，形状是对的——C2 是它覆盖面的漏洞，不是这个想法的问题。
• 无 lockfile、依赖或 CI 面的改动；命令是纯增量的，对不含脚本的 diff 无副作用。

[qwen-code-ci-bot]

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.7-max via Qwen Code /review

[wenshao]

⚠️ Downgraded from Request changes to Comment: self-PR. Reviewed. Suggestions are inline. Not reviewed: reverse audit of chunk 1 round 3 — the auditor returned nothing substantive twice. Not reviewed: reverse audit — stopped at the five-round hard cap with only one consecutive dry round. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

— Codex GPT-5 via Qwen Code /review

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wenshao]

Why this feature exists — a measurement

I dogfooded /review against a known-answer PR to check the premise behind this change: that a shell/logic bug a reviewer confirms by running the code is a bug that reading the code misses.

I took PR #7724 at the exact commit where an earlier review round had verified three defects by running probes (a ! command that executes twice, a silently-dropped queued command, an isPreparing flag that disables Stop for a whole run), and ran the real /review Step-3 agents over that diff — full 3A roster on qwen3.8-max-preview plus qwen3.7-max on the owners, each with worktree access (the same information the human reviewer had).

Result: 0 of the 3 were caught. Not for lack of looking — the agents examined the exact code and reasoned past it:

• qwen3.7-max's attacker persona, verbatim: "Two rapid ! commands … both .then() chains dispatch their respective commands to the same newly-created session. This is correct (two user commands, two dispatches) and pre-existing behavior." Both models' attacker persona (and a third persona) walked into the double-execute mechanism and ruled it correct — reasoning about session dedup, missing that it's one user intent firing twice.
• The test-coverage agent called the tests "thorough," while the three defective branches had no test at all.

That is the whole argument for script-lint: a prose instruction to "run the changed step scripts" does not make a model run them (measured earlier: 0/4), so the running has to be a command, not a request. shellcheck catches an unquoted $x on a destructive path in the time it takes to reason wrongly about it.

It also sharpens the review feedback on this PR: the deterministic-gate follow-up (have compose-review read the JSON and compute the block itself, rather than trusting the agent to run the command and label severity) is the right end state — the same lesson, one level up. Tracking that now.

_{🤖 Claude Code · Claude Opus 4.8}

[qwen-code-ci-bot]

Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] C1 (actionlint inDiff misattribution for workflow run: blocks) — author acknowledged as real gap, deferred to follow-up; cannot determine if the deferred timeline is acceptable Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

中文说明

已审查。 建议见行内评论。 未决，请确认：[Critical] C1 (actionlint inDiff misattribution for workflow run: blocks) — author acknowledged as real gap, deferred to follow-up; cannot determine if the deferred timeline is acceptable 未审查：反向审计——有审计 agent 运行并打开了自己的 brief，但没有 agent 是用 CLI 构建的 prompt 启动的——启动 prompt 是手写的，agent 实际被要求做的并不是本 skill 所认证的内容。

— qwen3.7-max via Qwen Code /review

[wenshao]

Superseded by #7751 and closing in its favour.

Per this review's architectural findings (#10/#11/#12), the executable-script lint is better as a deterministic gate than an agent: #7751 has the orchestrator run qwen review script-lint and compose-review read its report as the sole authority — the block decision, the severity, and the proof-it-ran are all deterministic, with the agent removed entirely. #7751 contains every commit from this PR plus that pivot, is merged up to latest main, and every thread raised here is addressed there (including this round's five suggestions).

Thanks for the thorough review across both rounds — it's what drove the redesign.

_{🤖 Claude Code · Claude Opus 4.8}

[yiliang114]

Review Summary

This PR introduces a well-designed feature: running deterministic linters (shellcheck, actionlint, hadolint) over executable scripts changed in a diff, as a required, coverage-gated review step. The architecture follows the Build & Test precedent closely and the implementation demonstrates strong security awareness.

What's Done Well

1. Single detector pattern: pathTool is correctly shared between roster and command, preventing divergence in what counts as an "executable script."

2. Fail-closed approach: The hardened version in cd8702c properly handles:

   • Non-ENOENT spawn errors → errored[] (forces ok: false)
   • Symlinks/fifos → skipped via lstat (no follow)
   • PR-controlled config → shellcheck --norc + sanitized env

3. Proper triage: Three-way output (checked, skipped, errored) correctly distinguishes:

   • Findings on changed lines → block
   • Findings on unchanged lines → disclose only
   • Missing linter → unreviewed (never clean)

4. Context-line fix: The pivot from plan hunks (git's 3-line context) to diff's added-line ranges is the correct approach—pre-existing diagnostics near but not on changed lines must not block.

5. Test coverage: Mock tests (script-lint.mock.test.ts) properly exercise actionlint/hadolint normalization and fail-closed paths without requiring binaries.

Outstanding Items (Acknowledged for Follow-up)

The author has transparently noted these gaps for the fast-follow:

1. Actionlint shellcheck line misattribution: run: block findings anchor at the YAML key line, not the actual shell line—can suppress real bugs or over-block old ones. Needs embedded-metadata parse.

2. Extensionless shebang scripts don't trigger roster requirement: .husky/pre-commit linted by command but not required by roster (pathTool is path-only).

3. Hadolint config isolation: Missing --no-config equivalent to shellcheck's --norc.

4. Deterministic gate: The architectural pivot in feat(review): script-lint as a deterministic gate — compose-review reads the report, no agent #7751 (orchestrator runs script-lint, compose-review reads artifact) is the correct end state—removes model from the critical path.

Minor Observations

• parseFindings returns [] on JSON parse failure of non-empty stdout—could route to errored[] instead of checked[] for consistency
• Roster's addedLines === 0 guard (pure-deletion exclusion) lacks explicit test coverage

Conclusion

Approve for the design direction and implementation quality. The PR correctly addresses the core premise: prose instructions to "run the workflow scripts" don't get executed (0/4 measured), so running must be a command. The follow-up in #7751 to make this a deterministic gate (orchestrator → artifact → compose-review) is the right architectural evolution.

Good work on the thorough iteration through review feedback—each critical finding was addressed in cd8702c with appropriate tests.

---

中文摘要

本 PR 设计方向正确，实现质量高。核心价值：把 "运行检查器" 从请求变成命令，解决了模型在 prose 指令下不执行的问题（实测 0/4）。已修复的关键问题包括：hunk 上下文行误判、非 ENOENT 失败当干净、符号链接/管道挂起风险、PR 控制 shellcheck 配置等。遗留项（actionlint 行号归属、无扩展名 shebang 不触发要求、hadolint 配置隔离）已在 PR 中透明说明并跟踪为后续优化。架构演进至 #7751 的确定性门控（编排器运行 → compose-review 读结果）是正确的方向。