feat(review): remember explicit effort per project - #9883
Conversation
|
Re-run at the current head, after three rounds of review and the maintainer's re-verification. Template: complete and filled in now ✓ — what/why, a step-by-step reviewer test plan with expected verdict JSON, before/after evidence, risk & scope, and the bilingual summary. The two gaps from the last pass (untouched template, fail-open question) are both closed: the body documents the fail-open I/O semantics, and the code implements them. Problem: real and observed — filed as #9792 by a maintainer with a concrete proposal: Direction: aligned. Size: small and test-heavy — 187 production lines (149 in Approach: scope feels right, and it stayed minimal across the three rounds. The remembered value can't live in settings (Workspace scope is deliberately ignored for Risk: no high-risk path matches; no elevated risk signals. Moving on to code review. 🔍 中文说明在当前 head 上重跑——此前已经过三轮审查和 maintainer 的复验。 模板：现已完整填写 ✓ ——做了什么/为什么、带预期 verdict JSON 的逐步验证计划、before/after 证据、风险与范围、中文摘要。上一轮的两个缺口（未填写的模板、fail-open 疑问）都已闭合：正文写明了 fail-open I/O 语义，代码也已实现。 问题：真实且已被观测——由 maintainer 以 #9792 提出并给出具体方案： 方向：对齐。 规模：小而测试厚重——生产代码 187 行（ 方案：范围合理，且三轮之后仍保持最小。记忆值不能放在 settings 里（Workspace 作用域被刻意忽略、用户设置是全局的），所以按 Storage 项目目录键控的用户私有文件是最小机制；diff 只含解析器接线、路径助手、fail-open I/O、三处文档与测试。没有顺手改动。记忆值优先于配置值在正文中作为意图写明，且与参考实现一致。 风险：未命中高风险路径；无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline: for "remember the last explicitly typed effort per project" I'd persist an explicit choice in user-private, project-keyed state (settings are out — Workspace scope is deliberately ignored for What I verified against the base code and the current diff:
No blockers. The maintainer's remaining observations from re-verification are all explicitly deferred follow-ups, and I concur they don't hold this merge: (a) the read path is fail-open for errors but a FIFO at the memory path would block Test evidenceUnattended re-run — the evidence below is the PR's own CI on the reviewed commit, fetched via the API; nothing was built or executed locally. The unit suite pins the central claim end-to-end: the wiring test drives Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The behavioral claim is substantiated by 2b (the suite pins the flow through the real handler) plus the maintainer's two built-binary A/B rounds, so no sandboxed-lane line is needed this time. 中文说明代码审查独立方案：对"按项目记住上次显式输入的 effort"，我会把显式选择持久化到用户私有、按项目键控的状态里（settings 不可行—— 对照基线代码与当前 diff 验证过的点：
无阻断项。maintainer 复验中剩余的观察均为其明确延后的后续项，我也认为不应阻塞本次合并：(a) 读路径对错误 fail-open，但记忆路径上放一个 FIFO 会阻塞 测试证据无人值守重跑——以下证据来自被审提交上 PR 自己的 CI（经 API 获取），本地未构建或执行任何代码。单测端到端钉住核心主张：接线测试两次驱动 行为主张已由 2b（套件经真实 handler 钉住流程）加 maintainer 的两轮构建产物 A/B 证实，因此这次不需要沙箱验证行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — the design was right in round 1, the one blocker raised since is closed with tests that discriminate, and the maintainer's built-binary re-verification at this exact head already went deeper than a typical merge. Stepping back: my independent proposal for this feature — user-private per-project state, explicit > remembered > configured > default with the safety forcings on top, a visible reuse notice, fail-open I/O because the file sits on the review's critical path — is exactly what shipped. The first-round review passed the shape; the maintainer's round-1 A/B found the one real defect (fail-closed memory I/O able to kill a review over a file the user never asked for, invisibly — 0-byte verdict behind an exit-0 After seeing it run — CI green on the reviewed commit, plus two built-binary A/B rounds with fault injection, symlink plants, and 24-way concurrency — the results match what the PR promised. If I were maintaining this in six months, the fail-open NOTEs and the pinned precedence tests are exactly what I'd want to find. Three follow-ups for after merge, none blocking (all from the maintainer's re-verification, all explicitly deferred there): the FIFO/blocking-read + stderr-echo hardening (his measured one-line fix is ready), a supported reset path, and the file's location named in the docs. Approving, pinned to the reviewed commit — CI is fully settled on it (both 中文说明置信度：5/5 —— 设计在第一轮就是对的，此后提出的唯一阻断项已闭合并配有可区分的测试，且 maintainer 在本 head 上的构建产物复验已深于一般合并标准。 退一步看：我为这个功能写的独立方案——用户私有的按项目状态、显式 > 记忆 > 配置 > 默认且安全强制居其上、复用时可见提示、fail-open I/O（该文件位于 review 关键路径）——正是最终落地的形态。第一轮审查通过了整体形状；maintainer 的第一轮 A/B 找到了唯一真实缺陷（fail-closed 的记忆 I/O 可能因一个用户从未要求的文件杀死整个 review，且不显眼——exit 0 的 看它跑过之后——被审提交上 CI 全绿，外加两轮带故障注入、软链种植与 24 路并发的构建产物 A/B——结果与 PR 承诺一致。若六个月后由我维护，这些 fail-open NOTE 与钉住优先级的测试正是我希望看到的东西。 合并后的三个后续项，均不阻塞（皆出自 maintainer 复验、且已被其明确延后）：FIFO/阻塞读 + stderr 回显的加固（他已实测的一行修复现成可用）、受支持的清除路径、以及在文档中写明该文件的位置。 批准，锚定在被审提交——其上的 CI 已完全落定（两个 — 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.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/parse-args.ts:1001 — [probe] R2-2 readLastReviewEffort's malformed-content branch has zero flag-less test coverage (both fallback mutations survive 150/150 green)packages/cli/src/commands/review/parse-args.ts:1027 — [probe] R2-3 configured-rank non-persistence is untested; a persist-after-read mutation survives green and would permanently shadow review.effortpackages/cli/src/commands/review/lib/paths.test.ts:33 — [probe] R2-4 fallback-storage privacy assertions under-anchored; a workspace-leak mutation ships green
中文说明
已审查——无阻断问题。 建议见行内评论。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Local runtime verification of #9883 (real builds, real filesystem, A/B against the merge-base)I built both arms locally and drove the built binary, not the unit suite: PR head The feature does exactly what the PR says it does, and the storage hygiene is the right posture — I attacked it and it held. One thing I would want changed before merge, and it is small. 1. The advertised behaviour reproduces, end to endSteps 1–3 of the Reviewer Test Plan, verbatim, on the built CLI:
Also confirmed by direct probe, all on the built binary:
Suites, on the PR arm in my tree: CLI 2. What I would change before merge: the memory is fail-closed, and it sits on the critical path
(a) The escape hatch is not as visible as it reads. The triage note says the error names the path and the valid values, so the fix is actionable. That is true if you run (b) The failure class is wider than a corrupt file. It is every I/O fault on either side, and the write half fires even when the user typed an explicit
(c) The adjacent code in the same statement already rules the other way.
That comment describes the new code's failure mode precisely — over a file that, unlike a settings file, the user never asked for and cannot see. Same handler, same statement, opposite policy. No test covers any of this. The suite pins "explicit replaces malformed state" but never "malformed state without The fix, applied and verified on top of the PR (build clean, happy path byte-identical, every row above turns into a
function readLastReviewEffort(path: string): ReviewEffort | undefined {
let value: string;
try {
if (!existsSync(path)) return undefined;
value = readFileSync(path, 'utf8').trim();
} catch (error) {
writeStderrLineSafe(`NOTE: the remembered review effort could not be read (…); resolving from settings and defaults instead.`);
return undefined;
}
const effort = asEffort(value);
if (effort === null) {
writeStderrLineSafe(`NOTE: ${path} must contain low, medium, or high; got ${JSON.stringify(value)}. Ignoring it; type \`--effort <level>\` to record a new one.`);
return undefined;
}
return effort;
}
// and the same try/catch around mkdirSync + atomicWriteFileSync in writeLastReviewEffortWorth a regression test for the two shapes the current suite cannot see: malformed content with no 3. Two non-blocking observationsThere is no supported way back to the built-in default. Once a level is remembered,
Sequence: start Separately, credit where due: the SKILL.md resume enumeration ( VerdictThe design is right, the precedence table is right, the storage hygiene is genuinely good — Verification setup (for reproduction)Note on my box: 中文说明对 #9883 的本地运行时验证（真实构建、真实文件系统、与 merge-base 做 A/B）我在本地构建了两个 arm 并驱动构建产物而非单测：PR head 这个功能确实做到了 PR 所声称的一切，存储卫生的姿态也正确 —— 我做了攻击性探针，它扛住了。合并前我希望改一处，改动很小。 1. 声称的行为完整复现Reviewer Test Plan 的第 1–3 步在构建后的 CLI 上逐字重现（见上方第 1 张截图）。另外直接探针确认：
PR target 上 我这边的测试：CLI 2. 合并前我建议改的一处：记忆是 fail-closed 的，而且位于关键路径上
(a) 逃生门没有看上去那么显眼。 triage 说报错点明了路径和合法值，所以可操作。手动跑 (b) 失败面比"文件损坏"宽得多。 是两侧的每一种 I/O 故障，而且写的那一半即使用户显式输入了
(c) 同一条语句里的相邻代码早就给出了相反裁决。
这段注释精确描述了新代码的失败模式——而且这次的文件与 settings 文件不同：用户从没要求过它，也看不见它。同一个 handler、同一条语句、相反的策略。 这些路径没有任何测试覆盖。 套件钉住了"显式值替换损坏状态"，却从没覆盖"损坏状态 且不带 补丁已实际应用并验证（构建干净，happy path 逐字不变，上表每一行都变成一条 3. 两点非阻塞观察没有受支持的办法回到内置默认值。 一旦记住了某个档位，
场景： 另外该给的肯定：SKILL.md 的 resume 枚举（ 结论设计是对的，优先级表是对的，存储卫生是真的好—— |
a8c8268 to
8c4f0f7
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Addressed on
Validation on the rebased head:
I kept reset semantics ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Convergence: round 3 posted 5 inline comment(s), 3 of them reported for the first time; the previous round posted 4 (0 new). Findings keep coming back to the same files: packages/cli/src/commands/review/parse-args.ts (findings in round 1; 3 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
收敛情况：第 3 轮发布了 5 条行内评论，其中 3 条是首次提出；上一轮发布了 4 条（其中 0 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/parse-args.ts（第 1 轮已出过发现，本轮又有 3 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Addressed the round-3 review suggestions in
Local verification: focused CLI suite |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-5 last_used casing — already reported (comment 3860840752)
- R1-7 no reset path — already reported (comment 3860840758)
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/paths.test.ts:32 — [probe] fallback-storage privacy test under-guards its property — a workspace-local fallback survives all assertions (re-discovery of R2-4, deferred round 2)packages/cli/src/commands/review/lib/paths.ts:69 — [probe] settings-relocated runtime base fragments the effort memory between in-session and standalone invocations
Convergence: round 4 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 5 (3 new). Findings keep coming back to the same files: packages/cli/src/commands/review/parse-args.ts (findings in rounds 1, 3; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 5 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/cli/src/commands/review/parse-args.ts（第 1、3 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| }); | ||
|
|
||
| if (explicitEffort !== undefined) { | ||
| writeLastReviewEffort(effortPath, explicitEffort, initial.effort); |
There was a problem hiding this comment.
[Suggestion] No test pins which value is persisted when a safety floor overrides the explicit effort. The write call persists the typed explicitEffort — correct per the documented "last level explicitly typed" contract — while initial.effort is only used for the failure NOTE, and the one test combining a forcing with the write path (reports the resolved effort when remembering an explicit effort fails, 6711 --comment --effort low) makes atomicWriteFileSync throw, so nothing observes the persisted value.
A probe at the reviewed commit confirms the gap: a mutant persisting the forced level (writeLastReviewEffort(effortPath, initial.effort, initial.effort)) passes the whole suite 154/154, because the ENOSPC test asserts only stderr fragments the mutant also produces. If that mutant ever ships, after one /review 6711 --comment --effort low every later flag-less review in the project reuses high instead of the level the user typed, contradicting the contract this PR documents in three places.
Add one wiring test that observes a successful write under forcing:
it('persists the typed effort when a safety floor overrides it', async () => {
const storedEffort = lastReviewEffortPath(
process.cwd(),
process.env['QWEN_CODE_PROJECT_DIR'],
);
const got = await verdictFor('6711 --comment --effort low\n');
expect(got.effort).toBe('high');
expect(got.effortSource).toBe('forced-by-comment');
expect(fsState.written.get(storedEffort)).toBe('low\n');
});The new test must go red if the persisted argument at this write site is ever replaced with initial.effort — verify by running the mutation (the stored file becomes high\n instead of low\n).
中文说明
[建议] 目前没有任何测试钉住：当安全底线覆盖显式 effort 时，持久化的到底是哪个值。写入调用持久化的是用户显式输入的 explicitEffort——这是正确的，符合文档中"最后一次显式输入的档位"契约——而 initial.effort 仅用于失败提示；并且唯一同时覆盖强制底线与写入路径的测试（reports the resolved effort when remembering an explicit effort fails，6711 --comment --effort low）让 atomicWriteFileSync 抛错，因此没有任何测试观察到持久化的值。
在被审提交上的探针证实了这个缺口：把持久化值改为强制档位的变异体（writeLastReviewEffort(effortPath, initial.effort, initial.effort)）仍能通过全部 154/154 个测试，因为 ENOSPC 测试只断言了变异体同样会输出的 stderr 片段。若该变异体将来被引入，那么一次 /review 6711 --comment --effort low 之后，该项目后续所有不带参数的 review 都会复用 high 而不是用户输入的档位，与本 PR 在三处文档中写明的契约矛盾。
请补一个观察强制底线写入成功路径的测试（代码见上方英文部分）。若此写入位点的持久化参数将来被替换为 initial.effort，新测试必须变红——可运行该变异（持久化文件变为 high\n 而非 low\n）来验证。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Re-verification of #9883 at
|
| mutation | PR's suite | caught by |
|---|---|---|
drop the try/catch in readLastReviewEffort |
1 failed / 153 passed | ignores unreadable remembered state |
drop the try/catch in writeLastReviewEffort |
1 failed / 153 passed | reports the resolved effort when remembering an explicit effort fails |
Round 3's diagnostic fix also reproduces on the built binary — the write-failure NOTE names the resolved level, not the typed one, on all three forcing paths (--comment → high, --fix → medium, neither → low).
3. Feature and storage matrix — no regressions
Precedence, cross-project isolation, both forcings, "remember what was typed not what was forced", invalid-value containment, non-persistence of default/configured, QWEN_CODE_PROJECT_DIR ownership, the symlink plant (noFollow holds; victim untouched; replaced by a real 0600 file) and 24-way concurrency (no torn read, no leftover .tmp) all behave as documented.
I also checked the double-parse in parseReviewArgsWithMemory for the obvious hazard: parseReviewArgs has a single exit and writes nothing to stderr, so the second pass cannot duplicate a side effect. Measured on a run that produces three warnings: multiset size 3, unique 3.
Gates on my tree: tsc --noEmit clean for both workspaces; eslint --max-warnings 0 clean on all six changed files; focused tests 216/216 (CLI) and 42/42 (core skill); the whole packages/cli/src/commands/review suite 5117 passed / 2 skipped / 0 failed; generate:settings-schema produces no diff. (One [vitest-worker]: Timeout calling "onTaskUpdate" surfaced on the full run with every test green and exit 0 — the known worker-RPC flake, unrelated.) Last time one run-ledger test failed identically on both arms as a running-as-root artifact of my box; not running as root, it passes.
The docs moved with the behaviour, including the resume sentence I asked for.
4. Still open — all non-blocking, none of it should hold the merge
(a) The read path is fail-open for errors, but not for a blocking file. existsSync answers yes for a FIFO; readFileSync then waits for a writer that never comes. A try/catch cannot see this — there is no error and no timeout. With a FIFO at the memory path and no --effort, the PR build hangs forever where the merge-base exits 0. Separately, got ${JSON.stringify(value)} echoes the whole unusable file to stderr (200 KB in → 64 KB out on one line), and the read side follows symlinks, so a link planted there puts another file's bytes into the review model's context.
This needs same-user write access to a 0700 directory, so it is hardening rather than a privilege boundary — but this exact path already carries noFollow against that threat model, which is why I think it is worth one line. Both close together, and I measured it after applying and rebuilding:
- if (!existsSync(path)) return undefined;
+ if (!statSync(path, { throwIfNoEntry: false })?.isFile()) return undefined;plus .slice(0, 40) inside the JSON.stringify. FIFO → exit 0, high/default; a directory at the path stops emitting EISDIR noise and skips quietly; corrupt, unreadable, happy-path and symlink-plant behaviour all unchanged; tsc clean. Five unit tests go red only because the suite's node:fs mock supplies existsSync/readFileSync but no statSync — which is itself the reason this class was invisible: the remembered file lives in an in-memory map, so real-filesystem object types never reach the code under test.
(b) Still no supported way back to the built-in default. --effort auto|default|none|reset are all rejected as invalid and the remembered level survives; review.effort: "auto" no longer restores anything either. The only exit remains deleting ~/.qwen/projects/<sanitized-cwd>/review-last-effort by hand, and the docs still do not name that path. Explicitly deferred, and I agree it can be — but the docs line that says "type a new --effort to replace it" is one clause away from also saying where the value lives.
(c) --resume can discard an interrupted run, now including the configured case. Driving the real assessResume against an interrupted run recorded at high, with every other probe intact, the table in the screenshot is what happens. The last_used(low) row is the one I flagged last time and it is now disclosed in the docs, so I am not re-raising it. Worth naming, though: the configured(low) row is a behaviour change that arrives with this PR but is not part of the memory feature. The old SKILL.md said both "pass for explicit or forced-by-comment" and "omit ONLY when default", leaving configured genuinely undecided; this PR resolves the contradiction toward passing. I think that is the right call — a configured level is user intent — and it only bites someone who changes review.effort mid-review, but it is a resume-semantics change riding along with an effort-memory PR.
Verdict
The design was right the first time; what changed since a8c8268 is that the storage is now honestly fail-open and the behaviour is pinned by tests that can tell the difference. §4 is all follow-up: (a) is a one-line hardening I have already measured, (b) is the reset UI the PR explicitly scoped out, (c) is disclosed. LGTM from me on the runtime behaviour.
Reproduction setup
PR arm git worktree at pull/9883/head = 2150067809 , npm ci && npm run build
Base arm git worktree at merge-base = a770aefddd , npm ci && npm run build
driver node packages/cli/dist/index.js review parse-args --stdin
isolation fresh QWEN_HOME + fresh git-init'd project root per probe
memory $QWEN_HOME/projects/<sanitizeCwd(cwd)>/review-last-effort (0600)
resume assessResume() imported from the built dist, synthetic PreviousReport/ResumeProbes
mutations edit src, rebuild the cli workspace, re-run; source restored and rebuilt after
host macOS 15 (arm64), Node v24.18.1, not root
中文说明
对 #9883 在 21500678 的复验 —— 我提的阻断项已闭合
这是第二轮本地实跑，针对当前 head，而不是跑单测：PR head 2150067809 与 merge-base a770aefd 各自从零构建（npm ci + npm run build），以 node packages/cli/dist/index.js review parse-args --stdin 驱动，每个探针配独立的 QWEN_HOME 和真实项目根目录。macOS 15，Node 24.18.1。我上一份报告针对的是 a8c8268，之后又落了三轮改动。
结论：可以合并。 我上次 §2 要求的都已到位，而且被真正有辨别力的测试钉住了。剩下的都属于后续项。
1. §2 —— fail-open：已闭合
在 a8c8268 上会 exit 1 的每一种情形，现在都 exit 0，解析结果与 merge-base 完全一致，并在 stderr 说明原因（见第一张图）。
§2(a) 那一半也闭合了。在 SKILL.md Step 1 的真实形态 … parse-args --stdin | tee .qwen/tmp/qwen-review-parse-args.json 下，损坏或不可用的记忆状态过去会在 exit 0 的管道后面留下一个 0 字节的 verdict；现在落盘的是与 merge-base 完全相同的 488 字节合法 JSON，所以 skill "逐字使用该文件" 的契约在我能造出的每一种故障下都成立。
2. 新增分支确实被有辨别力的测试覆盖
我上次最尖锐的一条是：没有任何测试能区分 fail-open 和 fail-closed，也就是说没人真正做过这个选择。我直接对发布代码做了变异，再跑 PR 自己的套件：
| 变异 | PR 套件结果 | 被哪条测试抓住 |
|---|---|---|
去掉 readLastReviewEffort 的 try/catch |
1 failed / 153 passed | ignores unreadable remembered state |
去掉 writeLastReviewEffort 的 try/catch |
1 failed / 153 passed | reports the resolved effort when remembering an explicit effort fails |
round 3 的诊断修复在构建产物上也复现了：写失败的 NOTE 报告的是最终生效的档位而非输入值，三条强制路径都正确（--comment → high、--fix → medium、无强制 → low）。
3. 功能与存储矩阵 —— 无回归
优先级、跨项目隔离、两种强制、"记住输入值而非强制值"、无效值不污染记忆、默认值/配置值不持久化、QWEN_CODE_PROJECT_DIR 归属、符号链接植入（noFollow 生效，被指向的文件未被改动，链接被替换为真实 0600 文件）、24 路并发（无撕裂读、无残留 .tmp）均与文档一致（见第二张图）。
我也检查了 parseReviewArgsWithMemory 里两次解析的明显风险：parseReviewArgs 只有单一出口且不写 stderr，第二趟无法重复任何副作用。在一次产生三条 warning 的调用上实测：多重集大小 3，去重后 3。
我这边的门禁： 两个 workspace 的 tsc --noEmit 干净；六个改动文件 eslint --max-warnings 0 干净；定向测试 216/216（CLI）与 42/42（core skill）；整个 packages/cli/src/commands/review 套件 5117 通过 / 2 跳过 / 0 失败；generate:settings-schema 无 diff。（全量跑出现一次 [vitest-worker]: Timeout calling "onTaskUpdate"，但所有测试为绿且退出码 0 —— 已知的 worker RPC 抖动，与本 PR 无关。）上次有一个 run-ledger 测试在两臂上同样失败，那是我机器以 root 运行导致的；非 root 下通过。
文档与行为同步更新，包括我上次要求补的 resume 那句。
4. 仍然开放的问题 —— 全部非阻断，都不应该拦合并
(a) 读路径对错误 fail-open，但对会阻塞的文件不是。 existsSync 对 FIFO 返回 true，readFileSync 随后会一直等待永远不来的写端。try/catch 看不见这种情况 —— 既没有错误也没有超时。在记忆路径放一个 FIFO 且不带 --effort 时，PR 构建永久挂起，而 merge-base exit 0。另外，got ${JSON.stringify(value)} 会把整个不可用文件回显到 stderr（输入 200 KB → 输出 64 KB 一整行），而且读路径会跟随符号链接，因此在那里植入一个链接就能把另一个文件的字节送进 review 模型的上下文。
这需要对一个 0700 目录有同用户写权限，所以属于加固而非权限边界 —— 但正是这条路径已经带了 noFollow 来防同一威胁模型，所以我认为值得改这一行。两个问题一起关掉，我已经应用并重新构建后实测：
- if (!existsSync(path)) return undefined;
+ if (!statSync(path, { throwIfNoEntry: false })?.isFile()) return undefined;外加在 JSON.stringify 里补 .slice(0, 40)。实测：FIFO → exit 0, high/default；路径上是目录时不再产生 EISDIR 噪声而是安静跳过；损坏、不可读、正常路径和符号链接植入的行为全部不变；tsc 干净。有 5 个单测变红，仅仅是因为套件的 node:fs mock 提供了 existsSync/readFileSync 却没有 statSync —— 而这恰恰解释了这一类问题为什么此前不可见：记忆文件活在一个内存 map 里，真实文件系统的对象类型根本到不了被测代码。
(b) 仍然没有回到内置默认值的受支持路径。 --effort auto|default|none|reset 全部被判为无效值，记忆值原样保留；review.effort: "auto" 也不再能恢复什么。唯一的出口仍是手工删除 ~/.qwen/projects/<sanitized-cwd>/review-last-effort，而文档仍未提到这个路径。这一项已被明确列为后续，我同意可以后续做 —— 但文档里"输入新的 --effort 即可替换"那句，离顺带说明值存在哪里只差一个从句。
(c) --resume 可能丢弃中断的运行，现在还包括 configured 的情形。 用真实的 assessResume 驱动一个记录为 high 的中断运行、其余探针全部完好，结果见第三张图。last_used(low) 那一行是我上次提的，现在已在文档中披露，我不再重提。但值得点名的是：configured(low) 这一行是随本 PR 到来、却不属于记忆功能的行为变更。旧版 SKILL.md 同时写着"explicit 或 forced-by-comment 时传递"和"仅当 default 时省略"，configured 处于真正未决状态；本 PR 把这个矛盾消解为"传递"。我认为方向是对的 —— 配置的档位属于用户意图 —— 而且只会影响在 review 中途改 review.effort 的人，但它毕竟是一个搭在 effort 记忆 PR 上的 resume 语义变更。
结论
设计从一开始就是对的；相对 a8c8268 真正变化的是：存储现在诚实地 fail-open，且行为被有辨别力的测试钉住。§4 全部属于后续：(a) 是我已经实测过的一行加固，(b) 是 PR 明确划到范围外的重置能力，(c) 已有披露。就运行时行为而言，我这边 LGTM。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (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: 78 passed · 0 failed · 78 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：78 通过 · 0 失败 · 78 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9883 Deep Verification — feat(review): remember explicit effort per projectVerdict: 中文摘要
Central claim + A/BCentral claim. An explicitly typed Control. Base worktree at
Head-only sibling sweep (all passed): Verdict is the pair of counts: on the reuse scenario head flips from base’s silent CorrectionsNone to the code. One description clarification, recorded so the next reader is not misled: the body’s “Resume handling also passes remembered and configured levels to Findings1. (Low, observation) An explicit effort is persisted even when the target is refused. The remember callback fires at the end of D=$(mktemp -d) && printf -- '--effort low https://github.com/foo/bar/pull/42oops\n' | \
QWEN_CODE_PROJECT_DIR="$D" node packages/cli/dist/index.js review parse-args --stdin | grep effortSource
ls "$D" # review-last-effort exists with "low\n"Observed: verdict is 2. (Info) Resume continuity claim verified at the level the sandbox allows. Not covered
Targeted gates (all green, captured in
|
| mutant | what it reverts | suite | result |
|---|---|---|---|
| M0 (positive control) | notice wording “reusing” → “recycling” | cli parse-args | KILLED (5 red) |
| M1 | last_used resolution branch |
cli parse-args | KILLED (8 red) |
| M2 | remember-callback invocation | cli parse-args | KILLED (6 red) |
| M3 | reuse-notice warnings.push block |
cli parse-args | KILLED (5 red) |
| M4 | malformed-state stderr NOTE (silent fall-through) | cli parse-args | KILLED (1 red) |
| M5 | noFollow: true on the atomic write |
cli parse-args | KILLED (1 red) |
| M7 | handler bypasses parseReviewArgsWithMemory entirely |
cli parse-args | KILLED (9 red) |
| M6 | SKILL.md resume list drops last_used/configured |
core SKILL.test | KILLED (1 red) |
M4’s red is the intended behavioral assertion: ignores malformed remembered state when no explicit effort replaces it fails with writeStderrLineSafe receiving no call (expect(...mock.calls[0]?.[0]).toContain(…) on undefined), i.e. the missing NOTE, not an import/compile break. Every mutant was applied to the pristine source by exact-anchor replacement and restored afterward (git status clean after each; scripted kill-assertion wrapper: mutation-assert.mjs, 8/8 PASS). The new tests are not vacuous, and no guard the PR introduces is dead or redundant.
Methodology
Environment: the CI verify container (node v22.23.2, linux, merge-ref checkout pull/9883/merge at depth 2; npm ci + npm run build pre-done at HEAD). The A/B drove the real built dist of both arms through review parse-args --stdin as child processes with isolated HOME/QWEN_HOME/QWEN_CODE_PROJECT_DIR scratch dirs and real file-mode checks (no mocks); settings injection used QWEN_HOME/<dir>/settings.json (User scope). The base arm rebuilt core+cli in a tmp/base-tree worktree after re-pointing node_modules/@qwen-code/* symlinks into it and asserting their realpath; a first base build attempt failed on ajv/dist/2020.js, traced to missing nested per-package node_modules links in the worktree (fixed by symlinking them; root lockfile untouched by the PR). Mutations were exact-string reverts applied one at a time with git checkout-equivalent restoration, each followed by the focused vitest suite. Raw logs: runs/results.json (all 65 A/B cell outcomes), mutation-runner.mjs/mutation-assert.mjs outputs (matrix), runs/ per-arm scratch. Evidence images were produced by scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/commands/review/lib/paths.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/paths.test.ts
file packages/cli/src/commands/review/parse-args.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/parse-args.test.ts
file packages/core/src/skills/bundled/review/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/review/SKILL.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/paths.test.ts: PPPPP
packages/cli/src/commands/review/parse-args.test.ts: PPPPP
packages/core/src/skills/bundled/review/SKILL.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/lib/paths.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/parse-args.test.ts: P (exit 0)
round 1 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/paths.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/parse-args.test.ts: P (exit 0)
round 2 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/paths.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/parse-args.test.ts: P (exit 0)
round 3 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/paths.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/parse-args.test.ts: P (exit 0)
round 4 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/paths.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/parse-args.test.ts: P (exit 0)
round 5 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. ✅
qwen-code-ci-bot
added
category/cli
One conflict, in review/lib/paths.ts: both sides grew the import block — main added `Storage` for QwenLM#9883's lastReviewEffortPath, this branch added the fs/path/crypto imports for the generated-workflow dir helpers. Resolution is the union: one combined core import, both sides' functions kept untouched. client.telemetrySwap.test.ts resolved itself: this branch's 48a01fa and main's 6dfb5a1 (QwenLM#10252) are byte-identical fixes for the same TS1117 duplicate getToolRegistry. Verified: npm run build, npm run typecheck, npm run lint all clean; packages/cli src/commands/review 105 files / 5249 passed; packages/core client.telemetrySwap.test.ts 10 passed.
|
Released in v0.22.3. |
4 participants
What this PR does
Persists the last
/review --effortlevel the user explicitly typed in user-private, project-scoped storage. A later/reviewwithout--effortreuses that value, reportseffortSource: "last_used", and emits a notice that the review skill relays before work begins.The resolution order is explicit
--effort> remembered level > operatorreview.effort> target default. Effective--commentand--fixsafety requirements still override the resolved level. An explicit value is remembered even when it equals the target default. The value is written atomically with mode0600, using the parent session'sQWEN_CODE_PROJECT_DIRas the project owner when available.Remembered-state I/O is fail-open and visible: malformed or unreadable state is reported on stderr and ignored so resolution continues from
review.effortand the target default; a persistence failure is reported without discarding the explicit effort selected for the current review.Resume handling also passes remembered and configured levels to
fetch-pr, so a mismatch starts fresh at the level required by the current invocation instead of silently pinning the interrupted run's level.Why it's needed
Users who repeatedly choose the same review depth currently have to type the same flag on every invocation. The existing
review.effortsetting is an operator-wide default, while an explicitly selected review depth is project-specific user intent. Remembering only explicitly typed levels preserves that intent without turning automatic or target-default decisions into persistent policy.Reviewer Test Plan
How to verify
printf '%s\n' '--effort high 9792' | QWEN_CODE_PROJECT_DIR="$PROJECT_DIR" node packages/cli/dist/index.js review parse-args --stdin. Confirm the verdict reports"effort": "high"and"effortSource": "explicit".printf '%s\n' '9792' | QWEN_CODE_PROJECT_DIR="$PROJECT_DIR" node packages/cli/dist/index.js review parse-args --stdinfrom the same project. Confirm it reports"effort": "high","effortSource": "last_used", and the reuse notice.--effort medium; confirm the next invocation reusesmedium. Also confirm an explicit value overwrites malformed remembered state, malformed state without an explicit replacement falls through with aNOTE:, and a simulated persistence failure does not block the explicitly requested effort.Evidence (Before & After)
Before: an explicitly typed effort applied only to the current invocation; the next invocation without
--effortresolved fromreview.effortor the target default.After: the first invocation reports
{"effort":"high","effortSource":"explicit"}and persistshigh; the next invocation reports{"effort":"high","effortSource":"last_used"}plusNo effort level given — reusing high, the level you typed last time.Focused tests pass on the rebased head: CLI214/214, Core skill42/42.Tested on
Environment (optional)
macOS 15.3.1, Node.js 24.13.1, npm 11.8.0; built CLI exercised end to end with an isolated project owner directory.
Risk & Scope
review.effortfor that project until another explicit choice replaces it.Linked Issues
Fixes #9792
中文说明
本 PR 做了什么
将用户最近一次显式输入的
/review --effort档位持久化到用户私有、按项目隔离的存储中。之后调用/review时如果没有提供--effort，参数解析会复用该值，返回effortSource: "last_used"，并生成一条提示，由 review skill 在开始工作前转达给用户。解析优先级为：显式
--effort> 项目记忆值 > 操作者配置review.effort> 目标类型默认值。生效的--comment和--fix安全要求仍会覆盖最终档位。即使显式输入恰好等于目标默认值，也会被记住。该值使用原子写入并设置为0600权限；父 session 提供QWEN_CODE_PROJECT_DIR时，沿用该目录作为项目存储归属。记忆状态的 I/O 采用可见的 fail-open 语义：损坏或无法读取的状态会在 stderr 报告后被忽略，并继续按
review.effort与目标默认值解析；持久化失败会被报告，但不会丢弃本次 review 已显式选定的档位。恢复中断 review 时，也会把记忆值和配置值传给
fetch-pr。如果与中断任务记录的档位不一致，就按当前调用要求的档位重新开始，不会静默固定在旧档位。为什么需要
用户如果经常为同一项目选择相同的 review 深度，现在每次都必须重复输入同一个参数。现有
review.effort是操作者范围的通用默认值，而用户显式选择的 review 深度属于项目级意图。仅记忆显式输入可以保留这个意图，同时不会把自动选择或目标默认值悄悄固化成长期策略。Reviewer 测试计划
如何验证
printf '%s\n' '--effort high 9792' | QWEN_CODE_PROJECT_DIR="$PROJECT_DIR" node packages/cli/dist/index.js review parse-args --stdin。确认结果包含"effort": "high"和"effortSource": "explicit"。printf '%s\n' '9792' | QWEN_CODE_PROJECT_DIR="$PROJECT_DIR" node packages/cli/dist/index.js review parse-args --stdin。确认结果包含"effort": "high"、"effortSource": "last_used"和复用提示。--effort medium，确认下一次调用会复用medium；同时确认显式输入可以覆盖损坏的旧记忆状态、无显式值时损坏状态会伴随NOTE:回落，以及模拟的持久化失败不会阻断显式请求的档位。证据（修改前与修改后）
修改前：显式输入的 effort 仅对当前调用生效；下一次省略
--effort时，会重新使用review.effort或目标类型默认值。修改后：第一次调用返回
{"effort":"high","effortSource":"explicit"}并持久化high；下一次调用返回{"effort":"high","effortSource":"last_used"}，同时提示No effort level given — reusing high, the level you typed last time.rebase 后的定向测试全部通过：CLI214/214，Core skill42/42。测试平台
环境（可选）
macOS 15.3.1、Node.js 24.13.1、npm 11.8.0；使用隔离的项目归属目录对构建后的 CLI 做了端到端验证。
风险与范围
review.effort，直到用户再次显式输入其他档位。关联 Issue
Fixes #9792