fix(review): make the incremental cache work for Aone AGit-Flow CRs - #9630
Merged
Conversation
github-actions
Bot
added
the
review/self-reported
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Under AGit-Flow an update amends the single CR commit in place, so Direction: aligned — this lands a rule the accepted platform design (D7, Phase 4 of the phasing plan) already specified but never implemented. The CHANGELOG shows this exact area under active development (#9100 incremental anchor validation, #9184 model-gated anchors, #9226 Aone read path), and the GitHub path is explicitly unchanged (the new option defaults off). Size: no core-module paths touched — 58 production lines in Approach: the scope feels right. The minimal change is exactly this: a Risk: no elevated risk signals — none of the changed files match the high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，非理论问题。AGit-Flow 下更新 CR 是原地 amend 单个 commit，因此 方向：对齐——落地的是已被接受的平台设计（D7，分期计划 Phase 4）早已规定但从未实现的规则。CHANGELOG 显示该方向正在活跃开发（#9100 增量锚点校验、#9184 模型门控锚点、#9226 Aone 读取路径），且 GitHub 路径明确不变（新选项默认关闭）。 规模：未触及核心模块路径—— 方案：范围合理。最小改动正是如此：给 风险：无升级风险信号——改动文件均未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: an amend-based review model makes ancestry a structurally wrong predicate on Aone, so the minimal fix is to skip the ancestry probes for Aone targets inside the anchor ruling — keeping the existence, up-to-date, and stale-base guards — and let the existing narrowing step intersect What I verified in the code:
No correctness, security, or convention issues found. Docs (design D7 "Landed" note, user-doc Aone paragraph, CI evidenceThe PR's own CI for Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the one remaining gap: Real-scenario testing: N/A — this is an unattended CI run; live-behavior signal comes from the lane above, per workflow. 中文说明代码审查我在读 diff 之前的独立判断：amend 审查模型下，祖先测试在 Aone 上是结构性错误的谓词，最小修复是在锚点判定内部对 Aone 目标跳过祖先探测——保留存在性、up-to-date 和 stale-base 守卫——并让既有的收窄步骤把 我在代码中核实了：
未发现正确性、安全性或规范问题。文档（设计 D7 "Landed" 注记、用户文档 Aone 段落、 CI 证据
沙箱验证可以补上唯一的缺口： 真实场景测试：不适用——本次为无人值守 CI 运行；实时行为信号来自上述通道。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — a documented design decision (D7) implemented exactly as specified, with the minimal production change and tests that pin the behavior rather than just the outcome. Stepping back: this is the incremental cache actually starting to work on the platform it was broken on. Under AGit-Flow every update amends the single CR commit, so the ancestry predicate refused every anchor deterministically — not a rare edge case, a 100% miss rate on Aone re-reviews. The fix is the smallest one available: one option, two skipped probes at exactly the right places, one call-site discriminant, and every other guard (existence, up-to-date, stale-base, same-model) demonstrably intact. I proposed the same shape before reading the diff and found nothing simpler. The test suite is the standout: the GitHub/Aone contrast pair runs the identical orphan shape through the real handler and gets opposite, correct rulings; the probe-call-log assertion catches the mutant a pure outcome assertion would miss. The docs — design "Landed" note, user-facing Aone paragraph, The only thing nobody can verify from here is traffic against the real Aone service (internal-only), and the PR says so plainly rather than papering over it. That is a property of the domain, not a defect of this change. Approval is deferred until CI lands green on 中文说明退一步看：这是增量缓存在它一直失效的平台上真正开始工作。AGit-Flow 下每次更新都 amend 单个 CR commit，祖先谓词对每个锚点都是确定性地拒绝——不是罕见的边界情况，而是 Aone 复审 100% 的未命中率。修复是可达范围内最小的：一个选项、在恰好正确的位置跳过两个探测、一个调用点判别，其余每个守卫（存在性、up-to-date、stale-base、same-model）都被证明保持原样。我在读 diff 之前就提出了同样的形状，没有找到更简的方案。 测试套件是亮点：GitHub/Aone 对照对用完全相同的孤儿形状驱动真实命令处理器，得到相反且都正确的判定；探测调用日志断言能抓住纯结果断言会漏掉的变异体。文档——设计"Landed"注记、面向用户的 Aone 段落、 从这里唯一无法验证的是真实 Aone 服务的流量（仅限内网），PR 对此如实说明而非掩盖。这是领域的属性，不是本改动的缺陷。 批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Author
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Collaborator
|
🤝 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 冲突，直到移除标签或达到轮次上限。移除 |
Contributor
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
previously approved these changes
Aug 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // at all under `noAncestry` — see the docstring's AGit-Flow paragraph: | ||
| // an amend orphans the cached head, so the test would fail for every | ||
| // update, and the two heads' diff is the update's delta anyway. | ||
| if (!options.noAncestry && !probe.isAncestor(resolved, fetchedSha)) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The SHA_RE doc block in packages/cli/src/commands/review/lib/ledger.ts (lines 119-126, a file this diff does not touch) states unconditionally that "fetch-pr --since additionally verifies the anchor is an ancestor of the fetched head before scoping to it … so a tampered sha costs a full-range review, never a mis-scoped one", framed as one answer applied at every gate that reads an anchor. This diff makes that ancestry verification platform-conditional (the !options.noAncestry && guard here and on the clamp below), so on Aone the comment now describes a behaviour the code no longer has. Every other documentation site was updated by this PR — user docs, design doc, --since help text, both docstrings — which makes the ledger.ts miss read as oversight rather than intent. The safety property itself survives on Aone (the published scope is still joined against the CR's own diff, so a tampered sha still costs at most a full-range review), but the comment attributes that guarantee to the ancestry check, which Aone no longer runs: a maintainer extending the anchor gates — adding a third platform, or relaxing the existence checks in the belief that ancestry validates lineage for every --since anchor — reads the unconditional guarantee at the stated trust-model site and reasons from a check that does not fire on Aone.
Qualify the ledger.ts sentence, e.g.:
`fetch-pr --since` additionally validates the anchor against the fetched
history — existence always; ancestry except on Aone, where AGit-Flow
amends orphan the cached head (design D7) — before scoping to it. The
published scope is joined against the CR's own diff either way, so a
tampered sha costs a full-range review, never a mis-scoped one.
中文说明
packages/cli/src/commands/review/lib/ledger.ts（119-126 行，本 diff 未改动的文件）中的 SHA_RE 文档块无条件地声明："fetch-pr --since 在划定范围前会额外验证锚点是已 fetch head 的祖先……因此被篡改的 sha 只会导致全量范围审查，绝不会导致错误划定"，并将其表述为应用于每个读取锚点的门控的统一答案。本 diff 使该校验变为平台条件化（此处及下方 clamp 上的 !options.noAncestry && 守卫），因此在 Aone 上该注释描述的已是代码不再具有的行为。本 PR 更新了其他所有文档位置——用户文档、设计文档、--since 帮助文本、两处 docstring——因此 ledger.ts 的遗漏更像疏忽而非有意。安全属性本身在 Aone 上依然成立（发布范围仍会与 CR 自身 diff 求交，被篡改的 sha 最多导致全量审查），但注释把该保证归因于 Aone 已不再执行的祖先检查：后续扩展锚点门控的维护者——新增第三个平台，或误以为祖先校验会为每个 --since 锚点验证 lineage 而放宽存在性检查——会在这一声明的信任模型位置读到无条件保证，并基于一个在 Aone 上不会生效的检查做决策。
建议对 ledger.ts 中的句子加以限定，例如："fetch-pr --since 在划定范围前会额外针对已 fetch 的历史验证锚点——存在性始终校验；祖先校验除外于 Aone（AGit-Flow 的 amend 会孤儿化缓存的 head，见设计决策 D7）。发布范围在任一平台都会与 CR 自身 diff 求交，因此被篡改的 sha 只会导致全量范围审查，绝不会导致错误划定。"
— qwen3.8-max via Qwen Code /review (v0.21.15)
Comment on lines
+368
to
+369
| * onto newer master, the range additionally carries the rebase drift, which | ||
| * the re-review should see anyway). The published scope is still assembled |
Collaborator
There was a problem hiding this comment.
[Suggestion] This docstring claims an amended-and-rebased Aone update stays scoped and that "the re-review should see [the rebase drift] anyway", but the code can produce neither outcome. narrowToDelta (lib/narrow-diff.ts) assembles every published byte from the base-derived base..head capture — the delta capture is read only for its path set and post-image ranges, "not one of its bytes reaches the result" — and rebase drift lives below the new merge base, so no published byte is ever drift. And when the drift touches any file outside the CR's diff, the shape-one guard returns null → nothing-to-narrow → full-range fallback, so scoping does not survive amended-and-rebased updates in general (only when the drift's file set ⊆ the CR's files). The same overclaim appears in the clamp-skip comment added ~75 lines below, the design-doc hunk ("the delta deliberately carries that drift") and the user-docs paragraph ("scopes … instead of falling back to a full review"), and the paragraph contradicts itself four lines later: the published scope "cannot carry a hunk the platform does not display" — drift hunks are exactly that. The cost is concrete: a maintainer modifying the clamp or the narrowing join reads this docstring, concludes drift bytes are supposed to reach the review, and admits delta-only paths — re-introducing the defect class narrow-diff.ts exists to eliminate (published hunks the platform diff does not display, which 422 on inline comments). Verified by probe against the real handler: drift touching an outside-CR file → {effective:false, reason:"nothing-to-narrow"} full-range fallback with no drift bytes; drift confined to a CR file → scoped, published byte-identical to the PR's own section, published.includes("master drift line") === false.
Suggested change
| * onto newer master, the range additionally carries the rebase drift, which | |
| * the re-review should see anyway). The published scope is still assembled | |
| * onto newer master, the range additionally carries the rebase drift; the | |
| * narrowing join reads it only for which files changed and never lets a | |
| * drift byte reach the published scope, falling back to the full range via | |
| * `nothing-to-narrow` when the drift touched files outside the CR's diff). | |
| * The published scope is still assembled |
Mirror the same correction in the clamp-skip comment, the design doc and the user docs.
中文说明
此 docstring 声称 amend+rebase 的 Aone 更新仍会被划定范围，且"复审本应看到 rebase 漂移"，但代码不可能产生这两种结果。narrowToDelta（lib/narrow-diff.ts）完全从 base 派生的 base..head 捕获组装发布的每一个字节——delta 捕获仅被读取其路径集合与 post-image 范围，"没有一个字节会进入结果"——而 rebase 漂移位于新 merge base 之下，因此发布的字节永远不可能是漂移。并且当漂移触及 CR diff 之外的任何文件时，shape-one 守卫返回 null → nothing-to-narrow → 全量范围回退，因此 amend+rebase 的更新通常无法保持划定（仅当漂移的文件集 ⊆ CR 的文件集时才可以）。同样的过度声明还出现在下方约 75 行新增的 clamp 跳过注释、设计文档片段（"delta 刻意携带该漂移"）以及用户文档段落（"划定范围……而不是回退到全量审查"）中；且该段落四行之后自相矛盾：发布范围"不可能携带平台 diff 不展示的 hunk"——而漂移 hunk 恰恰如此。代价是具体的：修改 clamp 或 narrowing join 的维护者读到该 docstring，误以为漂移字节本应进入审查，从而允许 delta-only 路径——重新引入 narrow-diff.ts 旨在消除的缺陷类别（发布平台 diff 不展示的 hunk，导致行内评论 422）。已用探针在真实 handler 上验证：漂移触及 CR 外文件 → {effective:false, reason:"nothing-to-narrow"} 全量回退且无漂移字节；漂移局限于 CR 文件 → 划定生效，发布内容与 PR 自身段落逐字节一致，published.includes("master drift line") === false。
请同步修正 clamp 跳过注释、设计文档与用户文档。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Comment on lines
+362
to
+365
| * updating a CR AMENDS the single commit in place: the amended H2 has H1's | ||
| * parent, never H1 itself, so the old head is orphaned and BOTH ancestry | ||
| * tests — the anchor-behind-head test and the clamp — fail for EVERY | ||
| * update, and an amend-and-re-review never scoped. Neither is asked: after |
Collaborator
There was a problem hiding this comment.
[Suggestion] This docstring claims BOTH ancestry tests "fail for EVERY update", but the clamp (isAncestor(mergeBase.sha, resolved), ~line 448) cannot fail for the common pure-amend geometry: for a pure amend, merge-base(target, H2) is still the shared parent P (or an ancestor of P), which IS an ancestor of the cached head H1 — the clamp answers yes and would not have refused. Only the head test fails for every amend; the clamp fires only when the update also rebased onto newer master, moving the merge base past the cached head — exactly the trigger the sibling clamp-skip comment this same diff adds names. Two sites in the same function thus give contradictory accounts of when the clamp fires. Verified against real git fixtures: pure amend → head test isAncestor(H1,H2) exits 1 (refuses) while clamp question isAncestor(M1,H1) exits 0 (passes — would NOT refuse); amend+rebase → clamp isAncestor(M2,H1) exits 1 (fires). The claim is repeated in the two test-comment blocks this diff adds (in the test file at ~2531 and ~2948), the call-site comment (~1179) and the design-doc paragraph. The cost: a maintainer tightening or restoring the Aone clamp reads the docstring and concludes the clamp was already refusing every update anyway (its skip vestigial), when it only ever refused the amend-plus-rebase shape.
Suggested change
| * updating a CR AMENDS the single commit in place: the amended H2 has H1's | |
| * parent, never H1 itself, so the old head is orphaned and BOTH ancestry | |
| * tests — the anchor-behind-head test and the clamp — fail for EVERY | |
| * update, and an amend-and-re-review never scoped. Neither is asked: after | |
| * updating a CR AMENDS the single commit in place: the amended H2 has H1's | |
| * parent, never H1 itself, so the old head is orphaned and the | |
| * anchor-behind-head test fails for EVERY update — the amended head never | |
| * descends from the cached one. (The clamp additionally fails whenever the | |
| * update also rebased onto newer master, since the merge base then moves | |
| * past the cached head; a pure amend passes it.) Neither is asked: after |
Mirror the correction in the two test-block comments, the call-site comment and the design doc.
中文说明
此 docstring 声称两个祖先测试"对每次更新都失败"，但 clamp（isAncestor(mergeBase.sha, resolved)，约 448 行）在常见的纯 amend 几何下不可能失败：纯 amend 时 merge-base(target, H2) 仍是共享父提交 P（或 P 的祖先），它本身就是缓存 head H1 的祖先——clamp 回答 yes，本不会拒绝。每次 amend 都失败的只有 head 测试；clamp 仅在更新同时 rebase 到更新的 master、merge base 移过缓存 head 时才触发——这正是本 diff 添加的相邻 clamp 跳过注释所指明的触发条件。同一函数中两处对 clamp 何时触发的描述互相矛盾。已在真实 git fixture 上验证：纯 amend → head 测试 isAncestor(H1,H2) exit 1（拒绝），clamp 问题 isAncestor(M1,H1) exit 0（通过——本不会拒绝）；amend+rebase → clamp isAncestor(M2,H1) exit 1（触发）。该说法在本 diff 添加的两处测试注释块（测试文件约 2531 与约 2948 行）、调用点注释（约 1179 行）以及设计文档段落中重复出现。代价：收紧或恢复 Aone clamp 的维护者读到该 docstring，会误以为它本来就拒绝了每次更新（跳过它毫无意义），而它实际只拒绝过 amend+rebase 形状。
请同步修正两处测试注释块、调用点注释与设计文档。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
The D7 comments described the ancestry gate as unconditional and both ancestry tests as failing for every AGit-Flow update; the head test alone fails for every amend (the clamp fires only on amend-plus-rebase), and the narrowing join never lets a drift byte reach the published scope. Qualify the ledger.ts SHA_RE block, the resolveIncrementalAnchor docstring, the clamp-skip and call-site comments, the test-block comments, and the design/user docs accordingly.
Collaborator
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9630 (branch
|
qwen-code-ci-bot
previously approved these changes
Aug 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
|
@qwen-code /resolve |
…ranch Resolve the fetch-pr.test.ts conflict where the AGit-Flow noAncestry fixtures and the one-hop import-widening tests landed side by side: keep the serveOrphanShape helper and all three widening tests. Update the Aone e2e assertion to the post-widening report shape (the scope field now rides on every narrowed incremental round, Aone included).
Collaborator
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — conflict resolutionFeedback triage: the round's feedback carried no new findings — no reviews, inline comments, issue-level comments, failed checks, or still-red checks newer than the last evaluation. Nothing to address, decline, defer, or escalate. Base-conflict resolution (
Mutation probe: temporarily removed the Verification
中文说明Autofix 审查轮次 —— 冲突解决反馈分类处理： 本轮反馈没有新发现 —— 没有比上次评估更新的审查意见、行内评论、 issue 级评论、失败检查或持续失败的检查。没有任何需要处理、拒绝、延后或升级的内容。 基线冲突解决（
变异探针： 临时移除发布分支中的 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
…mental-ancestry # Conflicts: # docs/users/features/code-review.md
Collaborator
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
qwen-code-ci-bot
approved these changes
Aug 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 6b": none — no check was cut short..
中文说明
未发现问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 6b"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.15)
wenshao
enabled auto-merge
August 21, 2026 11:03
yiliang114
approved these changes
Aug 21, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified the noAncestry rule against the head:
- The option gates exactly the two ancestry probes (head test + merge-base clamp) and nothing else — existence checks,
base-untrustedrefusal, and the up-to-date ruling all stay, and GitHub keeps both tests since the call site only passesnoAncestryforplatform.kind === 'aone'. - The safety claim holds by construction:
narrowToDeltabuilds the published scope out of the CR's ownbase..headcapture and readsanchor..headonly for post-image ranges, so an orphaned or foreign-lineage anchor can widen the re-review but never publish a hunk the platform does not display. - Test coverage matches the claims: the Aone/GitHub contrast pair pins the platform split, and the unit cases pin that neither ancestry question is asked while every non-ancestry refusal survives. CI is green on head.
This was referenced Aug 21, 2026
wenshao
added a commit
that referenced
this pull request
Aug 21, 2026
… and comment-status skips D6 described the AGit-Flow anchor as inert until the incremental rule landed, but that rule (#9630) merged while this branch was in flight — anchors now delta-scope Aone re-reviews. SKILL.md's comment-status section and Step 6's report-existence guard now name the Aone skip the setup batch already carries, so no path sends an Aone run at the unbacked command or at a report that was never written.
Collaborator
|
Released in v0.22.0. |
JadeCong
pushed a commit
to CloudEngineHub/qwen-code
that referenced
this pull request
Aug 23, 2026
* feat(review): back pr-context on Aone Code targets pr-context was the one read subcommand still gh-direct, so every Aone run was forced context-unavailable: the verdict capped at COMMENT (the wired a1 approval could never fire), Agent 0 skipped, and the machine ledger never recovered from posted summaries. Route it through the platform reader with a normalized context bundle; Aone serves it from mr view + the flat comment list (thread comments carry the ledger), GitHub's implementation is an extraction of the existing calls — its output stays byte-identical. The forced cap leaves the Aone write path for parity with GitHub's state-claim handling, and the refetch commands a context file emits bake --pr on Aone, where comment bodies are addressed per-MR. * fix(review): keep Aone ledger carriers out of the blocker re-check (QwenLM#9621) On Aone this pipeline's own round summaries are path-less comments, so they ride pr-context's issue channel, where their visible **[Critical]** lines self-promoted every prior Critical-bearing summary into "Blockers to re-check" — rendering each prior Critical three times (beside the ledger section and the inline roots that own the same findings) and spending the section budget on the pipeline's own prose until genuine human blockers degraded to snippets. Exclude bodies carrying the ledger marker from issue-channel promotion and the stdout count, strip the marker out of the settled snippet, and switch the pr_number guard to the canonical isPositivePrNumber so 0x10/5. spellings cannot fragment side-file continuity. Pin the witnesses the round's findings name: the guard, args.host forwarding, the issue-kind --pr refetch branch, the account-first author keying, and the GitHub test suites' independence from the cwd-origin probe. * fix(review): refuse pr_number spellings that do not round-trip (QwenLM#9621) isPositivePrNumber alone admits two spellings whose Number() value does not round-trip to the raw string: leading zeros (007 fetches 7 but the raw string labels the heading and the prev-ledger side file, so a later 7 run reads a different side file and the round counter restarts) and digit strings above Number.MAX_SAFE_INTEGER (Number() silently rounds them, fetching a different PR than the labels announce). Add the safe-integer and no-leading-zero conjuncts — matching fetch-pr's [1-9]\d* rule — so every admitted input satisfies String(Number(x)) === x. Also pin the witnesses the round-2 review names: the commit_id round-trip through the GitHub reader and toRawReview into the persisted side file (both spreads were unwitnessed), the stale force-applies comment in submit-aone.test.ts the cap removal outdates, and the setup batch's Aone carve-out for the unbacked comment-status call. * docs(review): align Aone docs with the landed no-ancestry anchor rule and comment-status skips D6 described the AGit-Flow anchor as inert until the incremental rule landed, but that rule (QwenLM#9630) merged while this branch was in flight — anchors now delta-scope Aone re-reviews. SKILL.md's comment-status section and Step 6's report-existence guard now name the Aone skip the setup batch already carries, so no path sends an Aone run at the unbacked command or at a report that was never written. * docs(review): annotate QwenLM#9616 as landed and define the report-less re-check rule The out-of-scope list still read self-PR detection as open work although QwenLM#9629 shipped it into this branch's merge base — annotate it like the sibling QwenLM#9618 entry. Step 6's report-existence guard pointed report-less runs at a re-derivation the skill never defines; replace it with the explicit rule: no per-thread status routing, no hand-derived substitute, rule from the code at the reviewed commit, cannot-tell over a guess. * fix(review): route the context head through aoneHeadSha and close the round-5 findings getReviewContext read sourceBranch raw while every other head read trims — a padded server value diverged the context file from the rest of the run (phantom-drift shape). getCurrentUser now honors the seam contract on the anomalous whoami shapes instead of leaking untagged throws and non-string accounts. Step 6's report-less rule no longer contradicts the comment-status failure contract: runs where the command ran and failed keep the "re-derive if needed" fallback. The Aone paragraph names comment-body among the backed reads, and witness tests pin the identity gate's carriers key and the head normalization. * fix(review): shape-check the Aone comment listing in getReviewContext a1 can answer repo mr comment list with an exit-0 a1.error/v1 error object (backend auth failure or client timeout — measured by cleanup's a1CommentList on the identical payload). Without a guard the object survives the ?? [] coalesce and .filter throws an untagged TypeError, losing the envelope's actionable message at exactly the recoverable moment. Guard as the provider family already does and surface the cause; witness tests pin both envelope shapes (mutant-checked). * test(review): pin getCommentBody's body-field fallback (mutant-checked) * fix(review): union resolved comments into the Aone context bundle The default comment list excludes resolved comments (measured by the cleanup audit) while GitHub's REST fetches include them, so a resolved blocker/marker root never reached the re-check walk or the fail-closed identity gate. Union the default and --resolved listings as the audit does, dedupe by id, fail closed on either listing's error envelope, and disclose the residual that resolved replies stay invisible; witness tests mutant-checked. * fix(review): serve resolved comments and guard the envelope in getCommentBody getCommentBody queried only the default comment list while the context bundle it serves refetches for unions in resolved comments — a resolved id named by a truncation note threw "not found" every time, and an exit-0 a1.error/v1 envelope threw an untagged TypeError that lost the actionable message. Extract the shape-checked default+resolved union helper and read both sites through it; witness tests mutant-checked. * ci: correct qwen-autofix.yml size baseline to its actual post-migration size QwenLM#9677 shrank qwen-autofix.yml from 431526 to 397656 bytes (prose moved to the design record) but recorded the baseline at 392111, 5545 below the file's own post-change size, so the first PR to run the ratchet tripped it. This branch introduces zero growth to the file (byte-identical to main); the bump aligns the baseline with reality. No workflow content changes. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes incremental review scoping work for Aone Code CRs. Under AGit-Flow, updating a CR amends its single commit in place, which orphans the head the previous review round covered — so the ancestry test the incremental anchor ruling relied on fails for every update, and every post-amend re-review silently fell back to reviewing the full diff. On Aone targets the anchor is now ruled without ancestry: after the fetch both heads are local, and the diff between the cached head and the new head is exactly the update's delta. The existence checks, the same-model gate, and the stale-base refusal all stay, and the published scope is still assembled from the CR's own diff, so it can never carry a hunk the platform does not display. GitHub keeps the ancestry tests unchanged — there an anchor the head does not descend from is rewritten history, and the tests are the detection.
Why it's needed
The incremental cache exists so repeat rounds review only what changed. On Aone it never applied: an amend orphans the cached head,
merge-base --is-ancestorexits non-zero for every update, and every re-review paid the full-diff cost. This implements the rule the platform design already specified (decision D7) but never landed.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/fetch-pr.test.ts→ 153/153.a1CLI and a fixture repo servingrefs/merge-requests/<id>/head): round 1 reviews head H1; amend + force-update to H2; round 2 with the cached anchor. Before:Incremental anchor … refused (not-an-ancestor)and the full diff published. After:Incremental: scoped to <H1>..<H2>and the published diff is exactly the amend's delta.Evidence (Before & After)
Before (round 2 stderr):
Incremental anchor aa9f97e217 refused (not-an-ancestor); reviewing the full diff.— reportincremental: {since: …, effective: false, reason: "not-an-ancestor"}, published diff = the full merge-base range (2 files).After:
Incremental: scoped to 14ce532ed4..a1742ca740.— reportincremental: {since: …, effective: true, diffBase: <merge-base>}, published diff = the amend's one-line delta only.Tested on
Environment (optional)
Local bundle (
npm run build && npm run bundle,node dist/cli.js) with a stubbeda1CLI and fixture repos for the E2E; unit suites run with vitest.Risk & Scope
Linked Issues
Fixes #9618
中文说明
这个 PR 做了什么
让增量审查的范围划定在 Aone Code CR 上真正生效。AGit-Flow 下，CR 的每次更新都是原地 amend 单个 commit，这会让上一轮审查覆盖过的 head 变成孤儿——因此增量锚点判定所依赖的祖先测试对每次更新都失败，amend 之后的每一轮复审都悄悄退化为全量 diff 审查。现在对 Aone 目标判定锚点时不再做祖先测试：fetch 之后两个 head 都在本地，缓存 head 与新 head 之间的 diff 恰好就是这次更新的增量。存在性检查、same-model 门、stale-base 拒绝全部保留，发布范围仍然从 CR 自身的 diff 组装而来，因此不可能携带平台 diff 中不存在的 hunk。GitHub 路径的祖先测试保持不变——在 GitHub 上，head 不是其祖先的锚点意味着历史被重写，祖先测试本身就是检测手段。
为什么需要
增量缓存的存在意义是让复审只看变化的部分。在 Aone 上它从未生效过：amend 会孤儿化缓存的 head，
merge-base --is-ancestor对每次更新都返回非零，每一轮复审都付出了全量 diff 的成本。本 PR 落地了平台设计文档中早已规定（决策 D7）但一直未实现的规则。评审者测试计划
如何验证
cd packages/cli && npx vitest run src/commands/review/fetch-pr.test.ts→ 153/153。a1CLI + 提供refs/merge-requests/<id>/head的 fixture 仓库验证）：第 1 轮审查 head H1；amend + force-update 到 H2；第 2 轮携带缓存锚点。修复前：Incremental anchor … refused (not-an-ancestor)，发布全量 diff。修复后：Incremental: scoped to <H1>..<H2>，发布的 diff 恰好是 amend 的增量。证据（前后对比）
修复前（第 2 轮 stderr）：
Incremental anchor aa9f97e217 refused (not-an-ancestor); reviewing the full diff.——报告incremental: {since: …, effective: false, reason: "not-an-ancestor"}，发布 diff = 完整 merge-base 范围（2 个文件）。修复后：
Incremental: scoped to 14ce532ed4..a1742ca740.——报告incremental: {since: …, effective: true, diffBase: <merge-base>}，发布 diff 仅为 amend 的那一行增量。测试环境
环境（可选）
本地 bundle（
npm run build && npm run bundle、node dist/cli.js），E2E 使用桩a1CLI 与 fixture 仓库；单元测试用 vitest 运行。风险与范围
关联 Issue
Fixes #9618