fix(cli): preserve Qwen Review startup version in footers - #8431
Conversation
|
Gate re-run at the current head (
Moving on to code review. 🔍 中文说明在当前 head（
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRound 4 on
TestingUnattended CI run — no local execution of PR code. Evidence below is the PR's own CI on the reviewed head, fetched via the API: green. The tests are load-bearing, not decorative: the regression test drives the real
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The central behavioural claim (deterministic footer version at submit time) is substantiated by the green load-bearing suite above; a sandboxed verification run (30919303889) is already in flight on this head and will post its own A/B report — treat it as corroboration when it lands, not as a gate. Not verified: end-to-end posting of a real review through an installed 中文说明代码审查第 4 轮，针对
测试无人值守 CI 运行——不在本地执行 PR 代码。以下为被审 head 上 PR 自身 CI 经 API 抓取的结果：全绿。 测试真实约束住了修复：回归测试走真实的 CI 明细见上方表格（全绿，merge-queue 专属腿省略）。核心行为主张（提交时版本确定化）已由上述全绿且真实绑定的套件证实；针对本 head 的沙箱验证运行（30919303889）已在进行中，会单独发布 A/B 报告——落地后视为佐证，不作为门槛。 未验证：通过已安装 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — every prior blocker is resolved on this head and CI is green; the only open items are named follow-ups the approving maintainer already accepted. Four rounds in, stepping back: the diagnosis was real — I verified the version skew in the #8410/#8417 job logs before reading any code — and the fix is the minimal mechanism that closes both drift vectors. The iteration worked the way it should: each round's Criticals landed as focused commits with a dedicated test per link, and what's left open is a list of named Suggestions, not unresolved doubt. Those deferrals deserve a tracking issue so they don't evaporate: the quadratic-vs-linear comment in the hang test, the empty-entry filter that can flip a verdict, the local modelId guard, the Approving, pinned to the reviewed commit. ✅ 中文说明置信度：4/5 —— 此前所有阻塞项都已在当前 head 解决，CI 全绿；唯一开放的是已批准的维护者接受的、有明确清单的后续项。 四轮下来退一步看：诊断是真实的——我在读任何代码之前就已在 #8410/#8417 的 job 日志里核实了版本不一致；修复是同时关闭两条漂移路径的最小机制。迭代过程也是健康的：每一轮的 Critical 都以聚焦 commit + 逐环节的专门测试落地，剩下开放的是有明确清单的 Suggestion，而不是未解决的疑虑。这些延期项值得开一个跟踪 issue 以免蒸发：hang 测试里二次/线性的注释、可能翻转 verdict 的空条目过滤、本地 modelId 守卫、无版本 批准，锚定在被审 commit 上。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 The design and implementation review clean — the one thing blocking it is CI: the two new tests in submit.test.ts read the env with dot access, and the project's noPropertyAccessFromIndexSignature rejects that with eight TS4111 errors, so tsc --build fails before vitest runs. Switch them to process.env['QWEN_CODE_STARTUP_VERSION'] (bracket access, same as the production code) and this should go green. Details in the review comments above. 🙏
设计与实现都审查通过——唯一的阻塞是 CI：submit.test.ts 的两个新测试用点号访问 env，项目的 noPropertyAccessFromIndexSignature 以 8 处 TS4111 拒绝，tsc --build 在 vitest 之前失败。改成方括号访问 process.env['QWEN_CODE_STARTUP_VERSION']（与生产代码一致）即可恢复绿色。详见上方审查评论。
|
Qwen Code review did not complete successfully. Failed to determine state for PR #8431. A transient error is retried automatically; if you are seeing this, retry with |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const REVIEW_FOOTER_RE = | ||
| /(?:\s*_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; |
There was a problem hiding this comment.
[Critical] Catastrophic regex backtracking (ReDoS) in REVIEW_FOOTER_RE. The repeated group opens AND closes with \s*, so the whitespace run between two footer lines can be split arbitrarily across iterations; when the trailing $ fails (any body whose footer run is followed by non-footer text) the engine enumerates all those splits. Measured on the shipped regex: 12 footers + trailing text (613 bytes) ≈ 2.1 s; 22 footers ≈ 1.6 s; ~4× per 2 lines; the 25-space-separator shape grows ~27× per footer (n=7 did not finish in 60 s). The input is a model-authored comment body this file's header already treats as untrusted, and it runs in normalizeInlineComments before any posting decision, so an authorized run hangs at 100% CPU and nothing posts. Duplicated footers followed by a closing remark is exactly what a looping model naturally emits — the very shape this function exists to strip. — Failure scenario: an inline comment body with ~12-25 repeated footers followed by any trailing text hangs qwen review submit at 100% CPU for seconds→hours before anything is posted.
| const REVIEW_FOOTER_RE = | |
| /(?:\s*_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; | |
| const REVIEW_FOOTER_RE = | |
| /\s*(?:_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; |
The one-line fix hoists the leading \s* out of the repeated group so each whitespace run has a single owner. Verified linear (50 footers → 0.23 ms) and byte-identical strips on this PR's own duplicate-footer fixture. In-repo precedent pr-context.ts:429 bounds the same shape with [^\n]{0,200}. A near-miss timing regression test is worth adding.
中文说明
[Critical] REVIEW_FOOTER_RE 存在灾难性正则回溯(ReDoS)。重复分组以 \s* 开头又以 \s* 结尾,两个 footer 之间的空白可在迭代间任意切分;当结尾 $ 匹配失败(footer 串后还有非 footer 文本)时,引擎会枚举所有切分。实测:12 个 footer + 尾部文本(613 字节)≈ 2.1 秒;22 个 ≈ 1.6 秒;每 2 行约 4 倍;25 空格分隔形态每个 footer 约 27 倍(n=7 超 60 秒未完成)。输入是模型生成的评论正文(本文件头部已视为不可信),且在任何发布决策前运行,因此一次授权运行会 100% CPU 挂死、无法发布。而"重复 footer 后跟一句收尾"正是循环模型自然产出的形态。失败场景:约 12-25 个重复 footer、后接任意尾部文本的正文,会让 qwen review submit 在发布前挂死数秒到数小时。建议的一行修复把开头 \s* 提出重复分组,让每段空白只有一个归属;已验证线性且对本 PR 的重复 footer 用例剥离结果逐字节一致。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const cliVersion = | ||
| process.env['QWEN_CODE_STARTUP_VERSION'] || (await getCliVersion()); |
There was a problem hiding this comment.
[Suggestion] The sibling command qwen review compose-review still derives its (persisted, printed) body footer from the late mutable source — compose-review.ts:1728 passes await getCliVersion() into composeReview — while submit now pins the posted footer to the startup stamp. getCliVersion() resolves CLI_VERSION env || live package.json, and shared runners demonstrably rewrite installs under running processes (PR #8373's 100-job sample mixed 0.21.2×49 / 0.21.3×50 / 0.21.0×1). Before this diff both surfaces used getCliVersion() and could not diverge; the diff introduces the seam and widens the disagreement window to (startup, compose-time). — Failure scenario: install drift during a long review window → compose-review persists/prints a body footer (v0.21.4) while submit posts (v0.21.3) — the run's own archived artifact contradicts the posted review, the exact mismatch this PR exists to eliminate.
Fix: use process.env['QWEN_CODE_STARTUP_VERSION'] || (await getCliVersion()) in the compose-review handler too (ideally via a shared version-resolution helper).
中文说明
[Suggestion] 姊妹命令 qwen review compose-review 仍从晚到的可变来源取其(持久化/打印的)body footer——compose-review.ts:1728 传入 await getCliVersion()——而 submit 现已把发布的 footer 固定到启动时版本。getCliVersion() 解析 CLI_VERSION 环境变量 || 实时 package.json,共享 runner 已证实会在进程运行中改写安装(PR #8373 的 100 任务样本混有 0.21.2×49 / 0.21.3×50 / 0.21.0×1)。本 diff 之前两处都用 getCliVersion(),不会分叉;本 diff 引入了缝隙,并把分歧窗口扩大到(启动, compose 时刻)。失败场景:长评审窗口内的安装漂移 → compose-review 持久化/打印 (v0.21.4),而 submit 发布 (v0.21.3),运行自身存档与已发布评审相矛盾。修复:compose-review handler 也改用同一表达式(最好抽成共享版本解析助手)。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| expect(source).toMatch( | ||
| /const relaunchEnv = \{[\s\S]*?delete relaunchEnv\['QWEN_CODE_STARTUP_VERSION'\]/, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This test asserts source-text ordering, not behaviour — it greps cli-entry.js source and matches as long as the delete appears anywhere after const relaunchEnv = {, including positions where it has no effect. Mutation probe: moving the delete to AFTER the spawnSync(...) call (the behaviour-killing position) leaves the suite 6/6 green. — Failure scenario: a refactor moving delete relaunchEnv['QWEN_CODE_STARTUP_VERSION'] below the spawnSync(launcher, …) call (or into the if (!launcher) early-exit) still matches, ships green, and reintroduces the exact bug: after a managed update the relaunch inherits the old stamp and the first posted review carries the pre-update version. Conversely a behaviour-preserving requote ("…" vs '…') breaks the test. The harness already mocks spawnSync/existsSync, so a behavioural check is expressible.
Fix: drive the relaunch branch (first spawnSync → { status: 44, signal: null }, launcher present via existsSyncMock) and assert the captured relaunch spawn's env lacks QWEN_CODE_STARTUP_VERSION; at minimum anchor the regex so the delete must precede the spawn that uses relaunchEnv.
中文说明
[Suggestion] 该测试断言的是源码文本顺序而非行为——它对 cli-entry.js 源码做正则匹配,只要 delete 出现在 const relaunchEnv = { 之后任意位置即通过,包括无效位置。突变探针:把 delete 移到 spawnSync(...) 之后(破坏行为的位置),套件仍 6/6 全绿。失败场景:把 delete relaunchEnv['QWEN_CODE_STARTUP_VERSION'] 移到 spawnSync(launcher, …) 之下(或 if (!launcher) 提前退出分支)仍能匹配、绿灯上线,并重新引入原 bug:托管更新后重启继承旧版本戳,首个发布的评审带更新前版本。反之,保持行为的改写(引号风格)却会弄红测试。harness 已 mock spawnSync/existsSync,可写行为断言:触发重启分支后,断言捕获到的重启 env 不含 QWEN_CODE_STARTUP_VERSION。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| await submitCommand.handler?.(authorized({}) as never); | ||
| expect(posted().body).toContain('(v0.21.4)'); |
There was a problem hiding this comment.
[Suggestion] This fallback test hardcodes today's packages/cli version: getCliVersion() is NOT mocked here (only gh and stdioHelpers are), so it resolves via readPackageUp to the live manifest (0.21.4), asserted literally. The test also never clears inherited CLI_VERSION, which getCliVersion() prefers. Reproduced: with CLI_VERSION=9.9.9 exported the test fails (footer becomes (v9.9.9)); Dockerfile sets ENV CLI_VERSION and scripts/dev.js/start.js export it to children, so wrapped/containerised runs hit this today. (Also flagged as a non-blocking note in the earlier triage comment — still unaddressed at this head.) — Failure scenario: the next release bump breaks this assertion with no regression, inviting a blind bump of the magic literal; afterwards the sibling test's not.toContain('(v0.21.4)') silently degrades into a tautology.
Fix: mock ../../utils/version.js with a sentinel (the pattern compose-review.test.ts:40-41 already uses) or derive the expectation from package.json at test time, and save/restore CLI_VERSION.
中文说明
[Suggestion] 该回退测试硬编码了当前 packages/cli 版本:此处未 mock getCliVersion()(只 mock 了 gh 与 stdioHelpers),它经 readPackageUp 解析到实时 manifest(0.21.4)并被字面断言。测试也未清理继承的 CLI_VERSION(而 getCliVersion() 优先读它)。已复现:导出 CLI_VERSION=9.9.9 时测试失败(footer 变为 (v9.9.9));Dockerfile 设置了 ENV CLI_VERSION,scripts/dev.js/start.js 也会将其导出给子进程,故容器化/包装运行今天就会踩中。失败场景:下次发版即无回归地弄红断言,诱导盲目改魔数;之后同级测试的 not.toContain('(v0.21.4)') 会悄悄退化为恒真。修复:用哨兵 mock version.js(参考 compose-review.test.ts:40-41),或运行时从 package.json 推导期望值,并保存/还原 CLI_VERSION。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| export function runSubmit( | ||
| args: SubmitArgs, | ||
| cliVersion = process.env['QWEN_CODE_STARTUP_VERSION'] || 'unknown', |
There was a problem hiding this comment.
[Suggestion] The "which version goes in the footer" policy is implemented at two depths with divergent fallbacks: this default parameter (env || 'unknown') and the handler (env || await getCliVersion()). The handler is the only production caller and always passes explicitly, so this default is unreachable in production and exercised only by tests. — Failure scenario: a future direct caller of the exported runSubmit that omits the argument silently gets (vunknown) in a public footer when the env var is un-stamped, even though getCliVersion() would resolve the real version; tests calling runSubmit(args) with no version also become sensitive to whatever ambient QWEN_CODE_STARTUP_VERSION the runner inherits.
Fix: keep version resolution at one depth — revert the default to 'unknown' (or make it required), keep the handler as sole resolver, and rewrite the 'uses the inherited startup version' test to go through submitCommand.handler.
中文说明
[Suggestion] "footer 用哪个版本"的策略被写在两个深度、且回退不一致:此默认参数(env || 'unknown')与 handler(env || await getCliVersion())。handler 是唯一生产调用方且总是显式传参,因此该默认值在生产中不可达,仅被测试触发。失败场景:未来某个直接调用导出的 runSubmit 且省略参数的调用方,在环境变量未固化时会在公开 footer 里悄悄得到 (vunknown),尽管 getCliVersion() 本可解析出真实版本;不带版本调用 runSubmit(args) 的测试也会受运行环境继承的 QWEN_CODE_STARTUP_VERSION 影响。修复:把版本解析收敛到一个深度——将默认值还原为 'unknown'(或改为必填),由 handler 独自解析,并把"使用继承启动版本"的测试改为经由 submitCommand.handler。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const cliVersion = | ||
| process.env['QWEN_CODE_STARTUP_VERSION'] || (await getCliVersion()); |
There was a problem hiding this comment.
[Suggestion] This handler line — the exact line this PR exists to add — has no test exercising it with QWEN_CODE_STARTUP_VERSION SET. The test named for this behaviour ('uses the inherited startup version…') actually calls runSubmit's dead default parameter; the only test driving submitCommand.handler deletes the variable first, so this env branch never evaluates. Mutation probe: reverting the handler to runSubmit(argv, await getCliVersion()) — i.e. undoing the PR's own central change — leaves all 47 tests green; a probe test fails against the mutation and passes against the PR code (48/48). — Failure scenario: a future revert of this env-preference line ships green and resurrects the exact regression this PR fixes — a session on v0.21.3 whose submit child resolves a newer build posts the bundled version in the footer instead of the session's startup version.
Fix: add one test that sets process.env['QWEN_CODE_STARTUP_VERSION'] (save/restore as siblings do) and drives submitCommand.handler, asserting posted().body carries the inherited version, not the package version.
中文说明
[Suggestion] 这行 handler 代码——正是本 PR 要新增的核心行——没有任何测试在设置了 QWEN_CODE_STARTUP_VERSION 的情况下验证它。名为该行为的测试("uses the inherited startup version…")实际调用的是 runSubmit 的死默认参数;唯一驱动 submitCommand.handler 的测试又先删除了该变量,因此这个 env 分支从未被求值。突变探针:把 handler 还原为 runSubmit(argv, await getCliVersion())(即撤销本 PR 的核心改动),全部 47 个测试仍绿;而一个探针测试对该突变失败、对 PR 代码通过(48/48)。失败场景:未来对这行 env 优先逻辑的还原会绿灯上线,复活本 PR 要修复的回归。修复:新增一个测试,设置 QWEN_CODE_STARTUP_VERSION(像同级测试那样保存/还原)并驱动 submitCommand.handler,断言 posted().body 携带继承的版本而非包版本。
— qwen3.8-max via Qwen Code /review (v0.21.3)
yiliang114
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
1 similar comment
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round 2 — all findings addressedCommit: The blocking CI failureThe Inline findings — all 11 resolved in the code
The "integration tests did not run" noteThat CHANGES_REQUESTED item is the reviewer's own coverage disclosure, not a ConflictNone ( VerificationCommands actually run and their results:
中文说明评审第 2 轮 —— 所有发现均已处理提交: 阻塞的 CI 失败
行内发现 —— 全部 11 条已在代码中解决
关于"集成测试未运行"的说明该 CHANGES_REQUESTED 条目是评审者自身的覆盖披露,不是代码缺陷。改动的行为 冲突无( 验证实际执行的命令及结果:
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
… review timeouts (#8431) The timeout externalization in #8460 replaced the hardcoded 300/240 values in qwen-code-pr-review.yml with the QWEN_REVIEW_JOB_TIMEOUT_MINUTES and QWEN_REVIEW_MAX_TIMEOUT_MINUTES repository variables but left scripts/tests/qwen-resolve-workflow.test.js asserting the old literals, so the full-profile Test job fails on any branch carrying that change. Update the three affected assertions to pin the externalized shape.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #8431Feedback triage
No review comments, inline comments, or issue comments were in this round's feedback; the failed check was the only item. No conflict handling was needed ( DiagnosisThe failing check was not caused by this PR's own diff. The branch merged Evidence:
Changes
Notes
Verification
中文说明Autofix 本轮总结 — PR #8431反馈分类
本轮反馈中没有审查评论、行内评论或 issue 评论；失败的检查是唯一条目。本轮无需处理冲突（ 诊断该失败检查并非由本 PR 自身的改动引起。分支合并了 证据：
改动
说明
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
…er-startup-version # Conflicts: # scripts/tests/qwen-resolve-workflow.test.js
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Autofix address-review summary — PR #8431 (round: conflict resolution)Feedback triageNo actionable feedback this round: Conflict resolution
Both sides were examined against the workflow they assert on (
Resolution: the conflicting file was taken as main's #8486 content, byte-identical to Feedback pointsNone received this round (reviews, inline comments, issue comments, and failed-checks sections were all empty). VerificationAll commands run in the post-resolution tree, before committing the merge:
中文说明Autofix address-review 摘要 — PR #8431（本轮：基础分支冲突解决）反馈分诊本轮没有需要处理的反馈： 冲突解决自本分支上次合并 main 之后， 两侧断言均对照其断言对象工作流文件（
解决方式：冲突文件采用 main 的 #8486 内容，与 反馈条目本轮未收到任何反馈（评审、行内评论、issue 评论与失败检查各节均为空）。 验证以下命令全部在解决冲突后的工作树中、提交合并之前实际执行：
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
| export const REVIEW_FOOTER_RE = | ||
| /\s*(?:_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; |
There was a problem hiding this comment.
[Critical] R1-1: The round-1 ReDoS still stands. Hoisting the leading \s* removed the whitespace-split ambiguity, but [^\n]* inside the repeated group can swallow subsequent footers on the same line, leaving a 2^(N−1) partition ambiguity that V8 enumerates when the trailing $ fails. Measured on the shipped regex (Node 22, same-line footers + trailing text — the "model looping" shape this module's own comment names): n=8 → 8.4 ms, n=16 → 332 ms, n=20 → 5.3 s (~2× per footer); extrapolation n=24 ≈ 85 s, n=30 ≈ 1.5 h. normalizeInlineComments runs this synchronously on model-authored bodies before posting, so one ~30-footer comment body hangs qwen review submit for minutes→hours at 100% CPU. The new regression test uses only 8 footers (~8 ms) and passes green while the exponential remains; newline-separated runs are linear, only the same-line fail-shape explodes. — Failure scenario: an inline comment body with ~20-30 forged footers joined on one line followed by any closing text hangs the submit process for hours before anything posts.
| export const REVIEW_FOOTER_RE = | |
| /\s*(?:_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; | |
| export const REVIEW_FOOTER_RE = | |
| /\s*(?:_— (?:(?! via Qwen Code \/review)[^\n])* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; |
Forbids a group iteration from spanning another footer's start; a verifier flip-probe measured n=60 at 6.5 ms with all strip semantics preserved (versioned/legacy, trailing runs, no-strip when text follows). Also raise the hang test's footer count (see the comment on submit.test.ts).
中文说明
[Critical] R1-1：第 1 轮的 ReDoS 仍然存在。把开头 \s* 提出重复分组只消除了空白切分歧义，但重复分组内的 [^\n]* 仍可吞掉同一行上后续的 footer，当结尾 $ 失败时留下 2^(N−1) 种切分供引擎枚举。对提交版正则实测（Node 22，单行 footer 串 + 尾部文本——本模块注释自己命名的"模型循环"形态）：n=8 → 8.4 ms，n=16 → 332 ms，n=20 → 5.3 s（每个 footer 约 2 倍）；外推 n=24 ≈ 85 秒，n=30 ≈ 1.5 小时。normalizeInlineComments 在发布前对模型生成的正文同步运行该正则，一个约 30 footer 的评论正文会让 qwen review submit 以 100% CPU 挂死数分钟到数小时。新的回归测试只用 8 个 footer（约 8 ms），在指数行为仍在的情况下绿灯通过；换行分隔的串是线性的，只有单行失败形态会爆炸。失败场景：约 20-30 个伪造 footer 单行相连、后接任意收尾文本的正文，会让发布前挂死数小时。建议修复禁止一次分组迭代跨越另一个 footer 的起点；验证者的翻转探针实测 n=60 仅 6.5 ms 且保留全部剥离语义。同时请提高挂死测试的 footer 数量（见 submit.test.ts 上的评论）。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| // Same pin as `submit`: the startup stamp, not a version resolved at | ||
| // compose time — a shared runner can rewrite the install mid-session. | ||
| process.env['QWEN_CODE_STARTUP_VERSION'] || (await getCliVersion()), |
There was a problem hiding this comment.
[Critical] This env pin turns the PRE-EXISTING test 'reads --input, counts the drafted comments, and writes the result JSON to --out' (compose-review.test.ts:893-921, untouched by this diff) into an environment-dependent test: its assertion written.body.endsWith('… (v0.21.2)_') (the mocked getCliVersion() value) fails whenever QWEN_CODE_STARTUP_VERSION is present in the ambient environment, because the handler now prefers the ambient stamp. A/B reproduced on this checkout: QWEN_CODE_STARTUP_VERSION=9.9.9 npx vitest run src/commands/review/compose-review.test.ts -t 'reads --input...' FAILS at compose-review.test.ts:920; the identical command passes with the variable unset. Once this PR ships, scripts/cli-entry.js stamps the variable into every qwen session's env and every child inherits it — this repo's standard dogfooding / review-agent / autofix loops all run vitest under stamped sessions. Every new test in this diff saves/deletes/restores the variable; this pre-existing one does not. — Failure scenario: after release, this repo's own dogfooding/CI-like loops (any vitest run under a qwen session) fail on this test.
Fix: save/delete/restore process.env['QWEN_CODE_STARTUP_VERSION'] in that test (or in a suite-level beforeEach), the same pattern the new tests use.
中文说明
[Critical] 该 env 固化使既有测试 'reads --input, counts the drafted comments, and writes the result JSON to --out'（compose-review.test.ts:893-921，本 diff 未触碰）变成环境依赖：其断言 written.body.endsWith('… (v0.21.2)_')（mock 的 getCliVersion() 值）在环境中存在 QWEN_CODE_STARTUP_VERSION 时失败，因为 handler 现在优先读取环境固化值。已在本 checkout A/B 复现：QWEN_CODE_STARTUP_VERSION=9.9.9 npx vitest run … -t 'reads --input...' 在 compose-review.test.ts:920 失败；unset 时相同命令通过。本 PR 发布后，scripts/cli-entry.js 会为每个 qwen 会话固化该变量且子进程继承——本仓库标准的 dogfooding / review-agent / autofix 循环都在带固化值的会话下跑 vitest。diff 中所有新测试都保存/删除/恢复该变量，唯独这个既有测试没有。失败场景：发布后本仓库自身的 dogfooding/类 CI 循环（任何在 qwen 会话下运行的 vitest）在该测试上失败。修复：在该测试（或套件级 beforeEach）中保存/删除/恢复 process.env['QWEN_CODE_STARTUP_VERSION']，与新测试使用同一模式。
— qwen3.8-max via Qwen Code /review (v0.21.3)
| * on the model-authored bodies this regex strips. | ||
| */ | ||
| export const REVIEW_FOOTER_RE = | ||
| /\s*(?:_— [^\n]* via Qwen Code \/review(?: \(v[^\n)]*\))?_\s*)+$/; |
There was a problem hiding this comment.
[Suggestion] A forged trailing footer missing its closing _ is not stripped, so it posts as a duplicate attribution line under the canonical footer. Measured on the real module: body 'finding\n\n_— m via Qwen Code /review (v1.0)' (a truncated forged footer — a model cutting off the last character) is left entirely unchanged by the strip; normalizeInlineComments then appends the real footer, so the posted comment carries 2 attribution lines. Same for the version-less shape and when blank lines follow the unclosed footer. The strip exists so 'submit replaces any model-provided or duplicate inline footer with one canonical footer' (PR description); this shape escapes it. — Concrete cost: duplicate attribution lines post whenever a looping model truncates its forged footer. Fix: make the closing _ optional for the final footer, e.g. (?: \(v[^\n)]*\))?_?\s*$ — flip-checked: strips both unclosed shapes while still stripping well-formed footers byte-identically; or document the gap as accepted behavior.
中文说明
[Suggestion] 缺少结尾 _ 的伪造尾部 footer 不会被剥离，从而作为重复归属行贴在规范 footer 下方发布。对真实模块实测：正文 'finding\n\n_— m via Qwen Code /review (v1.0)'（被截断的伪造 footer——模型漏掉最后一个字符）完全不被剥离；normalizeInlineComments 随后追加真实 footer，发布的评论出现 2 行归属。无版本形态及未闭合 footer 后跟空行时同样。剥离的存在就是为了"submit 把模型提供或重复的 inline footer 替换为一个规范 footer"（PR 描述）；该形态逃过了它。具体代价：循环模型截断其伪造 footer 时即发布重复归属行。修复：让最后一个 footer 的结尾 _ 可选，例如 (?: \(v[^\n)]*\))?_?\s*$——已翻转验证：两种未闭合形态均被剥离，良构 footer 的剥离仍逐字节一致；或将该缺口文档化为已接受的行为。
— qwen3.8-max via Qwen Code /review (v0.21.3)
The strip regex kept a 2^(N-1) partition ambiguity for same-line footer runs (measured 5.3 s at n=20) and missed footers truncated before their closing `_`; forged footers also survived on the body channel through `bodyCriticals`, and the values interpolated into the footer were not shape-validated. Guard the repeated group so an iteration cannot span another footer's start, make the final `_` optional, strip body Criticals per entry, refuse footer-forging model ids and non-version stamps, refuse non-object comment entries, pin the CLI-glue test suite against an ambient startup stamp, and cross-assert the LGTM filter regex against the footer builder.
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Addressed — all findings resolved in code (8/8)One commit ( Critical
Suggestion
No findings declined, deferred, or escalated. Verification
No integration run was needed — the touched behavior is fully exercised by the unit suites above, not only through the bundled CLI. 中文说明已处理 —— 全部发现已在代码中解决（8/8）单个提交（ Critical（阻断）
Suggestion（建议）
无拒绝、推迟或升级维护者决策的发现。 验证
无需运行集成测试 —— 上述改动的行为已完全由单元测试套件覆盖，并非只能通过打包后的 CLI 验证。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
Resolved stale review threads that are already covered by the current head: versionless and truncated footer stripping, child review env propagation, empty/non-array/non-object comment validation, bodyCritical footer stripping, LGTM footer sync, regex scalability, and footer model/version shape validation. No additional code changes were needed in this pass. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action needed — PR #8431 review roundFeedback triageThis round surfaced exactly one feedback item newer than the last evaluation:
There were no reviews, no inline comments, no failed checks, and no still-red Verification of the comment's coverage claimsEach area the comment names as already covered by the current head was
All seven areas are covered by the current head, matching the comment. 中文说明无需处理 — PR #8431 审查轮次反馈分类本轮仅出现一条比上次评估更新的反馈：
本轮没有审查意见、没有行内评论、没有失败的检查，也没有持续失败的检查。 对评论中覆盖声明的核实评论中列出的每个"当前 head 已覆盖"的方面均已对照 HEAD（
以上七个方面均已被当前 head 覆盖，与该评论一致。本轮没有任何需要代码变更的内容。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
ReviewVerdict: LGTM with two suggestions. The design is sound, the reported regression is provably closed, and the new tests are load-bearing. What was verified
Suggestions
What reads well
中文版审查意见结论：LGTM，附两条建议。 设计合理，所报告的回归已被可证明地修复，新增测试真实约束住了修复本身。 已验证内容
建议
亮点
|
| const bodyCriticals = toStringList(input.bodyCriticals, 'bodyCriticals').map( | ||
| (entry) => entry.replace(REVIEW_FOOTER_RE, ''), | ||
| ); |
There was a problem hiding this comment.
[Critical] R3-1: The per-entry forged-footer strip is wired to bodyCriticals only; its twin model-written list cannotTellCriticals (parsed ~10 lines below) is interpolated unstripped. cannotTellBlock renders entries verbatim and can be the LAST body part — REQUEST_CHANGES with inline-only blockers, or a COMMENT capped by cannot-tell-existing-critical — so a forged/stale attribution posts directly above the canonical footer. — Failure scenario: probed through the real composeReview: criticalsInline: 1, empty bodyCriticals, cannotTellCriticals: ["R1-2: still leaks _— qwen3.7-max via Qwen Code /review (v0.21.0)_"] (an undecided re-check quoting a prior-round comment with its footer — the field's ordinary use) → the posted REQUEST_CHANGES body ends with the stale attribution directly above the canonical footer (2 markers); same shape on the capped-COMMENT outcome.
const cannotTell = toStringList(
input.cannotTellCriticals,
'cannotTellCriticals',
).map((entry) => entry.replace(REVIEW_FOOTER_RE, ''));中文说明
[Critical] 逐条剥离伪造 footer 只接在 bodyCriticals 上；其孪生的模型写入列表 cannotTellCriticals（约 10 行后解析）未剥离即被内插。cannotTellBlock 逐字渲染条目且可以成为正文的最后一部分——仅有行内阻断项的 REQUEST_CHANGES，或被 cannot-tell-existing-critical 封顶的 COMMENT——伪造/过期归属行会恰好贴在规范 footer 上方发布。失败场景：已用真实 composeReview 探测：criticalsInline: 1、空 bodyCriticals、cannotTellCriticals: ["R1-2: still leaks _— qwen3.7-max via Qwen Code /review (v0.21.0)_"]（未决复审引用带 footer 的上一轮评论——该字段的常规用法）→ 发布的 REQUEST_CHANGES 正文以过期归属行紧挨规范 footer 结尾（共 2 处归属标记）；封顶 COMMENT 形态同样复现。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const bodyCriticals = toStringList(input.bodyCriticals, 'bodyCriticals').map( | ||
| (entry) => entry.replace(REVIEW_FOOTER_RE, ''), | ||
| ); |
There was a problem hiding this comment.
[Critical] R3-2: The strip runs in composeReviewBody's render path only; ledgerMarkerFor (~line 311) independently re-reads the RAW bodyCriticals for buildLedger, so a single-line entry carrying a forged footer puts it into the posted ledger marker's title — the forge survives its own stripping and propagates to the next round. — Failure scenario: probed: bodyCriticals: ['**[Critical]** whole-PR blocker _— forged via Qwen Code /review (v0.21.4)_'] → the visible body contains no forged text (stripped), but the posted ledger marker carries title: "**[Critical]** whole-PR blocker _— forged via Qwen Code /review (v0.21.4)_". Next round pr-context recovers the marker and renders it as a previous-round Critical owed a ruling — a cannot-tell answer caps that round. Before this PR body and ledger agreed; the strip newly diverges the posted record from the posted rendering. — Fix: compute the stripped list once and feed both composeReviewBody and ledgerMarkerFor (or strip inside ledgerMarkerFor), so the ledger records what was actually posted.
中文说明
[Critical] 剥离只发生在 composeReviewBody 的渲染路径上；ledgerMarkerFor（约 311 行）为 buildLedger 独立地重新读取未剥离的 bodyCriticals，因此携带伪造 footer 的单行条目会进入已发布 ledger 标记的标题——伪造内容在自己的剥离机制下存活，并传播到下一轮。失败场景：已探测：bodyCriticals: ['**[Critical]** whole-PR blocker _— forged via Qwen Code /review (v0.21.4)_'] → 可见正文不含伪造文本（已剥离），但发布的 ledger 标记携带 title: "**[Critical]** whole-PR blocker _— forged via Qwen Code /review (v0.21.4)_"。下一轮 pr-context 恢复该标记并把它渲染为上一条"必须在本轮裁决"的 Critical——无法裁决的回答会封顶该轮。本 PR 之前正文与 ledger 一致；剥离使"已发布的记录"与"已发布的渲染"首次分叉。修复：只计算一次剥离后的列表，同时喂给 composeReviewBody 与 ledgerMarkerFor（或在 ledgerMarkerFor 内剥离），使 ledger 记录实际发布的内容。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const bodyCriticals = toStringList(input.bodyCriticals, 'bodyCriticals').map( | ||
| (entry) => entry.replace(REVIEW_FOOTER_RE, ''), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-4 / R3-15: Two residual coverage gaps at this strip site. (1) R3-4: a bodyCriticals entry consisting solely of a forged footer strips to '' but is still counted in C and rendered as a contentless **[Critical]** bullet (it also feeds criticalsNeedingVerify and the ledger). Probed: bodyCriticals: ['_— forged via Qwen Code /review (v0.21.4)_'] posts REQUEST_CHANGES with a bare marker and no content, C=1 driving the event. (2) R3-15: the sibling model-written lists unreviewedDimensions and uncoverableChunks are interpolated unstripped — probed: such an entry posts with 2 via Qwen Code /review markers embedded in the disclosure sentence (generated prose always follows these entries, so they cannot end the body — duplication-only harm). — Fix: .filter((entry) => entry.trim() !== '') after this .map (or a structured refusal), and extend the same per-entry strip to the two sibling lists (probe-verified: markers drop to 1); alternatively strip the assembled body once in render before the footer is appended.
中文说明
[Suggestion] 该剥离位置还有两处残余覆盖缺口。(1) 仅由伪造 footer 构成的 bodyCriticals 条目被剥离为 '' 后仍计入 C，并渲染为无内容的 **[Critical]** 空项（还进入 criticalsNeedingVerify 与 ledger）。已探测：bodyCriticals: ['_— forged via Qwen Code /review (v0.21.4)_'] 发布出只有空标记、无内容的 REQUEST_CHANGES，C=1 驱动事件。(2) 孪生的模型写入列表 unreviewedDimensions 与 uncoverableChunks 未剥离即被内插——已探测：此类条目发布后正文含 2 处 via Qwen Code /review 标记，嵌在披露句中（这些条目后总有生成的文案，不会收尾正文——仅重复之害）。修复：在此 .map 后加 .filter((entry) => entry.trim() !== '')（或改为结构化拒绝），并把同样的逐条剥离扩展到两个孪生列表（已探测验证：标记数降为 1）；也可在 render 中于追加 footer 前对组装后的正文整体剥离一次。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
Split disposition. R3-4 is resolved (by the previous commit fix(cli): strip review footers before ledger carryover): the strip's .filter((entry) => entry.trim() !== '') runs immediately after toStringList in both composeReviewBody and ledgerMarkerFor — before the C count, the ledger, and criticalsNeedingVerify — so an entry that strips to '' is neither counted nor rendered. R3-15 part (2) is deferred to a follow-up: the probe established duplication-only harm — generated prose always follows unreviewedDimensions/uncoverableChunks entries, so they cannot end the body or carry the last attribution line, and this round stayed scoped to the red CI test plus the maintainer's asks.
中文说明
拆分处置。R3-4 已解决（由上一次提交 fix(cli): strip review footers before ledger carryover 完成）：剥离后的 .filter((entry) => entry.trim() !== '') 在 composeReviewBody 与 ledgerMarkerFor 中都紧跟 toStringList 运行——先于 C 计数、ledger 与 criticalsNeedingVerify——因此剥离为 '' 的条目既不计入也不渲染。R3-15 第 (2) 部分暂缓至后续：探测已确认仅重复之害——unreviewedDimensions/uncoverableChunks 条目后总有生成文案，不会收尾正文、不会携带最后一行归属，且本轮范围保持在红色 CI 测试与维护者要求。
|
Addressed the remaining Critical footer carryover issue by stripping model-written review footers before rendering cannot-tell Criticals and before writing body Criticals into the posted ledger marker. Verified: Intentionally not changed: the remaining R3 Suggestions would add more regex policy, validation, presubmit sharing, or extra handler-level coverage. I am leaving them as follow-up instead of expanding this closeout pass beyond the Critical fixes. |
Review —
|
| footers | body length | String.replace |
|---|---|---|
| 64 | 3,965 | 9 ms |
| 256 | 15,869 | 150 ms |
| 1024 | 63,485 | 2,462 ms |
4× length → ~16× time. Also '_— ' + 'a'.repeat(40) repeated with no marker on the line: 70 KB → 336 ms. GitHub's comment-body limit is 65,536 chars, so a model looping on one comment can stall submit for seconds inside a .map that runs before anything posts.
No exponential blowup — the invariants in the doc comment did their job — but the unbounded [^\n] is what leaves the quadratic term. CANONICAL_LGTM_RE right next door already bounds its equivalent ([^\n]{0,200}); mirroring that here would cap it and keep the three copies structurally alike. At minimum, correct the "linear" claim to "polynomial, not exponential".
Nits / follow-ups
4. SKILL.md still tells the model to write the footer. packages/core/src/skills/bundled/review/SKILL.md:1016/1021/1060 instructs _— YOUR_MODEL_ID via Qwen Code /review (v{{cliVersion}})_ on every comment and states "the footer must be present". {{cliVersion}} has no substitution anywhere in the codebase — the model fills it from the session banner, which is precisely the late, mutable source this PR is eliminating. Now that submit strips and re-appends unconditionally, that instruction is dead weight the model pays tokens for on every finding. Worth telling the skill that submit owns the footer (the autofix-keying sentence stays true — more reliably so), and updating docs/users/features/code-review.md:186 to match.
5. Asymmetric modelId validation. composeReviewBody rejects a footer-unsafe modelId, but normalizeInlineComments — which runs first — interpolates payload.state.modelId verbatim with no check. Nothing leaks, because compose() throws immediately after and runSubmit refuses before any gh call (I confirmed this). Still, the guard belongs at the shared builder (reviewFooter) rather than at one of its two call sites.
6. Design alternative, for the record. planPath is CLI-written by fetch-pr and already rides into both compose-review and submit via state. Stamping the startup version there would keep it process-local instead of exporting a variable into every child the session spawns (shell tool calls, MCP servers) and would leave scripts/cli-entry.js untouched. The env var does cover local reviews with no plan, so this is a trade-off rather than a defect — but it's worth a line in the PR body given the entry script is the riskiest file here.
7. Non-issue, noted: the payload rebuild makes comments: [] always present even when the field was absent. post.comments already did ?? [], so nothing observable changes.
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Review —
|
| n footers | body len | String.replace |
|---|---|---|
| 64 | 4.3 KB | 10.7 ms |
| 128 | 8.6 KB | 40.1 ms |
| 256 | 17 KB | 160 ms |
| 512 | 34 KB | 640 ms |
| 1024 | 69 KB | 2579 ms |
Clean 4× per doubling — O(n²). A second shape reaches it without any complete footer at all: '_— '.repeat(16000) + 'tail' (48 KB, comfortably under GitHub's 65536-char body limit) → 2.2 s.
Failure scenario: a model looping on one comment emits a ~60 KB body of near-footer text → runSubmit burns 2–3 s of CPU inside a single .replace before anything posts, once per such comment. Not a hang and not a security boundary, but the test's stated guarantee is wrong and a future maintainer will trust it.
Suggested: correct the comment to what the shape actually buys (no exponential blowup), and/or bound the work — the footer run can only sit at the tail, so applying the regex to the last few KB, or refusing an over-limit body in structuralProblems, makes the cost independent of body size.
2. [Suggestion] .filter(entry => entry.trim() !== '') can silently delete a blocker and flip the verdict
compose-review.ts:341-342, 351-352. Both filtered lists feed verdict arithmetic:
const c = criticalsInline + bodyCriticals.length— withcriticalsInline === 0and a singlebodyCriticalsentry that strips to empty (an entry that is only a forged footer, or the pre-existing""case, which was previously counted),cgoes 1 → 0 and the event goesREQUEST_CHANGES→APPROVE.if (cannotTell.length > 0) cappedBy.push('cannot-tell-existing-critical')— dropping the last entry removes the cap, turning a cappedCOMMENTintoAPPROVE.
This is the one place in the file that silently normalizes model-written input; everything around it refuses (toStringList, toBool, toCount, and the new isFooterSafeModelId check 350 lines below). Throwing on an entry that strips to nothing would keep the guarantee that normalization can never remove a finding.
3. [Suggestion] normalizeInlineComments interpolates an unvalidated modelId
submit.ts:131-149 builds reviewFooter(modelId, cliVersion) from payload.state?.modelId before compose() runs, so the isFooterSafeModelId gate added in compose-review.ts:697 has not fired yet. Nothing posts today — compose throws first — so this is latent, and the only thing keeping a newline/marker-bearing modelId out of every inline body is statement order at submit.ts:450. Applying isFooterSafeModelId inside normalizeInlineComments (bail out, same as the blank-modelId early return) makes the guarantee local instead of order-dependent.
4. [Suggestion] The new env var is undocumented
QWEN_CODE_STARTUP_VERSION is now set on every qwen invocation and inherited by every shell subprocess the model spawns. docs/users/configuration/settings.md carries an exhaustive QWEN_CODE_* table (:731+) and this isn't in it. Worth noting there that first-writer-wins means an exported value in a user's shell profile pins every future footer to it — footerVersion validates shape, not plausibility, so a stale export is silently honoured forever. docs/users/features/code-review.md:177 also still describes the per-comment footer as something the model writes.
5. [Suggestion] The upstream version source is left in place
packages/cli/src/services/BundledSkillLoader.ts:111 still resolves {{cliVersion}} from config.getCliVersion(), and review/SKILL.md:1022,1027,1066 still instruct the model to author _— YOUR_MODEL_ID via Qwen Code /review (v{{cliVersion}})_. Harmless now that submit overwrites it, but it is the second version source this PR set out to eliminate, and it can still disagree with what posts. Pinning it to the same footerVersion(process.env['QWEN_CODE_STARTUP_VERSION']), or dropping the version from the template since submit owns it, would close the boundary.
Separately, SKILL.md:1066 claims .github/workflows/qwen-autofix.yml keys off both the prefix and the footer. It doesn't — every gate there is contains("**[Critical]**") (qwen-autofix.yml:3072-3234). That sentence was already stale and is now doubly so.
6. Nits
- Ownership is tail-only. The
$anchor means a footer run followed by ordinary text is not stripped —submit.test.ts:551-575asserts exactly that (64 forged footers post intact, plus the canonical one). Fine as a trade-off, but thereview-footer.tsheader reads as full ownership; a sentence there would save the next reader the discovery. - Content loss. A legitimate comment whose body ends with a literal footer example — e.g. a review of this very file quoting the format — is silently eaten. Bounded, since
submitre-appends one. comments: nullchanged meaning. Previously coerced to[]by?? [];structuralProblemsnow hard-refuses it (null !== undefined && !Array.isArray(null)). Probably intended, but it isn't called out in the PR body and has no test.- Blank-line accumulation.
`${body.replace(...)}\n\n${footer}`— when nothing is stripped and the body already ends in\n, the result carries 3+ newlines.trimEnd()before joining.
Coverage / CI
Test coverage for the new paths is genuinely good — handler-driven (not just runSubmit(args, version)), so reverting the handler to a bare getCliVersion() does redden. Two gaps: no test for the verdict-flip in §2, and none for the comments: null change in §6. The PR body notes npm run build / npm run typecheck were blocked locally by unrelated baseline failures — worth confirming the CI legs are green before merge, since pr-context.ts gained a new export and submit.ts a new import graph.
中文说明
总体。 本 PR 做两件事：(1) scripts/cli-entry.js 在启动时打一次 QWEN_CODE_STARTUP_VERSION（first-writer-wins，managed-update relaunch 时清除），submit / compose-review 优先用它而不是晚到的 getCliVersion()；(2) 新增 lib/review-footer.ts 统一 footer 字符串、strip 正则与 model-id/版本校验，submit 改为重写每条 inline comment 的 footer。
我核实了传播链是通的：scripts/cli-entry.js 就是发布的 bin，review workflow 走全局安装的 qwen（qwen-code-pr-review.yml:494-499），且 sanitizeChildEnv 是黑名单，所以这个变量能跟着 shell 子进程进到 qwen review submit。机制成立。
做得好的地方：lib/review-footer.ts 放置位置与 inline-counts.ts 一致；真正有价值的是把 builder、strip 正则、CANONICAL_LGTM_RE 三份形状绑在一起的同步测试；footerVersion() 在插值前校验环境变量是正确姿势；relaunch 清除 stamp 的测试按调用时刻快照 opts.env，避开了按引用记录的陷阱；structuralProblems 新增的两个检查把新 .map 会抛的裸 TypeError 变成 re-compose 循环能解析的结构化拒绝。
-
[Suggestion]strip 正则是二次复杂度，测试注释断言的不变量不成立。submit.test.ts:545写“The match must stay linear”，但n=64只能排除指数，不能证明线性。实测（Node 22，测试自身的形状）：n=64 → 10.7ms，128 → 40ms，256 → 160ms，512 → 640ms，1024（69KB）→ 2579ms，每翻倍 4×，即 O(n²)。另一种形状连一个完整 footer 都不需要：'_— '.repeat(16000)+'tail'（48KB，远低于 GitHub 65536 上限）→ 2.2s。失败场景：模型在同一条 comment 上打转输出约 60KB 近似 footer 文本，runSubmit会在单次.replace里烧掉 2–3 秒 CPU。建议：把注释改成它实际保证的东西（不发生指数爆炸），并/或限制扫描范围——footer 只可能在尾部，对最后几 KB 做匹配即可让开销与正文长度解耦。 -
[Suggestion].filter(entry => entry.trim() !== '')会静默删掉 blocker 并翻转 verdict。compose-review.ts:341-342, 351-352：c = criticalsInline + bodyCriticals.length，当criticalsInline === 0且唯一一条bodyCriticals被 strip 成空（纯 forged footer，或此前会被计数的""），c由 1 变 0，事件从REQUEST_CHANGES变APPROVE；cannotTell同理会让cannot-tell-existing-criticalcap 消失。这是本文件里唯一一处对模型输入做静默归一化的地方，周围全是抛错（toStringList/toBool/toCount/ 新增的isFooterSafeModelId）。建议改为抛错，保证归一化永远不会删掉一个 finding。 -
[Suggestion]normalizeInlineComments用了未校验的modelId。submit.ts:131-149在compose()之前就用payload.state?.modelId构造 footer，而isFooterSafeModelId（compose-review.ts:697）此时还没跑。今天不会真的发出去（compose 先抛），但唯一的保障是submit.ts:450的语句顺序。建议在normalizeInlineComments内部也做一次校验。 -
[Suggestion]新环境变量没有文档。QWEN_CODE_STARTUP_VERSION现在每次qwen调用都会设置并被所有 shell 子进程继承，但不在docs/users/configuration/settings.md:731+的QWEN_CODE_*表里。也应说明 first-writer-wins 的后果：用户 shell profile 里导出的值会被永久沿用，footerVersion只校验形状不校验合理性。docs/users/features/code-review.md:177仍把 footer 描述成模型写的。 -
[Suggestion]上游版本来源没有收口。BundledSkillLoader.ts:111仍用config.getCliVersion()替换{{cliVersion}}，SKILL.md:1022,1027,1066仍让模型自己写带版本的 footer。现在无害（submit 会覆盖），但这正是本 PR 想消除的第二个版本来源。另外SKILL.md:1066声称qwen-autofix.yml同时依赖 prefix 和 footer——实际只依赖contains("**[Critical]**")（qwen-autofix.yml:3072-3234），这句话已经过时。 -
小问题：
$锚点意味着 footer 后面跟正文时不会被 strip（submit.test.ts:551-575正是这么断言的），review-footer.ts的头注读起来像“完全拥有”，建议补一句；正文以 footer 字面例子结尾的合法评论会被静默吃掉（评审本文件本身就会撞上）；comments: null从被?? []吞掉变成硬拒绝，PR 描述未提及也无测试；`${body.replace(...)}\n\n${footer}`在正文已以\n结尾时会产生 3 个以上换行，建议先trimEnd()。
覆盖率 / CI：新路径的测试质量不错（走 handler 而非直接 runSubmit(args, version)，把 handler 改回裸 getCliVersion() 确实会红）。两处缺口：§2 的 verdict 翻转无测试，§6 的 comments: null 变更无测试。PR 描述提到本地 npm run build / npm run typecheck 被无关的基线失败挡住——合并前请确认 CI 绿，pr-context.ts 新增了导出、submit.ts 新增了导入。
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 59 passed · 4 failed · 63 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：59 通过 · 4 失败 · 63 总计 Verification reportPR #8431 Deep Verification — fix(cli): preserve Qwen Review startup version in footersVerdict: Verified head: 中文摘要结论：
Central claim and A/B proofCentral claim: Secondary claims: (1) the startup version survives through the production entry wrapper ( A/B method: identical scenario driven through Witness:
16/16 assertions: base reproduces the incident shape exactly (forged footer posts; summary stamped from the late mutable source even though the startup stamp said 0.21.3); head owns the footer on both channels, falls back to the resolved version only when no stamp exists, preserves severity prefixes, and posts to the exact same endpoint with the same computed event ( Channel matrix (sibling sweep)Witness:
Startup-version chain (real spawns, no mocks)Witness:
Findings1. Strip regex: exponential class eliminated, quadratic residual remains on the no-match shape (Suggestion, bounded)The PR's guard fixed what it claims to fix: with it, a matching run of 1,488 forged footers at the 65,536-char GitHub body cap strips in 0.58 ms, and removing the guard (mutation M4) makes the suite's own n=64 case hang past a 120 s cap — an exponential regex blocks the event loop so thoroughly vitest's 5 s timer can never fire. The fix direction is proven correct. The residual: on the no-match shape — a run of forged footers followed by ordinary text, which
Bisected inside the platform bound (witness What holds and what does not: this is not exponential any more; it is not a correctness or security problem (output identical on every rung; the input is model-authored, not attacker-injected, and the stall is bounded by the platform's body cap at ~2 s in a one-shot command). But the PR's own regression pin cannot see it: This is the reason the verdict is 2. Two bounded mid-sentence survival channels (informational — design-consistent)
Not covered
MethodologyEnvironment: the CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
@qwen-code-review-bot approve if no blockers |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
LGTM — approving per request, no blockers found at head 7a39909.
- All Critical findings from the review rounds are resolved and spot-checked: the ReDoS fix (guarded repeated group, measured linear), the forged-footer strip on
bodyCriticals/cannotTellCriticals, and the ledger-marker carryover strip (722a818a, each with a regression test). - Code CI is green on the current head (ubuntu tests, desktop shell, e2e smoke); the only in-flight checks are the automated review/triage bot jobs.
- Remaining R3 items are Suggestion-level and explicitly deferred as follow-ups — non-blocking.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Fix the failing ledger-marker test by matching its fixture to the shape production produces (bodyCriticals ride without a severity marker; the renderer adds it), stamp only a real package version at startup so a literal 'unknown' cannot suppress the CLI's own version fallback, and bound the footer strip's tokens to the caps CANONICAL_LGTM_RE already applies so a near-cap pathological body cannot cost seconds of CPU. The modelId guard moves into the shared footer builder, the resolved-version fallback goes through the same shape gate as the startup stamp, and the review skill no longer asks the model to write footers that submit strips and re-appends anyway.
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8431This round fixes the red CI Feedback points and dispositions@wenshao — review (Blocking / Should fix / Nits)
@wenshao — earlier LGTM review (suggestions 1–3) — all covered above: suggestion 1 = Should fix #2; suggestion 2 = the version clause's closing paren is now optional ( Automated reviewer (round 3) — inline findings
@yiliang114 — previous round's comment — the Critical fixes it describes (R3-1/R3-2) were re-verified and stand; this round repairs the failing test that accompanied them. Files changed
No conflicts ( VerificationCommands actually run on this checkout, after the fix commit:
中文说明本轮总结 — PR #8431本轮修复了红色 CI 反馈点与处置@wenshao — 审查（Blocking / Should fix / Nits）
@wenshao — 较早的 LGTM 审查（建议 1–3） — 均已在上述覆盖：建议 1 = Should fix #2；建议 2 = 版本子句的右括号现为可选（ 自动审查器（第 3 轮）— 行内发现
@yiliang114 — 上一轮评论 — 其描述的 Critical 修复（R3-1/R3-2）已重新验证并成立；本轮修复了伴随它们的失败测试。 变更文件
无冲突（ 验证修复提交后在本 checkout 上实际运行的命令：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.21.6. |
5 participants
What this PR does
This PR makes
qwen review submitthe deterministic owner of Qwen Review inline-comment footers and preserves the startup CLI version through the production entry wrapper, so both inline comments and the summary review body report the version of the process that started the review.Why it's needed
PR #8410's review job started on 2026-08-03 at 02:54:04Z and its Install step reported Qwen Code v0.21.3; the stream-json initialization also reported
qwen_code_version=0.21.3, but the inline comment posted at 05:22Z ended with v0.21.4. PR #8417 shows the same mismatch in its review job and comment. The footer was being produced from a late, mutable version source and inline footers were accepted from model-authored content. This is metadata drift, not a mid-session upgrade.The fix keeps the version captured by the startup process in a dedicated
QWEN_CODE_STARTUP_VERSIONenvironment variable, makes the submit layer replace any model-provided or duplicate inline footer with one canonical footer, and leaves the existing footer marker and severity-prefix contracts unchanged. The dedicated variable is intentionally notCLI_VERSIONbecause the bundle build replacesprocess.env.CLI_VERSIONwith a compile-time package version. Issues #8343 and #8373 address runner-global installation drift and do not make already-created Review-session metadata authoritative; this PR therefore stays scoped to the footer ownership boundary.Reviewer Test Plan
How to verify
Review the focused regression test with a simulated running version of v0.21.3 and forged/late v0.21.4 inline footers: the posted summary and inline comment each contain exactly one v0.21.3 footer, contain no v0.21.4 footer, and retain the
[Suggestion]severity prefix. Review the production entry tests: an inheritedQWEN_CODE_STARTUP_VERSION=0.21.3remains unchanged even when the wrapper package fixture reports v0.0.0-test, while an unset variable is initialized from the package metadata.Evidence (Before & After)
N/A — non-UI behavior.
Tested on
Environment (optional)
Focused Vitest tests and ESLint ran locally on macOS with Node.js 22.22.0.
npm run buildwas attempted but the existing CLI build fails on unrelated Ink type/API errors such as missingReadonlyFrameandScreenSelectionexports;npm run typechecklikewise encounters pre-existing missing generated/dist outputs and unrelated workspace errors.Risk & Scope
QWEN_CODE_STARTUP_VERSIONinitializes from its own package metadata.Linked Issues
Related to #8343 and #8373; those issues cover runner installation drift, while this change makes Review footer metadata deterministic at submission.
中文说明
本 PR 做了什么
本 PR 让
qwen review submit成为 Qwen Review inline comment footer 的确定性拥有者，并通过生产入口 wrapper 保留启动时的 CLI 版本，使 inline comment 与 summary review body 都报告启动该 Review 的进程版本。为什么需要它
PR #8410 的 review job 于 2026-08-03 02:54:04Z 启动，Install step 报告 Qwen Code v0.21.3；stream-json 初始化同样报告
qwen_code_version=0.21.3，但 05:22Z 发布的 inline comment footer 却是 v0.21.4。PR #8417 也出现了相同的 v0.21.3 job 与 v0.21.4 comment 不一致，见其 review job 和 comment。这是 footer 使用了晚到且可变的版本来源，并且提交层接受了模型生成的 inline footer 导致的元数据漂移，不是进程中途升级。修复会将启动进程捕获的版本保存在专用的
QWEN_CODE_STARTUP_VERSION环境变量中，让提交层把模型提供或重复的 inline footer 替换为一个规范 footer，同时保持现有 footer marker 与 severity prefix 契约不变。该变量不能改用CLI_VERSION，因为 bundle 构建会把process.env.CLI_VERSION替换为编译时 package 版本。#8343 和 #8373 处理的是 runner 全局安装漂移，并不能让已经创建的 Review session 元数据具备权威性；因此本 PR 只修改 footer 所有权边界，不扩大到 runner 安装策略。Reviewer Test Plan
如何验证
查看聚焦回归测试：模拟运行版本为 v0.21.3，并提供伪造或晚到的 v0.21.4 inline footer；最终发布的 summary 与 inline comment 都应恰好包含一个 v0.21.3 footer，不包含 v0.21.4，并保留
[Suggestion]severity prefix。查看生产入口测试：当继承QWEN_CODE_STARTUP_VERSION=0.21.3而 wrapper package fixture 报告 v0.0.0-test 时，版本仍保持不变；变量未设置时才从 package metadata 初始化。证据（Before & After）
不适用——非 UI 行为。
测试平台
环境（可选）
本地 macOS、Node.js 22.22.0 上运行了聚焦 Vitest 与 ESLint。已尝试
npm run build，但当前 CLI 构建被与本 diff 无关的既有 Ink 类型/API 错误阻塞，例如缺少ReadonlyFrame与ScreenSelection导出；npm run typecheck也遇到既有的 generated/dist 输出缺失及其他 workspace 错误。风险与范围
QWEN_CODE_STARTUP_VERSION的新 shell 才会从自身 package metadata 初始化。关联 Issue
Related to #8343 and #8373；这些 issue 覆盖 runner 安装漂移，本变更则让 Review footer 元数据在提交层确定化。