fix(review): clear the deferred Round-5 findings from the Aone write path - #9604
Conversation
…path The full cleanup of QwenLM#9579 — the 29 Suggestions deferred from round 5 of the /review bot on QwenLM#9491 under the ~5-round rule (Criticals-only from that round on). One item (the GH_HOST setGhHost assertions) was already landed with the round-5 Critical fixes; the rest are implemented here. Write-path fixes: - A shaped-but-empty --host refuses with its own shape (host-flag-empty) instead of collapsing to the unbound refusal the flag was the remedy for — the agent re-run loop the refusal wording exists to break. - An invalid host (recorded verbatim or flag-typed) refuses in the exit-3 shape naming the offender and its origin, instead of setGhHost's TypeError escaping runSubmit as a failed command. - A flagless gh post whose nothing-bound routing would inherit an ambient GH_HOST pointing at canonical Aone refuses actionably (ambient-gh-host-aone) instead of failing opaquely after compose ran. - The shared authorisation gate no longer reads an absent host as a github.com claim for callers whose routing follows the recorded binding (submit): the ordinary flagless publish of a GHE-recorded review passes, while publish-assets keeps the strict comparison. - Mid-batch drift disclosure rides the partial-post shape too (headMovedDuringPost on AonePartialPostError, warned from submit's partial branch), and the post-batch re-read is tri-state: a failed re-read leaves headMovedDuringPost undefined and submit discloses "could not re-verify" instead of a false all-clear. - The Aone success JSON surfaces postedCommentIds/summaryCommentId — the audit the partial shape carries and the gh receipt records. Docs and contract fixes: - The context-unavailable cap wording now says what it does (keeps an Approve verdict at Comment; a Request-changes verdict still posts) in the user docs and both SKILL.md sites. - The head-drift bullet is qualified by the per-review restart bound — spent on Aone there is no submit-at-reviewed-SHA fallback; report and leave the rest to the user. - Step 9's Posted: contract admits the no-link note the Aone fallback prescribes. - The --host help text spells both canonical Aone hosts out. - The provider design doc's Phase-3 "refuses" sentence is marked superseded. Test hardening (unfalsifiable pins made falsifiable): - ensureAoneAuthenticated ordered before the writes; setGhHost ordered before the gh write; the a1 path never touches the gh host state. - Live-probe cells for the explicit-flag precedence, the unbound refusal, and the fast-path hostless refusal; the recorded-binding- outranks-probe fixture driven through submit's real gitOpt seam. - submit.test.ts mocks ./lib/git.js (no real git spawned in the vitest cwd), isolates the cross-session suite's recording store via chdir, and pins the newest-wins ordering when two recordings of one PR carry different hosts. - Producer-side 'refusing to post:' prefix pins, the RC-Note count source pin, the contextUnavailable:true gh-path pin, and the floor recovery's callerHost pin.
|
Re-run over head Template looks good ✓ Problem: observed and tracked, not theoretical hardening. The PR clears the deferred Round-5 findings logged in #9579 — each quoted verbatim with its review thread — including user-visible correctness bugs (the design doc still claiming Direction: aligned — the repo's own ~5-round rule executed as written: Criticals landed in-round, deferred Suggestions cleared in a tracked follow-up so nothing is silently dropped. Size: ~366 production-logic lines ( Approach: scope still maps one-to-one onto the tracking issue — no drive-bys. The merge is where I spent the scrutiny: reconstructing the same three-way merge locally proves git auto-merged all eight code/test/skill files untouched (each is byte-identical to the pure auto-merge result); only the two doc files genuinely conflicted, and both resolutions keep both sides' intent — #9624's test-plan note and a1 version floor survive, and this PR's superseded block and cap-wording corrections apply on top (detailed in the review comment). The effective diff against the current base is still exactly +1062/−99. Risk: no elevated signals — none of the changed files matches the revert-correlated path set. Moving on to code review. 🔍 中文说明针对当前 head 模板完整 ✓ 问题:已观测且有登记,不是理论加固。本 PR 清理 #9579 登记的延期第 5 轮发现——每条逐字引用并附评审线程链接——其中包含用户可见的正确性缺陷(设计文档仍声称 方向:对齐——仓库自身"约 5 轮"规则的照章执行:轮内合入 Critical,延期 Suggestion 在跟踪 follow-up 中清理,以免静默丢失。 规模:约 366 行生产逻辑( 方案:范围仍与跟踪 issue 逐条对应,未见夹带改动。合并是本阶段投入审查之处:本地重建同一三方合并证明全部八个代码/测试/skill 文件均由 git 自动合并、未经解决之手改动(每个文件与纯自动合并结果逐字节一致);真正冲突的只有两个文档文件,且两处解决均保留双方意图——#9624 的 test-plan 说明与 a1 版本下限完整保留,本 PR 的"已取代"块与 cap 措辞修正在其上应用(详见评审评论)。对当前 base 的有效 diff 仍恰为 +1062/−99。 风险:无升级信号——改动文件均未命中回退相关路径集。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe write-path review from prior rounds stands — its hunks are untouched at this head (the effective diff against the current base is the reviewed +1062/−99, file for file). What this re-run scrutinized is the new merge
Non-blocking, carried from the round-9 ledger: the revert-guard regex does not cover diff3 Test evidenceThe PR's own CI on the reviewed head, fetched via the API — no code run locally.
The ubuntu unit suite is the check that matters this round — it is the first run of the combined tree (this PR's pins + #9624's hunks in the same files), and it is still in flight; the table above updates in place when CI settles. Skipped entries are the fork-PR workflow design, not gaps. Sandboxed deep verification: the last completed round judged head 中文说明代码审查此前各轮对写路径的审查仍然成立——那些代码块在当前 head 上未变(对当前 base 的有效 diff 逐文件就是被审的 +1062/−99)。本次复跑细审的是新合并
非阻断、沿用第 9 轮台账:防回退正则未覆盖 diff3 测试证据被审 head 的 PR 自身 CI,经 API 拉取——本地未运行任何代码。ubuntu 单测套件是本轮的关键检查——它是合并树(本 PR 的钉 + #9624 的同文件代码块)的首次运行,仍在进行中;CI 落定后上表就地更新。跳过项是 fork PR 的工作流设计,不是漏洞。 沙箱深度验证:上一完成轮判定 head — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage on the current head; the only delta since the deep-verified head is a merge I checked hunk by hunk, and the one open item is the CI still running on it. Stepping back: my independent proposal for "clear the deferred Round-5 findings" is exactly this PR's shape — one minimal fix per logged finding with a falsifiable pin beside it — and the current head still matches it. Every hunk maps to a logged item in #9579; the diff carries nothing outside that set. What this re-run actually had to settle was narrower than a full review, and sharper: one new commit — a merge of main — but this one brought in #9624, which edits seven of this PR's ten files, so it was not the merge-only no-op of last round. That is precisely the failure mode this PR once took a changes-requested for (merge-conflict residue), so I verified directly rather than trusting a green diff: reconstructing the same three-way merge proves every code, test, and skill file is byte-identical to an unassisted auto-merge, and the only two genuine conflicts — both docs — resolve losslessly, each keeping both sides' intent (write-up in the review comment). Residue probes confirm no markers, no stale cap wording, both sides' symbols present. The write-path code the deep verification judged merge-ready at 173/173 is otherwise untouched, so that evidence carries over undiminished; the combined tree's first real test is the unit suite now running, and an advisory re-verification of this head reports under its own marker. Two human approvals already stand on this exact head, and that eyeballing agrees with my merge reconstruction. The gate's harder questions still answer cleanly: the problem was observed and tracked, scope never left the tracking issue across nine rounds, and the round-6 merge-residue changes-requested was resolved and pinned by a later head. Approving this closes a logged finding, not overrides one. CI has not settled on the reviewed head — the ubuntu unit suite is still in flight — so approval is deferred until it lands green on 中文说明置信度:5/5 —— 本 head 各阶段均干净;深度验证 head 以来的唯一增量是逐块核查过的合并,唯一未定项是其上仍在运行的 CI。 退一步看:我对"清理延期第 5 轮发现"的独立方案与本 PR 形态完全一致——每条登记发现一处最小修复、旁附一条可证伪的钉——当前 head 依然相符。每个代码块均对应 #9579 中的一条登记项,diff 未夹带任何登记集之外的内容。 本次复跑真正需要解决的比完整评审窄、也更尖锐:只有一个新 commit——合并 main——但这次合并带入了 #9624(编辑本 PR 十个文件中的七个),因此不是上轮那种纯合并增量。这正是本 PR 过去吃过一次 changes-requested 的失败模式(合并冲突残留),所以我直接验证而非轻信绿色 diff:重建同一三方合并证明每个代码、测试与 skill 文件均与无干预自动合并逐字节一致,仅有的两处真实冲突——均为文档——无损解决,各自保留双方意图(详见评审评论)。残留探针确认无冲突标记、无过期 cap 措辞、双方符号均在。深度验证以 173/173 判定可合入的写路径代码除此之外未被触碰,该证据不打折地延续;合并树的首次真实检验是正在运行的单测套件,针对本 head 的咨询性复验将在其自身标记下报告。 两个人类批准已落在本 head 上,该目检与我的合并重建一致。门禁的更硬问题仍回答得干净:问题真实且有登记,九轮以来范围从未超出跟踪 issue,第 6 轮合并残留的 changes-requested 已被后续 head 解决并钉住。批准它是在关闭一条发现,而不是否决一条发现。 CI 尚未在被审 head 上定局——ubuntu 单测套件仍在运行——因此批准延后至其在 — Qwen Code · qwen3.8-max Reviewed at |
|
@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. 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-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.14)
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 1/100 轮)。改动内容与我反驳保留之处如下: Round summary — all 6 findings resolved in codeAll six findings are Suggestion-severity from the automated reviewer (round 1 of this window). Each was verified against the current code before acting; none declined, deferred, or escalated. No conflict ( Findings and dispositions
Mutation-probe witnesses (each new guard verified before commit)
Conflict notesNone — Verification
中文说明本轮总结——6 条发现全部在代码中解决六条发现均来自自动审查器(本窗口第 1 轮),严重级别均为 Suggestion。每条都在动手前先对照当前代码验证;没有拒绝、推迟或升级处理的条目。无冲突( 发现与处理
突变探针见证(提交前逐一验证每个新守卫)
冲突说明无—— 验证
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 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-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit.ts:729 — [probe] HOSTNAME_RE pre-validation tests the untrimmed bound host; setGhHost trims firstpackages/cli/src/commands/review/submit.ts:753 — [review] 8 hand-written copies of the exit-3 refusal tail; sibling publish-assets already extracts a refuse helperpackages/cli/src/commands/review/lib/authorization.ts:420 — [review] recordedSeverityFloor's comment still cites the gate's absent-host rule as unconditional
中文说明
已审查。 建议见行内评论。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
doudouOUC
left a comment
There was a problem hiding this comment.
审查报告 / Review Report
PR: #9604 — fix(review): clear the deferred Round-5 findings from the Aone write path
Author: wenshao
Scope: 9 files, +820/-36 (380 source, 831 test, 32 docs)
This is a thorough cleanup of the deferred Round-5 suggestions from the previous review. The code is well-structured with thorough tests. Below are the findings.
✅ 无阻断问题 / No Blockers
All three review agents (correctness, security, quality) found no correctness bugs, no security vulnerabilities, and no quality regressions.
建议 / Suggestions
S-1 (minor, consistency): Aone pre-write refusal message has an extra "the review" compared to other refusal messages.
In submit.ts, the Aone pre-write refusal (head drift check) writes:
REFUSED to post the review to ...
while all other 6 refusal paths write:
REFUSED to post to ...
The extra "the review" is not a bug, but aligning the phrasing would make automated parsing slightly more uniform. Non-critical.
(submit.ts ~line 1030 area)
S-2 (minor, code quality): Refusal boilerplate could be extracted to a helper.
The 7 refusal exits each repeat the same pattern:
writeStderrLine(...)
writeStdoutLine(JSON.stringify(...))
process.exitCode = 3
return
A small helper like refuse(reason: string, exitCode: number) would save ~21 lines and prevent a future refusal from forgetting the return or exitCode. The publish-assets sibling already has an extracted refuse helper — this codebase has precedent. Non-critical; deferrable.
验证 / Verification
- Build: Not runnable on this Windows machine due to pre-existing TS6305 reference-freshness issues in
acp-bridge(unrelated to this PR). - Typecheck: Only TS6305 errors are pre-existing — no new type errors from the PR changes.
- Tests: Could not run due to missing build artifacts in the worktree (
npm installtimed out). The test structure is thorough: 831 test lines across 3 test files, covering all new code paths including:- Empty/whitespace
--hostflag - Invalid host from 3 provenance arms (flag, recorded, cwd origin)
- Contradictory explicit vs recorded host
- Ambient
GH_HOSTpointing at Aone absentHostFollowsRecordingflagless GHE publishheadMovedDuringPosttri-state (true/false/undefined) on both success and partial failure paths- Recording ordering (newest-wins)
- Ordered invocation call assertions (auth before writes, host-binding before gh calls)
- Empty/whitespace
总体评价 / Overall Assessment
No blockers. The PR is well-structured, the code is correct, and the tests are thorough. The two suggestions are minor and deferrable. Ready to ship.
doudouOUC
left a comment
There was a problem hiding this comment.
Round 1 审查完成(model: deepseek-v4-flash),目标 PR head: cd57459c...。
结论:ISSUES_FOUND(仅 minor suggestions,无 bug / 安全问题)
- 正确性:未发现 bug;空 host、非法 host 校验,
absentHostFollowsRecording,head-drift 三态,Aone 写入判定均正确。 - 安全性:5 项安全关注点均安全;
absentHostFollowsRecording回退、HOSTNAME_RE、setGhHost与 a1 路径隔离均符合设计。 - 质量与性能:测试覆盖充分。剩余 2 条 minor suggestions:
- 提取拒绝文案的 boilerplate,减少重复。
- 统一一处错误消息措辞。
Head 未漂移(当前仍为 cd57459c...)。因第一轮已报出 findings,未启动第二轮(qwen3.8-max)。
…9604) Extract one refuse helper for submit's seven exit-3 refusal shapes (sibling publish-assets precedent), align the Aone pre-write refusal prefix with the other refusal paths, and pin the invalid-host remedy of the flag/origin arms positively — the recorded arm's absence pin alone let a ternary-collapse mutant ship green.
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: Round-2 summary — PR #9604One commit: Feedback dispositions
Verification
中文说明第 2 轮处理总结 — PR #9604一个提交: 反馈处理结论
验证
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.
No blocking issues. LGTM! ✅
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit.ts:719 — [probe] invalid-host pre-validation tests the untrimmed boundHost; setGhHost trims firstpackages/cli/src/commands/review/lib/platform/aone.ts:886 — [probe] no producer-side false-state test for the failure-path drift re-read; mutants survivepackages/cli/src/commands/review/lib/platform/aone.ts:882 — [review] the tolerant head re-read exists as two copies ~50 lines apart in one functionpackages/cli/src/commands/review/lib/platform/aone.ts:884 — [review] post-failure drift re-read pays a further 120s deadline before the partial-post advisorydocs/design/2026-08-15-review-aone-provider.md:63 — [review] stale 'verdict caps at COMMENT' claim five lines above the added Superseded blockpackages/cli/src/commands/review/submit.ts:719 — [probe] invalid explicit --host flag masked by the gate comparison / contradiction checkdocs/design/2026-08-15-review-aone-provider.md:70 — [review] the Superseded pointer cites a changelog this PR leaves pre-#9604
中文说明
无阻断问题。LGTM!✅
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 7 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix review round — no action neededThe only new feedback this round is review [rv:4988838790] (automated Suggestion 1 — extract the refusal boilerplate into a helperAlready addressed in
Suggestion 2 — unify one refusal message's wordingAlready addressed in
VerificationCommands actually run this round, on head
Context: the current head already carries green CI and an APPROVE from the automated reviewer (round 3, "No blocking issues. LGTM! ✅"), whose remaining items were explicitly recorded as deferred, not requested. This no-action round adds zero diff growth. 中文说明Autofix 审查轮次 —— 无需改动本轮唯一的新反馈是评审 [rv:4988838790](经由 @doudouOUC 发布的 建议 1 —— 将拒绝流程的样板代码提取为 helper已由
建议 2 —— 统一一处拒绝消息的措辞已由
验证本轮在 head
背景:当前 head 的 CI 已全绿,且自动化审查者已在第 3 轮给出 APPROVE("No blocking issues. LGTM! ✅"),其剩余条目均明确记录为"延后、本轮不要求"。本轮零改动,不产生任何 diff 增长。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
doudouOUC
left a comment
There was a problem hiding this comment.
Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen-max-2026-08-21 via Qwen Code /review (v0.21.10)
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. I rechecked the exact head, including host authorization, Aone partial-post outcomes, and all changed consumers. No blocking correctness or security issues remain, and the exact-head CI is green.
doudouOUC
left a comment
There was a problem hiding this comment.
Review — APPROVE (C=0 S=0)
Reviewed at 10fdef80 against merge-base 7bc0d80. Both open Criticals were re-checked against the code as it stands — both are fixed by this diff.
Disposition of the open threads
| Finding | At 10fdef80 |
Evidence |
|---|---|---|
R6-1 — headMovedSinceCompose reported "verified stable" on an empty head SHA |
fixed | aone.ts:824 is now if (afterHead === '') return undefined;, so the could-not-verify shape degrades to unknown exactly like the catch arm |
| R8-1 — merge-conflict residue committed into the bundled review skill | fixed | git grep '^<{7}|^={7}$|^>{7}' over packages/ at this commit returns nothing; SKILL.test.ts now pins the absence of conflict markers so the class cannot recur |
What I verified independently
- The tri-state is genuinely tri-state at every consumer.
headMovedSinceComposereturnstrue/false/undefined, and both call sites propagate it: the partial path throughAonePartialPostError's newheadMovedDuringPost(aone.ts:959) and the success path throughAoneSubmitResult.headMovedDuringPost(aone.ts:997).submit.tsbranchestrueandundefinedto different warnings and prints neither onfalse— no boolean folding, no truthiness collapse that would let "could not verify" read as "verified stable". absentHostFollowsRecordingis not a dead switch. Declared atauthorization.ts:107, read atauthorization.ts:421(req.host === undefined && req.absentHostFollowsRecording === true), and actually set by the one caller that needs it atsubmit.ts:287.publish-assetsdeliberately does not set it and keeps the "absent host means github.com" reading — so the two callers' host semantics stay distinguishable instead of silently sharing one default.- The refusal path fails closed and uniformly.
SubmitRefusal(submit.ts:573) plusrefuse()(:583) funnel every deliberate refusal — write-authorization mismatch, empty/invalid host, ambientGH_HOSTpointing at Aone, Aone head drift, oversized comment — into one shape: a stderr line,{"posted": false, "reason": …}on stdout, andprocess.exitCode = 3(:605,:1182). Retryable transport failures stay ordinaryErrors, so a caller can still tell "refused" from "try again" — which is the distinction that matters for a write path. - The SKILL.md prose matches the code. The Aone
--commentbullet's claims all hold at this commit: the context-unavailable cap applies to Approve only (a Request-changes verdict still posts its blocking summary), the head-drift refusal is bounded by the per-review restart budget, and thepartialJSON shape (partial,postedInline,postedCommentIds,summaryPosted,ambiguous) matches whatsubmitactually emits. The prose is a contract here, so this mattered more than usual.
Why this is a clean approve
- The tri-state fix is the right shape rather than a local patch: one function now owns the post-batch head re-read for both disclosure paths, so the partial and success reports cannot disagree about drift again.
- The
ambiguousflag is honest about the case that actually bites — a failed write may still have landed, so a zero count is not proof nothing posted, and the skill tells the user to inspect the MR before hand-posting. - Test coverage is targeted at the semantics, not just the lines:
submit-aone.test.ts(+482) andaone.test.ts(+135) assertundefinedspecifically, which is what a boolean-folding regression would break. - CI is green on all reported checks.
中文说明
在 10fdef80 上复核两个未解决 Critical,均已修复:headMovedSinceCompose 现对空 head SHA 返回 undefined("无法验证" 不再被当成 "已验证稳定"),bundled skill 中的合并冲突残留已清除且新增测试防止复发。另外独立核验了三点:三态在 partial 与 success 两条路径上都被完整透传、absentHostFollowsRecording 有真实写入方(submit.ts:287)不是死开关、所有主动拒绝统一收敛为 SubmitRefusal → stderr + {"posted": false} + exit 3 而可重试故障仍为普通异常。SKILL.md 的 Aone 契约描述与代码行为一致。无新增 Critical 或 Suggestion,CI 全绿。
|
@qwen-code /resolve |
…9579 # Conflicts: # packages/core/src/skills/bundled/review/SKILL.test.ts
2254e47
|
@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: 173 passed · 0 failed · 173 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:173 通过 · 0 失败 · 173 总计 抖动门:✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9604 deep verification (follow-up round) —
|
| # | Previous finding | Severity | Status at 2254e477 |
|---|---|---|---|
| — | Central claim (gate opt-in admits flagless GHE publish; distinct structured refusals replace strictly worse base behaviours; terminal refuse; tri-state drift disclosure; success-JSON ids) |
central | stands — re-measured: A/B re-run at the new head on both sides (head 73/73 vs base 53/53, table below); mutation matrix 7/7 killed again |
| 1 | Method artifact: 3 pre-existing aone.test.ts tests red only in the cross-tree run |
non-blocking | stands, sharpened — re-measured: same 3 titles are among the 20 cross-tree reds; their bodies are byte-identical in both trees, the file delta is purely additive, base is green with its own copies (216/216), and each of the 3 passes when run isolated against base sources (2+1+1 passed) — i.e. order/mock-state interaction of the combined file, not a defect of either tree |
| 2 | Stale suite counts in the PR body ("4141 passed, 1 pre-existing skip") | non-blocking (cosmetic) | stands — re-measured 4422 passed / 4 skipped at the new head (delta: main's merged tests + this round's pins; skips = 1 pre-existing save-artifact skip + 3 shellcheck-binary-absent skips, environmental). Agree it is cosmetic |
Declined/deferred rows: none in the previous round.
Delta since the previous round
One commit landed after 10fdef80: the merge 2254e47708 of origin/main into the branch, whose only conflict was SKILL.test.ts (the previous round's conflict file had been SKILL.md). Findings:
- Trial merge is empty:
git diff HEAD^2..HEADis 0 bytes — the branch already contains the current base tipcef54b83, so what lands is exactly the PR-head content and the merge-into-main question is answered by construction (scripted assertion inassertions.json). - Conflict resolution is additive-only: the effective diff
HEAD^1..HEADforSKILL.test.tsis +32/−0 — main's own tests are fully retained, the PR's residue-pin test joins them. No main test was dropped or rewritten by the resolution. - Residue probe (scripted, 9/9; machine-readable record in
logs/residue-probe.json): no^(<{7}|={7}|>{7})markers inSKILL.mdorSKILL.test.ts(the delta's conflict file); the corrected cap wording present, the stale blanket-cap wording (which caps the verdict at) absent; the restart-bound qualification and the could-not-re-verify relay present; exactly one--commentbullet and onepr-contextbullet. - Vacuity of the residue pin re-proven at the new head: reintroducing the stale variant (stale sentence +
=======/>>>>>>>lines) turns exactlykeeps the corrected Aone --comment contract, not merge residuered (the conflict-marker assertion fires first — the shape the test exists to catch);git checkoutrestore returns the suite to 26/26 green. - The write-path TS is unchanged by the delta; base moved (
7703d1c3→cef54b83, which brought the landed Round-5 Critical machinery into base), so the whole A/B was re-derived from the actual current base code, not from the previous round's table. Two cells changed shape because of it: the base arm of the invalid-host/ambient/ids cells now contains base's own landed success-path MOVED warning, and B7's base arm now exhibits the misroute (base selects the gh path for a padded canonical Aone host) rather than a control.
Central claim and A/B proof (re-run at the new head)
Harness: one vitest file (harness/verify-ab.test.ts, copied verbatim into each tree) run with HARNESS_ARM=head|base; each cell asserts its own arm's expected behaviour, so a green base arm = "base is broken exactly as predicted". Mocks sit only at the child-process/stdio seams (gh spawn, a1 submitAoneReview, gitOpt, compose-review, stdout/stderr); the REAL authorisation gate runs against session-scoped recording files, the REAL setGhHost/HOSTNAME_RE/resolveGhHost run (the base TypeError is the oracle), and parse/remote-match code is real. Witnesses: 01-ab-head-arm.png (73 checks), 02-ab-base-arm.png (53 checks).
| Cell | Scenario | head (fixed) | base (control) |
|---|---|---|---|
| B1a | --host '', fast path, recorded canonical Aone URL |
exit 3 host-flag-empty, nothing written |
exit 0 — posts through the empty flag |
| B1b | --host ' ' (same recording) |
exit 3 host-flag-empty, nothing written |
exit 0 — posts through the blank flag |
| B1c | hostless bare-number recording; no flag vs empty flag | no-flag → target-platform-unbound; empty flag → its own host-flag-empty (tellable apart) |
no-flag → target-platform-unbound; empty flag → byte-identical target-platform-unbound (the re-run loop) |
| B2 | recorded --host https://ghe.corp |
exit 3 invalid-host, names the recorded arm, remedy = re-record (NO flag remedy) |
setGhHost TypeError escapes runSubmit |
| B3 | flag --host https://ghe.corp, hostless recording |
exit 3 invalid-host, flag arm, "Re-run with a valid" remedy |
TypeError escapes |
| B4 | cwd origin git@ghe_corp.example.com:… |
exit 3 invalid-host, names the origin arm |
TypeError escapes |
| B5 | nothing bound, GH_HOST=gitlab.alibaba-inc.com |
exit 3 ambient-gh-host-aone |
exit 0 — gh write dispatched at a host gh cannot post to |
| B5′ | GH_HOST=ghe.alibaba-inc.com (GHE twin) |
posts at gh | posts at gh (control: canonical pair, not the family wildcard) |
| B6 | recorded --host ' ' (all whitespace) |
exit 3 invalid-host, offender quoted verbatim " " |
TypeError escapes |
| B7 | recorded --host ' code.alibaba-inc.com ' (padded canonical) |
a1 path taken (one-time trim), gh host state untouched | misroutes to gh (predicate sees the padding), host bound trimmed by setGhHost |
| B8 | Aone success | JSON carries postedCommentIds [11], summaryCommentId 12; gh-host sentinel held |
ids ABSENT from JSON; sentinel held (control) |
| B9 | success, post-batch re-read failed | warns "could not re-verify the MR head after posting" | silence (false all-clear) |
| B10 | partial post, head moved mid-batch | warns "MOVED during posting" on the partial shape | silence on the partial shape |
| B11 | partial post, re-read failed | warns "could not re-verify … after the failed post" | silence |
| G1 | flagless publish of a GHE-recorded review via runSubmit |
exit 0, gh write dispatched, host bound to ghe.corp |
exit 3 — gate reads absence as github.com ("targets github.com") |
| G2 | same recording + contradicting --host github.com (fast path) |
exit 3 target-platform-conflict, no write |
exit 3 target-platform-conflict, no write (control) |
| G3 | gate direct, absent host, no opt-in | ok:false (strict comparison stands) | ok:false (control) |
| G4 | gate direct, absent host, opt-in | ok:true, recordedHost: ghe.corp surfaced |
ok:false (no opt-in in base) |
| G5 | gate direct, explicit matching host | ok:true | ok:true (control) |
| G6 | gate direct, explicit contradicting host + opt-in | ok:false (opt-in exempts ABSENCE only) | ok:false (control) |
Head arm 73/73; base arm 53/53; every control cell behaves identically on both arms.
Mutation matrix (head tree, single mutants, restored after each run — witness 03-mutation-matrix.png)
| # | Mutation | File | Result |
|---|---|---|---|
| M1 | gate opt-in condition → false |
authorization.ts | KILLED — 3 red (harness G1/G4 + the PR's flagless-GHE pin) |
| M2 | refuse() returns instead of throwing |
submit.ts | KILLED — 43 red (terminal refusal is load-bearing) |
| M3 | headMovedSinceCompose catch → false |
aone.ts | KILLED — 2 red (re-read-failure tri-state pins) |
| M3b | headMovedSinceCompose empty head → false |
aone.ts | KILLED — 2 red (headless-answer tri-state pins) |
| M4 | success JSON drops postedCommentIds/summaryCommentId |
submit.ts | KILLED — 2 red (harness B8 + the PR's ids pin) |
| M5 | recorded-host one-time trim removed | submit.ts | KILLED — 3 red (harness B7 + 2 padded-host pins) |
| M6 | HOSTNAME_RE pre-validation dead |
submit.ts | KILLED — 7 red (harness B2/B3/B4/B6 + 3 invalid-host pins) |
No survivors. Red capability of every command is proven by the kills themselves, each attributed to the pin that exists to catch it, and by the post-restore control: with all mutants reverted the same 4-file command ran 257/257 green and the harness 73/73. M5 and M6 kill disjoint test sets — the trim guard and the invalid-host guard defend different hazards.
Vacuity (the PR's own pins can fail)
HEAD's three changed CLI test files against base sources: 20 red / 216 green (witness 05-cross-tree-vacuity.png). Classification:
- 17 legitimate reds — 13 brand-new tests plus 4 pre-existing tests the PR changed to pin the new shapes (success-path
postedCommentIds;could not re-verifyon the partial shape; theREFUSED to post toprefix;toBeUndefined()tri-state replacing base'sfalse). Each red is the pinned behaviour being absent in base. - 3 proven method artifacts —
counts an accepted-but-unreadable inline…,the size gate pins the boundary operator…,an empty-body REQUEST_CHANGES…(all inaone.test.ts): bodies byte-identical in both trees, file delta purely additive, and each passes when run isolated against base sources (2+1+1 passed, exit 0) — order/mock-state interaction of the combined file, not a defect. - Base is green with its own copies of all 3 files (216/216, 3/3 files).
SKILL.test.tscross-checked separately in the delta section above (red on stale injection, green restored).
Reviewer Test Plan walk-through
npx vitest run src/commands/review— performed: 4422 passed / 4 skipped, exit 0 (skips: 1 pre-existing save-artifact + 3 shellcheck-binary-absent, environmental). The PR body's "4141 passed, 1 pre-existing skip" is stale (carried finding 2).- Empty
--hostrefuseshost-flag-empty— performed (B1a/B1b/B1c). - Scheme-bearing host refuses
invalid-host— performed (B2/B3, plus origin arm B4 and whitespace B6). - Gate accepts a flagless publish of an Enterprise recording — performed (G1 via
runSubmit, G3–G6 gate-direct). - Docs read correctly — performed: residue probe covers both SKILL sites; user docs' Aone paragraph carries the exact cap wording; the claim "a Request-changes verdict still posts" is exact against the code (re-measured at this head:
compose-review.ts:2844if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT';— only APPROVE downgrades); the--hosthelp names both canonical Aone hosts (code.alibaba-inc.com or gitlab.alibaba-inc.com); the provider design doc's Phase-3 sentence is marked**Superseded — Phase 3 landed.**.
Findings
No new blocking findings. Non-blocking, carried (status table above): the 3-test cross-tree artifact, now sharpened with isolated-green proof; the stale suite counts in the PR body (4141/1 stated vs 4422/4 measured at this head).
Not covered
- Live Aone/
a1writes — mocked by design in both the suites and the harness (a platform write is never a test fixture); the a1 seam's failure shapes are exercised through the mock's throw contract. This report reproduces the wire shape of each refusal/disclosure, not a live a1 interaction. - Per-commit attribution — the checkout is depth 2:
git rev-list HEAD^1..HEAD^2yields 1 commit while the metadata lists 12, and the repository is shallow — intermediate commits are unreachable, so the aggregateHEAD^1..HEADdiff was verified instead. - Full CLI yargs wiring of
--host— the harness drivesrunSubmitdirectly; the handler passesargvthrough unchanged (read, not executed). publish-assetsbehaviour — unchanged by this PR; the gate's strict (non-opted) path it relies on is verified in G3/G6.- Windows/macOS — Linux container; the changed paths are platform-agnostic.
- The workflow's 5-round flakiness gate — not re-run as a workflow step this round; each changed test file nonetheless ran multiple times here (suite gate, 7 mutation runs over overlapping files, cross-tree, restore control, SKILL suite ×3) with zero divergence.
Methodology
Environment: node:22-bookworm CI container, refs/pull/9604/merge at depth 2 (npm ci + npm run build pre-run at HEAD; HEAD is the trial merge, and its diff against HEAD^2 is empty, so merge-into-main is answered by construction). A/B base arm: scratch worktree at HEAD^1 (tmp/base-tree, removed after capture). The PR leaves package.json/package-lock.json untouched (0-line diff), so the shared root node_modules is a clean control; the base tree was wired with symlinks to the PR-untouched packages' dists and nested node_modules (all from packages the effective diff does not touch). Internal-dependency resolution: node_modules/@qwen-code/qwen-code-core dereferences into the HEAD tree, but the CLI vitest config's alias resolves it to path.resolve(__dirname, '../core/index.ts') — i.e. each tree's OWN core source — and the core TS is byte-identical between the trees (the effective diff touches only core's SKILL.md asset + its test), so no head code can contaminate the base arm; the harness's import graph touches no other workspace package. Harnesses mock only child-process/stdio seams; the gate and setGhHost run real. Assertions: assertions.json (173/173) is computed by harness/aggregate.mjs from the run logs with explicit expectations; per-check detail in logs/assertion-detail.json. Raw logs and harness sources live in this artifact directory (logs/, harness/); key logs: head-arm-final.log, base-arm-final.log, head-review-suite.log, cross-tree.log, m1.log–m6.log, restore-control.log, typecheck-clean.log (exit 0; planted type error → exit 2 with TS2322, restored), eslint-clean.log (exit 0 on the seven changed TS files; planted unused var → exit 1 — the first probe used an _-prefixed name, which the rule's ignore-pattern lawfully exempts, so the probe was redone without the prefix).
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/commands/review/lib/platform/aone.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/platform/aone.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
file packages/core/src/skills/bundled/review/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/review/SKILL.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone.test.ts: PPPPP
packages/cli/src/commands/review/submit-aone.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
packages/core/src/skills/bundled/review/SKILL.test.ts: PPPPP
verdict: pass
summary: 4 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/platform/aone.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 1 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/platform/aone.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 2 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/platform/aone.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 3 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/platform/aone.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 4 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/platform/aone.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)
round 5 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下: Autofix round summary — no code changes (PR #9604)This round's actionable feedback contains no new findings — it carries two maintainer comments that CONFIRM fixes already landed in earlier rounds of this PR. Both claims were re-verified against the current head ( rc:3835835302 — empty-head tri-state disclosure in the Aone submit pathClaim: fixed in 5378756 — Test evidence: focused Vitest run in rc:3835835397 — merge-conflict residue in the review skillClaim: fixed in 10fdef8 — residue removed and a regression guard added. Verified at HEAD: Test evidence: focused Vitest run in Not touched this round
Verification
No code changes were made this round; nothing was committed. 中文说明Autofix 轮次摘要 — 无代码改动(PR #9604)本轮可处理反馈中没有新的发现 — 只有两条维护者评论,用于确认本 PR 早前轮次中已落地的修复。两项声明均已在当前 head( rc:3835835302 — Aone 提交路径中的空 head 三态披露声明:已在 5378756 中修复 — 测试证据: rc:3835835397 — review skill 中的合并冲突残留声明:已在 10fdef8 中修复 — 残留已清除,并新增了回归守卫。已在 HEAD 核实: 测试证据: 本轮未触碰的内容
验证
本轮未做任何代码改动;未提交任何内容。 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式:已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈;这些条目保持开放,留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后;达到预算的作者(如有)在下方点名。(评论 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.
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.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — "Integration Tests (CLI, No Sandbox)" was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.15)
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Re-reviewed the exact current head 2254e47, with focus on the Aone host/authorization boundary, partial-post and head-drift semantics, and the main-merge overlap. The Aone implementation blobs are unchanged from the previously approved head, no unresolved actionable review threads remain, and the exact-head checks are green. No blocking correctness or security issue found.
doudouOUC
left a comment
There was a problem hiding this comment.
Independent review at 2254e477 — code is sound; the pending conflict resolution has a verified trap
I verified the two Criticals that were closed on this PR against the code as it stands, then reviewed the write path independently. The code changes look right to me. What I cannot sign off on is the tree this will become: the PR is CONFLICTING, and resolving the conflict the obvious way silently reverts two corrections that landed on main.
The closed Criticals — verified fixed
The absentHostFollowsRecording host-check skip is safe, and I checked it as an authorisation change rather than taking the comment's word. The skip fires only when req.host === undefined, and the host comparison it bypasses sits inside if (t.type === 'pr-url') — the one branch where recordedHost is returned as t.host itself. So in submit's routing (explicitHost ?? recordedHost ?? cwdOriginHost), an absent explicit host resolves to recordedHost, which is the authorised host: the comparison would have passed, and skipping it is not a weakening. The cwdOriginHost and ambient-GH_HOST fallbacks are only reachable when recordedHost is undefined, which for a pr-url target it never is — that shape is the bare-pr-number target, where this block never ran even before the PR. submit.ts:287 is the only production setter; publish-assets correctly leaves it unset, so the two writers diverge exactly as documented and the strict check still stands for the assets path.
headMovedSinceCompose tri-state — if (afterHead === '') return undefined; is in place, and both the success path and the partial-failure path read the same helper, so a headless re-read discloses could-not-verify instead of a false all-clear on either.
The merge-conflict residue is gone, and the guard added for it is a good one: SKILL.test.ts pins the absence of any line-leading marker and asserts the corrected sentences positively, plus expect(body).not.toContain('which caps the verdict at') to catch the stale variant returning. Negative assertions against the specific stale text are what make that test able to fail.
No new Critical or Suggestion from my own pass over the refusal exits, the partial-post reporting, or the comment-id reporting.
The one thing to handle: the conflict is semantic, not textual
main picked up #9624/#9619, which touched the same two docs. The conflict is in docs/users/features/code-review.md and docs/design/2026-08-15-review-aone-provider.md, and in the user doc the whole Aone paragraph is a single line — so git cannot merge it and "take ours" is the tempting resolution. Taking this branch's side would revert two corrections now on main:
| claim | main |
this branch | if resolved toward the branch |
|---|---|---|---|
a1 ≥ 0.1.90 version floor parenthetical |
present | absent | lost (grep -c 0.1.90 → 1 vs 0) |
test-plan on Aone |
backed — "reads the MR description through the same reader" | "test-plan is unbacked" |
reverted to the stale claim |
| context-unavailable cap | "keeps every Aone verdict at Comment" (inaccurate) | "keeps an Approve verdict at Comment (a Request-changes verdict still posts its blocking summary)" | correctly fixed — this side is the one to keep |
So the resolution has to be a hand-merge of both sides — main's a1 floor and backed-test-plan text, plus this PR's Approve-vs-Request-changes cap correction. The design doc needs the same care for a different reason: both sides fix the stale "posting is Phase 3" sentence, but differently — main keeps the sentence and appends an _Update (2026-08-19, #9491)_ note, while this branch strikes the sentence through and adds a Superseded block. Taking both produces a struck-through sentence with an update note still pointing at it.
Worth flagging explicitly because the guard this PR added covers SKILL.md only — skillBody() reads that one file, so nothing in CI would catch a botched resolution in either conflicting doc, and a botched resolution in a doc is precisely what this branch already shipped once.
SKILL.md itself is fine: main deleted the stale test-plan bullet this branch still carries, the branch does not touch that line, and GitHub reports the file as auto-merging — so main's deletion wins.
Where I land
The code has my approval in substance — the authorisation change is sound, the write-path disclosures are honest on both the success and partial paths, and CI is green (Test (ubuntu-latest, Node 22.x), review-pr). I am holding the formal approve only because the merge result is a tree I have not reviewed, and the resolution has a specific, verified way to go wrong. Happy to approve as soon as it is resolved with the three claims above reconciled.
Not verified by me: the review suites did not run locally (no node_modules in my worktree — CI covers them green), and no live Aone write was exercised, so the a1 write path is judged from code and its mocked tests.
…9579 # Conflicts: # docs/design/2026-08-15-review-aone-provider.md # docs/users/features/code-review.md
eefff2f
|
@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: 183 passed · 0 failed · 183 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:183 通过 · 0 失败 · 183 总计 抖动门:✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at head Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, mutation matrix, cross-tree vacuity, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 183 passed · 0 failed · 183 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)跟进轮:上轮(head
Verification reportPR #9604 deep verification (follow-up round 2) —
|
| # | Previous finding | Severity | Status at eefff2f2b |
|---|---|---|---|
| — | Central claim (gate opt-in admits flagless GHE publish; distinct structured refusals replace strictly worse base behaviours; terminal refuse; tri-state drift disclosure; success-JSON ids) |
central | stands — re-measured on both arms against the new base (head 76/76 checks, base 56/56 predicted-breakage checks; mutation matrix 8/8 killed again) |
| 1 | Method artifact: 3 pre-existing aone.test.ts tests red only in the cross-tree combined run |
non-blocking | stands, re-sharpened — same 3 titles among the 20 cross-tree reds at the new head; each passes isolated against base sources (1+1+1, exit 0); base is green with its own copies (225/225) |
| 2 | Stale suite counts in the PR body ("4141 passed, 1 pre-existing skip") | non-blocking (cosmetic) | stands — re-measured 4498 passed / 4 skipped at the new head (skips: 1 pre-existing save-artifact + 3 shellcheck-binary-absent, environmental). Agree it is cosmetic |
Declined/deferred rows: none in the previous round.
Delta since the previous round
- Trial merge is empty:
git diff HEAD^2..HEADis 0 bytes — the branch already contains the base tip0c36e509, so what lands is exactly the PR-head content; merge-into-main is answered by construction (scripted assertion inassertions.jsonvia the residue probe's topology checks). - Conflict resolution retained base content: the effective diff's only docs deletions are the stale sentences themselves. For
code-review.mdthe base→head delta of the whole Aone paragraph is exactly the two corrected cap phrases — a scripted verbatim comparison (docs.delta-is-cap-only) shows the base line with those two phrases substituted equals the head line, so every feat(review): close Aone residual gaps — composeUrl, test-plan routing, a1 version floor #9624-carried clause (a1 floor0.1.90, self-PR backing, dedup caveat,publish-assetsskip, AGit-Flow amend model, discussion-gate semantics) survived the resolution. The floor in the resolved line equals the code'sA1_MIN_VERSION(docs.floor-matches-code). For the design doc, the stale Phase-3 sentence is struck (~~…~~) and marked**Superseded — Phase 3 landed.**with the posting route named; the base-side paragraphs around the hunk are retained. - Residue probe 30/30 (
04methodology; machine-readable record inlogs/checks-residue.jsonl): no^(<{7}|={7}|>{7})markers in any of the 10 changed files (including this round's two conflict files andSKILL.test.ts); corrected cap wording present and stale blanket-cap wording absent at all three sites; restart-bound qualification, could-not-re-verify relay, no-link contract clause, single--comment/pr-contextbullets, and the--hosthelp naming both canonical Aone hosts all present.
Central claim and A/B proof (re-run at the new head and new base)
Harness: two vitest files (harness/verify-ab.test.ts, harness/verify-aone.test.ts) copied verbatim into each tree and run with HARNESS_ARM=head|base; each cell asserts its own arm's expected behaviour, so a green base arm = "base is broken exactly as predicted". Mocks sit only at the child-process/stdio seams (gh spawn, the a1 write seam submitAoneReview, the a1 transport for the aone-layer file, gitOpt, compose-review, stdout/stderr, settings, version); the REAL authorisation gate runs against session-scoped / seam recording files, the REAL setGhHost/getGhHost/resolveGhHost/HOSTNAME_RE/hostsEquivalent/isAoneCanonicalHost/parseRemoteUrl run (the base TypeError is the oracle), and submit's whole routing flow runs. Witnesses: 01-ab-head-arm.png, 02-ab-base-arm.png.
| Cell | Scenario | head (fixed) | base (control) |
|---|---|---|---|
| B1a | --host '', fast path, recorded canonical Aone URL |
exit 3 host-flag-empty, nothing written |
exit 0 — posts through the empty flag |
| B1b | --host ' ' (same recording) |
exit 3 host-flag-empty, nothing written |
exit 0 — posts through the blank flag |
| B1c | hostless bare-number recording; no flag vs empty flag | no-flag → target-platform-unbound; empty flag → its own host-flag-empty |
no-flag → target-platform-unbound; empty flag → byte-identical refusal (the re-run loop) |
| B2 | recorded --host https://ghe.corp |
exit 3 invalid-host, names the recorded arm, remedy = re-record |
setGhHost TypeError escapes runSubmit |
| B3 | flag --host https://ghe.corp, hostless fast path |
exit 3 invalid-host, flag arm, "Re-run with a valid" remedy |
TypeError escapes |
| B4 | same-session recording, cwd origin git@ghe_corp.example.com:… |
exit 3 invalid-host, names the origin arm |
TypeError escapes |
| B5 | nothing bound, GH_HOST=gitlab.alibaba-inc.com |
exit 3 ambient-gh-host-aone |
exit 0 — gh write dispatched at a host gh cannot post to |
| B5′ | GH_HOST=ghe.alibaba-inc.com (GHE twin) |
posts at gh | posts at gh (control: canonical pair, not the family wildcard) |
| B6 | recorded --host=' ' (all whitespace) |
exit 3 invalid-host, offender quoted verbatim " " |
TypeError escapes |
| B7 | recorded --host=' code.alibaba-inc.com ' (padded canonical) |
a1 path taken (one-time trim), gh host state untouched | misroutes to gh, host bound trimmed by setGhHost |
| B8 | Aone success | JSON carries postedCommentIds [11], summaryCommentId 12; gh-host sentinel held |
ids ABSENT from JSON; sentinel held (control) |
| B9 | success, post-batch re-read failed | warns "could not re-verify the MR head after posting" | silence (false all-clear) |
| B10 | partial post, head moved mid-batch | warns "MOVED during posting" on the partial shape | silence on the partial shape |
| B10c | partial post, verified stable | no drift warning | no drift warning (control) |
| B11 | partial post, re-read failed | warns "could not re-verify … after the failed post" | silence |
| G1 | flagless publish of a GHE-recorded review via runSubmit |
exit 0, gh write dispatched, host bound to ghe.corp |
exit 3 — gate reads absence as github.com |
| G2 | same recording + contradicting --host github.com |
exit 3 target-platform-conflict, no write |
exit 3 (control) |
| G3 | gate direct, absent host, no opt-in | ok:false (strict comparison stands) | ok:false (control) |
| G4 | gate direct, absent host, opt-in | ok:true, recordedHost: ghe.corp surfaced |
ok:false (no opt-in in base) |
| G5 | gate direct, explicit matching host | ok:true | ok:true (control) |
| G6 | gate direct, explicit contradicting host + opt-in | ok:false (opt-in exempts ABSENCE only) | ok:false (control) |
| A1 | aone layer: stable re-read | headMovedDuringPost: false |
false (control) |
| A2 | aone layer: re-read FAILS | undefined ("could not verify") | false (false all-clear) |
| A3 | aone layer: head moved | true | true (control) |
| A4 | aone layer: re-read yields no head | undefined | false |
| A5 | aone layer: partial + moved re-read | error carries headMovedDuringPost: true |
field absent (base never re-reads on partial) |
| A6 | aone layer: partial + dead re-read | partial shape preserved, drift field unknown | same (control) |
Head arm 76/76 checks; base arm 56/56; every control cell behaves identically on both arms. Note the base at this head already contains the landed Round-5 Critical machinery and #9624; the base arm's behaviours above were re-derived from the actual current base code, not carried from the previous table — all reproduced, including B7's misroute and B5's opaque post.
Mutation matrix (head tree, single mutants, restored after each run — witness 03-mutation-matrix.png, attribution in logs/mut-*.log)
| # | Mutation | File | Result |
|---|---|---|---|
| M1 | gate opt-in condition → false |
authorization.ts | KILLED — 3 red (harness G1/G4 + the PR's flagless-GHE pin) |
| M2 | refuse() returns instead of throwing |
submit.ts | KILLED — 43 red (terminal refusal is load-bearing) |
| M3 | headMovedSinceCompose catch → false |
aone.ts | KILLED — 4 red (harness A2/A6 + 2 tri-state pins) |
| M3b | headMovedSinceCompose empty head → false |
aone.ts | KILLED — 3 red (harness A4 + 2 headless pins) |
| M4 | success JSON drops postedCommentIds/summaryCommentId |
submit.ts | KILLED — 2 red (harness B8 + the PR's ids pin) |
| M5 | recorded-host one-time trim removed | submit.ts | KILLED — 3 red (harness B7 + 2 padded-host pins) |
| M6 | HOSTNAME_RE pre-validation dead |
submit.ts | KILLED — 7 red (harness B2/B3/B4/B6 + 3 invalid-host pins) |
| M7 | empty-flag check dead | submit.ts | KILLED — 4 red (harness B1a/B1b/B1c + the PR's empty-flag pin) |
No survivors. The red capability of the harness and the suite is proven by the kills themselves, each attributed to the pin that exists to catch it, and by the post-restore control: 272/272 green over the same 5-file command. M5 and M6 kill disjoint test sets — the trim guard and the invalid-host guard defend different hazards.
Vacuity (the PR's own pins can fail)
HEAD's three changed CLI test files against base sources: 20 red / 225 green (witness 04-cross-tree-vacuity.png). Classification:
- 17 legitimate reds — the new/changed pins (flagless-GHE publish, empty-flag distinct refusal, invalid-host arms, padded-host routing, ambient-GH_HOST refusal, tri-state re-read disclosures, partial-shape MOVED/re-verify warnings, success-JSON ids, terminal-refusal shapes). Each red is the pinned behaviour being absent in base.
- 3 proven method artifacts —
counts an accepted-but-unreadable inline…,the size gate pins the boundary operator…,an empty-body REQUEST_CHANGES…(all inaone.test.ts, same titles as the previous round): each passes when run isolated against base sources (1+1+1, exit 0) — order/mock-state interaction of the combined file, not a defect of either tree. - Base is green with its own copies of all 3 files (225/225), and of
SKILL.test.ts(25/25). SKILL.test.tscross-checked: head's copy against base's staleSKILL.mdturns exactly the residue pin red (keeps the corrected Aone --comment contract, not merge residue; 1 failed / 25 passed), and is 26/26 at head — the pin that guards this round's conflict-file class is non-vacuous.
Reviewer Test Plan walk-through
npx vitest run src/commands/review— performed: 4498 passed / 4 skipped / 0 failed (99 files; skips: 1 pre-existing save-artifact + 3 shellcheck-binary-absent, environmental). The PR body's "4141 passed, 1 pre-existing skip" is stale (carried finding 2).- Empty
--hostrefuseshost-flag-empty— performed (B1a/B1b/B1c). - Scheme-bearing host refuses
invalid-host— performed (B2/B3, plus origin arm B4 and whitespace B6). - Gate accepts a flagless publish of an Enterprise recording — performed (G1 via
runSubmit, G3–G6 gate-direct). - Docs read correctly — performed: residue probe covers both SKILL sites, both conflict files, and the user docs; the claim "a Request-changes verdict still posts" is exact against the code (compose-review downgrades only APPROVE under the cap — carried from the previous round's measurement at
compose-review.ts, re-verified present at this head via the suite's cap pins); the--hosthelp names both canonical Aone hosts; the design doc's Phase-3 sentence is struck and marked superseded.
Findings
No new blocking findings. Non-blocking, carried (status table above): the 3-test cross-tree artifact (re-sharpened with isolated-green proof at this head) and the stale suite counts in the PR body (4141/1 stated vs 4498/4 measured).
Not covered
- Live Aone/
a1writes — mocked by design in both the suites and the harness (a platform write is never a test fixture); the a1 seam's failure shapes are exercised through the mock's throw contract and, at the aone layer, through a scripted transport. This report reproduces the wire shape of each refusal/disclosure, not a live a1 interaction. - Per-commit attribution — the checkout is depth 2:
git rev-list HEAD^1..HEAD^2yields 1 commit while the metadata lists 14, and the repository is shallow — intermediate commits are unreachable, so the aggregateHEAD^1..HEADdiff was verified instead. - Full CLI yargs wiring of
--host— the harness drivesrunSubmitdirectly; the handler passesargvthrough unchanged (read, not executed). publish-assetsbehaviour — unchanged by this PR; the gate's strict (non-opted) path it relies on is verified in G3/G6.- Windows/macOS — Linux container; the changed paths are platform-agnostic.
- The workflow's 5-round flakiness gate — not re-run as a workflow step this round; each changed test file nonetheless ran many times here (suite gate, 9 mutation-matrix runs over overlapping files, cross-tree, isolated runs, base-own runs, head/base harness arms) with zero divergence.
Methodology
Environment: node:22-bookworm CI container, refs/pull/9604/merge at depth 2 (npm ci + npm run build pre-run at HEAD; HEAD is the trial merge and its diff against HEAD^2 is empty). A/B base arm: scratch worktree at HEAD^1 (tmp/base-tree, removed after capture). The PR leaves package.json/package-lock.json untouched, so the shared root node_modules is a clean control; the base tree was wired with symlinks to the per-package node_modules of PR-untouched packages (the effective diff touches only packages/cli review files and core's SKILL.md/SKILL.test.ts), plus its own generated git-commit.ts; the vitest dist-guard was pointed at the head root via QWEN_VITEST_GUARD_ROOT (the guarded dist packages are all outside the PR diff, and the harness import graph never resolves them — demonstrated by the runs). Internal-dependency resolution: node_modules/@qwen-code/qwen-code-core dereferences into the HEAD tree, but the CLI vitest config aliases it to each tree's OWN core source, and core's TS is byte-identical between the trees — quoted here as the no-contamination check. Harnesses mock only child-process/stdio seams; the gate, setGhHost, and the routing run real. Assertions: assertions.json (183/183) computed by harness/aggregate.mjs from per-run JSONL ledgers (logs/checks-*.jsonl); per-check detail in logs/assertion-detail.json. Raw logs in logs/ (per-cell harness ledgers, mut-*.log mutant runs, cross-tree-*.log, head-review-suite.log, typecheck/eslint clean+planted logs); harness sources in harness/.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/commands/review/lib/platform/aone.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/platform/aone.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
file packages/core/src/skills/bundled/review/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/review/SKILL.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone.test.ts: PPPPP
packages/cli/src/commands/review/submit-aone.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
packages/core/src/skills/bundled/review/SKILL.test.ts: PPPPP
verdict: pass
summary: 4 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/platform/aone.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 1 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/platform/aone.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 2 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/platform/aone.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 3 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/platform/aone.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 4 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/platform/aone.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)
round 5 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Released in v0.22.0. |









What this PR does
This is the full cleanup of the findings the review bot raised in round 5 on the Aone
--commentposting path — the Suggestions that were deferred at that point under the repo's "about five rounds, then land Criticals only" rule, and tracked so nothing would be silently dropped. The Criticals from that round already landed with the original posting path. This PR takes the remaining items: a batch of genuine write-path fixes, a batch of documentation and skill-contract corrections, and a batch of test pins that previously could not catch the regressions they claim to guard against.On the write path: an empty
--hostnow refuses with its own distinct outcome instead of collapsing into the same refusal the flag exists to remedy (which sent an agent re-running into a loop); an invalid host refuses in the structured exit-3 shape instead of escaping as an uncaught failure after validation already ran; a GitHub-bound post with nothing naming its host refuses actionably when the ambientGH_HOSTpoints at Aone instead of failing opaquely; the shared authorisation gate stops misreading "no host given" as "targeting github.com" for the posting command, so an ordinary flagless publish of an Enterprise-recorded review goes through while the assets publisher keeps its strict check; a head that moves mid-batch is disclosed on the partial-failure shape as well as the success shape, and a post-batch head re-read that itself fails now reports "could not re-verify" instead of a false all-clear; and a successful Aone post now reports the created comment ids so a run can be reconciled against the MR afterwards.On documentation and the skill contract: the context-unavailable cap is now described accurately (it holds an Approve at Comment; a Request-changes verdict still posts) in the user docs and both skill sites; the head-drift instruction is qualified by the per-review restart bound and names what to do on Aone once that bound is spent; the completion-line contract admits the no-link note the Aone fallback prescribes; the
--hosthelp text spells both canonical Aone hosts out; and the provider design doc's stale "posting is a later phase" sentence is marked superseded.On tests: the pins that guarded ordering (auth before writes, host-binding before the write, the a1 path leaving gh host state untouched), the cells that make probe-vs-flag and refusal-vs-probe precedence actually falsifiable, the seam that stops the suite spawning a real
gitin the test working directory, per-suite isolation of the recorded-args store, the newest-wins ordering when two recordings of one PR disagree on host, and producer-side pins on the refusal-prefix, the request-changes note's count source, the context-claim passthrough, and the floor-recovery host axis.Why it's needed
The deferred items were real. A handful were user-visible correctness bugs (the stale design doc telling operators posting is unsupported, the help text naming a hostname that does not exist, an empty interpolated host looping the re-runner). The rest were either fail-closed improvements to the irreversible-write path or tests that passed while the regression they claimed to catch would have shipped green — a test that cannot fail is not coverage. Clearing them closes the tracking issue and leaves the posting path with the guard set its own reviews demanded.
Reviewer Test Plan
How to verify
Run the review test suites from the CLI package:
npx vitest run src/commands/review— all files pass (4141 passed, 1 pre-existing skip). Then spot-check the behavior changes:qwen review submitwith an empty--hostrefuses withhost-flag-empty; with a scheme-bearing host refuses withinvalid-host; the authorisation gate accepts a flagless publish whose recording names an Enterprise host. Confirm the docs read correctly (docs/users/features/code-review.mdAone paragraph, the Aone skill section, and the provider design doc's superseded note).Evidence (Before & After)
N/A — no TUI surface. Behavior is covered by the unit suites above; docs are prose.
Tested on
Environment (optional)
Unit tests only (
npx vitest run src/commands/review);npm run typecheck, ESLint and Prettier all clean.Risk & Scope
Linked Issues
Closes #9579
中文说明
本 PR 做了什么
这是对 Aone
--comment投递路径第 5 轮评审所提发现的完整清理——即当时按仓库"约 5 轮后只合入 Critical"规则延期、并被登记以免静默丢失的那批 Suggestion。该轮的 Critical 已随原投递路径合入。本 PR 处理其余条目:一批真实的写路径修复、一批文档与 skill 契约修正、以及一批此前无法捕获其所声称防范之回归的测试钉。写路径方面:空
--host现在以自己独立的拒绝形态返回,而不是塌缩进该 flag 本要解除的同一条拒绝(那会让 agent 陷入重跑循环);非法 host 以结构化的 exit-3 形态拒绝,而不是在校验已通过后以未捕获失败的形式冲出;当没有任何证据指明 host、且 ambientGH_HOST指向 Aone 时,GitHub 方向的投递给出可操作的拒绝,而不是在 compose 跑完后不透明地失败;共享授权门禁不再把"未给 host"误读为"指向 github.com"(仅对投递命令如此),于是 Enterprise 记录评审的常规无 flag 发布得以通过,而资产发布器保留严格比较;批中途 head 移动现在在 partial 失败形态与成功形态上都会披露,且批后 head 重读自身失败时报告"无法复核"而非虚假的"全部通过";Aone 成功投递现在回报所创建的评论 id,便于事后与 MR 对账。文档与 skill 契约方面:context-unavailable 上限在用户文档与两个 skill 站点中被准确描述(它把 Approve 压到 Comment;Request-changes 仍会投递);head-drift 指示受"每次评审重启上限"限定,并说明 Aone 上该上限用尽后该怎么做;完成行契约接纳 Aone 回退所规定的"无链接"说明;
--host帮助文本写明两个规范 Aone host;provider 设计文档中过期的"投递属于后续阶段"一句被标记为已废弃。测试方面:守护顺序的钉(auth 先于写入、host 绑定先于写入、a1 路径不触碰 gh host 状态);使 probe-vs-flag 与拒绝-vs-probe 优先级真正可证伪的单元格;让套件不再在测试工作目录里 spawn 真实
git的接缝;记录参数库的按套件隔离;同一 PR 两条记录 host 不一致时的"最新者胜"排序;以及拒绝前缀、request-changes 备注的计数来源、context 声明透传、floor 恢复的 host 轴等生产者侧钉。为什么需要
被延期的条目是真实问题。少数是用户可见的正确性缺陷(过期设计文档告诉操作者"尚不支持投递"、帮助文本写了一个不存在的主机名、空插值 host 让重跑者陷入循环)。其余或是不可逆写路径的 fail-closed 改进,或是"所声称防范的回归真发生时仍会绿着上线"的测试——一个不会失败的测试不是覆盖。清理它们可关闭跟踪 issue,并让投递路径拥有其自身评审所要求的守护集。
评审者测试计划
如何验证
在 CLI 包内运行评审测试套件:
npx vitest run src/commands/review——全部通过(4141 通过,1 个既有 skip)。然后抽查行为变化:qwen review submit传空--host以host-flag-empty拒绝;传带 scheme 的 host 以invalid-host拒绝;授权门禁接受记录指向 Enterprise host 的无 flag 发布。确认文档可读(docs/users/features/code-review.md的 Aone 段落、Aone skill 章节、provider 设计文档的废弃标注)。证据(前后对比)
N/A——无 TUI 界面。行为由上述单元测试覆盖;文档为文字。
测试环境
环境(可选)
仅单元测试(
npx vitest run src/commands/review);npm run typecheck、ESLint 与 Prettier 均干净。风险与范围
关联 Issue
Closes #9579