feat(review): post --comment reviews to Aone Code via the a1 CLI - #9491
Conversation
The Aone chain of /review was read-only: a review of an Aone MR ran fully but `--comment` refused. This lands the Phase 3 submit slice — an authorised run now posts through `a1`: one comment per inline finding, then the summary comment, and `a1 repo mr approve` on an APPROVE. Aone has no native request-changes state, so that verdict posts a blocking summary header and leans on the discussion merge gate; the terminal names the difference. Writes ride a no-retry transport (a transient retry after an accepted write would double-post). The commit_id gate GitHub enforces server-side lives in the provider as a pre-write head-drift refusal, and a mid-batch failure reports exactly what landed with exit-3 do-not-re-run advice instead of a retryable throw. The recorded-but-hostless refusal stays fail-closed, now between two writable platforms.
|
Re-run at the same head — no new commit since the last pass ( Template ✓ — unchanged; all sections present, bilingual, test plan stays concrete. Problem: unchanged posture — this is the planned completion of the Phase 3 write path (the read path already landed; Direction: aligned — no auth/sandbox/model-selection/telemetry/public-contract surface is touched; the changes tighten the write gate's fail-closed posture (an explicit Size: 991 production lines (submit.ts 443, aone.ts 335, authorization.ts 148, aone-client.ts 37, remote-match.ts 28) / 2214 test lines / 87 docs lines + 13 lines of bundled review SKILL.md prose. The only Approach: the scope remains one thing — closing the round-5 ledger — and the prior pass checked each of the nine fixes against its finding. Round 6 added five Suggestion-level findings on top (a vacuous test cell in the mtime ordering, the compose-review floor-recovery call site still binding Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. Moving on to code review. 🔍 中文说明在同一 head 上重跑——上遍之后没有新 commit（ 模板 ✓——不变，各节齐全、中英双语，测试计划依然具体。 问题：姿态不变——Phase 3 写路径的计划内补全（读路径已合入； 方向：对齐——未触及 auth/sandbox/模型选择/telemetry/公共契约面；改动收紧写门禁的 fail-closed 姿态（显式 规模：991 生产行（submit.ts 443、aone.ts 335、authorization.ts 148、aone-client.ts 37、remote-match.ts 28）/ 2214 测试行 / 87 文档行 + 13 行内置 review SKILL.md 文本。唯一触及 方案：范围仍是一件事——关闭第 5 轮清单——上遍已逐条核对九个修复与发现的对应。第 6 轮在其上追加五条 Suggestion 级发现（mtime 排序的空测试位、compose-review floor 恢复调用点仍将 风险：无升级风险信号——变更文件均未命中回滚相关路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo code moved since the last pass — The write path holds up under a cold read. The shared pipeline (authorisation gate → payload validation → verdict composition) runs before the platform branch; the a1 transport never retries a write ( The five round-6 Suggestions are all factually grounded — I checked the two that cite code outside the diff: compose-review.ts:4274 does still pass No Critical findings; nothing blocking. Test evidence — the PR's own CI, at the reviewed headAll substantive checks are green at
One row per check name (latest run); skipped checks omitted except where the skip is itself the signal; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped——除非 skip 本身即信号，失败项排在最前。 Sandboxed verification has already settled this head's behavioural claim: the round posted at 11:03 ran the tree at 中文说明代码审查上遍之后代码未动—— 写路径经得起冷读：共享管线（授权门禁→payload 校验→verdict 合成）先于平台分支运行；a1 传输对写绝不重试（ 五条第 6 轮 Suggestion 均有事实依据——核验了两条引用 diff 之外代码的：compose-review.ts:4274 确实仍向 无 Critical 发现，无阻塞项。 测试证据——该 PR 自己的 CI（评审 head 上）
沙箱验证已就本 head 的行为主张落定：11:03 发布的一轮在隔离、无凭证容器中对 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — six review rounds, five verification rounds, and a cold re-read of the production diff all converge on the same answer at this head; the withheld point is the disclosed residue, not doubt — no live-platform E2E exists in CI by design, the integration suite is skipped on this PR, and five real-but-non-blocking Suggestions sit deferred in #9579. Stepping back: the round that could have sunk this PR — round 5 finding nine Criticals in a tree a prior round approved — is also the round that proved the process works. The fix commit did exactly the honest thing: one fix per finding, no rewrite, no scope creep, each pin engineered to die if its fix regresses. This re-run added two independent signals on top: the round-6 Verdict: approve. CI is fully green at 中文说明Confidence: 4/5 —— 六轮评审、五轮验证、加一次对生产 diff 的冷读，在本 head 上收敛到同一答案；扣掉的一分是已披露的遗留，而非疑虑——CI 按设计没有真实平台 E2E，本 PR 跳过集成套件，五条真实但不阻塞的 Suggestion 延期在 #9579。 回顾：最可能击沉本 PR 的一轮——第 5 轮在先前已批准的树上找出九个 Critical——恰恰证明了流程有效。修复 commit 做的正是诚实的事：一条发现一个修复，不重写、不扩 scope，每个钉子都按"修复回归即死"设计。本次重跑在其上叠加两个独立信号：第 6 轮 结论：approve。 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
The triage review of #9491 flagged the gap: if `a1` ACCEPTED a comment but answered unparseable JSON, the read-back threw before the post was counted — the partial-post report undercounted by exactly that comment, and if it was the first, the do-not-re-run advisory did not fire at all, so a retry would double-post it. Split the read-back semantics: an exec failure still propagates (the write genuinely failed), but a succeeded exec whose answer fails to parse now degrades to "landed, result unreadable" — counted as posted, only the id dropped. The fail-open empty-sourceBranch drift gate is left as the deliberate, tested trade-off it was reviewed as; it is now named in the Phase 3 design-doc note beside the Q4 follow-ups.
Names the two deliberate trade-offs the triage review of #9491 flagged for the Q4 era: the head-drift gate is fail-open on an empty sourceBranch, and the created-comment id read-back is best-effort — plus the tolerant read-back semantics the follow-up fix introduced.
|
Thanks for the review — both observations handled:
|
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 128 passed · 2 failed · 130 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：128 通过 · 2 失败 · 130 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9491 deep verification —
|
| Cell | Base (39fc769d) |
Head (f837d92c) |
|---|---|---|
| Authorised post, one Critical | exit 3, {"posted":false,"reason":"aone-read-only-phase"}, stderr "not supported yet", 0 a1 calls, 0 gh calls |
exit 0, {"posted":true,"event":"COMMENT","inlineComments":1,"summaryPosted":true,"url":"https://code.alibaba-inc.com/g/p/codereview/7"}; a1 sequence whoami → view → create-inline → create-summary, inline anchored --file src/foo.ts --line 12, 0 gh calls |
| Zero findings | exit 3, same refusal, 0 calls | exit 0; sequence whoami → view → create-summary (summary-only post) |
| Recorded-but-hostless target (bare-number recording) | exit 3, aone-read-only-phase (old string), 0 calls |
exit 3, target-platform-unbound (new string), 0 calls on either platform, stderr names the --host remedy — fail-closed preserved, string change is the documented one |
Base control purity: base packages/cli recompiled from base source in a scratch worktree; grep confirms the base dist contains aone-read-only-phase and neither target-platform-unbound nor submitAoneReview. The end-to-end verdict event at head is a capped COMMENT (cappedBy: unreviewed-dimension, criticals-unverified) — compose-review fails closed without a plan + harness transcripts, and that capped shape is exactly the production default for an Aone run (context-unavailable caps the verdict). Event-specific behavior is covered at the provider seam below.
Routing / gate probes (head; witnesses 02-routing-arms-both-directions.png, 03-drift-gate-and-no-retry-write.png)
| Probe | Result |
|---|---|
Recorded Aone pr-url binding, no --host, non-Aone cwd |
posts via a1 (exit 0), gh never invoked — recorded host wins over the environment |
Recorded github.com pr-url binding, cwd origin on gitlab.alibaba-inc.com |
posts via gh (api repos/g/p/pulls/7/reviews --input -, exactly one call), 0 a1 calls — cwd probe does not hijack |
--dry-run on Aone target |
exit 0, {"posted":false,"wouldPost":true,"target":"a1 repo mr comment create --mr 7 --repo g/p (1 inline + summary)"}, 0 a1 calls |
Live head ≠ composed commit_id |
exit 3, aone-post-failed, "the MR head moved — … composed against abc123, but the live head is deadbeefcafe"; only whoami+view ran — zero writes; no do-not-re-run advisory (nothing landed) |
Empty sourceBranch (declared fail-open) |
posts proceed unanchored (exit 0) — the documented trade-off, behaves as written |
Transient HTTP 502 on the first write |
exit 3, aone-post-failed, exactly 1 create attempt (the read path would retry 3×; the write never does), report "0 of 1 landed", no advisory |
| 2nd inline write fails | exit 3, report "1 of 2 landed" + "do NOT re-run submit"; the summary and approve were never attempted |
| Accepted write answers unparseably | exit 0 — the comment counts as posted (inlineComments: 2), only the id is dropped |
| Garbage answer #1 + failure #2 | exit 3, report counts the unreadable one ("1 of 2") and fires do-not-re-run |
| APPROVE whose approval fails (provider seam) | no throw; posted:true, approved:false, WARNING names a1 repo mr approve 7 --repo g/p |
| Drift-gate boundaries | padded sourceBranch trimmed → posts; case-mismatched head refuses (fail-closed); padded commit_id refuses (fail-closed); absent sourceBranch key = empty = declared fail-open |
Provider seam — D6 verdict mapping (witness 07-provider-seam-d6-mapping.png)
Real submitAoneReview from the compiled dist, driven with constructed requests against the real a1 child transport:
| Event | Observed |
|---|---|
| COMMENT | order [whoami, view, inline, inline, summary]; summary body verbatim; no approve; ids [101,102], summary id 103 |
| REQUEST_CHANGES | summary prefixed **Request changes**\n\n; inline bodies untouched; no approve |
| APPROVE | approve LAST (repo mr approve 7 --repo g/p), approved: true |
| id read-back shapes | top/comment/note/result/data all read the id back; noid and string-id degrade to "posted, id unknown" (postedInline still 2); float ids pass through (observation only — ids are report metadata) |
Commit-2 separation — the tolerant read-back (witness 05-commit2-separation-int-vs-head.png)
The shallow checkout holds only merge/base/head, so commit 2 ("count an accepted-but-unreadable Aone answer as posted") was separated by reconstructing the intermediate build: head's compiled a1JsonOnce patched back to a throwing JSON.parse (the exact pre-commit-2 semantics), in a scratch dist copy. Scenario: comment #1 accepted but answers garbage; comment #2 fails.
| Build | Report | do-not-re-run advisory |
|---|---|---|
| intermediate (throwing read-back) | "failed after 0 of 2 inline comment(s)" — undercounts the accepted comment; cause shown is Unexpected token 'h', "this is not"... |
suppressed — a re-run would double-post comment #1 |
| head (tolerant read-back) | "failed after 1 of 2 inline comment(s)" | fires |
Commit 2's claimed effect is real and load-bearing.
Findings
F1 (moderate) — partial-post failure report names the comment body, not the platform error, as the cause
a1Cause (new in this PR) assumes the first line of the execFileSync error message is the whole "Command failed: a1 …" preamble and takes the first line after it as the cause. But the preamble embeds the full argv, and a comment create argv carries the whole comment body — which is multi-line in production shapes (the attribution footer appends \n\n_— <model> via Qwen Code /review …_; any multi-line finding does the same). The preamble then spans several lines, and lines.slice(1) picks up a line of the body — the real a1 error sits further down, unshown.
Measured at head, end to end (first inline write fails with HTTP 502 Bad Gateway; witness 04-partial-post-and-cause-finding.png):
FAILED to post the review to g/p#7 on Aone Code: posting to MR 7 of g/p failed after
0 of 1 inline comment(s) landed: _— test-model via Qwen Code /review (v0.21…
The cause line is the comment's own attribution footer. The counts, exit shape (3 + {"posted":false,"reason":"aone-post-failed"}) and the do-not-re-run advisory are all correct — the defect is confined to the one line that should say why. Blast radius: every failed comment create with a multi-line body (inline and summary — i.e. the normal case with attribution on). Approve-failure causes are unaffected (no body in the argv; measured: prov-approve-fail shows the correct permission denied: cannot approve). The same a1Cause feeds both sites.
Reproduce: cd tmp/pr9491-verify-20260819-174004 && node harness.mjs transport-cause-line (the two FAIL rows are this finding's scripted witnesses).
Suggested fix (measured)
The original execFileSync error keeps the child's streams; read the cause from them instead of splitting the rebuilt message (packages/cli/src/commands/review/lib/platform/aone.ts):
function a1Cause(err: unknown): string {
const e = err as { stderr?: Buffer | string; stdout?: Buffer | string; message?: string };
const firstLine = (s: Buffer | string | undefined): string | undefined =>
s?.toString().split('\n').map((l) => l.trim()).find(Boolean) || undefined;
const cause =
firstLine(e.stderr) ?? firstLine(e.stdout) ?? firstLine(e.message) ?? String(err);
return cause.length > 300 ? `${cause.slice(0, 300)}…` : cause;
}Measured in a scratch dist copy (fix-tree; witness 06-fix-measured-cause-line.png):
- hostile fixture (502 on first write): cause now
HTTP 502 Bad Gateway; exit shape and "0 of 1" count unchanged; - hostile fixture (2nd write fails): cause now
permission denied: cannot comment on this MR; "1 of 2" + do-not-re-run unchanged; - benign fixture: byte-identical behavior — exit 0, same call order, url relayed.
No unit test reaches this layer either way — the suite mocks the a1 seam and the existing mid-batch test asserts only toContain('1 of 2') on a single-line mock error, where both implementations agree — so the fix should ship with a fixture that pins it: a partial-post test whose failing a1 carries multi-line argv (a body with a footer) plus a distinct stderr line, asserting the stderr line appears in the message and the footer does not.
F2 (nit — coverage gap, not a defect) — createdCommentId's result/data nestings are unpinned
Mutant S1 (drop 'result', 'data' from the nested-key loop) survived: 3 files, 63/63 tests green. The behavior is alive and correct — the provider-shape probe reads ids back through both nestings — but nothing asserts it, while comment/note/top-level shapes are pinned. Completeness reporting only; the next regression in that loop would land exactly on the unpinned keys.
Verified declared trade-offs (not findings)
- Fail-open drift gate on empty
sourceBranch— measured: absent or emptysourceBranchposts unanchored; any present mismatch refuses pre-write. Matches the Phase 3 design-doc note the PR adds. - Tolerant id read-back — measured across 8 shapes (above); parse misses degrade to "posted, id unknown", never to a failed submit.
Mutation matrix — the PR's new tests are not vacuous
Run in a scratch worktree at the merge commit; each mutant edits one guard in the production source, runs the pinning files, restores (restore verified clean each time); unmutated control green. Witness 08-mutation-matrix.png; raw per-test failures in mutation-log.txt.
| Mutant (guard removed) | Suite response | Attribution |
|---|---|---|
M1 a1Once retries writes |
KILLED (1 test) | a1Once NEVER retries… fails; the failing run's 9 s duration is the 3 s + 6 s retry-sleep fingerprint (3 attempts where 1 is asserted) |
M2 a1JsonOnce throws on parse miss |
KILLED (1) | …returns undefined (not a throw)… |
| M3 drift gate removed | KILLED (1) | refuses BEFORE writing when the head drifted |
| M4 gate also fires on empty branch | KILLED (1) | an empty sourceBranch cannot gate… |
M5 postedInline counts only readable ids |
KILLED (1) | counts an accepted-but-unreadable answer as POSTED |
| M6 REQUEST_CHANGES header removed | KILLED (1) | …prefixes the blocking header |
| M7 approve failure rethrown | KILLED (1) | an approve failure alone does not fail the post |
| M8 hostless refusal removed | KILLED (2) | a recorded-but-hostless target still refuses + FAILS CLOSED on a bare-number recording |
S1 drop result/data nesting keys |
SURVIVED (63/63 green) | coverage gap → F2 |
| Control (unmutated) | 4 files, 167/167 green | positive control |
Every killed mutant failed exactly the test(s) the commit says pin it — no misattribution. An earlier matrix pass of mine cross-contaminated mutants through a broken restore; it was discarded and re-run clean (the restore now verifies git status after each revert).
Targeted gate (witness 09-head-review-suite-gate.png)
cd packages/cli && npx vitest run src/commands/review/ at head: 90 files, 3827 passed | 4 skipped (3831), exit 0 (31.6 s). The description's "3828 tests" is substantively accurate. Gate liveness is proven by the mutation matrix (same vitest setup goes red eight distinct ways). The description's claim that the reason strings are consumed only by producer + tests + skill prose holds: grep finds target-platform-unbound/aone-post-failed only in submit.ts and its tests.
Not covered
- E2E against a real Aone MR. No
a1backend or credentials in this sandbox (and the PR deliberately defers live posting to a maintainer dry-run). The harnesses reproduce the wire shape (exact argv, JSON request/response) against a fake endpoint that encodes the documented semantics — not the real platform's behavior. - APPROVE / REQUEST_CHANGES through the real end-to-end compose. Without a plan + harness transcripts,
composeReviewfails closed (coverage/verification caps), so the verdict composes to a capped COMMENT — which is the production-default shape of an Aone run (context-unavailable). The event-specific paths (blocking header, native approve, approve-failure isolation, request-changes terminal note) were exercised at the provider seam against the real transport, and the note line itself is pinned by the unit suite; a full-plan end-to-end APPROVE was not constructed. - Per-commit attribution. The checkout is depth 2 (merge, base tip, PR head only;
git rev-listat the shallow boundary reports 1 where the metadata names 3 commits). The aggregateHEAD^1..HEADdiff was verified; commit 2's contribution was isolated via the reconstructed intermediate build; commit 3 is docs-only (verified by reading). - Repo-wide gates. Only the review suite plus the four PR test files; no repo-wide test/lint/typecheck round (CI covers those; budget went to the A/B).
- Windows. The transport is
execFileSync+ PATH resolution (Linux-verified here); the PR's own table marks Windows⚠️ untested. - Remaining Phase 3 items the PR declares out of scope (cleanup write audit, AI-comment marking, URL composition, render-adjudication carve-out) — not probed.
Methodology
Environment: the CI verify container (node:22-bookworm), repo at refs/pull/9491/merge (depth 2), npm ci + npm run build pre-run at head. All harnesses are mock-free with respect to the code under test: the compiled packages/cli/dist/index.js (head), a base worktree recompiled from base source (referenced-workspace dists copied from head — those packages are byte-identical between base and head per git diff HEAD^1..HEAD, which outside packages/cli touches only the SKILL.md data file), and scratch dist copies for the intermediate (pre-commit-2 read-back) and fix variants. The platform endpoint is simulated by a fake a1 executable on PATH (the code's real config seam — execFileSync('a1', …)), which records every invocation (argv, order) to a JSONL log per cell and answers per scenario env; a fake gh serves the recorded-GitHub routing cell. Cells ran in fresh temp cwds with isolated HOME, GH_HOST/QWEN_CODE_SESSION_ID cleared; recorded-host scenarios plant real .qwen/tmp/s-*/qwen-skill-args-review.txt recordings. The base control's workspace links were realpath-asserted (node_modules/@qwen-code/qwen-code-core → head-tree core, proven equivalent by the diff). The mutation matrix mutated source in a scratch worktree and ran vitest per mutant with verified restores. Raw artifacts beside this report: harness.mjs / provider-harness.mjs / provider-probe.mjs / mutation-matrix.mjs (rerunnable; the head, provider and fix-tree cells run as-is, the base/int cells need their scratch trees rebuilt), fake-a1-impl.cjs / fake-gh.cjs (the seams), checks.jsonl (all 130 assertions with details), assertions.json, mutation-log.txt, head-review-suite.log, base-build.log, and evidence/*.png.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/commands/review/lib/platform/aone-client.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/platform/aone-client.test.ts
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
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone-client.test.ts: PPPPP
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
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-client.test.ts: P (exit 0)
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 2 · packages/cli/src/commands/review/lib/platform/aone-client.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 3 · packages/cli/src/commands/review/lib/platform/aone-client.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 4 · packages/cli/src/commands/review/lib/platform/aone-client.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 5 · packages/cli/src/commands/review/lib/platform/aone-client.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)
Evidence images
1 additional image(s) did not pass the hosting checks (PNG magic, unique sanitized name, ≤2 MB, max 8) and remain in the run artifacts.
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. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
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. ✅
The maintainer review of #9491 found five ways the new Aone write path could post to the WRONG platform or double-post; all five closed: 1. The target-platform-unbound refusal never read the --host flag it names as the remedy, so the --user-authorized re-run refused again forever. An explicit flag on the re-run is platform proof and now lifts the refusal. 2. The write gate compared hosts with raw equality, but Aone is one platform under two names (the CR URL records the web host, the skill's --host rule carries the git host). Hosts now bind through hostsEquivalent, like every other host comparison in remote-match. 3. Platform routing keyed on the family wildcard and the ambient GH_HOST, so a ghe.alibaba-inc.com GHE export selected a1, and a recorded Aone host outranked an explicit --host github.com (the opposite of the registry's documented precedence). Write routing now keys on the canonical Aone pair (isAoneCanonicalHost), never consults the ambient GH_HOST (reads never detect from it), and lets an explicit --host outrank the recorded binding in both directions. 4. a1 takes a comment body as one argv element; Linux caps that at 131072 bytes while compose-review's cap counts characters, so a long bilingual CJK summary died with E2BIG only after every inline had landed. A size gate now refuses the whole batch before any write. 5. An accepted-then-died write (timeout after the POST committed, a reset mid-response) read back as not-landed, suppressing the do-not-re-run advisory and inviting the double-post a1Once exists to prevent. Exec failures now count as possibly-landed (ambiguous), so the advisory fires even when the count is zero.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 227 passed · 2 failed · 229 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：227 通过 · 2 失败 · 229 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9491 deep verification (follow-up round) —
|
| # | Finding | Severity | Status at new head cb5ebe520c |
|---|---|---|---|
| F1 | Partial-post failure report quotes a line of the comment body instead of the platform error (a1Cause splits the rebuilt message; a multi-line body makes the "Command failed: a1 …" preamble span several lines, and lines.slice(1) picks up a body line) |
moderate | Stands — re-measured. Same class, unchanged severity. New head's cause line: _— test-model via Qwen Code /review (v0.21.0)_ --format json where HTTP 502 Bad Gateway should be (f1-probe.mjs, witness 04-f1-cause-line-head-vs-fixed.png). a1Cause is byte-identical to the shape the last round quoted. The previous round's fix re-measured clean on this head (Finding F1 below). |
| F2 | createdCommentId's result/data nestings behave correctly but are unpinned |
nit | Stands — re-measured. Mutant S1 (drop 'result', 'data' from the key loop) survives at the new head: 3 files, 70/70 green. The new tests added in commit 4 pin comment/note/top-level shapes only. |
| — | Verified trade-offs: fail-open drift gate on empty sourceBranch; tolerant id read-back |
— | Hold — re-executed at the new head (provider cells P4 and P8). |
The previous round's routing table described commit-3 semantics; commit 4 reworked routing (canonical-pair predicate, GH_HOST never consulted, explicit --host precedence). The A/B matrix below re-measures the new semantics from scratch.
Scope selection
- Central claim: an authorised
--commentrun on an Aone target posts the composed review througha1— onerepo mr comment createper inline finding, the summary comment last,repo mr approveafter the summary on APPROVE — where base refused (aone-read-only-phase). - Secondary 1 (the five new fixes):
--hostlifts the unbound refusal; auth binds the Aone web/git host alias; routing keys on the canonical pair, never GH_HOST, explicit--hostfirst; the 131072-byte argv size gate refuses the whole batch pre-write; an exec failure counts as possibly-landed (ambiguous) so the do-not-re-run advisory fires even at zero count. - Secondary 2 (write discipline, carried): no-retry write transport, pre-write head-drift refusal, mid-batch
AonePartialPostErrorreporting, tolerant read-back of an accepted-but-unparseable answer.
Central claim — A/B table
Real compiled runSubmit from each arm's dist (no mocks anywhere in the code under test; the base packages/cli recompiled from base source in a scratch worktree). Platform peers are fake executables on PATH — a1 records every argv vector to JSONL and answers per scenario env; gh records argv + stdin payload. Identical scenario per cell; only the build differs. Witness: 01-ab-matrix-base-refuses-head-posts.png. 97 scripted assertions, all green.
| Cell (11) | Base (39fc769d) |
Head (cb5ebe520c) |
|---|---|---|
ab1 recorded Aone host + --host, zero findings |
exit 3, aone-read-only-phase, 0 calls |
exit 0, posts via a1: whoami → view → summary create (no inline), url relayed from detailUrl¹ |
| ab2 no recording, cwd origin on Aone, one Critical | exit 3, refusal, 0 calls | exit 0, REQUEST_CHANGES: whoami → view → inline create → summary create, stderr names "no native request-changes state" |
r1 ambient GH_HOST=gitlab.alibaba-inc.com, GitHub cwd |
exit 3 (GH_HOST refusal arm) | posts via gh — GH_HOST never selects a1 (zero a1 calls) |
r2 explicit --host ghe.alibaba-inc.com (family GHE) |
exit 3 (family wildcard) | posts via gh — canonical pair, not family resemblance |
r3 recorded bare 42 --comment (hostless), no --host |
exit 3, aone-read-only-phase |
exit 3, target-platform-unbound — fail-closed preserved, 0 calls on either platform |
r4 hostless recording + --host gitlab… |
exit 3 (base ignores the remedy) | exit 0 — the --host remedy lifts the refusal (fix #1) |
r5 hostless recording + --host github.com |
exit 3 | exit 0 via gh (fix #1, other direction) |
r6 codereview-URL recording (web host) + --host gitlab… |
exit 3 — auth host-mismatch refusal (raw equality; hole #2) | exit 0 — hostsEquivalent binds the alias pair (fix #2) |
r7 Aone target + --dry-run |
exit 3 (refusal precedes dry-run) | exit 0, wouldPost.target = "a1 repo mr comment create --mr 42 … (1 inline + summary)", 0 calls |
r8 first write fails (permission denied) |
— | exit 3, aone-post-failed, exactly 1 create attempt, stderr "do NOT re-run" fires at zero landed count (fix #5 end-to-end) |
| r9 first write accepted but answers garbage | — | exit 0 — accepted-but-unreadable counts as posted (inlineComments: 1), commit-2 semantics end-to-end |
¹ ab1's end-to-end verdict is a capped COMMENT, not APPROVE: the pre-existing compose coverage gate (unmodified by this PR — compose-review.ts is not in the diff) fails closed without chunk-read evidence. The native-approve behavior itself is proven at the provider seam below (P1/P9) and pinned by the PR's unit tests; ab1 still proves routing, summary-only fan-out, and the url relay.
Base control purity: readlink -f node_modules/@qwen-code/qwen-code-core from the base worktree resolves into the head tree — but git diff HEAD^1..HEAD -- packages/core/src touches only the SKILL.md data file (no core code in the PR), so the shared core dist is a clean control; the dependency tree is untouched (0 changed package.json/lockfile lines). The base dist contains aone-read-only-phase and neither target-platform-unbound nor submitAoneReview (grep).
The five fixes — mutation matrix (10/10 killed, 0 survivors)
Each mutant reverts exactly one property in the production source of a scratch worktree at HEAD, runs the pinning test files, restores (restore verified clean). Unmutated control: 218/218 green over the same 5 files. Every kill failed the exact test that asserts the reverted property — quoted expected-vs-actual for two samples; full names in mutations-run2.log, witness 03-mutation-matrix-ten-of-ten-killed.png.
| Mutant | Property reverted | Suite response (failing test) |
|---|---|---|
| M1 | fix #1: unbound refusal honours --host again |
KILLED — the --host remedy LIFTS the unbound refusal — the re-run posts: expected 3 to be undefined (exit code) |
| M2 | fix #2: auth binds the Aone alias pair | KILLED — a codereview-URL recording posts with the ALIASED git host (web vs git name of one platform) |
| M3a | fix #3: routing keys on the canonical pair | KILLED — an explicit wildcard-family --host (a GHE host) routes to gh, not a1 |
| M3b | fix #3: routing never consults GH_HOST | KILLED — the AMBIENT GH_HOST never selects Aone for a write |
| M4 | fix #4: 131072-byte argv size gate | KILLED — refuses WHOLE, before any write, when a message overruns the a1 argv limit + oversized-inline variant |
| M5 | fix #5: exec failure counted as possibly-landed | KILLED — a mid-batch failure throws AonePartialPostError naming what landed: expected false to be true at expect(partial.ambiguous).toBe(true) |
| M6a | commit-2: accepted-unparseable degrades, never throws | KILLED — a1JsonOnce returns undefined (not a throw) when an ACCEPTED write answers unparseably |
| M6b | no-retry write transport | KILLED — a1Once NEVER retries… (the failing run's 9 s duration is the 3 s+6 s retry-sleep fingerprint) |
| M7 | pre-write head-drift gate | KILLED — refuses BEFORE writing when the head drifted |
| M8 | D6 blocking header on REQUEST_CHANGES | KILLED — REQUEST_CHANGES prefixes the blocking header |
Provider seam — write discipline at head (56/56 green)
Real submitAoneReview from compiled dist, fake a1 encoding the platform semantics; asserts both sides of the seam (every argv vector recorded + returned results). Witness 02-provider-harness-56-of-56.png. Highlights:
- P1/P2 APPROVE order
whoami → view → inline×2 → summary → approvewith exact argv (--file/--lineanchors, body as ONE argv element, approve argv exact); REQUEST_CHANGES summary prefixed**Request changes**\n\n, no approve call. - P3/P4 drift refusal BEFORE any write (log shows only whoami+view); empty
sourceBranchposts unanchored (declared fail-open). - P5 size gate: a 43691-char CJK summary — inside compose-review's 65536-char cap, 131073 bytes, outside the OS single-argv limit — refuses the whole batch with zero writes; the 131071-byte boundary posts; an oversized inline is named (
inline comment 1 (big.ts:1)). - P6/P7 mid-batch failure with a transient-flavoured message (
HTTP 503…): exactly one attempt for the failed write (reads retry — P10 shows whoami at 3 attempts — writes never);AonePartialPostErrorcarriespostedInline/summaryPosted/ambiguous=true, including at zero count. - P8 garbage answer counts as posted with the id dropped;
{comment:{id}}nesting read back. - P9 approve failure: no throw,
approved:false,approveErrornames the cause. - P11 blank summary body posts no summary.
Findings
F1 (moderate, carried over — STANDS) — partial-post failure report names the comment body, not the platform error
a1Cause is unchanged since the previous round and the defect re-measures at the new head. Multi-line bodies are the production shape (the attribution footer; any multi-line finding; every composed summary body), so the "Command failed: a1 …" preamble spans several lines and lines.slice(1) picks up a body line. Measured end-to-end at head (f1-probe.mjs; the 2 FAIL rows are this finding's scripted witnesses — they count in assertions.json):
posting to MR 7 of g/p failed after 0 of 1 inline comment(s) landed:
_— test-model via Qwen Code /review (v0.21.0)_ --format json
The real cause (HTTP 502 Bad Gateway) is gone. Blast radius: every failed comment create with a multi-line body — i.e. every attributed inline finding and every summary post. Counts, exit shape (3 + {"posted":false,"reason":"aone-post-failed"}) and the do-not-re-run advisory are all correct; only the one line that should say why is wrong. Approve-failure causes are unaffected (no body in the argv).
Suggested fix (re-measured at this head)
The previous round's fix, applied verbatim to a scratch dist copy and to the source in the mutant worktree:
function a1Cause(err: unknown): string {
const e = err as { stderr?: Buffer | string; stdout?: Buffer | string; message?: string };
const firstLine = (v: Buffer | string | undefined): string | undefined =>
v?.toString().split('\n').map((l) => l.trim()).find(Boolean) || undefined;
const cause =
firstLine(e.stderr) ?? firstLine(e.stdout) ?? firstLine(e.message) ?? String(err);
return cause.length > 300 ? `${cause.slice(0, 300)}…` : cause;
}Measured results at cb5ebe520c (04-f1-cause-line-head-vs-fixed.png):
- hostile fixture (502 on first write, multi-line body): cause becomes
HTTP 502 Bad Gateway; no body line leaks — probe 3/3; - benign fixtures: the full 56-assertion provider harness is byte-identical (56/56 on the fixed dist);
- suite counts unchanged: the five affected test files stay 218/218 green with the source-level fix (vitest-transform level; a strict
tscpass was not run on the scratch copy).
The suite cannot see this layer either way (it mocks the a1 seam), so the fix should ship with a pinning fixture: a partial-post test whose failing a1 carries multi-line argv (a body with a footer) plus a distinct stderr line, asserting the stderr line appears in the message and the footer does not.
F2 (nit, carried over — STANDS) — result/data id-readback nestings remain unpinned
Mutant S1 (drop 'result', 'data' from createdCommentId's key loop) survives at the new head: 3 files, 70/70 green. The behavior is alive and correct (read back through {comment:{id}} live in P8; the code path for result/data is byte-identical), but nothing asserts those two keys. Completeness reporting only — the next regression in that loop lands exactly on the unpinned keys.
Observation (not a finding)
The PR description's "3828 tests" reflects the suite before commit 4; at head the review suite is 3840 passed | 4 skipped (3844) across 90 files, exit 0 (head-suite.log). The four reason-string consumers named in the description check out: target-platform-unbound/aone-post-failed appear only in submit.ts, its tests, and the skill prose.
Not covered
- E2E against a real Aone MR. No
a1backend or credentials in this sandbox (the PR deliberately defers live posting to a maintainer dry-run). The harnesses reproduce the wire shape (exact argv, JSON request/response) against a fake peer encoding the documented semantics — not the real platform's behavior. - APPROVE end-to-end through the real compose. The pre-existing coverage gate caps a zero-finding verdict at COMMENT without chunk-read evidence (probed: plan + brief + diff-reading transcript still caps;
compose-review.test.tsowns that scaffolding). Approve semantics proven at the provider seam (P1/P9) and by the PR's mocked-seam unit tests instead. - Per-commit attribution. The checkout is shallow (
git rev-parse --is-shallow-repository= true) andgit rev-list HEAD^1..HEAD^2yields 1 commit where the metadata names 4 — the gap sits at the shallow boundary. The aggregateHEAD^1..HEADdiff was verified; commits 1–3 were verified at the previous round; commit 4's five fixes are the mutation matrix above. - Repo-wide gates. Only the review suite (90 files at head) plus the five PR test files under mutation; no repo-wide test/lint/typecheck round (CI covers those). Base-arm suite counts not re-run — head is fully green, so there is nothing to attribute.
- Windows. Transport is
execFileSync+ PATH resolution (Linux-verified here); the PR's own table marks Windows⚠️ . The 131072-byte gate is Linux-specific by construction (MAX_ARG_STRLEN) and harmless elsewhere. - Remaining Phase 3 items the PR declares out of scope (cleanup write audit, AI-comment marking, URL composition, render-adjudication carve-out) — not probed.
Methodology
Environment: the CI verify container (node:22-bookworm), repo at refs/pull/9491/merge (depth 2), npm ci + npm run build pre-run at head. All harnesses are mock-free with respect to the code under test: the compiled head dist, a base worktree rebuilt from base source (tmp/base-tree, scripts/build_package.js), and a scratch dist for the F1 fix variant (tmp/fix-dist). The platform is simulated by fake a1/gh executables on PATH — the code's real transport seam (execFileSync('a1', …)), recording every invocation to per-cell JSONL and answering per scenario env; cells ran in fresh temp cwds with GH_HOST/QWEN_CODE_SESSION_ID controlled, planting real .qwen/tmp/s-*/qwen-skill-args-review.txt recordings and real git origins for the cwd probe. Base control purity: internal workspace links realpath-asserted; the PR touches no core code and no dependency manifests, so the shared root node_modules is a clean control. The mutation matrix mutated source in tmp/mutant-tree (symlinked dists/node_modules from the main tree — same commit) and ran vitest per mutant with verified restores (git status clean after each). Raw artifacts beside this report: ab-routing.mjs, provider.mjs, mutations.mjs, f1-probe.mjs (rerunnable), bin/a1/bin/gh (the seams), ab-run4.log, provider-run2.log, mutations-run2.log + mutlog-M*.txt, mutant-control.log, head-suite.log, base-build.log, f1-src-fix-suite.log, s1-mutant.log, and evidence/*.png.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/commands/review/lib/platform/aone-client.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/platform/aone-client.test.ts
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/lib/remote-match.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/remote-match.test.ts
file packages/cli/src/commands/review/submit-aone.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit-aone.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone-client.test.ts: PPPPP
packages/cli/src/commands/review/lib/platform/aone.test.ts: PPPPP
packages/cli/src/commands/review/lib/remote-match.test.ts: PPPPP
packages/cli/src/commands/review/submit-aone.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
verdict: pass
summary: 5 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-client.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/platform/aone.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/remote-match.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- M5 fast-path no-recording test's missing armed-cwd-origin cell — already reported (comment 3820383695, deferred to #9579)
- N10 setGhHost negative pin missing on the Aone posting path — already reported (comment 3820383678, deferred to #9579)
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5 (rounds 3, 4 and 5 each reported new findings; all were verified).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 7: run submit-aone.test.ts under vitest (requires npm ci + npm run build first; worktree has no node_modules); chunk 5: executing remote-match.test.ts and submit-aone.test.ts under vitest — the review worktree has no node_modules , and dependency install plus the prerequisit….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/submit.ts:912 — [probe] commit_id presence-only structural gate — non-string value yields a false 'MR head moved' refusal (D3-1)packages/core/src/skills/bundled/review/SKILL.md:101 — [review] subcommand classification sentence inaccurate — comment-body has a1 backing; compose-review/plan-diff are local (D3-2)packages/cli/src/commands/review/lib/platform/aone.test.ts:968 — [probe] summaryFate narrowing condition lacks a negative pin — widening mutation ships green (D3-3)packages/cli/src/commands/review/lib/platform/aone.test.ts:974 — [probe] 'approve never ran on a failed batch' vacuously pinned — no APPROVE+write-failure cell; catch-side approve mutation ships green (D3-4)packages/cli/src/commands/review/lib/remote-match.test.ts:446 — [probe] hostsEquivalent lacks the family-wildcard negative pin its sibling predicate got (D3-5)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5 (rounds 3, 4 and 5 each reported new findings; all were verified)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：run submit-aone.test.ts under vitest (requires npm ci + npm run build first; worktree has no node_modules)；chunk 5：executing remote-match.test.ts and submit-aone.test.ts under vitest — the review worktree has no node_modules , and dependency install plus the prerequisit…。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| // bind the write. Under vitest the session-scoped candidate IS the | ||
| // root file, so this also pins that the publishing session's own | ||
| // recording joins the ordering instead of preceding it. |
There was a problem hiding this comment.
[Suggestion] This new comment claims a pin that does not exist: under vitest the session-scoped candidate is the caller-supplied seam file, not the root file — args() defaults skillArgs to a file recording PR 6771 while the test submits PR 42, so bindHost returns null for that candidate and its ordering position can never affect the outcome. Nothing pins that the publishing session's own recording JOINS the mtime ordering instead of PRECEDING it: an early-return optimisation checking the session-scoped recording first ships green (both mtime tests stay passing — 2 passed | 110 skipped under the mutation) while a gate probe flips routing (ghCalls:1 → aoneCalls:1). Such a change re-opens the stale-evidence hole the mtime ordering exists to close — session A's stale hosted record outranks session B's newer hostless re-run and binds the write. Add a distinguishing cell (pass skillArgs recording the submitted PR WITH a host, plant a NEWER sibling recording of the same PR WITHOUT a host, assert the target-platform-unbound refusal), then fix the comment to describe what the test actually pins.
Witness: mutation (session-scoped-first early return): mtime tests 2 passed | 110 skipped; gate probe flipped ghCalls:1 → aoneCalls:1 — reverted after.
中文说明
这条新注释声称钉住了一个并不存在的性质：vitest 下 session-scoped 候选是调用方提供的 seam 文件，而不是根文件——args() 默认把 skillArgs 指向一个记录 PR 6771 的文件，而本测试提交的是 PR 42，所以 bindHost 对该候选返回 null，其排序位置永远影响不了结果。没有任何测试钉住「发布 session 自己的记录是加入 mtime 排序而不是优先于排序」：一个「先检查 session-scoped 记录」的 early-return 优化可以全绿通过（变异下两个 mtime 测试仍为 2 passed | 110 skipped），而门禁探针的路由会翻转（ghCalls:1 → aoneCalls:1）。这种改动会重新打开 mtime 排序本要堵上的陈旧证据漏洞——session A 的陈旧带 host 记录压过 session B 更新的无 host 重跑并绑定写入。请补一个区分性用例（skillArgs 记录提交目标 PR 且带 host，另植入同一 PR 的更新但无 host 的兄弟记录，断言 target-platform-unbound 拒绝），并把注释改为描述该测试实际钉住的内容。
证据：变异（session-scoped 优先的 early-return）：mtime 测试 2 passed | 110 skipped；门禁探针 ghCalls:1 → aoneCalls:1 翻转——验证后已还原。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
Verified at HEAD — args() defaults skillArgs to a file recording PR 6771 while the mtime cells submit #42, so the session-scoped candidate can never match and its ordering position cannot affect the outcome: the comment's claim ("this also pins that the publishing session's own recording joins the ordering instead of preceding it") overstates what the test pins, and the early-return mutant survives as described.
This is a test-pin + comment fix, so it is deferred to #9579 per AGENTS.md's ~5-round rule: this PR is past five review rounds, and both the maintainer's current-head review and the automated APPROVAL treat the remaining non-blocking findings as tracked rather than merge-blocking, so this round lands no Suggestion-level changes at all. The finding stands as written; #9579 tracks it with its full text and witness evidence.
中文说明
已在 HEAD 核实——args() 默认 skillArgs 指向记录 PR 6771 的文件，而 mtime 单元提交的是 #42，因此会话内候选永远无法匹配、其排序位置不影响结果：注释声称（"这同时钉住发布会话自己的记录加入排序而非先于排序"）夸大了测试实际钉住的内容，早返回突变如描述存活。
此为测试钉 + 注释修复，按 AGENTS.md 的 ~5 轮规则延后至 #9579：本 PR 已经过 5 轮以上评审，维护者对当前 head 的评审与自动审查的 APPROVAL 均把剩余非阻断发现视为已跟踪而非合入阻断，因此本轮不落地任何 Suggestion 级改动。发现原文保留，#9579 以其全文与证据跟踪。
| callerHost: | ||
| explicitHost ?? recordedHost ?? cwdOriginHost ?? resolveGhHost(args.host), |
There was a problem hiding this comment.
[Suggestion] The round-5 fix re-bound the floor recovery's callerHost to the routing evidence chain here, but compose-review's call of the SAME shared recordedSeverityFloor still passes callerHost: resolveGhHost(host) (compose-review.ts:~4274) — the two boundaries the helper's doc contracts to feed with the SAME identity source now diverge. Flagless Aone post: the recorded binding carries code.alibaba-inc.com and the record carries the operator's --severity-floor critical; Step 6 runs compose-review without --host, resolveGhHost(undefined) yields github.com/ambient, the URL-shaped record fails the hostsEquivalent bar and recovery returns undefined — the archive composes under the model-written state floor. Submit then binds callerHost to recordedHost, recovers critical, and floorEnforcedReroute moves below-floor Suggestions out of the posted set — the Step-8 archived report and the posted review describe different comment sets. Mirror the same evidence chain at compose-review's call site, or factor the effective-host resolution into one shared helper both call sites invoke, so the two boundaries stay on one floor.
中文说明
第 5 轮修复在此处把 floor 恢复的 callerHost 重新绑定到路由证据链，但 compose-review 对同一个共享 recordedSeverityFloor 的调用仍传 callerHost: resolveGhHost(host)（compose-review.ts:~4274）——该助手函数文档承诺「以同一身份源喂养两个边界」，如今两者已经分叉。无 --host 的 Aone 投递：记录绑定携带 code.alibaba-inc.com 且记录带有操作者的 --severity-floor critical；Step 6 运行 compose-review 时不带 --host，resolveGhHost(undefined) 得到 github.com/环境值，URL 形态的记录过不了 hostsEquivalent 门槛，恢复返回 undefined——归档按模型写入的 state floor 合成。随后 submit 把 callerHost 绑定到 recordedHost，恢复出 critical，floorEnforcedReroute 把低于 floor 的 Suggestions 移出投递集合——Step 8 归档报告与实际投递描述的是不同的评论集合。请在 compose-review 的调用点镜像同一条证据链，或把有效 host 的解析提取为两个调用点共用的单一助手，使两个边界保持在同一 floor 上。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
Verified REAL at HEAD — submit.ts binds the floor's callerHost to explicitHost ?? recordedHost ?? cwdOriginHost ?? resolveGhHost(args.host), while compose-review.ts (~4274) still passes callerHost: resolveGhHost(host), so the two boundaries the helper's docstring contracts to feed with the SAME identity formula diverge: on a flagless Aone post the archived compose and the post can resolve different floors, and the post alone applies floorEnforcedReroute.
The fix lives at compose-review's call site (or a shared effective-host helper both call) — compose-review.ts is not touched anywhere by this PR, i.e. OUTSIDE its footprint, so this is not fixed here: it is recorded in the deferred-findings follow-up queue (per-PR tracking issue) with its full text and witness evidence, and left to a maintainer-scheduled follow-up. The finding stands as written; this thread stays open.
中文说明
已在 HEAD 核实为真实存在——submit.ts 把 floor 的 callerHost 绑定为 explicitHost ?? recordedHost ?? cwdOriginHost ?? resolveGhHost(args.host)，而 compose-review.ts（约 4274 行）仍传 callerHost: resolveGhHost(host)，于是 docstring 声称以同一身份公式喂给的两个边界发生分歧：无 --host 的 Aone 投递下，存档合成与投递可解析出不同的 floor，且只有投递侧执行 floorEnforcedReroute。
修复位于 compose-review 的调用点（或两个调用点共用的生效 host 助手）——compose-review.ts 完全不在本 PR 的改动面内，即在其足迹之外，故不在此修复：已记入 deferred-findings 后续队列（按 PR 跟踪的 issue），附全文与证据，留给维护者排期。发现原文保留，本线程保持打开。
| - Agent 0 (issue fidelity) is gated on `pr-context` success, so it is **skipped** on Aone — do not claim issue fidelity ran. (`issue-context` works standalone for the workitem evidence, but it is not wired to Agent 0.) | ||
| - Step 9's bypass audit queries GitHub by host; on an Aone report (host null) skip it instead of querying github.com. | ||
| - The run **is** read-only toward the platform in this phase: `publish-assets` (Step 7) is a Contents-API write that is not Aone-backed — skip it on Aone — and **`--comment` is refused** — report the findings in the terminal and saved report only, and tell the user posting to Aone is not supported yet. | ||
| - `--comment` posts through `qwen review submit` exactly as on GitHub — it routes the write at the `a1` CLI itself (one comment per inline finding, then the summary comment). Aone has **no native request-changes state**: on that verdict the summary comment carries a blocking header, and any inline Criticals block the merge while their discussions stay unresolved — relay the `Note:` line `submit` prints about this (it names whether inline Criticals actually posted). The native `a1 repo mr approve` is wired for an APPROVE verdict but does NOT fire this phase: every Aone run is context-unavailable (above), which caps the verdict at `COMMENT`, and `submit` forces that cap regardless of what the state claims — an approval bought by an omitted field would be a real platform approval no discussion backs. Four failure/refusal shapes are Aone-specific: a **head-drift** refusal (the MR was amended between review and post — re-review the new head, do not re-submit the stale payload); a **mid-batch failure** (stdout carries `"partial": true` with the landed counts/ids and an `ambiguous` flag — part of the review IS on the MR; never re-run `submit`; report what landed and what remains, and leave posting the remainder to the user; when `ambiguous` is true, the FAILED write itself may have reached the MR — a zero count is not proof nothing landed, so tell the user to inspect the MR before hand-posting anything); an **oversized-comment** refusal (a single comment or the summary exceeds a1's 131072-byte single-argument limit — the whole batch refuses before anything lands, there is nothing to re-run, and the user can post by hand); and an **ordinary pre-write error** (auth expiry, a network blip — nothing landed, it surfaces as a normal command failure, and a re-run is safe). `submit` also discloses a head that moved DURING posting (`WARNING: the MR head MOVED during posting`) — relay it. Two more disclosures the user must hear before a second-or-later Aone round: Aone has **no dedup backing yet** (`presubmit`/`comment-status` are skipped above), so every `--comment` round re-posts every still-valid finding as a NEW comment — the MR accumulates a duplicate of the whole review per amend-and-re-review; and **self-PR detection has no Aone backing**, so a review of the user's own MR gets no self-PR downgrade. `publish-assets` stays skipped: the Contents-API write is not Aone-backed. |
There was a problem hiding this comment.
[Suggestion] The refusal-shape enumeration ('Four failure/refusal shapes are Aone-specific') never names the two NEW recoverable refusals this PR introduces — target-platform-unbound (re-run WITH --host) and target-platform-conflict (re-run WITHOUT --host) — while Step 7's contract frames exit 3 as terminal ('a complete, correct outcome, not an error to route around') and this same bullet drills a never-re-run discipline for the partial shape; grep at HEAD finds zero occurrences of target-platform in SKILL.md. Failure path: a user runs /review 123 (bare-number recording, no host) and then asks in-session to publish — submit --user-authorized exits 3 with {"posted": false, "reason": "target-platform-unbound"} whose stderr spells the one-flag remedy; the skill gives the agent no instruction that this exit-3 shape is the recoverable exception, so it reports the not posted disposition, prints the follow-up tip, and stops — an authorised post the gate would have accepted is silently lost (and symmetrically, a conflict refuses until the flag is removed, while the terminal-refusal framing invites retrying the identical command). Add both shapes to the refusal handling and state explicitly that they are the recoverable exceptions to the exit-3-is-terminal rule — re-run once, adding --host <host> (unbound) or dropping it (conflict) — distinct from the never-re-run partial shape.
中文说明
拒绝形态枚举（"Four failure/refusal shapes are Aone-specific"）从未提及本 PR 引入的两个可恢复拒绝——target-platform-unbound（带 --host 重跑）与 target-platform-conflict（去掉 --host 重跑）——而 Step 7 的契约把 exit 3 框定为终结态（"a complete, correct outcome, not an error to route around"），同一条目又为 partial 形态钉死了"绝不重跑"纪律；在 HEAD 上 grep SKILL.md 中 target-platform 为零次。失败路径：用户运行 /review 123（纯号码记录、无 host），随后在会话内要求投递——submit --user-authorized 以 {"posted": false, "reason": "target-platform-unbound"} exit 3，stderr 已给出一面旗的补救；skill 没有告诉 agent 这个 exit-3 形态是可恢复的例外，于是它报告 not posted 处置、打印后续提示并停止——一次门禁本会接受的授权投递被静默丢失（对称地，conflict 在去掉旗标前一直拒绝，而"终结态拒绝"的框定会诱导原样重试）。请把这两种形态加入拒绝处理，并明确说明它们是 exit-3-即终结规则的可恢复例外——重跑一次，加上 --host <host>（unbound）或去掉它（conflict）——与绝不重跑的 partial 形态区分开。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
Verified at HEAD — grep finds zero occurrences of target-platform in SKILL.md: the refusal-shape enumeration names neither target-platform-unbound nor target-platform-conflict, while Step 7's exit-3 framing reads terminal and the partial shape carries a never-re-run discipline. The refusals' own stderr does spell the one-flag remedy, which bounds the cost.
This is skill-doc completeness, so it is deferred to #9579 per AGENTS.md's ~5-round rule: this PR is past five review rounds, and both the maintainer's current-head review and the automated APPROVAL treat the remaining non-blocking findings as tracked rather than merge-blocking, so this round lands no Suggestion-level changes at all. The finding stands as written; #9579 tracks it with its full text and witness evidence.
中文说明
已在 HEAD 核实——grep 在 SKILL.md 中找不到任何 target-platform：拒绝形态枚举既未列出 target-platform-unbound 也未列出 target-platform-conflict，而 Step 7 对 exit-3 的表述读起来是终局性的，partial 形态又携带绝不重跑纪律。两种拒绝自己的 stderr 确实写明了单 flag 补救，限制了代价。
此为 skill 文档完备性问题，按 AGENTS.md 的 ~5 轮规则延后至 #9579：本 PR 已经过 5 轮以上评审，维护者对当前 head 的评审与自动审查的 APPROVAL 均把剩余非阻断发现视为已跟踪而非合入阻断，因此本轮不落地任何 Suggestion 级改动。发现原文保留，#9579 以其全文与证据跟踪。
| // `viaSkillArgsOverride` below, and the write gate fails closed on | ||
| // its hostless form instead of probing the submission cwd. |
There was a problem hiding this comment.
[Suggestion] The new override arm fails closed on the HOST axis only: a bare-number --skill-args override recording that carries --host authorises a write whose destination REPO remains the caller's free choice — the gate binds repo only for pr-url records, and submit validates args.repo only as isOwnerRepo format, while this comment's own rationale (another cwd's record 'must not stand in') applies equally to the repo axis. Probe through the real gate (sessionless): override recording 7 --host gitlab.alibaba-inc.com --comment + submit --pr 7 --repo sibling/other --host gitlab.alibaba-inc.com --skill-args <rec> → posted: true with submitAoneReview called for {prNumber: 7, ownerRepo: 'sibling/other'} — repo X's composed review posts to repo Y's MR #7; the flip (refusing non-pr-url override recordings) exits 3. Reachability rides the same sessionless-wrapper seam as the override-precedence hole in this round's Needs-Human-Review list. For override-authorised bare-number recordings, fail closed on the repo axis as on the host axis (refuse, or require the recording to name --repo), or state in the comment that the repo axis is deliberately caller-supplied for override recordings.
Witness: probe (real gate, sessionless): posted: true, submitAoneReview({prNumber: 7, ownerRepo: 'sibling/other'}); flip arm → exit 3.
中文说明
新的 override 分支只在 HOST 轴上 fail-closed：携带 --host 的纯号码 --skill-args override 记录可以授权一次写入，而目标 REPO 仍由调用方任选——门禁只对 pr-url 记录绑定 repo，submit 对 args.repo 只做 isOwnerRepo 格式校验，而本注释自己的理由（另一个 cwd 的记录"must not stand in"）同样适用于 repo 轴。真实门禁探针（无 session）：override 记录 7 --host gitlab.alibaba-inc.com --comment + submit --pr 7 --repo sibling/other --host gitlab.alibaba-inc.com --skill-args <rec> → posted: true，submitAoneReview 收到 {prNumber: 7, ownerRepo: 'sibling/other'}——repo X 合成好的评审投到了 repo Y 的 MR #7；翻转臂（拒绝非 pr-url override 记录）exit 3。可达性与本轮"需人工复核"清单中 override 优先级漏洞同走无 session 包装器接缝。对 override 授权的纯号码记录，请在 repo 轴上像 host 轴一样 fail-closed（拒绝，或要求记录携带 --repo），或在注释中写明 override 记录的 repo 轴就是刻意交给调用方提供。
证据：探针（真实门禁、无 session）：posted: true，submitAoneReview({prNumber: 7, ownerRepo: 'sibling/other'})；翻转臂 → exit 3。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
Verified the shape at HEAD — an override-authorised bare-number recording carrying --host passes overrideHostless and proceeds with args.repo unbound. That matches the repo semantics of EVERY bare-number recording (in-session ones carry no repo axis to bind either — the platform gate protects the platform axis; the repo is the caller's submission target by construction), so the resolution the finding offers — fail closed on the repo axis for override recordings, or state that the repo axis is deliberately caller-supplied — is a design call.
Deferred to #9579 per AGENTS.md's ~5-round rule: this PR is past five review rounds, and both the maintainer's current-head review and the automated APPROVAL treat the remaining non-blocking findings as tracked rather than merge-blocking, so this round lands no Suggestion-level changes at all. The finding stands as written; #9579 tracks it with its full text and witness evidence.
中文说明
已在 HEAD 核实该形态——带 --host 的 override 授权纯号码记录通过 overrideHostless 检查并继续执行，args.repo 不受绑定。这与所有纯号码记录的 repo 语义一致（会话内记录同样没有可绑定的 repo 轴——平台门禁保护平台轴；repo 按构造就是调用方提交的目标），因此发现给出的两个解法——对 override 记录在 repo 轴 fail-closed，或声明 repo 轴有意由调用方提供——是一个设计决策。
按 AGENTS.md 的 ~5 轮规则延后至 #9579：本 PR 已经过 5 轮以上评审，维护者对当前 head 的评审与自动审查的 APPROVAL 均把剩余非阻断发现视为已跟踪而非合入阻断，因此本轮不落地任何 Suggestion 级改动。发现原文保留，#9579 以其全文与证据跟踪。
|
|
||
| - `APPROVE posted` | `REQUEST_CHANGES posted (<C> Critical, <S> Suggestion inline)` | `COMMENT posted (<C> Critical, <S> Suggestion inline)` — a Step 7 submission happened; use the event actually sent. | ||
| - `<verdict>, not posted (<C> Critical, <S> Suggestion)` — **high or medium** effort without `--comment`/publish authorization (medium never posts — `--comment` forces high); `<verdict>` is Approve / Request changes / Comment (a medium verdict never exceeds Comment — see Step 5). | ||
| - `<verdict>, partial (<N> inline posted, summary posted)` — Aone mid-batch failure only: `submit` answered `{"posted": false, "partial": true}` (part of the review IS on the MR). Use `summary not posted` when `summaryPosted` is false. This disposition is NEITHER `posted` NOR `not posted` — see the Aone refinements below — and it never carries a `Posted:` line. |
There was a problem hiding this comment.
[Suggestion] The new partial disposition embeds the literal retry token not posted inside the machine-readable Review complete: line — and because Aone posts the summary LAST, the sole AonePartialPostError throw site always throws with summaryPosted: false, so EVERY partial line renders as <verdict>, partial (<N> inline posted, summary not posted). Pre-PR the substring not posted appeared on completion lines only in the disposition-keyword position — exactly the runs where nothing was written and a retry was safe. A wrapper that token/substring-matches not posted to pick retry targets — the consumer the refinement paragraph below itself names ('a retry-on-not-posted wrapper … double-posting everything that landed') — matches every partial line and re-runs submit, double-posting the landed comments: the exact harm the partial disposition and the partial: true JSON flag were created to prevent, smuggled back in through the agent-composed line. The JSON-level signal is sound; the defect is the line-level token collision — pick a summary-fate wording that cannot contain the retry token (e.g. summary missing when summaryPosted is false), or state that disposition parsing must take the segment between — and the first ( and never substring-match.
中文说明
新的 partial 处置把字面重试令牌 not posted 嵌进了机器可读的 Review complete: 行——而 Aone 最后才投总结，唯一的 AonePartialPostError 抛出点抛出时 summaryPosted 恒为 false，所以每一条 partial 完成行都渲染为 <verdict>, partial (<N> inline posted, summary not posted)。本 PR 之前，子串 not posted 只出现在完成行的处置关键词位置——恰恰是什么都没写出、重试安全的运行。一个以 not posted 令牌/子串匹配来挑选重试目标的包装器——正是下方精炼段落自己点名的消费者（"a retry-on-not-posted wrapper … double-posting everything that landed"）——会匹配每一条 partial 行并重跑 submit，把已落地的评论重复投递：这正是 partial 处置与 partial: true JSON 标志本要防止的危害，经由 agent 合成的完成行被偷运回来。JSON 层信号是健全的；缺陷在行级令牌冲突——请换一个不可能包含重试令牌的总结命运措辞（如 summaryPosted 为 false 时用 summary missing），或声明处置解析必须取 — 与第一个 ( 之间的片段、绝不做子串匹配。
— qwen3.8-max via Qwen Code /review (v0.21.14)
There was a problem hiding this comment.
Verified at HEAD — Aone posts the summary LAST and the sole AonePartialPostError throw site always throws with summaryPosted: false, so every partial completion line renders <verdict>, partial (<N> inline posted, summary not posted) — the literal retry token not posted appears on EVERY partial line, exactly the substring a retry-on-'not-posted' wrapper matches. The JSON-level partial: true signal is sound and the refinement instructs the agent correctly (never the not posted form); the hazard is the line-level token collision for third-party scrapers.
This is machine-contract hardening of the agent-composed line, so it is deferred to #9579 per AGENTS.md's ~5-round rule: this PR is past five review rounds, and both the maintainer's current-head review and the automated APPROVAL treat the remaining non-blocking findings as tracked rather than merge-blocking, so this round lands no Suggestion-level changes at all. The finding stands as written; #9579 tracks it with its full text and witness evidence.
中文说明
已在 HEAD 核实——Aone 最后投递总结，且唯一的 AonePartialPostError 抛出点抛出时 summaryPosted 恒为 false，因此每条 partial 完成行都渲染为 <verdict>, partial (<N> inline posted, summary not posted)——重试令牌 not posted 字面出现在每一条 partial 行上，恰是按 'not-posted' 重试的 wrapper 会子串匹配的形态。JSON 层的 partial: true 信号是健全的，条款也正确指示 agent（绝不用 not posted 形态）；隐患在于第三方抓取器面对的完成行层面的令牌碰撞。
此为对 agent 合成行的机器契约加固，按 AGENTS.md 的 ~5 轮规则延后至 #9579：本 PR 已经过 5 轮以上评审，维护者对当前 head 的评审与自动审查的 APPROVAL 均把剩余非阻断发现视为已跟踪而非合入阻断，因此本轮不落地任何 Suggestion 级改动。发现原文保留，#9579 以其全文与证据跟踪。
|
@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: 487 passed · 0 failed · 487 total Flakiness gate: ✅ 5 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：487 通过 · 0 失败 · 487 总计 抖动门：✅ 5 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9491 deep verification (follow-up round 6) —
|
| # | Finding | Severity | Status at head 79f049439c (re-measured this round) |
|---|---|---|---|
| F1 | Partial-post cause line quoted the comment body | moderate | Fixed — holds. r8: failing write with hostile multi-line body reports cause HTTP 502 Bad Gateway; sentinel absent from both channels; exactly one create attempt (no retry). |
| F1R | Empty-stderr failures still quoted the body | minor | Fixed — holds. f1rempty: a1 failed without stderr (exit 4), no Command failed preamble, sentinel absent; f1rsigkill: (signal SIGKILL). Revert control: MUT-f1r (fallback parses the exec message) turns both sentinel pins red while the stderr-present shape (r8) stays green. |
| F2 | createdCommentId result/data nestings unpinned |
nit | Fixed — holds. f2shapes reads [201,202,203,204,205] back from {id},{comment},{note},{result},{data}; MUT-f2 (drop result/data) killed by exactly the ids pin ([201,202,203]), postedInline: 5 control green. |
| F3 | Forced context-unavailable cap unpinned | suggestion | Fixed — holds. capaone posts COMMENT with context-unavailable in cappedBy and zero approve calls; MUT-f3 (drop the force) killed by the cappedBy pin (mutant cappedBy = ["unreviewed-dimension"] only). The approve leg remains additionally masked by the plan-less unreviewed-dimension cap in this harness (see Not covered). |
| N1 (rounds 3–5) | fastPathHostless's first disjunct provably subsumed |
nit | Stands — re-measured. MUT-unbound-shape (drop the first disjunct) SURVIVES for the third consecutive round: zero behavior change across r3/n1/n8a/r4/r5/n8b. Same classification: dead clause in the gate decision (the observable consumer — the refusal-message branch — is load-bearing and pinned elsewhere); not a merge condition. |
| — | Verified trade-offs: fail-open drift gate on empty sourceBranch; tolerant id read-back |
— | Hold — re-executed, and T1 measured directly this round. t1: live view answering sourceBranch: "" posts unanchored (exit 0, summary lands) — the documented fail-open; n5a: non-empty drift still refuses pre-write (aone-post-refused, zero writes); r9: accepted-but-unreadable counts as posted. |
Scope selection
- Central claim (carried): an authorised
--commentrun on an Aone target posts the composed review througha1— onerepo mr comment createper inline finding, the summary last, approve wired after the summary — where base refused everything Aone (aone-read-only-phase) and, for case-drifted recordings, posted at the wrong platform. - Secondary 1: the six round-5 claims, each re-proven against the round-4 head (
a66d7e94) — the hole must reproduce there and be closed at the head — plus a dist-level revert mutant per guard. - Secondary 2: the carried findings (F1/F1R/F2/F3), N1, and the two verified trade-offs, re-measured at the head.
No new delta exists this round (head identical to the previous verified head), so no delta-specific cells were added; the round-5 delta arm was re-run in full instead.
Central claim — A/B table
Real compiled runSubmit from each arm's dist (no mocks anywhere in the code under test). Platform peers are fake executables on PATH — a1 records every argv vector to JSONL and answers per scenario file; gh records argv + stdin payload + the GH_HOST it was invoked with. Cells ran in fresh tmpdir cwds with real .qwen/tmp/… recordings (mtime-stamped for the ordering cells) and real git init + remote add origin for the cwd-probe cells. 273 scripted assertions (99 base + 149 head + 25 round-4), all green. Witnesses: 01-ab-base-arm-refuses.png, 02-ab-head-arm-posts-via-a1.png, 03-ab-r4-arm-round5-flips.png (each arm re-run live through the capture tool after the full run; both runs green).
| Cell | Base (5f3165f17e) |
Head (79f049439c) |
|---|---|---|
ab1 recorded CR-URL (web host) + aliased --host gitlab…, zero findings |
exit 3 — raw-equality host mismatch at the gate, 0 calls | exit 0 via a1: whoami→view→summary create→view; cappedBy carries context-unavailable; url relayed from detailUrl |
ab2 bare MR + recorded --host, one inline Critical |
exit 3 aone-read-only-phase |
exit 0; inline→summary order with --file/--line; capped by criticals-unverified + context-unavailable |
ab2b deterministic [build] body Critical |
exit 3 aone-read-only-phase |
exit 0 REQUEST_CHANGES end-to-end: summary carries **Request changes** header; advisory-only note (no inline Criticals → nothing mechanically blocks); no approve call |
r1 hostless recording, ambient GH_HOST=gitlab… |
exit 3 read-only (base consulted the ambient export) | posts via gh; a1 never selected; gh child inherited the ambient host |
r3 hostless recording, --user-authorized, no --host |
exit 3 read-only | exit 3 target-platform-unbound, names the bare-PR-number shape, 0 calls |
r4 r3 + --host gitlab… |
exit 3 read-only | exit 0 via a1 — the remedy lifts the refusal |
r5 r3 + --host github.com |
exit 3 read-only | exit 0 via gh, child rebound to github.com |
r7 Aone target + --dry-run |
exit 3 read-only | exit 0, wouldPost.target names a1 repo mr comment create --mr 42 --repo maxcompute/odps_src, 0 calls |
| r8 first write fails (502 on stderr, hostile body in argv) | exit 3 at the gate, 0 calls | exit 3 aone-post-failed, partial:true, ambiguous:true, 1 create attempt, cause HTTP 502 Bad Gateway, sentinel absent (F1) |
| r9 first write accepted but answers garbage | exit 3 at the gate | exit 0 — accepted-but-unreadable counts as posted (inlineComments: 1), summary still posts |
n1 --user-authorized, no recording anywhere, canonical Aone cwd |
exit 3 read-only | exit 3 unbound, "no recorded review names this target at all", 0 calls |
n2 hostless recording, cwd origin ghe.alibaba-inc.com |
exit 3 read-only | posts via gh — family resemblance rejected by the canonical predicate; child bound at the cwd origin |
| n3 hostless recording, cwd origin canonical Aone | exit 3 read-only | posts via a1 — canonical cwd probe selects |
| n4 summary write dies mid-batch (503) | exit 3 at the gate | exit 3, partial:true, postedInline:1, ids [901], "the summary did NOT land" + advisory, ambiguous:true |
| n5a head drifted before posting | exit 3 at the gate | exit 3 aone-post-refused; only whoami+view ran; stderr names the drift |
| n5b unexpected pre-write error (401 on the view read) | exit 3 at the gate | rethrown — exit 1, no posted:false JSON, zero writes |
n7 recorded CR-URL with case-drifted repo path, --user-authorized |
exit 0 — posts at the WRONG platform (binding vanishes, write lands on gh) | exit 0 via a1 — binds case-insensitively, zero gh calls |
| n8a cross-session: newest recording hostless | exit 3 read-only | exit 3 unbound — older host cannot mask newest hostlessness |
| n8b cross-session: newest recording hosted | exit 3 read-only | exit 0 via a1 — mtime-newest-wins |
| capgh zero findings, GitHub target | exit 0 via gh | exit 0 via gh; cappedBy WITHOUT context-unavailable |
| capaone zero findings, Aone target | exit 3 at the gate | exit 0 via a1; event COMMENT capped by unreviewed-dimension + context-unavailable (forced, F3); zero approve calls |
| f1rempty / f1rsigkill (head-only) | — | cause = exit facts / signal; sentinel never quoted (F1R) |
| f2shapes (head-only) | — | ids [201..205] from all five nestings (F2) |
Round-5 delta — A/B against the round-4 head (a66d7e94), re-run
Each hole reproduces on the round-4 control and closes at the head; witness 03-ab-r4-arm-round5-flips.png.
| Cell | Round-4 head (a66d7e94) |
Head (79f049439c) |
|---|---|---|
| m1 newest FILE hostless vs newer stale-hosted DIRECTORY | posts via a1 on the stale host (directory-mtime sort) | exit 3 unbound — the newest FILE decides |
| m1c sessionless root recording, hosted sibling newest | unbound refusal (root pinned first) | exit 0 via a1 — the root record joins the ordering |
| m1d publishing session's own recording older than a hosted sibling | unbound refusal (session-scoped pinned first) | exit 0 via a1 — sibling wins by file mtime |
m2a recorded Aone host + explicit --host github.com |
retargeted: posts via gh | exit 3 target-platform-conflict, 0 calls |
m2b recorded github.com + explicit --host gitlab… |
retargeted: posts via a1 | exit 3 target-platform-conflict, 0 calls |
| m2alias recorded web host + aliased git-host flag | posts via a1 | posts via a1 — one platform, parity |
m3hostless hostless --skill-args override + canonical Aone submission cwd |
posts via a1 (cwd probe stands in for another cwd's record) | exit 3 unbound, names the --skill-args override, 0 calls |
m3remedy same + --host gitlab… |
posts via a1 | posts via a1 — remedy lifts it, parity |
| m3hostful hostful override recording | posts via a1 at its recorded host | same — parity |
m4 cwd-selected gh post, ambient GH_HOST=ghe.example.com |
gh child routes at ghe.example.com (ambient inheritance restored) | gh child bound at the cwd origin github.com |
m5 contextUnavailable: "true" (string) on the gh path |
coerced away: posts | THROWN at compose (does not compose into a verdict), 0 gh calls |
| m6 first write fails (503) | partial JSON without the ambiguous key |
partial JSON with ambiguous: true |
Mutation matrix (dist-level, head build)
Witness 04-dist-mutation-matrix.png. 11 mutants; every kill names the pin that caught it; every listed control stayed green in every mutant run. 191 assertions, all green.
| Mutant | Reverts | Result |
|---|---|---|
| MUT-conflict | the target-platform-conflict check | KILLED — m2a/m2b flip to posts (6 red); m2alias + ab1 green |
| MUT-override | the overrideHostless refusal | KILLED — m3hostless flips to an a1 post (4 red); remedy/hostful parity green |
| MUT-filemtime | file-mtime ordering (sort reversed) | KILLED — m1/m1c/m1d/n8a/n8b all flip (13 red); r3 control green |
| MUT-ghcwd | the cwd-origin bind in setGhHost |
KILLED — m4/n2 observe the ambient/unset host (2 red); r1 control green |
| MUT-rawctx | raw contextUnavailable passthrough | KILLED — m5 flips to a post (3 red); ab1/capgh/capaone green |
| MUT-ambjson | ambiguous in the partial JSON |
KILLED — m6/n4/r8 ambiguous assertions red; stderr advisories green |
| MUT-unbound-shape | fastPathHostless first disjunct (N1) |
SURVIVES as predicted — zero behavior change (dead clause) |
| MUT-f1r | empty-stderr exit-facts fallback | KILLED — both sentinel pins red; r8's stderr-present shape green |
| MUT-f2 | result/data id nestings |
KILLED — the ids pin red ([201,202,203]); postedInline-5 green |
| MUT-f3 | the forced context-unavailable cap | KILLED by the cappedBy pin (mutant cappedBy loses context-unavailable); capgh green |
| POS-CTRL | conflict refusal exit 3→4 | KILLED — proves the harness detects a red |
Source-level pin mutants (the PR's own new tests are not vacuous)
Witness 05-src-pin-mutants-roundtrip.png (rendered from the run log). Baselines green unmutated; each revert fails the intended behavioral assertion; restores sha256-verified byte-identical; post-restore green. 18 assertions, all green.
| Mutant | Reverted hunk | Pin test failure |
|---|---|---|
| SRC-M1 | file-mtime sort in authorization.ts |
submit.test.ts "…last-writer-wins by the FILE mtime…" → AssertionError: expected undefined to be 3 |
| SRC-M2 | the conflict check in submit.ts |
submit-aone.test.ts "…CONTRADICTS the recorded host…" → AssertionError: expected undefined to be 3 |
| SRC-M3 | raw contextUnavailable passthrough |
submit.test.ts "…refuses a malformed contextUnavailable…" → AssertionError: expected [Function] to throw an error |
Reviewer Test Plan walk
- Step 1 (unit level):
cd packages/cli && npx vitest run src/commands/review/at head — 92 files, 4000 passed | 4 skipped (4004), exit 0 (06-review-suite-gate-4004.png). The PR body's "3828 tests" is stale (it predates later commits of this same PR); the suite shape matches the plan's claims. - Step 2 (behavior level, real Aone MR): cannot be performed in this sandbox (no
a1backend/credentials — see Not covered). The harness reproduces the wire shape end to end (exact argv vectors, JSON answers, failure shapes) against fake peers encoding the documented semantics, including the ordering (inline→summary→approve), the RC blocking header, the pre-write drift refusal, the no-retry transport, and the mid-batch failure report. - Step 3 (GitHub path unchanged): the diff removes 9
aone-read-only-phaseassertion lines (5 insubmit-aone.test.ts, 3 insubmit.test.ts, 1 insubmit.tsitself) and replaces them with Aone-routing pins — the description says "four pins", an undercount (see Corrections). The full suite is green at head, and the capgh parity cell posts via gh identically on base and head.
Corrections
- Carried from rounds 2–5, still standing at this head: the PR description's "and the platform's native approval when the verdict is Approve" predates the forced context-unavailable cap. On this head the cap holds every Aone verdict at COMMENT (capaone: event COMMENT, zero approve calls), so
a1 repo mr approveis wired (proven at the seam and by MUT-f3's cappedBy pin) but cannot fire end-to-end this phase. The user docs and SKILL.md say exactly this; the description does not. Correction to the description, not a request to change the code. - Carried: the description's "3828 tests" is stale — the review suite at this head is 92 files, 4000 passed | 4 skipped (4004), re-confirmed this round.
- New this round: the description's "four pins that asserted the old Aone refusal" understates the test change — the diff removes 9
aone-read-only-phaseassertion lines acrosssubmit-aone.test.ts(5),submit.test.ts(3) andsubmit.ts(1), all replaced by Aone-routing pins. Cosmetic inaccuracy in the description; the substance (GitHub-path tests otherwise unmodified, suite green) holds.
Findings
No blocking, minor, or nit findings against the PR this round. One completeness observation, carried and re-confirmed for the third round, explicitly non-blocking:
N1 (nit, completeness — not a merge condition) — fastPathHostless's first disjunct is provably subsumed
auth.recordedUnbound === true || (args.userAuthorized && recordedHost === undefined) — the first disjunct can never decide (it is only set when the second is true). MUT-unbound-shape survived again with zero behavior change; the OBSERVABLE consumer of recordedUnbound (the refusal-message branch) is load-bearing and pinned elsewhere. Dead clause in the gate decision; flagged for the author's awareness only.
Not covered
- E2E against a real Aone MR. No
a1backend or credentials in this sandbox; the harnesses reproduce the wire shape (exact argv vectors, JSON answers) against fake peers encoding the documented semantics. This reproduces the wire shape, not a live-platform round trip. - APPROVE end-to-end through the real compose. Structurally unreachable at this head (forced cap). Additionally, in this harness the approve leg of MUT-f3 stayed green because the plan-less
unreviewed-dimensioncap independently holds the event below APPROVE — the cap's wiring is proven by thecappedBypin, but the approve-call leg is only proven by unit pins, not by a live end-to-end revert here. - Per-commit attribution. Shallow checkout: the metadata names 10 commits; only the aggregate
HEAD^1..HEADdiff was verified. The round-4 head object is present, so the round-5 delta was diffed and A/B'd directly; no new commit landed since the previous round. - Repo-wide gates. Only the review suite (92 files) ran; CI covers the rest. The base/r4 worktree builds reported 225/231 pre-existing type errors, all outside
commands/review, with emit completed (same shape rounds 3–5 documented; zero errors insidecommands/reviewon every arm). - Windows. Transport is
execFileSync+ PATH resolution (Linux-verified); the 131072-byte gate is Linux-specific by construction. - Remaining Phase 3 items the PR declares out of scope — not probed.
Methodology
Environment: the CI verify container (node:22-bookworm), repo at refs/pull/9491/merge (depth 2), npm ci + npm run build pre-run at head. Input-closure identity with the previous round: HEAD/HEAD^1/HEAD^2 SHAs and tree hashes match the previous report's cited commits exactly; the lockfile and config therefore feed identical bytes, and the previous round's measurements were nonetheless re-executed, not carried. Harnesses are mock-free with respect to the code under test: the compiled head dist (merge commit), a base worktree rebuilt at HEAD^1 and a round-4 worktree rebuilt at a66d7e94 (both via scripts/build_package.js; tsc --build emitted despite pre-existing type errors outside commands/review). Control purity: the PR and the base move touch no dependency manifests; the only workspace import of the review commands is @qwen-code/qwen-code-core, whose realpath from the control worktrees resolves into the head tree's packages/core — asserted and disclosed; the PR's sole core change is SKILL.md data (7+/6−), which the submit/compose flow never reads, so the control is clean for the changed surface. Platform peers: fake a1/gh executables on PATH recording every invocation to per-cell, per-arm JSONL and answering from per-cell scenario files; cells in fresh tmpdir cwds with real recordings (explicit file and directory mtimes for the ordering cells) and real git origins. Mutation matrix: scratch copies of the head dist inside the repo tree, single-replacement edits with occurrence-count assertion, probe cells re-run per mutant; expected-red scoring (a red in a flip cell is kill evidence and counts as pass; a red in a control cell would be a failure — none occurred). Source mutants: in-tree edits with sha256-verified byte-identical restores. Raw artifacts beside this report: cells.mjs, lib.mjs, ab.mjs, runner-entry.mjs, mutations.mjs, src-mutants.mjs, tradeoffs.mjs, bin/a1, bin/gh, ab-results.json (+ ab-results-full-run2.json), ab-full-run2.log/-final.log, cell-<name>-<arm>.log, mutations-run2.log, src-mutants-run1.log, tradeoffs-run1.log, head-suite-run1.log, base-build-cli.log, r4-build-cli.log, and evidence/*.png.
Flakiness gate log
rounds=5 files=5 skipped=0
file packages/cli/src/commands/review/lib/platform/aone-client.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/platform/aone-client.test.ts
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/lib/remote-match.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/lib/remote-match.test.ts
file packages/cli/src/commands/review/submit-aone.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit-aone.test.ts
file packages/cli/src/commands/review/submit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/submit.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone-client.test.ts: PPPPP
packages/cli/src/commands/review/lib/platform/aone.test.ts: PPPPP
packages/cli/src/commands/review/lib/remote-match.test.ts: PPPPP
packages/cli/src/commands/review/submit-aone.test.ts: PPPPP
packages/cli/src/commands/review/submit.test.ts: PPPPP
verdict: pass
summary: 5 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-client.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/platform/aone.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/lib/remote-match.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 1 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/platform/aone-client.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/lib/remote-match.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit-aone.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/submit.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the current head (79f04943) across authorization and target binding, Aone write sequencing and failure semantics, and the added tests. I found no additional merge-blocking issue beyond the existing threads. The current-head checks that ran are green; the remaining non-blocking concerns are already tracked on the PR and in #9579, so I am not duplicating them here.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The current head has no unresolved Criticals, the checks that ran are green, and the remaining Suggestions are non-blocking and tracked in #9579.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Round outcome: no code changes. This round's feedback is nine new Suggestion-level findings (four from the second-ledger review, five from the third) — no Criticals, no failed checks, no base conflict. Why no changes landed. This PR is past five review rounds, so AGENTS.md's ~5-round rule applies — the same rule the previous round invoked when it deferred 29 Suggestions: land Critical fixes only (correctness, security, data loss, regressions) and defer remaining Suggestions to the follow-up so nothing is silently dropped. The human maintainer's current-head review and the automated reviewer's APPROVAL both treat the remaining non-blocking concerns as tracked (in #9579) rather than merge-blocking. Every one of the nine was verified against HEAD before disposition; none qualifies for the correctness exception:
Each deferral is recorded on its own thread (bilingual replies) and in the deferred-findings follow-up queue with the finding's full text and evidence. Thread cleanup. The 61 previously fixed findings (rounds 1–5) were re-verified at this exact head — the five changed test files run green (252/252), and the load-bearing shapes were checked directly in code (host-equivalence gate, canonical-host write predicate, size gate, file-mtime ordering, conflict/unbound/override refusals, cwd-origin gh bind, raw VerificationCommands actually run this round (no commit was made):
中文说明本轮结果：无代码改动。 本轮反馈为九条新的 Suggestion 级发现（第二份账本评审四条、第三条本评审五条）——无 Critical、无失败检查、无 base 冲突。 为何不落地改动。 本 PR 已经过五轮以上评审，AGENTS.md 的 ~5 轮规则适用——与上一轮延后 29 条 Suggestion 时援引的规则相同：只落地 Critical 修复（正确性、安全、数据丢失、回归），其余 Suggestion 延后至后续跟踪，确保无一被静默丢弃。人类维护者对当前 head 的评审与自动审查的 APPROVAL 均把剩余非阻断关注视为已跟踪（在 #9579）而非合入阻断。九条发现均先在 HEAD 核实再处置；无一符合正确性例外：
每条延后都记录在各自线程（双语回复）与 deferred-findings 后续队列中，附发现全文与证据。 线程清理。 61 条此前已修复的发现（第 1–5 轮）已在此 head 重新核实——五个变更测试文件全绿（252/252），承重型状态已在代码中直接核查（host 等价门禁、规范主机写谓词、大小门、文件 mtime 排序、conflict/unbound/override 拒绝、cwd origin gh 绑定、Aone 强制下 GitHub 路径的原始 验证本轮实际运行的命令（未做任何提交）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.21.15. |
…path (QwenLM#9604) * fix(review): clear the deferred Round-5 findings from the Aone write 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. * fix(review): address round-1 findings on the Aone write path (QwenLM#9604) * fix(review): address round-2 findings on the Aone write path (QwenLM#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. * fix(review): address round-3 findings on the Aone write path (QwenLM#9604) Make submit's exit-3 refusal terminal: refuse now throws a SubmitRefusal that runSubmit's single catch renders into the refusal shape (stderr line, posted:false JSON, exit 3), so a gate that says no cannot fall through toward the write — the helper previously returned and relied on every call site adding its own `return;`. Also extract the post-batch MR-head re-read, duplicated between submitAoneReview's partial-post and success paths, into one helper. * fix(review): address round-4 findings on the Aone write path (QwenLM#9604) * fix(review): address round-5 findings on the Aone write path (QwenLM#9604) * fix(review): address round-6 findings on the Aone write path (QwenLM#9604) * fix(review): resolve merge-conflict residue in the review skill (QwenLM#9604) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
4 participants
What this PR does
The Aone Code chain of the /review skill could review a merge request end to end but was read-only:
--commentrefused with a "not supported yet" message. This PR implements the write path for that chain. An authorised run now posts the composed review through the org-standarda1CLI: one comment per inline finding, then the summary comment, and the platform's native approval when the verdict is Approve. The platform has no native "request changes" state, so that verdict posts the summary with an explicit blocking header and leans on the merge gate that blocks on unresolved discussions — the terminal names this semantic difference.The posting discipline of the GitHub path is preserved and extended, not forked: the authorisation gate, payload validation and verdict composition all run shared before the platform branch. The write transport never retries, because a retry after the server accepted a comment would double-post. The commit anchoring GitHub enforces server-side lives here as a pre-write head-drift refusal, since the platform's comments carry no commit anchor and an amended MR would otherwise pin every comment at code the author already replaced. A mid-batch failure reports exactly which comments landed and exits in the same shape as an authorisation refusal, with do-not-re-run advice — a rerun would duplicate whatever landed. The recorded-but-hostless refusal stays fail-closed, now because the write would guess between two writable platforms rather than one writable and one read-only. Skill prose, user docs and the design doc are updated to match.
Why it's needed
The platform abstraction was built so internal repositories (the motivating one: MaxCompute's
odps_srcon Aone Code) can be reviewed with the same write discipline as GitHub PRs. The read path landed earlier; the write path was the missing half — without it, findings from an Aone review had to be re-posted by hand, and the skill's--commentstory stopped at the platform boundary.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/— the full review suite is green (3828 tests), including the new write-path coverage: inline→summary→approve ordering, the request-changes blocking header, the head-drift refusal before any write, the no-retry write transport, mid-batch failure reporting, and submit's routing arms (a recorded Aone host selects the a1 path regardless of the effective host; a recorded non-Aone binding is not vetoed by an Aone cwd probe; a recorded hostless target still refuses)./review <global-MR-id> --commentand confirm the inline findings and the summary land on the MR; an Approve verdict should also run the native approve; a Request changes verdict prints the semantic note. A scratch/test CR is the recommended target.Evidence (Before & After)
N/A — not a TUI change; behavior is pinned by the unit tests above.
Tested on
Environment (optional)
Unit tests only (
npm run build+ vitest), no sandbox.Risk & Scope
pr-context/comment-status/presubmitremain unbacked on Aone, so the context-unavailable verdict cap is unchanged.aone-read-only-phase→target-platform-unbound; write failures reportaone-post-failed) — both are consumed only by the skill prose, updated here.Linked Issues
None — continuation of the Phase 2 Aone read path; the plan lives in the design docs under
docs/design(the platform-provider abstraction and the Aone provider phase doc), whose Phase 3 status this PR updates.中文说明
这个 PR 做了什么
/review skill 的 Aone Code 链路此前可以完整评审一个 MR，但只读：
--comment会以"尚不支持"拒绝。本 PR 实现了该链路的写路径。一次获得授权的运行现在通过组织标准的a1CLI 投递合成好的评审：每条 inline 发现一条评论，然后是总结评论，verdict 为 Approve 时再执行平台原生的批准。平台没有原生的"request changes"状态，因此该 verdict 会在总结评论前置一个显式的阻塞标题，并依赖"未解决讨论阻塞合并"的门禁——终端会明确说明这一语义差异。GitHub 路径的投递纪律被完整保留并扩展，而不是分叉：授权门禁、payload 校验和 verdict 合成都走共享管线，之后才分平台。写传输绝不重试——服务端已接受后再重试会造成评论重复投递。GitHub 在服务端强制的 commit 锚定，在这里实现为投递前的 head 漂移拒绝：平台的评论不带 commit 锚点，若 MR 被 amend，重投会把每条评论钉在作者已经替换掉的代码上。批次中途失败会精确上报已落地的评论，并以与授权拒绝相同的形状退出、附带"不要重跑"的提示——重跑会把已落地的内容重复一遍。"有记录但无平台证据"的目标仍然 fail-closed 拒绝：现在是因为写入要在两个都可写的平台之间猜，而不是在一个可写一个只读之间猜。SKILL 文案、用户文档和设计文档同步更新。
为什么需要
平台抽象的初衷就是让内部仓库（动机目标：Aone Code 上的 MaxCompute
odps_src）能以与 GitHub PR 相同的写纪律被评审。读路径此前已落地，写路径是缺的另一半——没有它，Aone 评审的发现只能手工重发，skill 的--comment能力在平台边界处中断。评审者测试计划
如何验证
cd packages/cli && npx vitest run src/commands/review/——review 全量套件绿（3828 个测试），包含新增写路径覆盖：inline→summary→approve 顺序、request-changes 阻塞标题、任何写入之前的 head 漂移拒绝、写传输不重试、批次中途失败上报，以及 submit 的路由分支（记录的 Aone host 无论运行时 host 如何都走 a1 路径；记录的非 Aone 绑定不会被 Aone cwd 探测否决；无 host 记录的目标仍然拒绝）。/review <全局MR-id> --comment，确认 inline 发现与总结落在 MR 上；Approve verdict 应同时执行原生批准；Request changes verdict 会打印语义说明。建议用测试/草稿 CR 验证。前后对比证据
N/A——非 TUI 变更；行为由上述单测钉住。
测试环境
运行环境（可选）
仅单测（
npm run build+ vitest），无沙箱。风险与范围
pr-context/comment-status/presubmit在 Aone 上仍无支持，因此 context-unavailable 的 verdict 上限不变。aone-read-only-phase→target-platform-unbound；写入失败报aone-post-failed）——二者仅被本 PR 同步更新的 skill 文案消费。关联 Issue
无——是 Phase 2 Aone 读路径的延续；计划记录在
docs/design下的两份设计文档（平台 provider 抽象与 Aone provider 阶段文档），本 PR 更新了其中的 Phase 3 状态。