feat(review): give the convergence observation a machine-readable half - #9623
Conversation
The diagnosis could tell a human why a loop was not settling and gave a
caller nothing to act on. This adds the two remaining pieces of that work
item: matched recommendations as a closed code set, and the round's own
report on whether its machinery is working.
**`recommendations: [{code, basis}]`** — measurement to advice, with no
constants and no decisions. Four codes are matched from facts this round
already holds: `root-cause-triage` (files that carried findings before and
carry new ones now), `batch-fixes` and `stem-surface` (a trend that is not
falling, the latter only where a floor rung is left to take), and
`land-and-defer` (a round that posts no Critical — a loop whose blockers are
all fixed can end by merging, and a merged pull request cannot diverge).
Every entry carries the deterministic fact it was matched from. The set is
DERIVED from the diagnosis rather than stored on it, and the paragraph's
prose is generated from the same derivation, so the codes a caller wires and
the sentences a human reads cannot describe different rounds. It rides the
composed result and the durable artifact.
The design's menu is larger, and the codes left unimplemented are each
absent for a stated reason rather than forgotten — `split` needs the
diff-topology test, `reset-drift`/`rescope` need `srcDelta`, `fix-pipeline`
needs marker content-hash dedup, `reduce-cadence` is two thresholds, and
`re-anchor` was matched to a cause this repository's measurements did not
bear out. The last one is now DISCLOSED instead: an anchor chain that has
stopped is stated as a fact and prescribes nothing.
**Mechanism health** — a pipeline that has stopped and one with nothing to
do are both silent, so the round says what it can see about itself. Two
checks: a posting floor the reporting reading resolved to critical while the
enforcement backstop failed open (the default configuration's standing gap,
invisible from either side alone — and deliberately not a count of what
posted, since the deterministic `[test]`/`[build]` carve-out is the
mechanism working), and two consecutive rounds withholding the incremental
anchor (every later round re-reads the whole diff until one closes cleanly).
Stated, never acted on. The anchor decision moves to one shared predicate so
the disclosure and the marker cannot describe different rounds.
Also clears the seven Suggestions deferred through rounds 5-7 of #9461: a
dead `floorKnown` parameter and its dead helper, two comments still
describing the fold that was narrowed, the drafted-id path missing the
length bound `idFor` applies (which let the two ends disagree about one
comment over a shortened list), an English cluster clause whose plural form
read the new-finding count as another round, a comment opening the wrong
validator block, and the merged-provenance wiring's missing end-to-end
assertion.
Ten mutations, each verified to turn a named test red.
|
Thanks for the PR! Template looks good ✓ Problem: real and tracked. This is the machine-readable half of the #9278 work item that #9461 started — that diagnosis shipped prose-only, so a caller had nothing to act on. The mechanism-health half is grounded in observed failures (the Direction: aligned. "The tool measures, the caller decides" is the stated governance principle of #9278, and a closed, derived code set is exactly the interface the tracked follow-up needs. Keeping the set to four codes — each matched from facts the round already holds — and disclosing why the remaining menu is absent rather than shipping it speculatively is the right scope call. Size: no core-module paths touched (all six files under Approach: scope feels right — deriving the codes from the diagnosis instead of storing them keeps the prose and the codes from describing different rounds, which is exactly the drift class this module's header exists to prevent. One structural question I'm chasing in code review: the health block is wired into all three body-assembly branches, and the APPROVE branch's "no convergence signal can fire here" invariant predates it. Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:真实且已被跟踪。这是 #9278 工作项中 #9461 开启后余下的机器可读一半——彼时交付的诊断只有散文,调用方无可据以行动。机制健康部分有观测到的失败作支撑(#9461 上 方向:对齐。"工具测量、调用方决策"是 #9278 明示的治理原则,封闭且派生的码集合正是已跟踪后续项所需的接口。把集合保持在四个码——全部匹配自本轮已持有的事实——并对其余码披露缺席原因而非投机上线,范围取舍正确。 规模:未触及核心模块路径(六个文件全部位于 方案:范围合理——从诊断派生码而非存在诊断上,使散文与码不可能描述不同的轮次,恰是本模块头部注释要防的漂移。有一个结构性疑问留到代码审查:健康块接入了三个正文组装分支,而 APPROVE 分支"此处不可能触发收敛信号"的不变量早于它存在。 风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe architecture is exactly right — deriving the codes from the diagnosis instead of storing them, sharing the anchor predicate between the marker and the disclosure, carrying the codes through the artifact validator. The mutation-testing discipline shows. Two findings block, and both are in the mechanism-health half. 1. The posture-health check fires on every default-config round 6+, whether or not a Suggestion posted — and the prose claims one did. (Critical)
The check exists to tell a broken mechanism from one with nothing to do; as written it also accuses the rounds where the posture is engaging. Either gate it on a Suggestion actually having posted inline (effective 2. The APPROVE branch spreads
3. The misplaced-comment cleanup re-created itself. (Suggestion)
4.
Verified statically against the reviewed commit, for the record: no Testing evidenceCI on the reviewed commit, fetched via the API — no PR code was executed (per this run's rules). At fetch time the main unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle this: Not verified: runtime behaviour generally (no execution on this path), and the author-reported 4245-test run is the author's claim, quoted from the PR body, not independently re-run. No user-visible TUI surface changes, so there is no terminal capture to drive — the only rendered surface is bot-posted review text, which is what findings 1–2 pin. 中文说明代码审查架构方向完全正确——从诊断派生码而非存储、marker 与披露共享锚点谓词、码经 artifact 校验器传出。变异测试的严谨也看得见。两条阻断项都在机制健康这一半。 1. 姿态健康检查在缺省配置的每一轮 6+ 都会触发——无论本轮是否发布了 Suggestion,而散文却声称发布了。(Critical)
2. APPROVE 分支把
3. 错位注释的清理把同类缺陷又造了出来。(Suggestion)
4.
静态核验(针对被审 commit): 测试证据来自 API 的 CI 信号——本次运行规则下未执行任何 PR 代码。抓取时主单测套件( — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the architecture is right and the testing discipline is unusually good, but the mechanism-health half misfires on exactly the shapes the posture is designed to produce, and this module's whole contract is that what it states is true. Stepping back: the approach matches what I'd have proposed independently — derive the codes rather than store them, keep the set closed and matched from facts the round holds, disclose the machinery's state without prescribing — and the shared What I can't get past is finding 1 in the review above. The posture check exists to distinguish a broken mechanism from one with nothing to do — and as written it fires on every default-config round 6+, including the Critical-only rounds where the posture is working and the APPROVE rounds where nothing posted at all, asserting each time that "Suggestion-level findings posted inline anyway". A disclosure that cries wolf on the healthy shape destroys precisely the signal it was built to provide, and finding 2 (the duplicated spread) prints that false clause twice on the clean LGTM body. Both fixes are small — one condition and one line — but they are the difference between a trustworthy self-report and noise in every late-round comment. Not requesting changes lightly: the other half of this PR (the recommendation codes, the artifact wiring, the drafted-id bound, six of the seven deferred cleanups) is ready. Fix the firing condition and the duplicate, drop the stranded comment, and this is in good shape to re-run. @wenshao 中文说明置信度:2/5 —— 架构正确、测试纪律罕见地严谨,但机制健康这一半在姿态设计上最常见的轮次形态上误触发,而本模块的全部契约正在于"所陈述者为真"。 退一步看:方案与我独立会提出的做法一致——派生而非存储码、集合封闭且全部匹配自本轮已持有的事实、披露机制状态但不开处方——而共享的 我无法放过的是上面审查中的第 1 条。姿态检查的本意是区分"机制坏了"与"机制无事可做"——而现状是它在缺省配置的每一轮 6+ 都触发,包括姿态正在生效的纯 Critical 轮和什么都没发布的 APPROVE 轮,每次都断言"Suggestion 级发现仍以行内评论发布"。一个在健康形态上喊狼的披露,毁掉的恰是它要提供的信号;第 2 条(重复展开)还会在干净的 LGTM 正文里把这句错误断言打印两遍。两处修复都很小——一个条件、一行——但这正是"可信的自报告"与"每轮晚期评论里的噪音"之间的差别。 请求修改并非轻率:PR 的另一半(建议码、artifact 接线、drafted id 长度界、七条遗留清理中的六条)已经就绪。修好触发条件与重复展开、删掉游离注释,就可以重新跑一轮。@wenshao — Qwen Code · qwen3.8-max Reviewed at |
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-ci-bot
left a comment
There was a problem hiding this comment.
Two findings in the mechanism-health half block this: the posture disclosure fires on every default-config round 6+ and asserts a manifestation that did not happen (including Critical-only and APPROVE rounds), and the APPROVE branch renders the health block twice. Both fixes are small — details in my review comment above. 🙏
Round 1 found two blockers, both in the mechanism-health half. The posture check held on EVERY default-config round from 6 on — the reporting and enforcement readings differ only in folding an absent floor to `auto`, so the gap itself is permanent there — while the sentence it renders asserts that a Suggestion posted inline because of it. On a Criticals-only round, or on an APPROVE, that claim was simply false: the clause accused the posture of failing on rounds where it was not asked to do anything. It now requires all three conjuncts, because the sentence asserts all three. Gating on the posted count is safe precisely where the enforcement reading is false: `floorEnforcedReroute` never ran, so no inline Suggestion can be its deliberate deterministic carve-out — and when enforcement does engage, the second conjunct is false and the carve-out can never trip it. The APPROVE branch also spread the health block twice. Removed — and the branch's invariant is now written down beside it: neither half can fire there (the posture half needs a posted Suggestion, which makes the event COMMENT; the anchor half needs a fail-closed scope, which caps the verdict off this branch), verified by probe rather than argued. The spread is kept for symmetry with the convergence block above it, which carries the same invariant, so a later check that CAN fire here does not have to rediscover the wiring. The duplicate has no test, and deliberately so: the branch admits no shape where the block is non-empty, so any test for it would assert a state the code cannot reach. The manifestation gate is pinned three ways — the clause renders once, a Criticals-only round is silent, and a nothing-to-report round is silent while its anchor-chain disclosure still stands.
|
Both fixed in 1. The posture check needed the manifestation it asserts. You are right that the first two conjuncts hold on every default-config round from 6 on — the two readings differ only in folding an absent floor to I took the gate rather than the reword, because the gap without a consequence is not something an operator can act on. It now requires all three conjuncts. Worth recording why the gate is safe here, since I had rejected it earlier for the wrong reason: I was worried about Pinned three ways: the clause renders exactly once, a Criticals-only round is silent, and a nothing-to-report round is silent while its anchor-chain disclosure still stands (that check is about the machinery and does not depend on what the round found). 2. The APPROVE branch spread the health block twice. Removed. On your structural question from triage — whether the branch's "no convergence signal can fire here" invariant extends to the health block — I probed it instead of arguing it: on an APPROVE round the block is empty ( Which is why the duplicate ships with no test, deliberately: the branch admits no shape where the block is non-empty, so a test for it would assert a state the code cannot reach. I kept the spread rather than dropping it, for symmetry with the convergence block above it that carries the same invariant — so a later check that can fire here does not have to rediscover the wiring. 4245 tests pass; the manifestation gate is mutation-checked (removing the third conjunct turns the Criticals-only assertion red). 中文说明两条都已在 1. 姿态检查缺了它自己断言的那个"发生"。 你说得对:前两个合取在缺省配置下从第 6 轮起每一轮都成立——两种读数只差"把缺失楼层折叠为 auto"这一点,所以那个落差是常态而非事件。而渲染出的句子却断言"因此有 Suggestion 以行内评论发布"。在只有 Critical 的轮次、或 APPROVE 轮次上,这个断言直接就是假的;而这项检查的意义正是把"机制坏了"和"无事可做"分开——去指控姿态正在正常工作的轮次,与它的职责相反。 我选了加门而不是改措辞:没有后果的落差不是操作者能据以行动的东西。现在三个合取全都要求。有一点值得记下来,因为我此前以错误的理由否决过这个方案:当时担心 三处钉住:子句只渲染一次;只有 Critical 的轮次保持沉默;无事可报的轮次也保持沉默,而它的锚点链披露依然成立(那项检查针对机制本身,不依赖本轮发现了什么)。 2. APPROVE 分支把健康块展开了两次。 已删。关于你在 triage 里提的结构性疑问——该分支"此处不可能触发收敛信号"的不变量是否延伸到健康块——我用探针实测而非论证:APPROVE 轮次上该块为空( 也正因如此,这个重复没有测试,且是刻意的:该分支不存在任何能让该块非空的形态,为它写测试等于断言代码到不了的状态。我保留了展开而非删除,与它上面同样带这条不变量的收敛块保持对称——这样将来若加入一项能在此触发的检查,不必重新发现这套接线。 4245 测试通过;这道"发生"门做了变异测试(移除第三个合取会让"只有 Critical 的轮次"断言变红)。 |
|
@qwen-code /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 冲突,直到移除标签或达到轮次上限。移除 |
doudouOUC
left a comment
There was a problem hiding this comment.
Review summary: This PR delivers the machine-readable half of the convergence diagnosis and the mechanism-health disclosures, completing the remaining work items from #9461. The architecture is sound — deriving the codes from the diagnosis rather than storing them, sharing the anchor predicate between the marker and the disclosure, and carrying the codes through the artifact with proper validation.
Key strengths:
- The code set is closed, deterministic, and matched from measurement facts — no constants or decisions in the matching logic.
- is computed once and shared between the verdict and the diagnosis, preventing drift.
- is extracted as a single shared predicate used by both the marker and the mechanism-health check, ensuring they cannot disagree about the anchor state.
- Test coverage is comprehensive: 10 mutation tests verified each load-bearing decision, and the compose-level tests validate the end-to-end wiring.
- The two findings from the CI bot's previous review (posture check firing on every default-config round 6+ and the APPROVE branch rendering the health block twice) are both fixed in
3fc1adb.
Environment note: Build-test could not run on this Windows machine (network connectivity issue during npm ci — ECONNRESET on the registry). The PR author reports npx vitest run src/commands/review/ passes with 4245 passed, 1 skipped, and tsc --noEmit + eslint are clean.
Verdict: APPROVE — the code is well-structured, well-tested, and follows the project's conventions. The CI bot's previous CHANGES_REQUESTED findings have been addressed.
Two blockers. The posture check still accused a round the posture was running correctly. SKILL Step 6 excludes a `[build]`/`[test]`/`[probe]` finding by source at ANY floor — it is pre-confirmed and stays inline whether or not the floor engaged — so a fully compliant round that defers every deferrable Suggestion and posts one such finding satisfied all three conjuncts. My previous round's argument for the gate was true and beside the point: when the code-side reroute has failed open, the MODEL-side posture is the layer in charge, and it carries the same carve-out. The count now excludes deterministic findings, read off the claim line through the same projection `floorEnforcedReroute` uses. The health block shared the convergence paragraph's trim rank while having no copy outside the posted body — the exact false-record class the convergence paragraph's own fix exists to close. Worse at the sharpest corner: with no diagnosis firing, rank 0 held ONLY the health note, so the trim notice named "the convergence observation" for a section that never existed. The block gets its own rank with its own name (shed before the paragraph, because its reader is the operator who has the terminal line), and the note now rides `ComposeReviewResult`, the `HEALTH:` terminal line and the durable artifact. Also: the anchor-chain check states what it measures and names what it cannot see (the scope is the only withholding leg visible from the body composer; a plan with no fetched sha, an unreadable plan, and a model identity drift also withhold, and are decided where the marker is built); `recommendations[].code` is checked against the closed set instead of cast into it; the absent-code list now covers the whole eleven-code menu; and the comment claiming an absent floor reads as `auto` "throughout this module" now says which reading folds and which does not. Nine mutations, each verified to turn a named test red. Two of them needed the fixtures fixed first: the health field and the codes are spread into three separately-maintained result constructions, and the tests only reached one of them.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round result: no action neededThis round made no code changes. Every feedback point in this window was verified against Verification of each pointRound-0 review (two blockers, fixed in
Round-1 ledger (nine findings, fixed in
Commands run and results
No failed checks, no still-red checks, no conflicts. Nothing remains for this round to implement. 中文说明Autofix 本轮结果:无需处理本轮未做任何代码改动。本窗口内的每一条反馈都已对照 逐条核验第 0 轮评审(两条阻断项,已在
第 1 轮清单(九条发现,已在
实际执行的命令与结果
无失败检查、无持续飘红的检查、无冲突。本轮没有需要实现的内容。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:2653 — [probe] openCriticals suppression wiring unpinned end-to-end
中文说明
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
The blocker: `land-and-defer` matched on a zero that was not a confirmed zero. A round capped `cannot-tell-existing-critical` posts no Criticals precisely BECAUSE existing ones could not be ruled on — the entries ride their own channel, are never counted, and were never shown fixed — and `findings-unverified-at-compose` is the same shape. The body would then say "Unresolved, please confirm:" and "no Critical is open" at once, and the artifact would tell a machine consumer to merge. The count is passed only when the blocker state is established, and the module's own "absent is not zero" rule withholds the code otherwise. Four of the seven suggestions are corrections to things this branch itself introduced, three of them last round: - the health note's own rank made the convergence block's "shed before every other" comment false; it now says which one goes first and why; - the accusation counted pathless Suggestions, which the floor structurally cannot defer — no deferral entry can be built without a path — so it accused the posture of failing to move something it has nowhere to move; - the anchor-chain sentence attributed the withhold to the round's SCOPE while the predicate it reads also fires on a dimension gap and on verdict caps; it now says only that the round did not close cleanly, and the docstring names all three legs; - and the comment misplacement I claimed to have cleared was reintroduced one line up, by an insert anchored on the statement below its own comment. Also: the closed code vocabulary is declared once and the type derived from it, rather than a union and a runtime set kept in step by hand; the `HEALTH:` terminal line and the health validator's refusal path are pinned. Six mutations, each verified to turn a named test red. One needed the fixture fixed first — every test drove a Suggestion that had a path.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:4974 — [probe] claim-line-only deterministic-tag window in deferrableSuggestionsInline untested — a forged body-tail [test] tag silences the posture disclosurepackages/cli/src/commands/review/compose-review.test.ts:10164 — [probe] renders-once pin covers only the COMMENT branch — a double healthBlock spread in the REQUEST_CHANGES branch ships greenpackages/cli/src/commands/review/save-artifact.ts:317 (+4 locations) — [review] five sibling 'convergence sheds first' comments falsified by the rank −1 health note — incomplete remainder of the R2-6 fixpackages/cli/src/commands/review/compose-review.test.ts:9893 — [probe] no occurrence-count pin for convergenceBlock/continuityBlock — a duplicated convergence spread ships green
中文说明
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…ed too The blocker: last round's gate covered two of the three unestablished shapes. An unproven scope — a chunk nobody read, an uncoverable chunk, an idle agent, context unavailable — also posts zero Criticals while prior-round blockers sit unread, and the non-repost inference then reads them as fixed. Such a round carried "cannot show that any of the diff was read" and "no Critical is open" in one body, and the artifact told a machine consumer to merge over the unreviewed chunk. The diagnosis moves below `scopeUnproven` so the gate can read it. Also: the anchor-chain sentence claimed a clean close ENDS the full-diff re-reads, which the suite's own passing tests contradict — the marker also withholds on a missing fetched sha and on a model-identity drift, both of which a cleanly-closed round can carry. It now says "until a round's marker carries an anchor again", matching the docstring beside it. And the duplicate comment I reported fixed last round was only half-fixed: the file carried the sentence twice, and I deleted the copy above the health block while leaving the one that opens the recommendations block. Removed. Six mutations. Three needed the fixtures fixed first, all the same shape: the `cannot-tell` leg was measured against a bare plan whose unproven scope withheld the code anyway, and the round's own `land-and-defer` test likewise never had an established scope. A test that cannot fail without the line it names is not a test of that line.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 1: verify whether coverageFromTranscripts/requiredAgents/verificationGaps still produce a coverage entry when QWEN_CODE_SESSION_ID/QWEN_CODE_PROJECT_DIR are set (d…; chunk 1: run the amended test to confirm (no node_modules in the review worktree; npm ci not attempted within budget).
Test Plan (not a blocker): 4245 passed — this review observed 22795 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/save-artifact.ts:317 (+6 locations) — [review] six stale 'convergence sheds first' comments falsified by the rank −1 health note — re-discovery of the round-3 deferred family
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):chunk 1:verify whether coverageFromTranscripts/requiredAgents/verificationGaps still produce a coverage entry when QWEN_CODE_SESSION_ID/QWEN_CODE_PROJECT_DIR are set (d…;chunk 1:run the amended test to confirm (no node_modules in the review worktree; npm ci not attempted within budget)。
Test Plan(非阻断):4245 passed — this review observed 22795 passed。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Three rounds added one leg at a time to the same gate, and each addition left the previous rationale describing a gate that no longer existed. Round 5 found three more legs missing. This replaces the stack with one condition and one comment listing every leg, so the next one cannot be added in a place the others do not mention. The legs round 5 named: - a whiffed dimension withholds the anchor but was not withheld here, so the artifact told a machine consumer to merge over lines nobody re-read. The gate now reads the marker's OWN `anchorFailsClosed` predicate rather than enumerating its legs — which also SUBSUMES the two blocker states the gate listed separately, since both are caps and neither is `unreviewed-dimension`. Those conjuncts are removed rather than left as dead code that reads like extra protection. - a truncated work list makes the non-repost inference unsound: a shed Critical is neither re-posted nor ruled on. The same `complete` flag the freshness rule already reads. - a pure-foreign list holds none of this account's entries, so its own open Criticals cannot be re-posted at all. Also the three stale comments this branch left behind: the superseded "until one closes cleanly" wording in the `PrevRound.anchored` docstring and in a test comment (the rendered text and its pin say "until a round's marker carries an anchor again", and the negative pin is widened to a regex that catches both spellings); and the health computation's claim that `cappedBy` is still being appended below it, which the previous round's move made false — it now names the real constraint, `dimensionGapsAreDepthOnly`. And the fixture whose own numbers proved the list incomplete (`fresh: 9` over a one-entry work list) no longer blesses an inference conditioned on completeness; it uses a shape the pipeline's writer can produce. Six mutations, one per leg, each verified to turn a named test red. The new arms are table-driven from the shape that DOES offer the ending, flipping exactly one leg per arm — the fixture failure that let three of these legs ship unpinned was arms that would have withheld the code anyway.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": full npm run typecheck not run (tests compiled and passed; no type-level concerns surfaced in the walk).
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/convergence.ts:552 — [probe] non-zero openCriticals passthrough through diagnoseConvergence is mutation-blind
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)":full npm run typecheck not run (tests compiled and passed; no type-level concerns surfaced in the walk)。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Round summary — PR #9623Two new automated Suggestions this round; both fixed. No Criticals, no failed Feedback dispositionsFixed this round
Mutation probes (witnesses for the new pins)
Each pin kills exactly the mutant the finding named, and nothing else. Re-verified, already fixed in earlier commitsThe rounds-2/3/4 findings listed in this window's feedback were fixed in Not work this round
Files changed
Verification
中文说明轮次总结 — PR #9623本轮有两条新的自动化 Suggestion,均已修复。没有 Critical、没有失败的 反馈处置本轮修复
变异探针(新钉住测试的见证)
每条钉住测试恰好杀死对应发现点名的变异体,不影响其他测试。 已在早期提交修复、本轮重新验证本窗口反馈中列出的第 2/3/4 轮发现已分别在 本轮不处理
变更文件
验证
🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈(门自动测量):bite 检查无法验证测试侧修复,该解决仅以轮次摘要为凭。 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/模型 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 4245 passed — this review observed 22803 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):4245 passed — this review observed 22803 passed。
— qwen3.8-max via Qwen Code /review (v0.21.15)
doudouOUC
left a comment
There was a problem hiding this comment.
Round 6 review
The new commit (771e52e) pins the two round-5 Suggestions with targeted tests. The depth-only dimension and merged-foreign legs now each have a fixture that asserts land-and-defer still fires on the positive shape. The deferred items from round 4 (stale "sheds first" comments) and round 5 (mutation-blind probe) remain standing and are not blockers.
Finding: the posture health check is dead code
postureNotEngaging is gated on three conjuncts:
convergence.criticalFloorKind !== undefined &&
convergence.floorEnforcementEngaged === false &&
deferrableSuggestionsInline(input.draftedComments) > 0,
The first two are computed from the same function chain — criticalFloorKind calls normalizeSeverityFloor → floorResolvesCritical, and criticalFloorInEffect (which computes floorEnforcementEngaged) calls normalizeSeverityFloor → floorResolvesCritical. Both resolve "auto" for prevRound >= 5 (current code: thisRound >= 6). So criticalFloorKind !== undefined is equivalent to floorEnforcementEngaged === true, and the conjunction A && !A is always false, for every input:
| severityFloor | criticalFloorKind | floorEnforcementEngaged | A && !A |
|---|---|---|---|
| undefined (default) | "auto-resolved" | true | false |
| "critical" | "explicit" | true | false |
| "suggestion" | undefined | false | false |
| "auto" + context failure | undefined | false | false |
| garbage | undefined | false | false |
The deferrableSuggestionsInline > 0 guard is never reached. The mechanism-health body never renders the posture clause, and the HEALTH: terminal line never fires for the posture gap.
The fix: floorEnforcementEngaged is described as "strict" (its docstring says "a posture the state never named cannot move a finding out of the posting set"), but the implementation uses criticalFloorInEffect which resolves "auto" like the reporting reading. A strict check would be:
floorEnforcementEngaged: normalizeSeverityFloor(input.severityFloor) === "critical",
This makes floorEnforcementEngaged false for default config, and the deferrableSuggestionsInline > 0 guard (already present) then correctly distinguishes "a posture that is engaged in name and not in effect WITH a manifestation" from "a posture that is engaged in name but had nothing to defer" — the three-state shape the round-1 review intended.
The criticalFloorInEffect function itself is used correctly by floorEnforcedReroute (it SHOULD resolve "auto" there, because the code-side reroute must act on the resolved floor). The issue is only that the same function is used for a different purpose in floorEnforcementEngaged, where a stricter check is needed.
(Suggestion — not a blocker, because the dead code is harmless: it just never fires. But it means the "engaged in name and not in effect" disclosure is never emitted, so the mechanism-health half of this PR covers only the anchor-chain leg in practice.)
Static verification (from the diff, no code execution)
- openCriticals computation: after both bodyCriticals.push sites (stray-marker, script-lint gate). Verified.
- anchorFailsClosed: shared predicate, used by ledgerMarkerFor and renderMechanismHealth through the same inputs. Verified.
- draftedFindingsOf length bound: id.length <= LEDGER_MAX_ID (24), matching idFor's rejection at > 24. Verified.
- severityFloorKnown: fully removed, no stragglers. Verified.
- APPROVE branch health block: spread once (not twice). Comment documents the fix. Verified.
- deferrableSuggestionsInline: excludes deterministic findings (build/test/probe via DETERMINISTIC_TAG_RE) and pathless comments, matching floorEnforcedReroute's exclusion. Verified.
- recommendationCode: validates against RECOMMENDATION_CODES closed set; throws on unknown values. Verified.
- Prose derived from matched set: renderConvergenceDiagnosis uses recommendationsFor(d) and checks has(code) for each clause. Verified.
- save-artifact.ts comments: old "sheds first" comment before convergence was removed; new comments correctly placed before each block. Verified.
Summary
The architecture — deriving codes from the diagnosis, sharing the anchor predicate, mutation-testing every load-bearing decision — is what makes this codebase's review system reliable. The finding above is a straightforward fix (floorEnforcementEngaged using a strict check instead of the resolved check). The round-5 deferred items (one probe, one stale-comment family) are still deferred and not requested here.
doudouOUC
left a comment
There was a problem hiding this comment.
[qwen-code PR review] #9623 — 一轮审查结果(deepseek-v4-flash)
结论:ISSUES_FOUND
发现 1 条 Suggestion:
The posture health check is dead code.
criticalFloorKind !== undefinedandfloorEnforcementEngaged === falseare mutually exclusive because both are computed from the samenormalizeSeverityFloor→floorResolvesCriticalchain — both resolve"auto"forprevRound >= 5, sofloorEnforcementEngagedis alwaystruewhencriticalFloorKindis defined. ThedeferrableSuggestionsInline > 0guard is never reached.Fix: Use a strict check (
normalizeSeverityFloor(input.severityFloor) === "critical") forfloorEnforcementEngagedinstead ofcriticalFloorInEffect, which is correctly used byfloorEnforcedReroutefor a different purpose. ThedeferrableSuggestionsInline > 0guard (already present) then correctly distinguishes the three-state shape the round-1 review intended.
Static verification (all passed): openCriticals timing, anchorFailsClosed sharing, draftedFindingsOf bound, severityFloorKnown removal, APPROVE branch spread (once, not twice), deferrableSuggestionsInline exclusion, recommendationCode validation, prose derivation from matched set, save-artifact.ts comments.
(qwen 尝试自动发布 inline 评论,但未在 PR 上检测到新评论,因此以 body-only 形式补充提交。)
doudouOUC
left a comment
There was a problem hiding this comment.
[qwen-code PR review] #9623 — 一轮审查结果(deepseek-v4-flash)
结论:ISSUES_FOUND
发现 1 条 Suggestion:
The posture health check is dead code.
criticalFloorKind !== undefinedandfloorEnforcementEngaged === falseare mutually exclusive because both are computed from the samenormalizeSeverityFloor→floorResolvesCriticalchain — both resolve"auto"forprevRound >= 5, sofloorEnforcementEngagedis alwaystruewhencriticalFloorKindis defined. ThedeferrableSuggestionsInline > 0guard is never reached.Fix: Use a strict check (
normalizeSeverityFloor(input.severityFloor) === "critical") forfloorEnforcementEngagedinstead ofcriticalFloorInEffect, which is correctly used byfloorEnforcedReroutefor a different purpose. ThedeferrableSuggestionsInline > 0guard (already present) then correctly distinguishes the three-state shape the round-1 review intended.
Static verification (all passed): openCriticals timing, anchorFailsClosed sharing, draftedFindingsOf bound, severityFloorKnown removal, APPROVE branch spread (once, not twice), deferrableSuggestionsInline exclusion, recommendationCode validation, prose derivation from matched set, save-artifact.ts comments.
(qwen 尝试自动发布 inline 评论,但未在 PR 上检测到新评论,因此以 body-only 形式补充提交。)
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #9623 — feat(review): give the convergence observation a machine-readable half
Verdict: COMMENT — no Criticals. One Suggestion below.
Blocker re-checks
All 3 prior Criticals (R5-1, R5-2, R4-1) are verified fixed at HEAD:
- R5-1 (health block trim):
ComposeReviewResult.healthadded,HEALTH:terminal line present,RANK_NAMES[-1]entry for its own trim rank — confirmed by Chunk 3/4 review. - R5-2 (posture accusation over deterministic findings):
deferrableSuggestionsInlineexcludes deterministic-tagged findings viaDETERMINISTIC_TAG_RE— confirmed by Chunk 4 review and test coverage matrix. - R4-1 (openCriticals gate missing dimension-coverage leg):
anchorFailsClosedpredicate now used consistently, with tests coveringdimensionGapsAreDepthOnlypath — confirmed by Chunk 4 review and'still offers the ending when the only cap is the depth-only dimension'test.
Changes reviewed
The diff adds a machine-readable recommendations: [{code, basis}] field to ComposeReviewResult, a mechanism health note (health) with its own terminal line and trim rank, and fixes the postureNotEngaging false-accusation bug. 6 files, +1400/−84. All 6 chunk territories were reviewed; the removed-behavior audit (1b) and test-coverage matrix both ran clean.
Suggestion
deferrableSuggestionsInlineanddraftedFindingsOflack direct unit tests (both are internal functions tested only through downstream consumers). ThedraftedFindingsOflength-bound fix (LEDGER_MAX_IDguard) and thedeferrableSuggestionsInlinethree-way exclusion logic (deterministic tag, pathless, deferrable) are correct at HEAD, but a future change that introduces a new exclusion path indeferrableSuggestionsInlinediverging fromfloorEnforcedReroutewould not be caught. Consider adding direct unit tests for both functions.
Reviewed at: 771e52e375664c78794e665b1b69270a5b73f581
…lper Round 6's two actionable suggestions, both test-side: - `deferrableSuggestionsInline` and `draftedFindingsOf` were pinned only through composeReview. Both are exported and unit-tested directly: the severity gate, the claim-line-only deterministic-tag window, the pathless exclusion, the LEDGER_MAX_ID bound, the first-keeps dedupe, and a parity test asserting the count equals the exact set `floorEnforcedReroute` moves when the floor engages — the divergence the suggestion names. - The identical covered-plan + prev-ledger setup pasted at 12 sites collapses to one `coveredWithLedger()` beside `coveredPlan()`, deriving the side-file name from the same prNumber. A typo in a hand-coupled name failed nothing — the reader swallows ENOENT and the test silently measured round 1; broken at the one place now, 13 tests redden. Five mutation probes (helper name, pathless gate, deterministic gate, id bound, dedupe), each verified red and restored to green. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: Round summary — PR #9623 (round-6 feedback)Three distinct findings arrived (one of them posted by three review bodies). One is declined with probe evidence, two are implemented. No Criticals, no failed checks, no conflict.
Also noted, no action available: rv:4997160214's disclosures (the skipped integration suite and the Test-Plan count mismatch Declined: "the posture health check is dead code"The claim is that So the first two conjuncts hold on every default-config round from 6 on — precisely the shape the comment above The path is also exercised end to end at HEAD: the existing test For these reasons the suggested rewrite ( Implemented: direct unit tests for the two draft projections (rv:4997304518)Both functions are exported from compose-review.ts (source diff: the two
Implemented: one ledger-setup helper (rc:3833465431)
Environment repair (not a code change)
Verification
中文说明本轮总结 — PR #9623(第 6 轮反馈)共收到 3 条不同发现(其中一条由三个审查主体分别发布)。1 条附探针证据予以拒绝,2 条已实现。无 Critical,无失败检查项,无冲突。
另有一条无法处理的记录:rv:4997160214 的披露(被跳过的集成测试套件,以及 Test Plan 计数不一致 拒绝:"姿态健康检查是死代码"该主张认为 因此,从第 6 轮起,每一个默认配置轮次都同时满足前两个合取项 — 这正是 该路径在 HEAD 上也有端到端覆盖:既有测试 基于上述理由,未采纳建议的改写( 已实现:两个草稿投影的直接单元测试(rv:4997304518)两个函数均已从 compose-review.ts 导出(源码 diff 仅有两个
已实现:一个 ledger 初始化辅助函数(rc:3833465431)
环境修复(非代码变更)在任何编辑之前,本检出上的 验证
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/模型 |
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.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:3822 — [probe] postureNotEngaging's first conjunct (criticalFloorKind !== undefined) is unpinned on its negative sidepackages/cli/src/commands/review/compose-review.ts:3828 — [probe] anchorChainBroken's round-1 guard ((prev.round ?? 0) > 0) is unpinnedpackages/cli/src/commands/review/compose-review.test.ts:10168 — [probe] renders-once pin for the posture clause exists only on the COMMENT shape; the REQUEST_CHANGES branch is toContain-onlypackages/cli/src/commands/review/compose-review.ts:4931 (+8 locations) — [review] eight stale "convergence sheds first" / "only other copy" records falsified by the rank −1 health note
中文说明
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🔀 Base updated: red check(s) [Dependency CVE audit] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base:红色检查 [Dependency CVE audit] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main,CI 将重新运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): chunk 5: could not execute convergence.test.ts — the shared review worktree has no node_modules and npm ci there would race concurrent review agents; every assertion….
Test Plan (not a blocker): 4245 passed — this review observed 22938 passed.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/compose-review.ts:1786 — [review] stale criticalFloorKind fold comment — half of the claimed two-comment cleanuppackages/cli/src/commands/review/lib/convergence.test.ts:675 — [probe] batch-fixes basis pin — symmetric fixture, first clause onlypackages/cli/src/commands/review/compose-review.test.ts:9831 — [probe] land-and-defer positive controls stand on an unproducible work listpackages/cli/src/commands/review/compose-review.test.ts:2738 — [review] PR-edited test carries two comments each claiming the first ladder slotpackages/cli/src/commands/review/compose-review.test.ts:10168 — [probe] posture-clause render-once pin leaves the REQUEST_CHANGES spread unpinnedpackages/cli/src/commands/review/save-artifact.ts:317 (+2 locations) — [review] stale 'convergence sheds first' exclusivity comments (2 locations)packages/cli/src/commands/review/compose-review.ts:4939 — [review] stale 'first thing the ladder sheds' comment beside the new HEALTH blockpackages/cli/src/commands/review/lib/convergence.test.ts:689 — [probe] land-and-defer basis string never asserted
中文说明
无阻断问题。LGTM!✅
未探索到全部深度(达到工具调用预算):chunk 5:could not execute convergence.test.ts — the shared review worktree has no node_modules and npm ci there would race concurrent review agents; every assertion…。
Test Plan(非阻断):4245 passed — this review observed 22938 passed。
收敛姿态下延后(第 8 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round summary — PR #9623 (no action taken)This round made no code changes and no commits. Every feedback point is either already resolved in the current tree (verified with evidence below) or explicitly not requested this round. Feedback triage[rv:4998299951] "CI failing: Dependency CVE audit" (automated reviewer downgrade)Already resolved by the merge from main that is on this branch; no change is needed or permitted this round:
[rv:4998756797], [rv:4998756871], [rv:4998756948] (@wenshao)Empty review bodies — nothing to address. [rc:3834912108], [rc:3834912176], [rc:3834912261] (@wenshao) — verified as already resolved in codeEach comment reports a prior-round finding as already addressed on the branch; all three claims were re-verified against HEAD:
The three ids are recorded in Deferred under convergence posture (round 7)The four deferred items in the review are explicitly "recorded, not requested in this round" — left untouched per the reviewer's deferral. No code changes, thread resolutions, or replies for them. VerificationCommands actually run this round (for evidence; nothing was committed):
中文说明Autofix 轮次总结 — PR #9623(无需处理)本轮未做任何代码修改,也没有提交。所有反馈点要么已在当前代码树中解决(下文附证据核实),要么本轮明确不要求处理。 反馈分类[rv:4998299951] “CI failing: Dependency CVE audit”(自动审查器降级)该问题已由本分支上的 main 合并解决;本轮无需也不允许做任何修改:
[rv:4998756797]、[rv:4998756871]、[rv:4998756948](@wenshao)审查正文为空——无可处理内容。 [rc:3834912108]、[rc:3834912176]、[rc:3834912261](@wenshao)——已核实为代码中已解决每条评论都报告某个先前轮次的发现已在分支上处理;三条声明均已对照 HEAD 重新核实:
三个 id 已记入 收敛姿态下延后(第 7 轮)审查中四条延后项明确为“已记录,本轮不要求修改”——按审查器的延后决定保持不动。不对它们做代码修改、线程解决或回复。 验证本轮实际运行的命令(用于取证;未提交任何内容):
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified against the head:
- The single-source wiring holds where it matters:
openCriticalsis computed once (after the lastbodyCriticals.push) and feeds both the verdict and the diagnosis; the prose reads the floor rung off the matched set (has('stem-surface')) instead of re-deriving it;anchorFailsClosedis the one predicate both the marker and the mechanism-health check read, so the disclosure cannot describe a round the marker anchored. - The conservatism is real:
land-and-deferrequires an explicitopenCriticals === 0and is withheld exactly where the "not re-posted means fixed" inference is unsound (anchor fail-closed, incomplete work list, pure-foreign list); the health checks under-report by stated design rather than over-report. - The drafted-id bound aligns both ends of the pipeline: the carry path now applies the same
LEDGER_MAX_IDboundidForenforces, and the tests pin the at-cap/over-cap edge plus the shortened-work-list case where the bound is what carries it. - The artifact validator checks codes against the closed set rather than casting, and the health note's rank -1 shed order plus terminal copy mirrors the existing convergence-paragraph pattern.
CI is green on head (15 passing checks, none failing).
main landed #9623, which gives the convergence observation a machine-readable half (`recommendations`, a mechanism-health note, and a `HEALTH:` terminal line). Every conflict is two independent additions to the same seam, so both sides ride; nothing of either was dropped. Resolutions: - Imports, the early-return parts list, the terminal lines and the returned verdict: plain unions. The health note prints under `HEALTH:` and the advisory under `RESIDUAL-RISK:`, one record each. - `validateVerdict`: main's `recommendations` and `health` blocks and this branch's `residualRisk` block are all additive; the shared closing brace that the conflict left over now closes the first of them explicitly. - `save-artifact.test.ts`: same shape, two whole `it()` blocks sharing one terminator. Two things the merge changed that needed more than a union: - `postedFresh` moved. #9623 settles the whole diagnosis derivation after the gate pushes, and this branch's window runs on `postedFresh`, so the assessment moved down beside it — where both of its placement reasons now coincide (a complete `bodyCriticals`, and a defined fresh count). Caught by tsc as use-before-declaration, not by the tests. - Rank -1. #9623 gives the health note its own rank below the observation, so the trim ladder is five ranks, not four. The tail clause still keys on rank 3 and is still correct (the health note keeps the composed result and the `HEALTH:` line), but three enumerations that name the ranks were left stale by the merge and are updated: the ladder docstring, the `noteTrimmedRanks` docblock, and the bundled skill's trim-order prose with its two assertions. An enumeration that silently loses a member is the stale-record class this branch keeps fixing. One test fixture moved rather than its assertion: the rank-2-only overflow test exists to show the advisory named for itself as the ONLY trimmable section, and the health note fired beside it because that fixture's predecessor carried no anchor. The predecessor is now anchored, so the test still measures what it was written to measure instead of being relaxed to two sections. packages/cli 4429 pass / 1 skipped; packages/core skills 377 pass; typecheck, ESLint and Prettier clean on both. Mutation matrix re-run after the merge — reverting the window to totals, deleting the backlog veto, deleting the posture guard, and dropping `residualRisk` from the artifact each still turn the suite red.
…stamp (#9526) R8-1 is correct. The posture-change guard paired two different readings across the window's ends: this round's engagement is the strict `criticalFloorInEffect`, but the predecessor's `floor` stamp is written from `criticalFloorKind`, the reporting fold — which folds an absent `severityFloor` into `auto` and stamps `c` on any round >= 6 the enforcement backstop never touched. Reproduced through the real code first: criticalFloorKind(undefined, false, 6) = 'auto-resolved' -> stamps 'c' criticalFloorInEffect(undefined, false, 6) = false -> Suggestions post so a predecessor that still posted Suggestions passed the guard, and the advisory published "the severity floor will not converge it" one round after enforcement actually started. Neither fix direction the finding names is taken. Restamping the marker from the enforcement reading would leave the sibling diagnosis comparing this round's reporting stamp against a predecessor's enforcement stamp — the same cross-reading defect moved into #9623's feature — and #9623 chose the reporting reading deliberately, because its advice quotes the floor back to the author. Special-casing a "newly named" floor needs the predecessor's raw `severityFloor`, which no marker carries. The evidence is already in the work-list instead. Enforcement moves drafted Suggestions out of the posting set before the marker is built, so an engaged round's list is Critical-only and an un-enforced one is not — measured through the real composer across all four postures: floor=critical (engaged) work list ["C"] stamp c floor=auto, round 7 (engaged) work list ["C"] stamp c floor ABSENT, round 7 (folded c) work list ["C","S"] stamp c <- the hole floor=suggestion (not engaged) work list ["C","S"] stamp o `prevPostedSuggestion` is that fact, and it suppresses on the POSITIVE observation so the two ways it can be wrong land on opposite sides: a shortened list that shed its Suggestion reads as engaged (the truncation caveat the backlog veto already carries), while a pathless Suggestion an engaged round left inline reads as un-enforced and costs one round of silence. Unknown abstains, like every other fact read off that list. Mutation matrix: deleting the guard, tightening it so an unknown predecessor suppresses, and pointing the wiring at the wrong severity each turn the suite red — the first on both the unit arm and the end-to-end fixture built from the finding's own witness. packages/cli: 4432 pass / 1 skipped. Typecheck, ESLint and Prettier clean.
|
Released in v0.22.0. |
…d-with-residual-risk) (QwenLM#9526) * feat(review): add the persistently-critical convergence advisory The severity floor converges a healthy loop — Suggestions stop posting and the volume falls to the Criticals, then to zero as those get fixed. But a loop whose Criticals never clear — the security-sensitive PR under adversarial review — posts Criticals every round forever: the floor engages, the Suggestions stop, and the volume flatlines at the Critical count instead of falling. Nothing before this said so. This adds the shape detector and its ONE recommendation: - lib/convergence.ts — `convergenceAssessment` computes one fact from the carried telemetry (Criticals stood in the previous round's work-list AND stand again this round, with the two-round posting window present and not shrinking) and, when it fires, returns the `land-with-residual-risk` recommendation. Pure data, never authority: no threshold, no blocking, no merge/close — every input degrades OPEN, so absence is fail-safe, never a suppressed finding. - compose-review wires it: `prevLedgerFacts` now recovers the previous work-list's Critical presence beside the round and volume; the assessment surfaces on three surfaces — a structured `convergence` field on the composed JSON, a rank-1 non-capping body disclosure, and a terminal CONVERGENCE line — each advisory-only and self-disclaiming, with a blank residual-risk inventory scaffold (attack surface · attacker-dependency · blast radius) for the maintainer's risk-acceptance decision. The exit the floor cannot provide: when the loop is provably stuck on Criticals, the tool names the maintainer's decision (merge, carrying the residual risk) instead of opening another round. Advisory only — it never blocks this review. Closes the convergence-exit gap in QwenLM#9278; evidence and design in QwenLM#9410. * fix(review): surface the convergence advisory on every reachable event, gated on floor engagement (QwenLM#9526) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): give the trimmed convergence advisory its own disclosure rank (QwenLM#9526) The advisory shared trim rank 1 with the deferral display, but every rank-1 disclosure surface names "the deferred-findings list" — a fired zero-deferral round whose body overflowed posted a trim notice asserting a deferral list that never existed while the dropped advisory went unnamed. The advisory now holds its own rank (and RANK_NAMES entry), yielding after the deferral display and before the not-reviewed disclosures. Adds the overflow fixture that pins the yield and the relocated-arm firing fixture that pins the third thisCriticals term, and corrects the prevLedgerFacts threat docstring: under `auto` the floor-engagement conjunct is forgeable via the carried round, so the only unforgeable conjunct is this round's own standing Critical. * fix(review): count the script-lint gate's Criticals in the convergence signal (QwenLM#9526) The persistently-critical signal read `thisCriticals` before the gate pushed its Criticals into `bodyCriticals`, and the ledger work-list feeding the next round's persistence half omitted them too. A loop whose standing blocker is the deterministic [lint] gate — the exact shape the signal exists to name — held the whole conjunction semantically while the advisory stayed silent: the count was taken before the array was complete, and the gate-only round recorded no sev 'C' for its successor to recover. The assessment now runs after the relocated and gate pushes and reads the completed array with the same semantics as the verdict's own `c` (the explicit relocated term drops with the push that already carries it), and the gate's Criticals join the marker work-list. Adds the handler fixture arming the gate end to end — advisory fires, marker records sev 'C' — and pins both branches of the trim notice's copy-location conditional, which had no oracle on either side. * fix(review): close the round-5 oracle gaps on the residual-risk advisory (QwenLM#9526) Round 5 reviewed the merge that landed QwenLM#9461 underneath this branch and found four suppress paths the merge introduced with no end-to-end oracle, plus one standing comment overclaim. Each finding was reproduced as a surviving mutant before the fix and re-run after, so every test added here is one that actually kills something. R5-2 — `residualRisk` is carried into the durable artifact instead of being omitted from it. The omission's stated reason ("the advisory rides the persisted body") is false on exactly the rounds that need the record: rank 2 sheds before the not-reviewed disclosures, so a fired-but-trimmed round left a maintainer reading `.qwen/reviews` a "did not fit" breadcrumb and none of the facts behind the `land-with-residual-risk` call. Its sibling `convergence` is allow-listed one paragraph up for that precise reason, and the merge had put the two on opposite rules. Shape-checked like every other field on this boundary rather than passed through. R5-1 — the persistence conjunct had no silence fixture. Every firing fixture carries sev `C` in the prev ledger, so replacing the derivation with a bare `true` shipped the suite green while a round introducing its FIRST Critical would fire `land-with-residual-risk`. Added a fixture whose predecessor holds Suggestions only, all other conjuncts true. R5-3 — the enforcement-vs-reporting floor reading had no oracle for the one input where the two disagree: a genuinely ABSENT `severityFloor` at round >= 6, which the reporting reading folds to `auto`. Every advisory fixture passed `severityFloor: 'auto'` explicitly, so the swap shipped green and would publish "The severity floor will not converge it" over a round whose enforcement backstop moved nothing. Added a fixture with no `severityFloor` key at all. R5-4 — the two silence fixtures asserted only absences. `prevLedgerFacts` swallows every recovery failure into round 0, so a predecessor that never loaded produced the same silence and the arms they claim to pin were vacuous. Both now assert the VOLUME line quoting the predecessor's volume as a positive recovery sentinel. R4-1 — the marker path's second `scriptLintGate` run is left in place: it lives in a different function from the body composer's, threading the value across would add a seventh positional parameter for plumbing, and the two agree because the gate is pure in `planPath` over inputs immutable within one synchronous compose. What was wrong was the comment claiming it was "the same gate the body ran"; it now states the actual invariant and the actual hazard — an edit that filters what the BODY pushes must change this list too. packages/cli: typecheck, ESLint and Prettier clean; src/commands/review 4301 pass / 1 skipped. Mutation matrix (compose-review + save-artifact, 475 tests): baseline green; `prevHadCritical: true`, the reporting-reading swap, dropping `residualRisk` from the persisted verdict, and a vacuous ledger recovery each turn the suite red. * fix(review): act on the round-6 deferred list for the residual-risk advisory (QwenLM#9526) Round 6 posted no findings and deferred ten observations under the convergence posture. Eight are addressed here; each was reproduced as a surviving mutant first and re-run after, and the two that are not addressed are recorded below with the reason rather than left silent. Correctness: - The volume window straddled a posture change. The round the floor engages on compares a Critical-only volume against a predecessor that was still posting Suggestions — a drop that is the posture, not the loop — and on a flat pair the advisory could publish "the severity floor will not converge it" after one round of the floor. `ConvergenceFacts` now carries `prevFloor` and a recorded `o` predecessor suppresses. Read the way the sibling diagnosis in the same module reads it: a floor that was never recorded is not a floor that DIFFERS, so pre-field markers evaluate exactly as before. Pinned in both directions — deleting the guard and tightening it to reject unrecorded floors each turn the suite red. - `noteTrimmedRanks`' tail clause keyed on the advisory instead of on the disclosures. Over a combined rank-2-and-3 drop it read "another copy — the advisory also rides the composed JSON", telling the operator the trimmed set was backed up when the half that is not backed up was exactly the half the sentence exists to rescue; over a rank-0 drop it read "their only other copy" for a paragraph the composed result does carry. It now keys on rank 3. The artifact stays unnamed here — naming it sent the operator to a deferral list that does not exist, which the existing test caught. - The terminal `RESIDUAL-RISK:` record spread one labelled line over seven, six of them unlabelled, because the advisory carries a markdown table for the body. Collapsed at the print site only: the pipes survive, so the inventory's three columns still reach the operator on the round where the body budget shed the formatted copy. Accuracy of the record: - The `PersistedVerdict` comment claimed `residualRisk` sheds "before anything else". It is rank 2; `convergence` is rank 0. What they share is that both CAN go. - The bundled skill enumerated two of the four trim ranks and stated the no-durable-copy rule without its exception. Both assertions in `SKILL.test.ts` move with the prose. New oracles (test-only): - The advisory-only guarantee — the claim the whole feature rests on — was unpinned: a fired round now asserts the event stays where the findings put it and that `cappedBy` gains nothing. - The floor-futility sentence was pinned only negatively; it now has a positive assertion in both languages. - The zh advisory's scaffold columns and its Critical-count interpolation slot had no oracle. `FIRE` is deliberately asymmetric (2 Criticals, volume 3/3) so a template reading the wrong slot shows. - The rank-ordering guard could not tell rank 2 from rank 3; the combined- drop test closes it — the `trim: 3 -> 2` mutant now fails five tests. Not done, deliberately: - The validator does not re-assert `criticals >= 1` / `posted >= prevPosted`. Those are `convergenceAssessment`'s construction invariants, and a second statement of them at the save boundary is a rule free to drift from the first — with the artifact, the durable record, as what gets thrown away when it does. Identity is pinned instead (`shape`, `recommendation`) and the counts are shape-checked. - The marker path's second `scriptLintGate` run stands (R4-1); the reasoning is on that thread and at the call site. packages/cli: 4305 pass / 1 skipped. packages/core skills: 376 pass. Typecheck, ESLint and Prettier clean on both workspaces. * fix(review): measure the residual-risk window on fresh findings, not totals (QwenLM#9526) Round 7 posted one Critical and it is correct. The volume conjunct compared posting TOTALS, and Step 6 re-posts every still-standing ledger Critical under its original id — so the total only ever rises and a converging loop reads as a stuck one. Reproduced through the real `composeReview` before touching anything: round 6 posts 5 first-time Criticals; the author fixes 3; round 7 re-posts the 2 that stand and drafts 4 new. Fresh 5 -> 4 is a loop settling, the total went 5 -> 6, and the advisory fired `land-with-residual-risk` over it. The window now runs on the fresh pair the marker already carries — `postedFresh` and `prev.fresh`, the same numbers the loop-settling observation in the same module trends on, so the two features cannot disagree about what a round produced. `prev.fresh` absent degrades open. Applying only that change would have introduced a second false fire, so it does not ship alone. The posting total was silently covering a case the fresh window is blind to: a reviewer finding nothing new for two rounds while the author clears blockers sits at fresh 0 against fresh 0, which "not falling" reads as stuck. Probed on the pre-change code — backlog 5 -> 3 with zero fresh both rounds is silent today (3 < 5) and would have fired under a fresh-only window. The assessment therefore also takes the standing Critical count and vetoes on observed shrinkage. A veto rather than a requirement, on positive evidence only: the work-list it counts is the one the marker's byte budget may have shortened, and an undercount can only hide shrinkage, never manufacture it — so an unknown predecessor abstains instead of silencing a genuinely stuck loop. Unlike the sibling diagnosis, this signal does NOT require `prev.fresh > 0`. That module is about a loop generating work; this one is about work that never clears, and Criticals standing round after round with nothing new is the shape itself, not a quiet loop. The backlog veto is what separates it from a backlog being worked down. The reported numbers are renamed with what they now measure — `posted` / `prevPosted` become `fresh` / `prevFresh` on `ConvergenceFacts`, `ConvergenceAssessment` and the persisted artifact — and the advisory prose follows in both languages. Feeding fresh counts into fields printed as "the posting volume" would have swapped one false record for another. Verified as five shapes through the real command, then pinned as tests: the reported fresh-shrinking loop is silent; the clearing backlog is silent; a pre-fresh marker is silent; and both firing shapes still fire — the same Criticals re-posted at zero fresh, and new Criticals every round. Mutation matrix (539 tests): reverting the window to totals, deleting the backlog veto, and tightening the veto to suppress on an unknown predecessor each turn the suite red. packages/cli: 4310 pass / 1 skipped. Typecheck, ESLint and Prettier clean. * fix(review): prove the predecessor's floor enforced, don't trust its stamp (QwenLM#9526) R8-1 is correct. The posture-change guard paired two different readings across the window's ends: this round's engagement is the strict `criticalFloorInEffect`, but the predecessor's `floor` stamp is written from `criticalFloorKind`, the reporting fold — which folds an absent `severityFloor` into `auto` and stamps `c` on any round >= 6 the enforcement backstop never touched. Reproduced through the real code first: criticalFloorKind(undefined, false, 6) = 'auto-resolved' -> stamps 'c' criticalFloorInEffect(undefined, false, 6) = false -> Suggestions post so a predecessor that still posted Suggestions passed the guard, and the advisory published "the severity floor will not converge it" one round after enforcement actually started. Neither fix direction the finding names is taken. Restamping the marker from the enforcement reading would leave the sibling diagnosis comparing this round's reporting stamp against a predecessor's enforcement stamp — the same cross-reading defect moved into QwenLM#9623's feature — and QwenLM#9623 chose the reporting reading deliberately, because its advice quotes the floor back to the author. Special-casing a "newly named" floor needs the predecessor's raw `severityFloor`, which no marker carries. The evidence is already in the work-list instead. Enforcement moves drafted Suggestions out of the posting set before the marker is built, so an engaged round's list is Critical-only and an un-enforced one is not — measured through the real composer across all four postures: floor=critical (engaged) work list ["C"] stamp c floor=auto, round 7 (engaged) work list ["C"] stamp c floor ABSENT, round 7 (folded c) work list ["C","S"] stamp c <- the hole floor=suggestion (not engaged) work list ["C","S"] stamp o `prevPostedSuggestion` is that fact, and it suppresses on the POSITIVE observation so the two ways it can be wrong land on opposite sides: a shortened list that shed its Suggestion reads as engaged (the truncation caveat the backlog veto already carries), while a pathless Suggestion an engaged round left inline reads as un-enforced and costs one round of silence. Unknown abstains, like every other fact read off that list. Mutation matrix: deleting the guard, tightening it so an unknown predecessor suppresses, and pointing the wiring at the wrong severity each turn the suite red — the first on both the unit arm and the end-to-end fixture built from the finding's own witness. packages/cli: 4432 pass / 1 skipped. Typecheck, ESLint and Prettier clean. * fix(review): refuse a pure-foreign work-list as this account's history (QwenLM#9526) Correct, and reproduced through the real composer before changing anything. Recovery adopts the highest-round marker whoever posted it. Where that marker was NOT merged over this account's own findings, this account's entries are in no work list at all — the state `openCriticals` already refuses to infer across, one screen up in the same function. Every prev-round fact this signal reads comes off that list, and it read it unconditionally: pure-foreign {foreign:true, merged:false} -> FIRES own list {foreign:false} -> FIRES merged {foreign:true, merged:true} -> FIRES An own round-6 marker that was a clean LGTM (empty findings, fresh 0, floor stamped `c`), a foreign same-round marker carrying Criticals and no Suggestions winning recovery, and one Critical drafted this round were enough to publish "Criticals stood in the previous round's work-list and stand again this round — land-with-residual-risk" over this account's own LGTM. All three list-derived facts are withheld on that state, not just `prevHadCritical`: it alone silences the assessment today, but leaving the other two reading a stranger's list is a hole waiting for the next edit to re-open. `prevPostedSuggestion` in particular reads ABSENCE, and a stranger's Critical-only list is exactly the shape that reads as "the floor enforced". Merged foreign lists are deliberately NOT withheld: the union keeps this account's own certified entries under their own ids, which is the part that makes the list speak for this account again — the same distinction `openCriticals` draws. The test drives all three arms and asserts them as one table, so the fix is pinned in both directions: a mutant disabling the gate fires on the stranger, and a mutant widening it to any `foreign` marker silences the merged arm. Both turn the suite red, as does un-gating `prevHadCritical` alone. Not changed, and recorded rather than left implicit: a TRUNCATED work-list still reads as this account's. Truncation shortens our own list, which is a different thing from a stranger's, and the direction it errs in is already documented on `prevPostedSuggestion` and the backlog veto. Requiring completeness would silence the advisory on precisely the deep-work-list rounds it exists for. packages/cli: 4433 pass / 1 skipped. Typecheck, ESLint and Prettier clean. * fix(review): stop a gate Critical compounding, and qualify a truncated reading (QwenLM#9526) Round 11's two Criticals. Both reproduced through the real composer before anything was changed. R11-2 — a standing gate Critical entered the posting set twice, and the pair compounded. This is a regression from this branch's own commit d72287c: once the gate's `[lint]` Criticals are in the carried work-list, SKILL Step 6's still-standing rule tells the model to re-post the entry under its original id while compose re-derives the same Critical from the report. `buildLedger` keys by claimed id and the regenerated copy claims none, so it minted a second id beside the carried one: ROUND1 work-list [R1-1] blocker rendered once ROUND2 work-list [R1-1, R2-1] rendered twice ROUND3 work-list [R1-1, R2-1, R3-1] rendered three times FLIP (revert the gate spread): round-1 work-list [], rendered once `withoutGateReposts` drops the re-post, keeping the GATE's copy rather than the model's. That direction is load-bearing: `[lint]` is not in `DETERMINISTIC_TAG_RE` (`[build]`/`[test]`/`[probe]` only), so the model's copy counts toward `criticalsNeedingVerify` — a linter-proven blocker was pulling the unverified-blocker cap on every re-post round, and the probe shows that cap disappearing with the fix. For the same reason the dedup runs BEFORE `modelBodyCriticals` is captured: dropping the re-post from the body alone left provenance still counting it, so the first draft of this fix fixed the rendering and kept the cap. Matched on the gate line's LOCATOR (the `` `path`:line CODE `` it opens with, backticks normalised), not the whole string: a re-post is model prose that carries the entry forward without reproducing the message byte for byte, and an exact-match rule stopped deduping the moment the wording drifted. The carried id is stripped through the ledger's own `LEDGER_ID_READBACK`. The body composer's gate call is now the only one on that path, so this also removes one half of R4-1's double invocation. R11-1 — the residual-risk facts are read off a work-list that may be known-truncated, without the completeness gate `openCriticals` applies. The completeness gate is NOT restored, and that is the same call as round 8: a whole-list requirement would silence the advisory on exactly the deep-work-list rounds it exists for, which are the rounds the byte budget shortens. What was wrong is what the code SAID about it. The block comment claimed "every input degrades open to no assessment"; two of these inputs do not. "No Suggestion, so the floor was enforcing" and "the backlog is not shrinking" are read off ABSENCE, and a shortened list can only lose entries, so both lean toward firing. `prevTruncated` now rides the facts and the assessment — deciding nothing — and the paragraph discloses, in both languages, that those two readings came off an incomplete list. The sibling diagnosis in the same module qualifies its own recurrence reading on the same fact; this follows that precedent rather than inventing one. The block comment states the exception instead of the blanket claim. Mutation matrix: never rendering the caveat, wiring `prevTruncated` to a constant, disabling the gate dedup, reverting the dedup to exact-match, and removing it from the marker work-list each turn the suite red — alongside the carried set (window on totals, pure-foreign gate, enforcement-evidence guard). packages/cli: 4697 pass / 1 skipped. Typecheck, ESLint and Prettier clean. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
What this PR does
Gives the convergence observation a machine-readable half, and lets the round report on its own machinery. The diagnosis shipped in #9461 could tell a human why a review loop was not settling; it gave a caller nothing to act on. This adds the two remaining pieces of that work item.
recommendations: [{code, basis}]is a closed code set matched from measurement, with no constants and no decisions. Four codes are matched from facts the round already holds:root-cause-triage(files that carried findings before and carry new ones now),batch-fixesandstem-surface(a trend that is not falling, the latter only where a floor rung is left to take), andland-and-defer(a round that posts no Critical — a loop whose blockers are all fixed can end by merging, and a merged pull request cannot diverge further). Every entry carries the deterministic fact it was matched from. The set is derived from the diagnosis rather than stored on it, and the paragraph's prose is generated from the same derivation, so the codes a caller wires and the sentences a human reads cannot describe different rounds. It rides the composed result and the durable artifact.The design's menu is larger than four, and every unimplemented code is absent for a stated reason rather than forgotten:
splitneeds the diff-topology test,reset-driftandrescopeneedsrcDelta,fix-pipelineneeds marker content-hash dedup,reduce-cadenceis two thresholds — the one thing this module does not own — andre-anchorwas matched to a cause this repository's measurements did not bear out. That last one is disclosed instead of prescribed: an anchor chain that has stopped is stated as a fact and recommends nothing.Mechanism health is the second piece. A pipeline that has stopped and a pipeline with nothing to do are both silent, so the round now says what it can see about itself. Two checks: a posting floor the reporting reading resolved to critical while the enforcement backstop failed open — the default configuration's standing gap, invisible from either side alone — and two consecutive rounds withholding the incremental anchor, after which every later round re-reads the whole diff until one closes cleanly. Both are stated and neither is acted on. The anchor decision moves into one shared predicate so the disclosure and the marker cannot describe different rounds.
It also clears the seven Suggestions that were deferred through rounds 5–7 of #9461 and merged unfixed: a dead
floorKnownparameter with its dead helper, two comments still describing a fold that was later narrowed, the drafted-id path missing the length boundidForapplies, an English cluster clause whose plural form read the new-finding count as another round number, a comment opening the wrong validator block, and the merged-provenance wiring's missing end-to-end assertion.Why it's needed
The advisory's whole point is that the tool measures and the caller decides. Without a machine-readable output there is no way for a caller to decide: the tracked follow-up — this repository's review workflow reading the telemetry and applying its own threshold to stop or continue the automatic loop — cannot be built against prose.
recommendationsis that interface, and the closed code set is what makes it one.The mechanism-health half comes from a failure observed while building this feature:
/takeoveron #9461 silently did nothing for about three hours — no label, no workflow run, no bot response, the head untouched — and from outside that was indistinguishable from a mechanism working with nothing to do. A round that can see its own posture not engaging, or its anchor chain stopped, is a round that can say so.The drafted-id bound is the one leftover with a behavioural edge: over a shortened work list,
idForre-mints an out-of-bounds claimed id while the freshness reader treated it as a re-post, so the two ends disagreed about one comment.Reviewer Test Plan
How to verify
npx vitest run src/commands/review/inpackages/cli— 4245 passed, 1 skipped.npx tsc --noEmitandnpx eslint src/commands/review --max-warnings 0are clean.The behaviour is covered by unit tests over the matching table (
recommendationsFor) and the health renderer, and by compose-level tests that drivecomposeReviewend to end and assert on the posted body and the composed result: the matched codes reachresult.recommendations, a round with no diagnosis emits none, the posture gap and the stopped anchor chain each render their disclosure and each fall silent on the control arm, and the artifact validator carries the codes through and refuses a wrong-shaped value.Every load-bearing decision was mutation-checked: ten single-line mutations, each verified to turn a named test red and reverted (the floor rung offered unconditionally,
land-and-defermatched on an absent count, the prose decoupled from the matched set, each health check forced false, the previous round's anchor forced false, the codes dropped from the result and from the artifact, and the drafted-id bound removed). The last one initially survived, which showed the first fixture was not exercising the bound — over a whole work list the stray-id rescue already reaches that draft — so the fixture now uses a shortened list, where the bound is what carries the case.Evidence (Before & After)
N/A — no user-visible TUI change. The rendered paragraph gains one sentence when
land-and-defermatches, and a separateMechanism health:clause appears only on the two shapes above; both are asserted in the compose-level tests.Tested on
Environment (optional)
Unit and compose-level tests only; no daemon or sandbox involved.
Risk & Scope
recommendationsis a new public surface on the composed result and the artifact, so the code set is a contract from here on. It is deliberately closed and deliberately small — four codes matched from facts this round holds — rather than the design's full menu, so that no code ships without the evidence behind it.reduce-cadencein particular has no threshold-free reading that also produces a paragraph. The caller-side enforcement that consumes these codes is a separate PR.Linked Issues
Part of #9278. Follows #9461.
中文说明
这个 PR 做了什么
给收敛观察加上机器可读的另一半,并让轮次报告自身机制的状态。#9461 交付的诊断能告诉人类某个评审回路为何没有收敛,却没有给调用方任何可据以行动的东西。本 PR 补上该工作项余下的两块。
recommendations: [{code, basis}]是一个由测量匹配出的封闭码集合,没有常数、没有决策。四个码全部匹配自本轮已经持有的事实:root-cause-triage(此前出过发现、本轮又产生新发现的文件)、batch-fixes与stem-surface(趋势没有下降;后者仅在还有下限档位可用时给出)、land-and-defer(本轮没有发布 Critical——阻断项都已修好的回路可以靠合入结束,而已合入的 PR 不会继续发散)。每一条都带着它被匹配出来的确定性事实。该集合是从诊断派生的而非存在诊断上,段落的散文也由同一次派生生成,因此调用方接线的码与人类阅读的句子不可能描述不同的轮次。它随合成结果与持久化 artifact 一起传出。设计里的菜单比四个大,而每个未实现的码都有明确原因、不是遗漏:
split需要 diff 拓扑判定;reset-drift与rescope需要srcDelta;fix-pipeline需要 marker 内容哈希去重;reduce-cadence本身是两个阈值——恰恰是本模块不拥有的东西;而re-anchor所匹配的成因未被本仓库的实测数据支持。最后这一条改为披露而非开处方:锚点链已停止只作为事实陈述,不给出任何建议。第二块是机制健康。已经停摆的流水线与无事可做的流水线同样安静,因此轮次现在会说出它能看到的自身状况。两项检查:报告读数把发布下限解析为 critical 而执行侧兜底放行(缺省配置下的既有落差,单看任一侧都看不见),以及连续两轮扣留增量锚点(此后每一轮都会重读整个 diff,直到某轮干净收尾)。两者都只陈述、都不据以行动。锚点判定被收进唯一一处共享谓词,使披露与 marker 不可能描述不同的轮次。
本 PR 同时清掉 #9461 第 5–7 轮延后、随合入进 main 的 7 条 Suggestion:一个无人读取的
floorKnown参数及其死掉的辅助函数、两处仍在描述后来被收窄的折叠规则的注释、drafted id 路径缺少idFor已施加的长度界、英文簇句的复数形态把新增计数读成了又一个轮次号、一行注释开在了错误的校验块上,以及 merged 出处接线缺失的端到端断言。为什么需要
这套建议机制的全部要义是:工具测量,调用方决策。没有机器可读的输出,调用方就无从决策——已跟踪的后续项(本仓库的评审 workflow 读取遥测、用自己的阈值决定停止或继续自动回路)无法基于散文构建。
recommendations就是那个接口,而封闭码集合正是让它成其为接口的东西。机制健康这一半源自开发本功能期间观察到的一次失败:
/takeover在 #9461 上静默了约三小时什么也没做——无 label、无 workflow run、无机器人响应、head 未动——从外部看,这与"机制正常且无事可做"完全无法区分。一个能看见自身姿态未生效、或锚点链已停止的轮次,才能把这件事说出来。drafted id 的长度界是这批遗留中唯一有行为边界的一条:在被截断的工作清单上,
idFor会为越界的声称 id 重铸,而新鲜度读取方却把它当作重发——两端对同一条评论给出了不同判定。复核测试计划
如何验证
在
packages/cli执行npx vitest run src/commands/review/—— 4245 通过、1 跳过。npx tsc --noEmit与npx eslint src/commands/review --max-warnings 0均干净。行为由针对匹配表(
recommendationsFor)与健康渲染器的单测覆盖,并由驱动composeReview的 compose 级端到端测试断言发布正文与合成结果:匹配出的码到达result.recommendations;没有诊断的轮次不产生任何码;姿态落差与停止的锚点链各自渲染披露、并在对照组保持沉默;artifact 校验器把码带过去,并拒绝形状错误的值。每个承重判定都做了变异测试:10 个单行变异,逐个验证能让指定测试变红后还原(楼层档位无条件给出、
land-and-defer在计数缺失时匹配、散文与匹配集合解耦、两项健康检查各自强制为假、上一轮锚点强制为假、码从结果与 artifact 中丢弃、drafted id 的界被移除)。最后一条起初存活,说明第一版夹具没有真正触及那道界——在完整的工作清单上,游离 id 的救回已经先一步覆盖了那条草稿——因此夹具改用被截断的清单,那里才是这道界起作用的地方。证据(前后对比)
N/A —— 无用户可见的 TUI 变化。当
land-and-defer匹配时渲染段落多一句;Mechanism health:是独立子句,仅在上述两种形态下出现;两者均由 compose 级测试断言。测试环境
运行环境(可选)
仅单测与 compose 级测试,不涉及 daemon 或沙箱。
风险与范围
recommendations是合成结果与 artifact 上的新公开面,因此码集合从此成为契约。它被刻意做成封闭且很小——四个匹配自本轮已持有事实的码——而不是设计里的完整菜单,以确保没有任何码在缺少支撑证据的情况下上线。reduce-cadence尤其没有既无阈值又能产生段落的读法。消费这些码的调用方执法是另一个 PR。关联 Issue
属于 #9278。承接 #9461。