feat(review): validate Aone inline anchors against the captured diff before posting - #9634
Conversation
…before posting Aone Code performs no server-side anchor validation — a controlled probe (scratch MR 29427547, a1 v0.2.51) proved any --line integer posts, and an old-side number silently lands on the same-numbered new-side line. The old side cannot be anchored at all, and file-level comments drop their path. Pin the removed-line semantics for the Aone write path: submit's Aone branch now validates every well-formed inline anchor against the review's captured diff before posting. An unanchorable Critical is relocated into the summary body, an unanchorable Suggestion discarded and counted — the GitHub 422-recovery dispose, performed in code — each disclosed in the terminal. A missing captured diff refuses the whole post; malformed shapes (missing path/line, reversed range, renders-as-nothing) keep their consistency-gate refusals, and a garbage state.bodyCriticals stands the gate down so compose's pinned refusal fires. The GitHub path is untouched — its server performs this validation. Issue QwenLM#9615
github-actions
Bot
added
the
review/self-reported
E2E test reportProbe (platform facts, scratch CR 29427547 of base-biz/sqlt, a1 v0.2.51): a one-file diff shaped so every probe is unambiguous — two deletion blocks plus an appended line, new-side hunk coverage [1-6] ∪ [9-14] ∪ [29-32].
Baseline gap (pre-change build): Gated behavior (this PR): the same dry-run reports 1 inline, prints Live post: a real submit of the mixed payload landed exactly ONE inline comment ( Unit: |
|
Re-run — this pass reviews
Moving on to code review. 🔍 中文说明Re-run —— 本轮审查
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThis pass covers the current head
The integration points re-verified on the current head:
No critical blockers and no convention violations found. One non-blocking carry-over from round 8's deferred list, verified still standing: Files changed (12 of 12)
Testing — the PR's own CI (never run locally in unattended triage)At review time the PR's own CI is fully settled on
The suite pins the change: ~40 submit-level gate scenarios drive the real Sandboxed verification would settle the remaining in-repo question — whether the new suite is load-bearing (fails on the base build): the 中文说明代码审查：本轮覆盖当前头部
集成点在当前头部重新核实：
未发现阻塞问题与规范违规。一项非阻塞遗留（第 8 轮明确延后项，核实仍存在）： 测试证据（无人值守 CI，不在本地运行 PR 代码）：审查时 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review on the current head, capped by policy rather than doubt: the six fix commits since the last pass each trace to a specific review finding, the two remaining round-8 Criticals are verified fixed in code (not just in thread replies), CI is fully green on Stepping back: my independent proposal for this problem was exactly the shape this PR lands — validate anchors against the captured diff in the Aone branch, degrade like GitHub's 422 recovery, fail closed when the diff is absent — and the PR exceeds it where it matters (the compose-acceptance stand-down, the built-entry validation, the authored-index tracking). The growth since the last pass is legitimate: every fix commit answers a named finding from eight adversarial review rounds, and the production line count grew by hardening, not by scope creep. The one drive-by (the prettier import wrap in Two open items remain before an approval decision, and neither is something the diff can settle:
⏸️ Deferring to @wenshao — Stage 0 maintainer-awareness escalation on an otherwise-clean PR, plus the pending verify report. You are the maintainer of record here (and the author): either route this to a second maintainer for the sign-off the escalation asks for, or weigh the green CI + resolved threads + probe record yourself. The bot's approval vote stays withheld until then; nothing in this review requests changes. 中文说明置信度：3/5 —— 当前头部的审查是干净的，压分来自政策而非疑虑：上次审查之后的 6 个 fix 提交每一条都能对应到具体的评审发现；第 8 轮剩余的两个 Critical 已对照代码确认修复（而非仅采信线程回复）； 退一步看：我为这个问题给出的独立方案正是本 PR 落地的形态——在 Aone 分支对捕获 diff 校验锚点、按 GitHub 422 恢复降级、diff 缺失时 fail-closed——而 PR 在关键处做得更深（compose 验收整体退避、构造条目校验、原作者索引跟踪）。上次审查之后的增量是合理的：每个 fix 提交都回应 8 轮对抗性评审中点名的发现，生产行数增长来自加固而非范围蔓延。唯一顺手改动（ 批准决定前还剩两个开放项，均非 diff 本身能定夺：
⏸️ 转交 @wenshao —— 审查其余部分全部干净，但本 PR 触发 Stage 0 维护者关注升级，外加未落地的 verify 报告。你是此处的维护者（也是作者）：可以按升级要求转请第二位维护者签认，也可以自行权衡绿 CI + 已解决线程 + 探测记录。在此之前 bot 的批准票保留；本审查不要求任何修改。 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /resolve |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 10.
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——在 10 轮的反审轮数上限内未收敛。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
…e refusals - validateNewSideAnchors now rejects the input domain (fractional/zero/negative lines and reversed ranges) before the hunk scan, so its verdict can no longer certify an anchor the zero-validation Aone platform would post silently wrong. - Extract the consistency gate's per-comment shape checks into one shared predicate (commentShapeProblems) read by both the loud refusal and the Aone anchor gate, so a shape the gate disposes is never a refusal the operator misses (open fence, start_line-without-side). The path check becomes a type check, closing truthy non-string paths that reached the write seam unvouched.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9634 (Aone anchor gate)This round merged Base merge (
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 still surfaced 6 findings, all confirmed; the loop ended at the cap, not at a dry fixed point).
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5 (round 5 still surfaced 6 findings, all confirmed; the loop ended at the cap, not at a dry fixed point)。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…or-gate Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ocated entries Round-2 review fixes for the Aone anchor gate: - The stand-down now keys on ANY degrade that touches the payload and covers every compose-owned garbage shape: bodyCriticals that is not an array of strings, or a suggestionsDiscarded compose's counter refuses. The countability test reads compose's OWN acceptance table (toCount, exported as the total tryToCount), so the gate's merge and compose's counter can never drift — an integer-but-not-safe count now merges instead of silently dropping the gate's discards. - The relocated entry's claim extraction strips a leading marker RUN (fixpoint, like every other strip) and treats a fence-delimiter claim line as absent — both shapes used to leak raw markers or junk delimiters into the posted summary-body blocker line. - The gate keeps the model-authored comment indices through its removal (and floor enforcement keeps them through its own), so the consistency gate's refusal names the culprit in the model's payload JSON instead of a renumbered position the re-compose loop cannot act on. - A --dry-run with a missing capture no longer exits 3: it writes nothing, so it skips the gate with a disclosure and reports wouldPost: false (reason: aone-diff-missing); the exit-3 refusal stays reserved for the real write. - The MULTI_DIFF fixture's second hunk header becomes byte-exact git output (@@ -20,0 +22,2 @@, probed against git itself). - The design doc gains the gate-relocation doctrine (relocated entries deliberately inherit the model's own tag-exemption treatment), the dry-run carve-out in the failure-shape table, and the corrected fence/one-line-channel claim.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #9634This round addressed 9 of the 17 open findings and explicitly deferred the Conflict resolution (
|
…the relocated entry Gap-fill on top of the round-2 gate hardening: - The relocated entry's claim extraction strips the canonical footer FIRST: with an empty claim line (a marker-plus-separator-only body), the separator strip eats the newline+colon and the extraction falls THROUGH into the appended footer's first line, posting it as the claim. Witness added for the placeholder shape. - Pin the multi-line relocation entry CONTENT (it must cite the claimed end line, not the start — the start sits inside the hunk and looks fine) and the disclosure naming it. - Witnesses for the remaining mutant-tested gaps: a range whose start sits outside every hunk and end inside (the startLine mapping), the dry-run compose parity (preview composes from the gate-corrected payload), the suggestionsDiscarded 0 merge boundary, the empty-path shape (loud refusal, never a gate disposal), a declared LEFT start_side without a start_line, and the equal-boundary range (start_line === line, a shape GitHub itself produces). - The routing suites run from a per-test fixture cwd, so the captured-diff seeding and its cleanup can no longer overwrite or delete a same-numbered live capture in the real vitest cwd. Issue QwenLM#9615
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — ended at the 5-round cap without a dry convergence (round 5 still surfaced one candidate, rejected in verification).
Not explored to full depth (tool budget reached): chunk 2: executing anchors.test.ts under vitest — the review worktree and the parent checkout both lack node_modules, and a full npm ci was beyond budget; all tests we….
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
docs/design/2026-08-21-review-aone-removed-line-anchoring.md:3 — [review] Status line says "implementation planned" while the same PR ships the implementationdocs/design/2026-08-21-review-aone-removed-line-anchoring.md:186 — [review] "Files affected" omits compose-review.ts (the tryToCount export the doc's prose depends on)packages/cli/src/commands/review/lib/anchors.test.ts:1112 — [probe] Reversed-range test does not exercise the in-hunk-endpoints degeneracy its title advertises
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — ended at the 5-round cap without a dry convergence (round 5 still surfaced one candidate, rejected in verification)。
未探索到全部深度（达到工具调用预算）：chunk 2：executing anchors.test.ts under vitest — the review worktree and the parent checkout both lack node_modules, and a full npm ci was beyond budget; all tests we…。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— 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 将重新运行。 |
# Conflicts: # docs/design/2026-08-13-review-platform-provider-abstraction.md # packages/cli/src/commands/review/submit-aone.test.ts # packages/core/src/skills/bundled/review/SKILL.md
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R4-1 ranged non-RIGHT over-promise (second location, docs/design/2026-08-13-review-platform-provider-abstraction.md:377) — already reported (comment 3834195100)
Not explored to full depth (tool budget reached): chunk 5: none — though note the suite was verified by reading against HEAD sources, not by execution (no node_modules in the shared worktree; installing there would dist….
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）：chunk 5：none — though note the suite was verified by reading against HEAD sources, not by execution (no node_modules in the shared worktree; installing there would dist…。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced candidates; the loop ended at the cap, not at a dry fixed point).
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit.ts:1142 — [probe] Gate-renumbering × floor-enforcement composition of…docs/design/2026-08-21-review-aone-removed-line-anchoring.md:81 — [probe] Doc promises relocate/discard for every non-RIGHT-side…packages/cli/src/commands/review/submit-aone.test.ts:2025 — [review] The hostile-paths test still has no \r witness for the…packages/cli/src/commands/review/lib/anchors.test.ts:1112 — [probe] Reversed-range test's name/comment describe geometry the…packages/cli/src/commands/review/lib/anchors.test.ts:1062 — [probe] Cross-hunk refusal test never constructs a range ACROSS…packages/cli/src/commands/review/lib/anchors.test.ts:1050 — [probe] No unit witness for the lo endpoint (startLine below the…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced candidates; the loop ended at the cap, not at a dry fixed point)。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced one candidate, confirmed in verification; the loop ended at the cap, not at a dry fixed point).
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/anchors.test.ts:1112 — [probe] reversed-range test's title/comment geometry is false for the fixture; the advertised in-hunk-endpoints degeneracy is unwitnessedpackages/cli/src/commands/review/submit.ts:1141 — [probe] R3-3 lineage: the gate-renumbering × floor-enforcement authoredIndices composition has zero coverage for its non-identity branchpackages/cli/src/commands/review/submit.ts:994 — [probe] the stand-down probes only the two merged fields; a degrade can be disclosed then die on another state field's compose refusalpackages/cli/src/commands/review/lib/anchors.test.ts:1061 — [probe] the across-hunks refusal has no discriminating witness; a per-endpoint mutant survives the whole suitedocs/design/2026-08-21-review-aone-removed-line-anchoring.md:215 — [review] the failure-shapes table omits the corrupt/zero-byte capture degrade-and-post shapedocs/design/2026-08-21-review-aone-removed-line-anchoring.md:195 — [review] Files affected omits compose-review.ts, the module owning the acceptance tables the stand-down depends on
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced one candidate, confirmed in verification; the loop ended at the cap, not at a dry fixed point)。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
…receipt state The Aone receipt suite's beforeEach wiped the whole .qwen tree to start from no receipt — deleting the captured diff the anchor gate needs along with it. Every post then died at the gate's missing-capture refusal and no receipt was ever written (ENOENT in the four receipt tests on CI). Remove only the receipt file; the seeded diff survives.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced two candidates, both verified; the loop ended at the cap, not at a dry fixed point).
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit.ts:221 — [review] the path guard admits a whitespace-only path — the entry posts with silently dropped file attribution instead of the (no path) placeholderpackages/cli/src/commands/review/submit-aone.test.ts:1822 — [review] stand-down coverage matrix missing the relocation × garbage suggestionsDiscarded cell; a cell-isolated mutant survives 70/70packages/cli/src/commands/review/submit-aone.test.ts:1356 — [review] renumbered-refusal witness covers only the relocation arm; a discard-arm authoredIndices mutant survives 183/183docs/users/features/code-review.md:385 — [review] user-facing feature page still asserts the pre-gate Aone behavior, newly contradicted by this PR's degrade and missing-diff refusalpackages/cli/src/commands/review/submit.ts:221 — [review] the path arm lacks the renders-as-nothing projection; invisible-residue paths post as invisible attribution
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ended at the 5-round cap without dry convergence (round 5 still surfaced two candidates, both verified; the loop ended at the cap, not at a dry fixed point)。
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
…or-gate # Conflicts: # docs/design/2026-08-13-review-platform-provider-abstraction.md # packages/cli/src/commands/review/submit.test.ts # packages/core/src/skills/bundled/review/SKILL.md
…trated Round-3 remediation of the review comments on the Aone anchor gate: - R3-2 (structural): the BUILT relocated entry is now validated against compose's own ingestion (tryIngestBodyCriticals over the single entry) before the relocate is disclosed, and any refusal degrades the entry to the inert constant `finding — (no path):<line>` — the entrance space is unbounded model text and compose's acceptance is the authority, so a shape the enumerated guards never anticipated degrades the entry instead of refusing the whole post mid-degrade. The demonstrated entrance (a lone CR inside the claim: it passes the leading-fence guard, compose's CR normalisation then splits the entry and the second line leads with a fence delimiter) is covered by a witness. - Ledger collision: the relocated entry flips to `<claim> — <path>:<line>` — the claim leads, so a carried id keeps position 0 and the ^-anchored ledger readback matches instead of silently renumbering a carried finding as new. Witness asserts the id survives the readback regex. - R7-1: an explicit JSON null side/startSide reads as ABSENT (defaults to RIGHT), the model's idiom for an omitted optional field — never a declared old side. Unit and gate-level witnesses. - R3-3: witness for the non-identity authoredIndices branch — the gate renumbers the array, floor enforcement keys on the post-gate array, and the remap drops the comment floor enforcement names. - R4-2: the hostile-paths test gains the \r-bearing path (compose's ingestion normalises a bare CR to a line break — the same hostile shape as \n; the guard's \r half was unwitnessed). - R3-5: the design doc states the carve-out — the non-RIGHT degrade runs for single-line comments only; a multi-line non-RIGHT comment keeps the consistency gate's whole-post refusal; null side is absent, not a declaration. The failure-shapes table splits the row accordingly. Issue QwenLM#9615
|
@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: 128 passed · 0 failed · 128 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：128 通过 · 0 失败 · 128 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9634 — feat(review): validate Aone inline anchors against the captured diff before postingVerdict: 中文摘要
Central claim and A/B proofCentral claim: the Aone write path validates every well-formed inline anchor against the captured diff before anything posts; unanchorable Criticals relocate into the summary body, unanchorable Suggestions are discarded and counted, every degrade is disclosed, and a missing captured diff refuses the whole post. Secondary: (1) the GitHub path is unchanged; (2) malformed shapes and garbage compose-owned state still reach the existing refusals untouched. Harness:
Totals: head 51/51, base 31/31 scripted assertions. Witnesses: Reviewer Test Plan walk
Boundary probes (33/33,
|
| mutant | suite response | classification |
|---|---|---|
| C control: invert hunk-containment comparison | KILLED, 17 red (accept/relocate tests name expected-vs-actual) | positive control — harness can fail the suite |
| M1 gate keep-condition always true | KILLED, 21 red | gate is load-bearing |
| M2 swap relocate/discard direction | KILLED, 21 red | dispose direction pinned |
| M3 remove missing-diff refusal | KILLED, exactly 1 red: "refuses the WHOLE post when the captured diff is missing" | intended assertion |
| M4 null side treated as declared | KILLED, exactly 2 red (unit + gate null-side witnesses) | R7-1 pinned |
| M5 merge drops the state's discard count | KILLED, 3 red (merge witnesses) | merge pinned |
| M6 remove newline path guard | KILLED, exactly 1 red (hostile-path witness) | guard load-bearing |
| M7 combination: M6 + disable entry ingestion validation | KILLED, 2 red (both witnesses) | each guard individually load-bearing, not redundant |
M8 isWholeLine admits zero |
SURVIVED (275/275) | see Finding 1 |
Witness: 05-mutation-matrix.png; raw logs logs/mutant-*.vitest.log. Every killed mutant failed the intended assertion with expected-vs-actual text (verified per log), never a compile error.
Findings
- Suggestion — coverage gap on a redundant defence (M8 survivor).
isWholeLine's zero/negative half has no suite witness: admittingline: 0leaves all 275 tests green. I proved with a scripted probe on a mutated dist copy (logs/m8-probe.log) that the mutation is verdict-neutral on git-shaped diffs — line 0 still fails the hunk scan (newStart <= 0is never true for anewCount > 0hunk); only the terminal reason string changes ("sits in no new-side hunk" vs "not a positive whole number"). The fraction half of the same clause IS load-bearing (1.5 inside a hunk span would otherwise certify) and IS pinned. So the zero half is redundant defence: correct as-is; a one-line witness (validate(diff, [{path, line: 0}])) would pin it. Not a merge condition per the matrix rule. - Observation (non-defect) — lowercase
side: "right"is treated as a declared non-RIGHT side (strict!== 'RIGHT'), so such a comment relocates instead of posting. Fail-closed and preserving (the finding lands in the summary with attribution); GitHub's API spelling is uppercase, so the accepted input class is unchanged. Worth a sentence in the design doc, not a code change. - Observation (non-defect) — the
+0,0hunk of a fully-deleted file parses tonewStart: 0, and the gate correctly validates nothing inside it (probed). No action.
Not covered
- Live Aone posting. The stub
a1reproduces the wire protocol shapes the provider reads back (whoami/mr view/comment create), not the platform; the PR's own scratch-CR live verification was run on the intranet and is not reproducible in this credential-free container. The A/B therefore proves the CLI-side behavior at the write seam, matching the PR's claim about the CLI, not a re-probe of the platform. - GitHub real post path. Only the dry-run parity was exercised (byte-identical JSON, no gate fields); a full gh-stubbed post was out of budget. The gate block sits under
if (aoneWrite)by construction and the parity cell confirms no leakage at compose/dry-run level. - Per-commit attribution. Depth-2 checkout; only the aggregate
HEAD^1..HEADdiff was verified (the commit list in the snapshot matches the merge history; rows were not individually exercised). docs/design/*and SKILL.md prose accuracy — read for mechanism understanding, not re-probed against the platform.- Base control confound (disclosed above): base cli was compiled
--noCheckagainst head's core dist types; base core from-scratch typecheck is environmentally blocked (otel type packages absent from this image's node_modules), and the PR's core delta is formatting-only. - Repo-wide gates beyond the review tree: CI covers them; I ran the affected surface only (99 files / 4563 tests green).
Methodology
Environment: CI verify container, merge-ref checkout (HEAD 264855b), pre-built dist at head. Head arm drove the compiled dist/src/commands/review/submit.js via runSubmit from per-cell fixture cwds with a recording stub a1 first on PATH; base arm identical against a HEAD^1 worktree rebuilt with a noCheck scratch tsconfig. Authorization used the documented skillArgs test seam with a --comment recording naming each target (real parseReviewArgs/reviewWriteAuthorization code, no session id). Assertions live in driver.mjs / probe-boundaries.mjs (rerunnable), raw logs in logs/, mutation logs in logs/mutant-*.vitest.log. Gates: npx vitest run over the three named suites (275/275) and the whole src/commands/review tree (4563 passed / 4 skipped / 0 failed) at head.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/commands/review/lib/anchors.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/anchors.test.ts
file packages/cli/src/commands/review/submit-aone.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit-aone.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/anchors.test.ts: PPPPP
packages/cli/src/commands/review/submit-aone.test.ts: PPPPP
packages/cli/src/commands/review/submit.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/anchors.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/anchors.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/anchors.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/anchors.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/anchors.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.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
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at HEAD f146c811. Approving — both round-8 Criticals are fixed, verified against the code as it stands rather than the standing verdict (which is anchored before f146c811). All 50 threads are resolved and Test (ubuntu-latest) passes at this head.
R7-1 (anchors.ts) — fixed, including the sibling the finding named. validateNewSideAnchors now reads a JSON null side as absent rather than as a declared non-RIGHT side:
const sideDeclared = check.side !== undefined && check.side !== null;
const startSideDeclared =
check.startSide !== undefined && check.startSide !== null;Only a present, non-RIGHT value is refused, which restores the NewSideAnchorCheck contract ("a missing side defaults to RIGHT"). startSide gets the identical treatment — the finding explicitly warned start_side: null was affected the same way, and it is covered. The comment records why (null is the model's idiom for an absent optional field, a recurring shape this pipeline has been bitten by before). This matters more than a placement bug on Aone, where the inline Critical discussion is the merge block: the old behaviour posted a confirmed blocker as advisory-only, deterministically, so no retry could recover it.
R3-2 (submit.ts) — fixed with the structural remedy requested since round 3, not another guard. The relocated one-line entry is now validated against compose's own ingestion and degraded when compose would refuse it:
if (tryIngestBodyCriticals([entry]) === undefined) {
entry = `finding — (no path):${c.line}`;
}That is the right shape. The prior enumerated guard list could only ever chase entrances — the round-8 probes found two more (a invisible-entity claim, and ZWSP/NBSP residue defeating the ^-anchored fence test) — whereas delegating to compose's actual acceptance closes the unbounded space by construction, and the inert fallback passes by construction (no fence, no newline, renders as something). The stand-down half is fixed too: it now reads compose's own tables (tryIngestBodyCriticals / tryToCount) rather than a mirror, with the comment noting the two reads of each field therefore cannot drift.
One point I checked rather than assumed: the stand-down validates the pre-existing bc array, not the merged [...bc, ...relocated]. That is sound here, because every relocated entry is individually guaranteed to pass compose's ingestion at build time, so the merged array is safe by composition. And the residual failure mode is benign by design — per the comment, a compose refusal means the payload "dies the pinned death; nothing posts either way, and the findings stay in the saved report," i.e. a safe failure rather than the false-advisory post that made R7-1/R3-2 harmful.
The review-pr check is still in flight at this head; CI is otherwise green (13 pass / 0 fail).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit-aone.test.ts:2267 — [probe] multi-line non-RIGHT side shape check has no exactly-one-non-RIGHT-field witness; ||→&& mutant survivespackages/cli/src/commands/review/submit.ts:190 — [review] relocatedAoneCriticalEntry doc comment states the inverted entry format (path:line — claim; code builds claim — path:line)docs/design/2026-08-21-review-aone-removed-line-anchoring.md:234 — [probe] failure-shape table's MULTI-LINE row says an explicit null side 'validates', but the shared shape gate refuses it wholepackages/cli/src/commands/review/submit-aone.test.ts:1671 — [probe] no witness posts a multi-line range fully inside a hunk inline; kept-branch mutation for start_line comments survivespackages/cli/src/commands/review/submit-aone.test.ts:2616 — [probe] floor-remapped authoredIndices is never consumed by a subsequent refusal; remap mutations survive
中文说明
无阻断问题。LGTM！✅
Test Plan（非阻断）：src/commands/review/submit-aone.test.ts — no such file or directory; src/commands/review/lib/anchors.test.ts — no such file or directory; src/commands/review/submit.test.ts — no such file or directory。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Verified the gate end to end at head f146c81:
- Anchor math is right against parseDiff's semantics:
newEndis inclusive (newStart + newCount - 1) withnewCountas the authority, so thenewCount > 0 && newStart <= lo && hi <= newEndcontainment correctly excludes pure-deletion hunks, rejects reversed ranges and non-whole-line inputs before the scan, treats JSONnullsides as absent (default RIGHT) while a PRESENT non-RIGHT side is unanchorable by construction, and requires a multi-line range to sit in one hunk. - Degrade lands in the existing compose counters rather than a parallel path: relocated Criticals re-enter as guarded one-line
bodyCriticals(newline/fence-hostile paths handled, final-checked throughtryIngestBodyCriticals) and still count toward C; discards merge intostate.suggestionsDiscardedand count toward S; both disclosed with theanchorsRelocated/anchorsDiscardedcounters. No shape loses a Critical entirely or posts one twice. - Refusal ordering holds: a missing capture on a real post exits 3 with
aone-post-refusedbefore any write; dry-run previews compose as authored with ananchorsUncheckeddisclosure; a corrupt capture parses to zero files and degrades visibly instead of refusing a second shape; garbage state stands the gate down so the composition's pinned refusal fires. The GitHub arm emits no gate fields, and the extensionManager.ts hunk is a cosmetic type-import reformat.
One non-blocking observation for follow-up: the gate trusts the cwd capture file (.qwen/tmp/qwen-review-pr-<N>-diff.txt) with nothing tying it to the live MR head — an amended-then-resubmitted run without re-capture, or a local swap between review and post, makes the gate vouch against the wrong diff and the platform that validates nothing posts the wrong lines. The mid-batch head re-read mechanism already exists; pinning the capture to the head SHA it was fetched at (or re-fetching for real posts) would close both directions. Also minor: the capture is keyed by PR number alone, so same-numbered cross-repo targets sharing a cwd would collide captures (the missing-file direction fails safe; the collision direction trusts silently).
CI at approval time: 13 checks passing, review-pr still pending on head.
…p note Merges origin/main (f829a02) into the Aone dedup PR. Three doc conflicts: - SKILL.md --comment bullet: main's #9634 adds a fifth Aone failure shape (submit-side inline anchor validation against the captured diff, since Aone has no server-side anchor validation). Kept main's bullet with the anchor check, and swapped its stale "no dedup backing yet" caveat for the dedup-backed paragraph from this PR (#9627). - 2026-08-13 design doc: kept this branch's "Landed (#9627) dedup backing" note (main's side of the hunk was empty). - 2026-08-15 design doc open-question #2: kept BOTH resolutions — this branch's threading half (parentNoteId/closed/outdated -> GitHub buckets) and main's anchor half (new-side-only anchoring, per #9634's probe). Code files (aone.ts, submit.ts) auto-merged cleanly. Verified: npm run build green, tsc clean on packages/cli, SKILL.test.ts 30/30, and 582 review tests pass (presubmit GitHub+Aone, comment-status, aone/aone-client, submit GitHub+Aone, cleanup).
|
Released in v0.22.2. |
5 participants
What this PR does
This PR pins the inline-anchoring semantics of
/review --commenton Aone Code targets. Before anything posts, the Aone write path now validates every well-formed inline finding's anchor against the diff the review captured: an anchor must sit inside a new-side hunk of its file, and a declared old-side anchor is unanchorable by construction. A finding whose anchor cannot be vouched for degrades deterministically instead of posting — a Critical is relocated into the review summary body with itspath:lineattribution kept, a Suggestion is discarded and counted — and every degrade is disclosed in the terminal, exactly the dispose GitHub's server-side 422 recovery performs in prose. A missing captured diff refuses the whole post before anything writes. Malformed comment shapes (missing path/line, a reversed range, a renders-as-nothing body) keep the existing consistency refusals, and garbage state shapes stand the gate down so the composition's pinned field-naming refusal fires instead of being laundered. The GitHub path is untouched.Why it's needed
A controlled probe against a scratch Aone code review proved the platform performs zero server-side anchor validation: any positive integer posts, an old-side (deleted-line) number silently lands on the same-numbered NEW-side line — including on untouched code outside every hunk — and the read-back
outdatedflag cannot tell a misanchored comment from an anchored one. The old side cannot be anchored at all (the CLI expresses--lineas a new-side position only, even in the latest release), and file-level comments drop their path entirely, so they are not a usable degrade target. On GitHub this whole failure class is caught server-side with a 422 and the skill's recovery loop turns it into relocated Criticals and discarded Suggestions; on Aone the behavior was undefined — the anchor either failed at post time or, worse, landed on a wrong line silently. Findings on removed lines are a normal review output ("this deletion drops the last caller of X"), so the write path owed them a defined, disclosed dispose. That was open question Q2 in the platform-provider design document; this PR records the probe evidence and lands the behavior the probe supports.Reviewer Test Plan
How to verify
The unit tests narrate the full behavior table — run
npx vitest run src/commands/review/submit-aone.test.ts src/commands/review/lib/anchors.test.ts src/commands/review/submit.test.tsinpackages/cli. The shapes to confirm: a new-side in-hunk anchor posts unchanged; an out-of-hunk / file-not-in-diff / declared-LEFT Critical relocates into the summary body and counts toward C; the same as a Suggestion is discarded and counts toward S, with each named in the terminal disclosure; a missing captured diff exits 3 withreason: "aone-post-refused"and nothing written; malformed shapes and a non-array body-Criticals field reach the existing refusals untouched; and the GitHub path emits no gate fields and changes no behavior. End-to-end, a live post of a mixed payload (one valid anchor, one out-of-hunk Critical, one beyond-EOF Suggestion) to the scratch CR landed exactly one inline comment at the valid line and a summary carrying the relocated Critical plus the discard sentence — nothing at the two bad lines.Evidence (Before & After)
Before (current build, ungated): a
--dry-runof the mixed payload reportswouldPost: truewith 3 inline — both invalid anchors would post, the Critical silently landing on untouched code. After: the same run reports 1 inline, prints the anchor-check disclosure naming the relocated Critical and the discarded Suggestion, and carriesanchorsRelocated: 1, anchorsDiscarded: 1in its JSON. Live read-back after the real post shows one new inline comment (line: 32, side: right) and a summary body containing**[Critical]** qwen-review-anchor-probe.txt:18 — …plus the sentence1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; no comment exists at line 18 or 9999. The probe record lives in the repo's e2e-tests directory; the scratch CR is closed with branches retained as evidence.Tested on
Environment (optional)
Unit tests plus the bundled CLI driven against a real Aone scratch MR via the a1 CLI (v0.2.51); the probe and the live verification ran on the intranet.
Risk & Scope
Linked Issues
Closes #9615
中文说明
这个 PR 做了什么
本 PR 钉死
/review --comment在 Aone Code 目标上的行内锚点语义。在发布任何内容之前，Aone 写路径现在会用审查捕获的 diff 校验每个格式良好的行内发现的锚点：锚点必须落在其文件的新侧 hunk 内，声明的旧侧锚点构造上即不可锚定。无法担保锚点的发现会被确定性地降级而不是发布——Critical 连同其path:line归属一起迁入审查总结正文，Suggestion 被丢弃并计数——每一次降级都在终端披露，与 GitHub 服务端 422 恢复流程在 prose 中执行的处置完全一致。缺少捕获的 diff 时，在任何写入发生前整体拒绝发布。畸形评论形状（缺 path/line、反转的 range、渲染为空的 body）保留既有一致性拒绝；垃圾的 state 形状会让门整体退避，使组合阶段钉死的字段命名拒绝生效，而不是被洗白。GitHub 路径完全不受影响。为什么需要
在 scratch Aone CR 上做的受控探测证明：平台服务端不做任何锚点校验——任何正整数行号都能发布；旧侧（被删除行）的行号会静默落在新侧同号行上——包括落在所有 hunk 之外的未改动代码上；读回的
outdated标志无法区分错锚与正常锚点。旧侧完全无法锚定（即使是最新版 CLI，--line也只能表达新侧位置）；文件级评论会丢失其 path，因此也不是可用的降级目标。在 GitHub 上，这一整类失败会被服务端以 422 拦截，skill 的恢复循环会把它转化为迁入正文的 Critical 和被丢弃的 Suggestion；而在 Aone 上行为是未定义的——锚点要么在发布时报错，要么（更糟）静默落在错误的行上。关于被删除行的发现是正常的审查产出（"这个删除移除了 X 的最后一个调用方"），写路径欠它们一个有定义、有披露的处置。这正是平台 provider 设计文档中的悬而未决问题 Q2；本 PR 记录探测证据，并按探测支持的形态落地行为。评审测试计划
如何验证
单元测试叙述了完整的行为表——在
packages/cli下运行npx vitest run src/commands/review/submit-aone.test.ts src/commands/review/lib/anchors.test.ts src/commands/review/submit.test.ts。需要确认的形态：新侧 hunk 内的锚点照常发布；hunk 外 / 文件不在 diff 中 / 声明 LEFT 的 Critical 迁入总结正文并计入 C；同为 Suggestion 时被丢弃并计入 S，且每一条都在终端披露中点名；缺少捕获的 diff 时以 exit 3 退出、reason: "aone-post-refused"、无任何写入；畸形形状与非数组的 body-Criticals 字段原样到达既有拒绝；GitHub 路径不输出门字段、行为无任何变化。端到端方面：向 scratch CR 真实提交了一个混合 payload（一个合法锚点、一个 hunk 外 Critical、一个越界 Suggestion），结果恰好只在合法行落了一条 inline 评论，总结正文携带迁移后的 Critical 与丢弃句——两个坏行号上没有任何评论。证据（前后对比）
修复前（当前构建，无门）：混合 payload 的
--dry-run报告wouldPost: true且 3 条 inline——两个非法锚点都会发布，Critical 会静默落在未改动代码上。修复后：同样的运行报告 1 条 inline，打印锚点检查披露（点名迁移的 Critical 与丢弃的 Suggestion），JSON 中携带anchorsRelocated: 1, anchorsDiscarded: 1。真实提交后的读回显示恰好一条新 inline 评论（line: 32, side: right），总结正文包含**[Critical]** qwen-review-anchor-probe.txt:18 — …以及句子1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped；18 与 9999 行上不存在任何评论。探测记录存放在仓库的 e2e-tests 目录；scratch CR 已关闭，分支保留作为证据。测试环境
运行环境（可选）
单元测试 + 经 a1 CLI（v0.2.51）驱动打包后的 CLI 对真实 Aone scratch MR 验证；探测与 live 验证均在内网执行。
风险与范围
关联 Issue
Closes #9615