feat(review): detect self-MR on Aone targets in presubmit - #9629
Conversation
The self-PR verdict downgrade existed only for GitHub targets: the Aone read path skipped presubmit entirely, so a review of one's own MR silently carried the weight of an independent review (QwenLM#9616) — exactly the wrong direction for the most common local Aone flow, re-reviewing one's own CR before the next amend. presubmit now routes by platform. On an Aone target it compares the authenticated account (a1 auth whoami) against the MR author from one mr view fetch — case-insensitive, fail-soft on a deleted author, fail-closed on an unreadable MR — and emits the same report shape with the unbacked slices neutral (CI classification and comment dedup have no Aone backing yet). The same fetch backs head drift via sourceBranch, and a malformed pr_number/owner_repo stays a usage error rather than a metadata blip. SKILL.md runs presubmit on Aone targets instead of skipping it, and the "self-PR detection has no Aone backing" caveat is gone from the skill and the user docs.
github-actions
Bot
added
the
review/self-reported
|
E2E test report (reproduction + verification against the bundled CLI, incl. a read-only live smoke on real Aone infrastructure): Verification reportStatus: VERIFIED_FIXED Observed behavior1. Self-MR, mixed-case identities — shim account Exit 0. Shim log — three Report (the pre-fix run of this exact invocation reported {
"isSelfPr": true,
"downgradeApprove": true,
"downgradeRequestChanges": true,
"downgradeReasons": ["self-PR"],
"ciStatus": { "class": "no_checks", "totalChecks": 0 },
"existingComments": { "total": 0 },
"blockOnExistingComments": false,
"headDrift": { "drifted": false, "compare": null, "anchorsAtRisk": false }
}Case-insensitive comparison works ( 2. Someone else's MR ( 3. Head drift ( {
"isSelfPr": false,
"downgradeApprove": true,
"downgradeRequestChanges": false,
"downgradeReasons": ["PR head advanced during review: reviewed deadbeef, PR is now at ffff0000"],
"headDrift": {
"reviewedSha": "deadbeefcafe1234567890deadbeefcafe12345678",
"liveHeadSha": "ffff0000ffff0000ffff0000ffff0000ffff0000",
"drifted": true, "compare": null, "anchorsAtRisk": true
}
}Drift alone caps Approve (no compare API on Aone ⇒ 4. Detection without Exit 0; shim log again three 5. GitHub regression (shim answering GitHub shapes):
6. Live smoke (read-only, real Exit 0, no shims on PATH: Expected behaviorAll of the above: Aone targets (explicit Aone Key context
VERIFIED_FIXED |
|
Re-run after two autofix rounds and a merge of Template ✓ — all required sections present, bilingual body, reviewer test plan included. Problem: real and documented. Not a theoretical concern — the gap is explicitly documented today: both the review skill and Direction: aligned. This closes a Phase 3 "Still open" item from the existing platform-abstraction design doc ( Size: the only core-path touch is the bundled review skill wording ( Approach: scope looks right. One platform branch in Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明两轮 autofix 与一次 模板 ✓ —— 必填章节齐全，双语正文，含评审者测试计划。 问题：真实且有据。 该缺口目前被明确记录在案：review skill 与 方向：对齐。 收拢既有平台抽象设计文档 Phase 3 "Still open" 清单上的既有事项，而非开辟新战线。self-PR 降级是 review 门禁的核心完整性属性，补到本地最常用的平台上显然在范围内。claude-code CHANGELOG 中无对应物——这是 qwen-code 特有的 Aone 适配。 规模： 核心路径仅触及捆绑 review skill 的措辞（ 方案：范围合理。 风险： 无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
Code review (re-run on
|
| File | What changed |
|---|---|
packages/cli/src/commands/review/presubmit.ts |
runAonePresubmit branch: usage validation before the gate, one-fetch self-PR + drift, neutral unbacked slices; shared isSelfReview helper and PresubmitReport interface now type BOTH paths' result literals |
packages/cli/src/commands/review/lib/platform/aone.ts |
aoneHeadSha states the live-head fact once for every read site (fixing two untrimmed copies); new mrPresubmitFacts seam: author + head from one mr view, type-guarded, trimmed, fail-closed on missing mergeRequest |
packages/cli/src/commands/review/lib/platform/aone-client.ts |
ensureAoneAuthenticated runs ONE JSON whoami and returns the trimmed account, fail-soft on unreadable shapes |
packages/cli/src/commands/review/submit.ts |
Comment-only wording update to the forced-cap rationale |
packages/core/src/skills/bundled/review/SKILL.md |
presubmit moves from the Aone skip list to reduced backing; the self-PR caveat is removed, the dedup caveat stays |
packages/core/src/skills/bundled/review/SKILL.test.ts |
Revert guard pinning the new skill wording and the caveat's absence |
packages/cli/src/commands/review/presubmit.test.ts |
Aone suite: routing, self-MR downgrade, neutral slices, both failure classes, gate-throw path, drift, usage errors, degenerate empty-vs-empty guard |
packages/cli/src/commands/review/lib/platform/aone.test.ts |
mrPresubmitFacts suite + padded-sourceBranch regression cells for getPrMeta / getFetchMeta |
packages/cli/src/commands/review/lib/platform/aone-client.test.ts |
ensureAoneAuthenticated suite: argv + spawn-count pin, fail-soft shapes, trim parity |
docs/users/features/code-review.md |
Aone paragraph: presubmit backed for self-PR + drift, caveat removed |
docs/design/2026-08-13-review-platform-provider-abstraction.md |
Phase 3 changelog entry recording the backing and what stays open |
Testing
CI evidence (the PR's own checks on the reviewed commit) — fetched via the API; no PR code was run here. Everything is green on 75ffb30: the load-bearing Test (ubuntu-latest, Node 22.x) suite that was still in flight on the previous head now passes, alongside precheck, the Desktop Shell jobs, web-shell smoke, classify, CVE audit, and the secret scan. Test (macos-latest/windows-latest) and Integration Tests show skipped by design: ci.yml runs them only in the merge queue, with ubuntu as the PR signal. The branch is also fully up to date with main (0 behind, includes #9630) and mergeable.
CI results for 75ffb30 at review time:
| Check | Conclusion |
|---|---|
precheck-pr / precheck |
✅ success |
Test (ubuntu-latest, Node 22.x) |
✅ success |
web-shell E2E Smoke (ubuntu-latest, Node 22.x) |
✅ success |
Desktop Shell (ubuntu-22.04) |
✅ success |
Desktop Shell (windows-2022) |
✅ success |
Classify PR |
✅ success |
Dependency CVE audit |
✅ success |
Secret scan (TruffleHog) |
✅ success |
route |
✅ success |
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。
Not verified here: the live Aone behaviour. The unit suite mocks the a1 transport and platform detection, so a green run proves internal consistency, not that a live mr view/whoami answers the shape the seams read. The author's thread comment reports a shim-based E2E plus a read-only smoke against real Aone infrastructure (their own evidence, not independently re-run — this environment has no Aone credentials).
Sandboxed verification is already closing that gap: the verify job dispatched alongside this triage run is in progress now (@qwen-code /verify lane, report lands in the verify comment above) — it is what settles whether this diff flips presubmit on an Aone host from the six-gh-call route to the a1 route with the self-PR downgrade, which the mocked suite assumes rather than demonstrates.
中文说明
代码审查（在 75ffb30 上重审，两轮 autofix 之后）：独立方案与首轮一致，PR 仍然吻合，未发现阻塞性问题。两轮 autofix 的新增部分已逐项对照基础代码核实：填充 head 的 bug 真实存在且已修复——main 上 getPrMeta/getFetchMeta 读 sourceBranch 未 trim 而 submit 门禁读了 trim 后的值，带填充的服务端返回值会为从未移动的 MR 制造幻影 "PR head advanced during review" 与提交时拒绝；现在所有读取点（含两处修复点、两处 submit 门禁与新的 presubmit 接缝）统一走 aoneHeadSha，枚举 main 上的 sourceBranch 读取点无遗漏，两个 reader 都有 trim 回归用例。ensureAoneAuthenticated 的签名变更安全——返回值可赋给 reader 接口的 (): void，既有两处消费者（reader 接口、submit 门禁）均忽略返回值；门禁每次运行只跑一次 JSON whoami 且在 MR 拉取之前，fail-closed 路径在 mr view 抛错后不再产生任何 a1 调用，迟到的未捕获异常不会再丢弃优雅报告；无法解析或缺失的账号 fail-soft（''），与 GitHub 路径空登录语义一致。新 head 上 parity 依然成立：isSelfReview 单点陈述比较逻辑（含 #9212 承重的 author !== '' 守卫）；downgradeRequestChanges: isSelfPr 与 GitHub 语义逐字一致；Aone 的 downgradeApprove 组合恰为 GitHub 公式在中性切片下的取值；classifyHeadDrift 对 fail-closed 路径的空 live head 判为无 drift，metaUnavailable 不会二次触发幻影 drift。路由仍在 setGhHost 之前经 detectPlatformKind({host}) 完成；submit.ts 确认为纯注释改动，Aone 写路径的强制上限未动。第 2 轮唯一 posted 建议已解决（门禁抛错路径有了自己的测试）；第 3 轮零 findings，其三条延后项（drift/metaUnavailable 未断言 downgradeRequestChanges: false、catch 丢弃错误原因、--host 描述对 .alibaba-inc.com host 家族的措辞）属探针/措辞且针对既有行为——同意不阻塞。诚实缺口不变：单测钉住的都在 mock 接缝之后，绿 CI 证明逻辑自洽，不能证明真实 a1 repo mr view 的返回形状。
测试：以上为通过 API 读取的本 PR 自身 CI 证据（未在此运行任何 PR 代码）。75ffb30 全绿：上一 head 仍在运行的承重套件 Test (ubuntu-latest) 已通过，precheck、Desktop Shell、web-shell smoke、分类、CVE 审计、密钥扫描均绿。macOS/Windows 与集成测试按 ci.yml 设计仅在 merge queue 运行，PR 上跳过属预期。分支与 main 完全同步（落后 0，含 #9630）且可合并。未在此验证：Aone 真实行为——作者在线程中贴出基于 shim 的 E2E 与真实 Aone 的只读冒烟（作者自述证据，本环境无 Aone 凭据，未独立复跑）。沙箱验证已在闭合该缺口：随本次 triage 一起派发的 verify job 正在运行（@qwen-code /verify 通道，报告会发布在上方 verify 评论中）——它将落定"正是本 diff 把 Aone host 上的 presubmit 从六次 gh 调用翻转为 a1 路由并触发 self-PR 降级"这一 mocked 套件只能假设的命题。
— Qwen Code · qwen3.8-max
Reviewed at 75ffb30b0f20ee7d06873444f2dfe7b5775fe0e1 · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean re-review on the new head; the autofix rounds fixed a real bug (padded-head phantom drift) and tightened every seam the first pass flagged; the only standing reservation is the live Stepping back: this remains a small, honest completion of a tracked gap (#9616, Phase 3 of the platform-abstraction design), and the new head is strictly better than the one I reviewed last time. My independent proposal and what landed are still the same shape — I found no simpler path the PR missed. The autofix rounds didn't pad it: Failure directions still point the right way everywhere I looked: a thrown fetch caps Approve rather than sailing through, an empty whoami can never match a deleted author, malformed ids are usage errors that double as the argv gate for the value reaching Not a 5 for the same honest reason as the first pass: the suite mocks the Verdict: approve — CI is fully green on 中文说明置信度：4/5 —— 新 head 上的复审干净；两轮 autofix 修复了一个真实 bug（填充 head 造成的幻影 drift），并把首轮标记的每个接缝都收紧了；唯一保留项是真实 退一步看：这仍是对已跟踪缺口（#9616，平台抽象设计 Phase 3）的小而诚实的收拢，且新 head 严格优于上次审查的版本。我的独立方案与落地形状依旧一致——没有发现被错过的更简路径。autofix 各轮没有注水： 失败方向处处正确：拉取抛错压住 Approve 而非放行；空 whoami 永远不会与已删除作者匹配；非法 id 是使用错误、且兼作该值作为 positional 传给 不给 5 分的原因与首轮相同：单测 mock 了 结论：批准 —— — Qwen Code · qwen3.8-max Reviewed at |
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.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const me = aoneCurrentUser(); | ||
| const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); |
There was a problem hiding this comment.
[Suggestion] The self-PR comparison — including the load-bearing author !== '' empty-guard — is copied verbatim from the GitHub path's inline site at presubmit.ts:651, and this PR extends the duplication to a second platform path in the same file (a third, already-divergent variant exists at pr-context.ts:512, where the guard is inverted: me !== '' && …). This exact comparison has been patched before — the new test itself calls the guard house-pinned, citing the #9212 currentUserLogin lineage. The next semantics fix (an extra normalization rule, a new fail-soft case) applied to one inline site only silently diverges the platforms: one keeps downgrading self-reviews while the other stops — and since each path's tests pin its own site independently, the untouched side's tests keep passing and the drift ships. The comparison is the protection this feature exists to deliver, so a one-sided fix degrades exactly the behavior #9616 was filed for, on one platform. Extract one small helper used by both paths:
function isSelfReview(author: string, me: string): boolean {
return author !== '' && author.toLowerCase() === me.toLowerCase();
}called at both sites (presubmit.ts:651 and :930).
中文说明
self-PR 比较——包括承重的 author !== '' 空值守卫——是从 GitHub 路径在 presubmit.ts:651 的内联原样复制而来的，本 PR 把这份重复扩展到了同一文件里的第二个平台路径（pr-context.ts:512 还存在第三个已经分叉的变体，其守卫是反的：me !== '' && …）。这个比较以前就被修补过——新增测试自己也称该守卫为 house-pinned，并引用了 #9212 currentUserLogin 的谱系。下一次语义修补（额外的归一化规则、新的 fail-soft 情形）如果只应用到其中一个内联位置，两个平台会静默分叉：一边继续对自评降级、另一边停止——而由于每条路径的测试各自钉住自己的位置，未改动一侧的测试仍然通过，分叉就这样带着绿灯上线。这个比较正是本功能要交付的保护，单边修补会让 #9616 所针对的行为恰好在一个平台上退化。建议抽取一个两边共用的小助手函数（如上），并在两处调用点（presubmit.ts:651 与 :930）使用。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| typeof view.author?.username === 'string' | ||
| ? view.author.username.trim() | ||
| : '', | ||
| headSha: (view.sourceBranch ?? '').trim(), |
There was a problem hiding this comment.
[Suggestion] The provider fact "an Aone MR's live head is mr view's sourceBranch" is now hand-derived in five places in this file, and the copies already disagree on normalization. This diff adds the fifth read site (here, trimmed); the four pre-existing ones are :355 (getPrMeta.headSha — untrimmed), :618 (getFetchMeta.headRefOid — untrimmed), :806 (submit's pre-write drift gate — trimmed), :946 (head-moved-during-post check — trimmed). The divergence is not hypothetical: this PR's own tests treat a padded server value as a real shape (pinning sourceBranch: ' sha123\n' and defending with .trim()), yet the two untrimmed copies stay as-is. The consequence is newly reachable with this diff: Step 7's reviewed-SHA fallback reads meta's untrimmed headSha (meta.ts:136 ← :355) while presubmit's drift check compares a trimmed live head — so a padded sourceBranch produces a spurious "PR head advanced during review" downgrade (and a submit-time refusal via :806) for an MR that never moved. State the fact once in the provider and route all five sites through it, which also repairs the two untrimmed copies:
function aoneHeadSha(view: AoneMrView): string {
return (view.sourceBranch ?? '').trim();
}中文说明
“Aone MR 的 live head 即 mr view 的 sourceBranch”这一 provider 事实，如今在本文件里有五处手工推导，而且各副本的归一化已经不一致。本 diff 新增了第五处读取点（此处，带 trim）；既有四处为 :355（getPrMeta.headSha——未 trim）、:618（getFetchMeta.headRefOid——未 trim）、:806（submit 写前 drift 门禁——已 trim）、:946（发布期间 head 移动检查——已 trim）。这种分叉不是假想的：本 PR 自己的测试就把带空白的服务端值当作真实形态（钉住 sourceBranch: ' sha123\n' 并用 .trim() 防御），但两处未 trim 的副本保持原样。后果因本 diff 而新变为可达：Step 7 的 reviewed-SHA 回退读取 meta 未 trim 的 headSha（meta.ts:136 ← :355），而 presubmit 的 drift 检查比较的是 trim 后的 live head——于是带空白的 sourceBranch 会让一个从未移动的 MR 得到虚假的 “PR head advanced during review” 降级（以及 :806 处的提交期拒绝）。建议在 provider 中把该事实表述一次，并让全部五处经由它读取，这同时修复两处未 trim 的副本（如上）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const noComments: CommentSummary[] = []; | ||
| const result = { | ||
| prNumber, |
There was a problem hiding this comment.
[Suggestion] The "SAME report shape as GitHub" invariant the PR's own design-doc entry asserts is enforced by convention only: runAonePresubmit hand-writes the second ~16-field copy of the report literal next to runPresubmit's (presubmit.ts:808), with no shared type, and consumers read the file as untyped JSON. The design doc this PR updates names the next callers of exactly these slices: dedup backing for Aone (existing-comment fields go non-neutral) and later CI backing. Any report field added in the meantime (the way findingsFileInvalid was added recently) must be remembered in both literals; a miss on the Aone side emits JSON where the key is absent, which toBool at the consumer (compose-review.ts) silently reads as false — so a downgrade or warning that should fire on an Aone run silently doesn't, with no compile-time or test-time signal (the Aone tests only pin the fields they assert). Declare a shared PresubmitReport interface — the envelope the two paths already agree on — and type both result literals with it; no builder/factory needed, the two bodies legitimately diverge in what they compute.
中文说明
本 PR 设计文档条目所断言的“与 GitHub 相同的报告形状”这一不变量，目前只靠约定来维持：runAonePresubmit 在 runPresubmit（presubmit.ts:808）的报告字面量旁手写了第二份约 16 个字段的副本，没有共享类型，消费方以无类型 JSON 读取该文件。本 PR 更新的设计文档点明了这些切片的下一批调用方：Aone 的去重支撑（既有评论字段将不再中立）以及后续的 CI 支撑。此后任何新增的报告字段（就像最近新增 findingsFileInvalid 那样）都必须在两个字面量里各记一遍；一旦 Aone 侧漏掉，输出的 JSON 就缺少该键，消费方（compose-review.ts）的 toBool 会静默读成 false——于是本应在 Aone 运行中触发的降级或警告静默不触发，且没有任何编译期或测试期信号（Aone 测试只钉住其断言到的字段）。建议声明一个共享的 PresubmitReport 接口——即两条路径本就一致的信封——并用它标注两个 result 字面量的类型；不需要 builder/factory，两个函数体在计算内容上本就合理地不同。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| ); | ||
| } | ||
|
|
||
| ensureAoneAuthenticated(); |
There was a problem hiding this comment.
[Suggestion] The Aone presubmit path spawns a1 auth whoami twice per run — once here as the auth gate (ensureAoneAuthenticated → a1('auth','whoami')) and once at :929 to read the login (aoneCurrentUser → a1Json('auth','whoami','--format','json')); the JSON call fully subsumes the gate. Every Aone presubmit invocation pays two synchronous spawns of the same command; when whoami hits a transient error, execA1's retry loop (3 s + 6 s blocking sleeps) is paid twice — up to ~9 s of duplicated retry sleep before the MR fetch. Unlike the GitHub path, where ensureAuthenticated (gh auth status) and currentUser (gh api user) fetch different data, here the identical endpoint runs twice and only the --format json spelling differs. Observed by probe on the unmodified code: EXEC sequence whoami(plain) → mr view → whoami(json), and commenting out the gate left the report byte-equivalent. Have the auth gate do the whoami once with --format json and return (or module-cache) the parsed account, e.g. const me = ensureAoneAuthenticated(); keeping the existing ENOENT/timeout/login error mapping, and drop the second spawn from runAonePresubmit.
中文说明
Aone presubmit 路径每次运行会发起两次 a1 auth whoami——此处一次作为认证门禁（ensureAoneAuthenticated → a1('auth','whoami')），:929 再一次用于读取登录名（aoneCurrentUser → a1Json('auth','whoami','--format','json')）；而 JSON 那次调用完全涵盖了门禁的全部信息。每次 Aone presubmit 调用都要为同一条命令支付两次同步 spawn；当 whoami 遇到瞬时错误时，execA1 的重试循环（3 秒 + 6 秒阻塞等待）会被支付两次——在 MR 抓取之前最多白白多耗约 9 秒重试等待。GitHub 路径与此不同：那里 ensureAuthenticated（gh auth status）与 currentUser（gh api user）拉取的是不同数据；而这里同一端点跑了两次，只差 --format json 的拼写。在未改动代码上的探针观测：EXEC 序列为 whoami(plain) → mr view → whoami(json)；注释掉门禁后报告字节级不变。建议让认证门禁以 --format json 只做一次 whoami 并返回（或模块级缓存）解析出的 account，例如 const me = ensureAoneAuthenticated();（保留现有 ENOENT/超时/未登录的错误映射），并去掉 runAonePresubmit 中的第二次 spawn。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const me = aoneCurrentUser(); | ||
| const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); |
There was a problem hiding this comment.
[Suggestion] aoneCurrentUser() runs unconditionally even when author is provably '', so its result cannot change the report — a retryable subprocess fetch for a dead value. When mrPresubmitFacts throws (the fail-closed path: network blip, HTTP 502, bad id) or the MR author is absent (deleted account), author is '' and isSelfPr short-circuits false regardless of me; the run still spawns a1 auth whoami, and if the same transient condition that sank mr view also hits whoami, the run pays another ~9 s of blocking retry sleep for a value that is never read, delaying the fail-closed report. Observed by probe: with mr view throwing, the run still spawned whoami-json (whoamiJson=1) although isSelfPr was already determined; the one-line guard below flipped it to whoamiJson=0 with an unchanged report (it also removes the error-path exposure noted in the sibling comment).
| const me = aoneCurrentUser(); | |
| const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); | |
| const me = author !== '' ? aoneCurrentUser() : ''; | |
| const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); |
中文说明
即使 author 已确定为 ''，aoneCurrentUser() 仍会无条件执行，其结果不可能改变报告——这是为一个死值发起的可重试子进程抓取。当 mrPresubmitFacts 抛错（fail-closed 路径：网络闪断、HTTP 502、非法 id）或 MR 作者缺失（账号已注销）时，author 为 ''，无论 me 是什么 isSelfPr 都会短路为 false；运行却仍会 spawn a1 auth whoami，若导致 mr view 失败的同一瞬时状况也命中 whoami，运行会为一个永远不会被读取的值再支付约 9 秒阻塞重试等待，推迟 fail-closed 报告的写出。探针观测：在 mr view 抛错时，运行仍 spawn 了 whoami-json（whoamiJson=1），尽管 isSelfPr 已确定；下方一行守卫将其翻转为 whoamiJson=0 且报告不变（同时也消除了相邻评论提到的错误路径暴露）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| const me = aoneCurrentUser(); | ||
| const isSelfPr = author !== '' && author.toLowerCase() === me.toLowerCase(); |
There was a problem hiding this comment.
[Suggestion] aoneCurrentUser() runs outside the metaUnavailable try/catch, so a thrown whoami kills the whole presubmit with a raw exec error and no report — the exact failure class the adjacent catch was built to degrade gracefully. Failure scenario: a transient a1 outage that begins after ensureAoneAuthenticated() passes — mrPresubmitFacts fails → caught → metaUnavailable = true, the code has already decided to emit the graceful "MR metadata unavailable" report; but this second a1 call in the same outage then throws uncaught out of runAonePresubmit (a1Json propagates both exec failures and JSON.parse errors), writeFileSync is never reached, and Step 7 gets a crashed subcommand instead of the capped report the function had already composed. Note the gate runs whoami without --format json, so a JSON-format anomaly passes the gate and detonates at this second call. Observed by probe: with both calls throwing, the handler rejected and no report was written; the conditional guard from the sibling comment (author !== '' ? aoneCurrentUser() : '') restored the graceful report. The outcome is fail-safe (loud abort, nothing posts) and the GitHub path has the identical uncaught currentUser() at :650 — hence Suggestion, not Critical.
中文说明
aoneCurrentUser() 位于 metaUnavailable try/catch 之外，因此一次抛错的 whoami 会以裸 exec 错误杀死整个 presubmit 且没有任何报告——这正是旁边那个 catch 生来要优雅降级的失败类别。失败场景：在 ensureAoneAuthenticated() 通过之后开始的 a1 瞬时故障——mrPresubmitFacts 失败 → 被捕获 → metaUnavailable = true，代码已经决定输出优雅的 “MR metadata unavailable” 报告；但同一故障中的这第二次 a1 调用随后未捕获地抛出 runAonePresubmit（a1Json 会传播 exec 失败与 JSON.parse 错误），writeFileSync 永远走不到，Step 7 得到的是一个崩溃的子命令，而不是函数本已组装好的封顶报告。注意门禁那次 whoami 不带 --format json，因此 JSON 格式异常能通过门禁、却在这第二次调用处引爆。探针观测：两次调用都抛错时，handler reject 且未写出任何报告；相邻评论中的条件守卫（author !== '' ? aoneCurrentUser() : ''）可恢复优雅报告。结果是 fail-safe 的（响亮中止、不会发布任何内容），且 GitHub 路径在 :650 有完全相同的未捕获 currentUser()——因此是 Suggestion 而非 Critical。
— qwen3.8-max via Qwen Code /review (v0.21.15)
The round-1 review of the Aone presubmit found four seams the new path had hand-derived a second time; each is now stated once: - The self-PR comparison (including the load-bearing `author !== ''` guard) existed as two inline copies in presubmit.ts; isSelfReview states it once for both platform paths so a future normalization rule cannot diverge one platform silently. - "An Aone MR's live head is mr view's sourceBranch" was hand-derived in five places in aone.ts, two of them untrimmed: a padded server value then manufactured a phantom "PR head advanced during review" (and a submit-time refusal) against the trimmed reads, for an MR that never moved. aoneHeadSha states the fact once; all five sites route through it, repairing the two untrimmed copies. - The "same report shape as GitHub" invariant was convention only; both presubmit result literals are now typed against one PresubmitReport interface, so a field added to the envelope is a compile error on the path that forgets it instead of a silent toBool(false) at the consumer. - The Aone path spawned `a1 auth whoami` twice per run (plain gate + JSON account read). The gate now runs the JSON whoami once and returns the account: one spawn per run, and no account fetch remains after the MR fetch that could throw uncaught and orphan the graceful metaUnavailable report — the fail-closed path pays no a1 work after a thrown mr view. The padded-head regression cells for getPrMeta/getFetchMeta fail on the pre-round code; the empty-guard, single-spawn, and report-shape witnesses each fail under mutation probes. 4143 review tests green.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Round 2 — review feedback addressedCommit: All six inline findings are [Suggestion] findings from the automated reviewer's round 1; all six are implemented (the last two via one shared root cause). No conflicts ( Feedback points and dispositions
VerificationCommands actually run on the final committed tree:
中文说明第 2 轮 — 审查意见处理提交： 六条行内发现均为自动审查器第 1 轮的 [Suggestion]；六条全部落实（最后两条经由同一个根因一并解决）。无冲突（ 反馈点与处置
验证在最终提交树上实际运行的命令：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1a": running the changed vitest suites ( presubmit.test.ts , aone.test.ts , aone-client.test.ts , SKILL.test.ts ) — the shared review worktree has no node_module…; "agent 1a": npm run typecheck for the same environment reason — assignability claims above (e.g. () => string into ensureAuthenticated(): void , classifyCi into Pre….
Test Plan (not a blocker): src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/presubmit.ts:946 — [review] pr_number gate grammar diverges from the sibling family (leading-zero ids accepted)packages/core/src/skills/bundled/review/SKILL.md:1035 — [review] presubmit's SKILL.md invocation block lacks the --host annotation its siblings carrypackages/cli/src/commands/review/presubmit.ts:1088 — [review] Aone branch skips the setGhHost hostname validation every sibling runs
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1a"：running the changed vitest suites ( presubmit.test.ts , aone.test.ts , aone-client.test.ts , SKILL.test.ts ) — the shared review worktree has no node_module…；"agent 1a"：npm run typecheck for the same environment reason — assignability claims above (e.g. () => string into ensureAuthenticated(): void , classifyCi into Pre…。
Test Plan（非阻断）：src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| // a whoami failure aborts at the gate's actionable error, and there is no | ||
| // second a1 call after the fetch that could throw uncaught and orphan the | ||
| // graceful metaUnavailable report below. | ||
| const me = ensureAoneAuthenticated(); |
There was a problem hiding this comment.
[Suggestion] The gate-throw path of the Aone presubmit flow is untested: no test makes ensureAoneAuthenticated throw, so the deliberate "gate outside the fetch try/catch" ordering has no test guard. A future edit that moves ensureAoneAuthenticated() inside the fetch try/catch leaves every current test green, because the Aone suite only ever mocks the gate to return 'wenshao'/''. When it bites: on a machine where a1 is missing or logged out, presubmit would then write a plausible report blaming "MR metadata unavailable — could not verify self-PR status or head drift" instead of failing with the actionable "a1 CLI not found on PATH" / "run a1 auth login" error, sending the user to investigate MR access instead of fixing the CLI install/login. The mutation was run to check: baseline 105/105 green, the gate-inside-try/catch mutant stays 105/105 green, and a discriminating probe (gate throws → handler must reject and write no report) fails on the mutant with promise resolved "undefined" instead of rejecting while passing on this code (106/106).
Add one test to the presubmitCommand — Aone targets suite:
it('fails with the gate\'s actionable error when whoami throws — no report', async () => {
ensureAoneAuthMock.mockImplementation(() => {
throw new Error('a1 CLI not found on PATH — install the `a1` CLI first.');
});
const handler = presubmitCommand.handler;
if (!handler) throw new Error('presubmit handler missing');
await expect(handler(aoneArgs as unknown as Parameters<typeof handler>[0]))
.rejects.toThrow(/a1 CLI not found/);
expect(writeFileSyncMock).not.toHaveBeenCalled();
expect(mrPresubmitFactsMock).not.toHaveBeenCalled();
});中文说明
Aone presubmit 流程的门禁抛错路径没有测试：没有任何测试让 ensureAoneAuthenticated 抛错，因此"门禁位于 fetch try/catch 之外"这一刻意排序没有测试守卫。若未来某次编辑把 ensureAoneAuthenticated() 移进 fetch try/catch，现有所有测试仍会全绿——因为 Aone 套件只把门禁 mock 成返回 'wenshao'/''。何时咬人：在一台未安装 a1 或未登录的机器上，presubmit 会写出一份看似合理的报告，归咎于 "MR metadata unavailable — could not verify self-PR status or head drift"，而不是以可操作的 "a1 CLI not found on PATH" / "run a1 auth login" 错误失败，把用户引去排查 MR 访问权限而不是修复 CLI 安装/登录。已实际运行变异验证：基线 105/105 全绿；把门禁移入 try/catch 的变异体仍然 105/105 全绿；判别探针（门禁抛错 → handler 必须 reject 且不写报告）在变异体上以 promise resolved "undefined" instead of rejecting 失败，在当前代码上通过（106/106）。建议按英文部分给出的测试补一个用例。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
@qwen-code /resolve |
…ubmit-self-pr # Conflicts: # docs/design/2026-08-13-review-platform-provider-abstraction.md # docs/users/features/code-review.md # packages/core/src/skills/bundled/review/SKILL.md
…-pr' into feat/review-aone-presubmit-self-pr
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Autofix round summary — PR #9629 (round 3)Feedback dispositions
Changes this round
Verification
中文说明Autofix 轮次总结 — PR #9629（第 3 轮）反馈处理结果
本轮改动
验证（Verification）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /resolve |
…ubmit-self-pr # Conflicts: # docs/users/features/code-review.md
…-pr' into feat/review-aone-presubmit-self-pr
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. Check the workflow run for full logs. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/presubmit.ts:1041 — [probe] Aone suite never asserts downgradeRequestChanges is false under drift/metaUnavailable — mutant survivespackages/cli/src/commands/review/presubmit.ts:987 — [review] Aone fetch catch discards the error — no cause for "MR metadata unavailable"packages/cli/src/commands/review/presubmit.ts:1082 — [probe] --host describe overpromises gh/GH_HOST routing for GHE hosts in the *.alibaba-inc.com family
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/commands/review/presubmit.test.ts — no such file or directory; src/commands/review/lib/platform/aone.test.ts — no such file or directory; src/commands/review/lib/platform/aone-client.test.ts — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — PR #9629 (no action needed)The only feedback newer than the last evaluation is review Disposition of each feedback point1. "Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI" — informational disclosure, no action. 2. "Test Plan (not a blocker): …
3. Three items "Deferred under the convergence posture (round 3, not a blocker)" — untouched, as instructed. Other state checks
Verification
中文说明Autofix 轮次总结 — PR #9629（无需操作）上次评估之后唯一的新反馈是评审 各反馈点的处理结论1. "Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI"（未审查：构建与测试——集成测试（CLI、无沙箱）在 CI 中被跳过）——信息性披露，不处理。 2. "Test Plan (not a blocker): …
3. 三条"Deferred under the convergence posture (round 3, not a blocker)"（收敛姿态下延后（第 3 轮，非阻断））条目——按指示未处理。 其他状态检查
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@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: 4427 passed · 0 failed · 4427 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：4427 通过 · 0 失败 · 4427 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9629 Deep Verification —
|
| cell | build | invocation | oracle | result |
|---|---|---|---|---|
| B1 (control) | base 04886c43 |
presubmit 29295886 <sha> maxcompute/odps_src out --host gitlab.alibaba-inc.com |
shim call census + report JSON | 0 a1, 6 gh — every call at GH_HOST=gitlab.alibaba-inc.com (auth status, pulls/29295886, api user, check-runs, status, comments); report: isSelfPr: false, both downgrade flags false — fails as predicted |
| H1 (head) | head 75ffb30b |
identical | identical | 2 a1 (argv byte-exact: auth whoami --format json; repo mr view 29295886 --repo maxcompute/odps_src --format json), 0 gh; report: isSelfPr: true, downgradeApprove: true, downgradeRequestChanges: true, downgradeReasons: ["self-PR"] |
| B2 (control) | base | meta 7 --repo … --host gitlab.alibaba-inc.com, sourceBranch: " sha123\n" |
emitted headSha |
" sha123\n" — padded, the phantom-drift input — fails as predicted |
| B2 (head) | head | identical | identical | "sha123" — trimmed |
Witness: evidence/01-ab-wire-oracle-91-of-91.png (the full 91-assertion run as printed), logs/B1-base-shimlog.txt (the raw base-arm wire log quoted above).
Secondary claims, same harness
| behavior | head cells | result |
|---|---|---|
Non-self MR: no downgrades, neutral CI/comment slices (byte-exact shape), garbage --new-findings ignored |
H2, H2b | pass |
Deleted author → fail-soft (isSelfPr: false, no metaUnavailable reason) |
H3 | pass |
Thrown mr view → fail-closed: report written, Approve capped, MR metadata unavailable reason, exactly ONE whoami which ran BEFORE the fetch |
H4 | pass |
Padded sourceBranch trimmed → no manufactured drift |
H5 | pass |
Real drift → compare: null, anchorsAtRisk: true, Approve-only downgrade |
H6 | pass |
Degenerate '' account × '' author does NOT self-match |
H7 | pass |
| Exec-successful but unparseable whoami → fail-soft, report still written | H8 | pass |
| Padded whoami account trimmed INTO the match (fail-open closed) | H13 | pass |
Usage errors before any auth/platform call: -1 0 1.5 abc 1e3 +5 " 5" "5 " "" and bogus a/b/c ../repo owner/repo/extra all rejected with zero shim calls and no report; --help intercepted by yargs (zero platform calls); unknown flag rejected |
H9 | pass |
Platform selection: Aone-origin cwd w/o --host → a1; explicit github.com from an Aone clone → gh (6 calls); padded web host code.alibaba-inc.com → aone; neutral cwd → gh |
H10a–d | pass |
| Report-shape parity: deep key set of a GitHub-path report == deep key set of an Aone-path report | H11 | pass |
First-run classes: missing a1 binary and unauthenticated whoami both fail the run at the gate with an actionable error, no report, no mr view attempted |
H14, H15 | pass |
Test vacuity / mutation matrix
Every guard the PR introduces was reverted (one mutation at a time, scratch edit → run the pinned suite → git checkout restore; tree verified clean after each). All mutants killed, zero survivors, each by its intended assertion:
| # | mutation (guard removed) | suite | kill evidence |
|---|---|---|---|
| M1 | drop author !== '' in isSelfReview |
presubmit.test.ts | 1 red: keeps an empty whoami from matching an empty author — expected true to be false on isSelfPr (witness: evidence/03-mutation-guard-removed-test-red.png) |
| M2 | delete the Aone routing branch (handler always GitHub) | presubmit.test.ts | 9/11 Aone tests red |
| M3 | un-trim getPrMeta + getFetchMeta back to base form |
aone.test.ts | 2 red: both padded-sourceBranch regression cells |
| M4 | downgradeRequestChanges: false in the Aone report |
presubmit.test.ts | 1 red: downgrades BOTH events — expected false to be true (positive control, landed in the same file as the mutant) |
| M5 | restore a second plain whoami spawn in the gate |
aone-client.test.ts | 2 red: expected "spy" to be called 1 times, but got 2 times |
| M7 | drop the typeof username === 'string' guard |
aone.test.ts | 1 red: .trim is not a function crash on username: 42 — exactly the crash class the guard prevents |
| M8 | delete the pr_number usage-error validation |
presubmit.test.ts | 1 red: promise resolved "undefined" instead of rejecting — the invalid id masqueraded as success |
| M9 | drop metaUnavailable from downgradeApprove |
presubmit.test.ts | 1 red: fails CLOSED — expected false to be true |
| M6 | restore base SKILL.md wording (skip list + caveat) | core SKILL.test.ts | 1 red: the revert guard — expected … to contain 'presubmit **runs on Aone targets too**' |
The two remaining Aone tests that stayed green under M2 are the platform-agnostic guard tests (also pinned on the GitHub path), which is the expected pattern for a routing-only mutation.
Targeted gates
packages/cli:presubmit.test.ts+aone.test.ts+aone-client.test.ts+registry.test.ts— 203/203 passed (witness:evidence/02-targeted-gates-green.png).packages/clifull review subsystem (src/commands/review, 98 files) — 4312 passed, 4 skipped, 0 failed in 33.6 s. This is the regression surface for the shared-helper changes (isSelfReviewis used by the GitHub path too;aoneHeadShais used by the submit write path).packages/core:SKILL.test.ts— 24/24 passed.- Type-check: the pre-verify
npm run buildat HEAD compiled all workspaces clean, and the base-side rebuild ofpackages/cli(tsc--buildincl. project references) finished with 0 errors.
Findings (severity order — no blockers)
Nit 1 — digit runs past MAX_SAFE_INTEGER ride through validation and reach a1 in exponential notation
pr_number = '9'×24 passes /^[0-9]+$/ and Number.isInteger(Number(…)) (the double is 1e24, an "integer"), and the measured mr view argv carried the positional "1e+24" (harness INFO line, H9). Bounds: this is not argv injection — the value can never start with -, yargs rejects unknown flags before the handler, and --help is intercepted with zero platform calls (all three measured). Impact is a malformed id round-tripping to a1 and failing there (fail-closed report) — no real MR id is near 2²⁴ digits. Not new to this PR: the sibling commands share the same numeric-coercion idiom (meta uses yargs type: 'number' + Number.isInteger, same double semantics). A strict fix would cap the digit length (e.g. reject prNumber.length > 15) — left to the author; nothing downstream is endangered today.
Nit 2 (environmental, pre-existing) — missing-a1 message loses its install-specific branch in this runtime
This container's kernel/runtime reports EACCES, not ENOENT, when spawning a nonexistent binary (probed directly: code: EACCES, errno: -13 for a name that does not exist, from both /__w and /tmp). The gate's code === 'ENOENT' branch therefore cannot fire here, and a missing a1 surfaces as the generic a1 auth check failed (if you have not logged in, run a1 auth login) without the "install the a1 CLI" remedy. A/A control: base meta and head meta emit byte-identical messages under the same condition, and the diff shows the ENOENT branch unchanged by this PR — pre-existing behavior on an anomalous runtime, not a regression. On ordinary ENOENT platforms (GitHub-hosted runners, dev machines) the install message fires as designed. The gate still fails loud (nonzero exit, no report) in both runtimes.
Not covered
- Live Aone infrastructure — the sandbox has no
a1credentials. The wire oracle serves canned payloads shaped exactly as the code reads (mergeRequest.sourceBranch/author.username, whoamiaccount), so this reproduces the wire shape, not a live-server session; the author's PR body reports its own read-only live smoke. - Aone write path —
submit.tschanged a code comment only (verified in the diff); the forced context-unavailable verdict cap is untouched and was not re-A/B'd. - Aone CI classification / comment dedup — deliberately unbacked; the neutral slice shape was verified (H2), which is the whole of the PR's claim for them.
- Per-commit attribution — the checkout is depth 2 (
git rev-parse --is-shallow-repository= true;git rev-list HEAD^1..HEAD^2reports 1 commit while the metadata snapshot lists 7, the classic shallow-boundary artifact). The aggregateHEAD^1..HEADdiff was verified; per-commit behavior deltas were out of reach. - Repo-wide lint and full test suite — the PR's own CI covers these; only the affected workspaces were re-run here (per method).
- Windows behavior (the PR's
⚠️ checkbox) — this lane runs on Linux; the changed code paths spawna1viaexecFileSyncwith argv arrays (no shell), with nothing platform-conditional in the diff. - ReDoS ladder — not applicable: the only regex the PR adds is
/^[0-9]+$/(linear, no backtracking) over a caller-supplied id; no new scanner runs over untrusted text.
Methodology
Environment: the CI verify container (node:22-bookworm), working tree at refs/pull/9629/merge (depth 2), npm ci + npm run build completed before this round. Head arm = the pre-built packages/cli/dist at the merge commit. Base arm = git worktree add tmp/base-tree-9629 HEAD^1, rebuilt packages/cli only (tsc --build walking project references; the base tree needed its generated git-commit.ts, the cli/core/sdk package-local node_modules, and the @lydell/node-pty tsconfig path bridged — all logged in logs/base-build*.log). Control purity: internal @qwen-code/* links were re-pointed into the base tree and readlink -f confirmed all six resolve there; web-templates points at the head tree's built copy (the PR does not touch that package); git diff HEAD^1..HEAD -- packages/core/src shows only SKILL.md + its test changed, so the core runtime the base cli loads is code-identical to base. Harness (harness/presubmit-harness.mjs, rerunnable): logging bash shims for a1/gh prepended to PATH, scenario dirs of canned stdout/stderr/exit codes, GH_HOST recorded per gh call; every cell asserts on exit code, the report JSON read back from disk, and the shim census (call counts + exact argv). Raw per-run outputs live in logs/ (harness-final.log is the authoritative 91/91 run). Mutations were applied to source, run against the pinned suite, and reverted with git checkout; git status --porcelain verified empty after every cycle. Evidence images were produced with scripts/verify-capture.mjs. The base worktree was removed after all base cells were captured (git worktree remove --force).
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/presubmit.test.ts: (cd packages/cli) npx --no-install vitest run ./src/commands/review/presubmit.test.ts
file packages/core/src/skills/bundled/review/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/review/SKILL.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/commands/review/lib/platform/aone-client.test.ts: PPPPP
packages/cli/src/commands/review/lib/platform/aone.test.ts: PPPPP
packages/cli/src/commands/review/presubmit.test.ts: PPPPP
packages/core/src/skills/bundled/review/SKILL.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/commands/review/lib/platform/aone-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/presubmit.test.ts: P (exit 0)
round 1 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 2 · packages/cli/src/commands/review/lib/platform/aone-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/presubmit.test.ts: P (exit 0)
round 2 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 3 · packages/cli/src/commands/review/lib/platform/aone-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/presubmit.test.ts: P (exit 0)
round 3 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 4 · packages/cli/src/commands/review/lib/platform/aone-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/presubmit.test.ts: P (exit 0)
round 4 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
round 5 · packages/cli/src/commands/review/lib/platform/aone-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/presubmit.test.ts: P (exit 0)
round 5 · packages/core/src/skills/bundled/review/SKILL.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — PR #9629 (no action needed)No actionable feedback arrived this round, so no changes were made. Feedback disposition
Supporting state
No commit was made. The working tree is unchanged. 中文说明Autofix 轮次总结 — PR #9629（无需改动）本轮没有收到可处理的反馈，因此未做任何改动。 反馈处理结果
相关状态
未创建任何提交，工作区保持不变。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
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.
LGTM. Verified against the head:
- Input surface is closed before any exec:
pr_numberis gated to a positive integer (so it cannot ride intoa1's argv as a flag),owner_repopassesisOwnerRepobeforemrView, and everya1call goes throughexecFileSyncwith an argument array — no shell anywhere in the transport. - Failure classification matches the GitHub path's two classes: a thrown
mr viewfails closed into the Approve-capping "MR metadata unavailable" reason, an absent author fails soft via the sharedisSelfReviewwhoseauthor !== ''guard rules out the'' === ''self-match, and the server-controlledusernameis type-guarded before the case-fold. - The
PresubmitReportinterface pins the same-shape invariant at compile time, the unbacked CI/comment slices report the exact neutral shape (no phantom downgrades or blocks), and theaoneHeadShaconsolidation removes the real untrimmed-read divergence with a test pinning it. Explicit--hostoutranks the cwd probe in both directions per the registry. CI is green on head.
#9629 landed the self-MR half of the Aone presubmit while this branch carried the full backing; the merge consolidates the two designs: - one MR-facts seam: getMrAuthorAndHead subsumes mrPresubmitFacts (tolerant aoneAccountName author read + the canonical aoneHeadSha head read), shared by presubmit and comment-status - the Aone presubmit runner keeps the full backing (merge-gate classification + comment dedup) and adopts #9629's argv gates and single-spawn whoami gate (ensureAoneAuthenticated doubles as the account read, before the MR fetch) - the shared report writer supersedes the PresubmitReport interface — both platform paths emit one literal through one function - docs/SKILL describe the full backing; the SKILL.md revert guard and the Aone presubmit suite are re-pinned on the merged wording
…check rule The out-of-scope list still read self-PR detection as open work although #9629 shipped it into this branch's merge base — annotate it like the sibling #9618 entry. Step 6's report-existence guard pointed report-less runs at a re-derivation the skill never defines; replace it with the explicit rule: no per-thread status routing, no hand-derived substitute, rule from the code at the reviewed commit, cannot-tell over a guess.
|
Released in v0.22.0. |
* feat(review): back pr-context on Aone Code targets pr-context was the one read subcommand still gh-direct, so every Aone run was forced context-unavailable: the verdict capped at COMMENT (the wired a1 approval could never fire), Agent 0 skipped, and the machine ledger never recovered from posted summaries. Route it through the platform reader with a normalized context bundle; Aone serves it from mr view + the flat comment list (thread comments carry the ledger), GitHub's implementation is an extraction of the existing calls — its output stays byte-identical. The forced cap leaves the Aone write path for parity with GitHub's state-claim handling, and the refetch commands a context file emits bake --pr on Aone, where comment bodies are addressed per-MR. * fix(review): keep Aone ledger carriers out of the blocker re-check (QwenLM#9621) On Aone this pipeline's own round summaries are path-less comments, so they ride pr-context's issue channel, where their visible **[Critical]** lines self-promoted every prior Critical-bearing summary into "Blockers to re-check" — rendering each prior Critical three times (beside the ledger section and the inline roots that own the same findings) and spending the section budget on the pipeline's own prose until genuine human blockers degraded to snippets. Exclude bodies carrying the ledger marker from issue-channel promotion and the stdout count, strip the marker out of the settled snippet, and switch the pr_number guard to the canonical isPositivePrNumber so 0x10/5. spellings cannot fragment side-file continuity. Pin the witnesses the round's findings name: the guard, args.host forwarding, the issue-kind --pr refetch branch, the account-first author keying, and the GitHub test suites' independence from the cwd-origin probe. * fix(review): refuse pr_number spellings that do not round-trip (QwenLM#9621) isPositivePrNumber alone admits two spellings whose Number() value does not round-trip to the raw string: leading zeros (007 fetches 7 but the raw string labels the heading and the prev-ledger side file, so a later 7 run reads a different side file and the round counter restarts) and digit strings above Number.MAX_SAFE_INTEGER (Number() silently rounds them, fetching a different PR than the labels announce). Add the safe-integer and no-leading-zero conjuncts — matching fetch-pr's [1-9]\d* rule — so every admitted input satisfies String(Number(x)) === x. Also pin the witnesses the round-2 review names: the commit_id round-trip through the GitHub reader and toRawReview into the persisted side file (both spreads were unwitnessed), the stale force-applies comment in submit-aone.test.ts the cap removal outdates, and the setup batch's Aone carve-out for the unbacked comment-status call. * docs(review): align Aone docs with the landed no-ancestry anchor rule and comment-status skips D6 described the AGit-Flow anchor as inert until the incremental rule landed, but that rule (QwenLM#9630) merged while this branch was in flight — anchors now delta-scope Aone re-reviews. SKILL.md's comment-status section and Step 6's report-existence guard now name the Aone skip the setup batch already carries, so no path sends an Aone run at the unbacked command or at a report that was never written. * docs(review): annotate QwenLM#9616 as landed and define the report-less re-check rule The out-of-scope list still read self-PR detection as open work although QwenLM#9629 shipped it into this branch's merge base — annotate it like the sibling QwenLM#9618 entry. Step 6's report-existence guard pointed report-less runs at a re-derivation the skill never defines; replace it with the explicit rule: no per-thread status routing, no hand-derived substitute, rule from the code at the reviewed commit, cannot-tell over a guess. * fix(review): route the context head through aoneHeadSha and close the round-5 findings getReviewContext read sourceBranch raw while every other head read trims — a padded server value diverged the context file from the rest of the run (phantom-drift shape). getCurrentUser now honors the seam contract on the anomalous whoami shapes instead of leaking untagged throws and non-string accounts. Step 6's report-less rule no longer contradicts the comment-status failure contract: runs where the command ran and failed keep the "re-derive if needed" fallback. The Aone paragraph names comment-body among the backed reads, and witness tests pin the identity gate's carriers key and the head normalization. * fix(review): shape-check the Aone comment listing in getReviewContext a1 can answer repo mr comment list with an exit-0 a1.error/v1 error object (backend auth failure or client timeout — measured by cleanup's a1CommentList on the identical payload). Without a guard the object survives the ?? [] coalesce and .filter throws an untagged TypeError, losing the envelope's actionable message at exactly the recoverable moment. Guard as the provider family already does and surface the cause; witness tests pin both envelope shapes (mutant-checked). * test(review): pin getCommentBody's body-field fallback (mutant-checked) * fix(review): union resolved comments into the Aone context bundle The default comment list excludes resolved comments (measured by the cleanup audit) while GitHub's REST fetches include them, so a resolved blocker/marker root never reached the re-check walk or the fail-closed identity gate. Union the default and --resolved listings as the audit does, dedupe by id, fail closed on either listing's error envelope, and disclose the residual that resolved replies stay invisible; witness tests mutant-checked. * fix(review): serve resolved comments and guard the envelope in getCommentBody getCommentBody queried only the default comment list while the context bundle it serves refetches for unions in resolved comments — a resolved id named by a truncation note threw "not found" every time, and an exit-0 a1.error/v1 envelope threw an untagged TypeError that lost the actionable message. Extract the shape-checked default+resolved union helper and read both sites through it; witness tests mutant-checked. * ci: correct qwen-autofix.yml size baseline to its actual post-migration size QwenLM#9677 shrank qwen-autofix.yml from 431526 to 397656 bytes (prose moved to the design record) but recorded the baseline at 392111, 5545 below the file's own post-change size, so the first PR to run the ratchet tripped it. This branch introduces zero growth to the file (byte-identical to main); the bump aligns the baseline with reality. No workflow content changes. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
4 participants
What this PR does
The
/reviewpre-submission gate (presubmit) is now platform-aware. On an Aone Code target it routes through thea1CLI instead ofgh, fetches the MR author and live head in onemr viewcall, compares the author against the authenticated account froma1 auth whoami(case-insensitively), and emits the same JSON report shape the GitHub path produces — with the backed slice filled in (self-PR detection, head drift) and the unbacked slices reported neutral (CI classification and existing-comment dedup have no Aone backing yet, so they come back asno_checkswith zero checks and zero comments, causing no downgrades and no overlap blocks). A readable MR whose author is absent fails soft (isSelfPr: false); an unreadable MR fails closed (an Approve-capping "MR metadata unavailable" downgrade reason), mirroring the GitHub path's two failure classes; malformedpr_number/owner_repostay usage errors instead of masquerading as metadata blips, and the id validation doubles as an argv gate since the value reachesa1as a positional. The skill now runspresubmiton Aone targets instead of skipping it, and the "self-PR detection has no Aone backing" caveat is removed from the skill and the user docs; the dedup caveat stays.Why it's needed
The self-PR downgrade exists because a review of one's own change is not an independent check, and its verdict must not carry the weight of one. Today an Aone run skips that protection entirely — the detection has no Aone backing, so reviewing your own MR silently gets no downgrade, which is exactly the wrong direction for the flow where the skill is most used locally: a developer re-reviewing their own CR before pushing the next amend. On Aone the load-bearing half is the Request-changes downgrade: without it a self-review with Criticals would compose a blocking header against its own author's MR.
Reviewer Test Plan
How to verify
packages/cli):npx vitest run src/commands/review/presubmit.test.ts src/commands/review/lib/platform/aone.test.ts src/commands/review/lib/platform/aone-client.test.ts— all green; the new Aone suite pins a1-only routing (zeroghcalls), the--hosthint wiring, case-insensitive self-MR matching, the''-vs-''degenerate guard, fail-soft/fail-closed classes, drift withcompare: null, the neutral unbacked slices, and thepr_number/owner_repousage-error classification.packages/coreSKILL.test.ts carries a revert guard for the skill wording.npm run build && npm run bundle), then inside a clone whose origin is ongitlab.alibaba-inc.comrunnode dist/cli.js review presubmit <mr-id> <sha> <owner>/<repo> <out> --host gitlab.alibaba-inc.com. Observed: the command makes onlya1calls (auth whoami, onerepo mr view,auth whoami --format json); when the MR author equals the authenticated account (verified live, read-only, including mixed-case identities), the report carriesisSelfPr: true,downgradeApprove: true,downgradeRequestChanges: true,downgradeReasons: ["self-PR"]; for someone else's MR no downgrade fires and the CI/comment sections are neutral; a stale reviewed sha reportsdrifted: truewithanchorsAtRisk: true. Reproduced pre-fix for contrast: the same invocation routed everything atgh(six calls, zeroa1) and reportedisSelfPr: falsefor a genuine self-MR.gh-call GitHub path unchanged — verified both from a neutral cwd and from inside an Aone clone (an explicit non-Aone--hostoutranks the cwd probe).Evidence (Before & After)
N/A (no TUI change). Report-level before/after for the same Aone self-MR invocation — before:
"isSelfPr": false, "downgradeApprove": false, "downgradeRequestChanges": false, "downgradeReasons": [](computed from GitHub identities viagh); after:"isSelfPr": true, "downgradeApprove": true, "downgradeRequestChanges": true, "downgradeReasons": ["self-PR"](computed from Aone identities viaa1).Tested on
Environment (optional)
npm run build && npm run bundlethennode dist/cli.js(bundled CLI) for the end-to-end runs — both with logginga1/ghPATH shims (deterministic routing proof) and a read-only live smoke against real Aone infrastructure; unit tests vianpx vitest runinpackages/cli/packages/core.Risk & Scope
presubmitis no longer skipped on Aone targets, adding twoa1calls per Aone review (onemr viewfetch + one whoami; the auth gate's plain whoami is pre-existing). The report shape is unchanged, so Step 7's apply-the-report rules and compose-review's downgrade fields need no branch; the forced context-unavailable cap in the Aone write path is untouched (comment-only edit there), so the verdict still caps at Comment this phase — the self-PR downgrade's observable effect is the Request-changes downgrade and the disclosed reasons.--commentrounds stays documented); the Aone write path itself is unchanged; no writes were performed anywhere during testing (live probes were read-only).Linked Issues
Fixes #9616
中文说明
这个 PR 做了什么
/review的预提交检查（presubmit）现在能感知平台。对 Aone Code 目标，它改走a1CLI 而不是gh：一次mr view调用同时取到 MR 作者与 live head，再与a1 auth whoami的当前认证账号做大小写不敏感比较，并输出与 GitHub 路径完全相同的 JSON 报告形状——有支撑的部分（self-PR 检测、head drift）填充真实值，无支撑的部分报告中立（CI 状态分类与既有评论去重目前还没有 Aone 支撑，因此报告为no_checks、零检查、零评论，不产生任何降级或 overlap 阻断）。MR 可读但作者缺失时失败放行（fail-soft，isSelfPr: false）；MR 不可读时失败关闭（fail-closed，给出压住 Approve 的 "MR metadata unavailable" 降级原因），与 GitHub 路径的两类失败语义一致；非法的pr_number/owner_repo仍是使用错误（usage error）而非伪装成元数据闪断，且 id 校验同时是 argv 防护——该值会作为 positional 传给a1。skill 现在会在 Aone 目标上运行presubmit而不是跳过它，"self-PR detection has no Aone backing" 的声明从 skill 与用户文档中移除；dedup 的声明保留。为什么需要
self-PR 降级的存在理由是：审查自己的改动不构成独立检查，其 verdict 不应享有独立审查的分量（尤其是 approval）。目前 Aone 运行完全跳过这层保护——该检测没有 Aone 支撑，审查自己的 MR 会静默跳过降级。而这恰恰是本地最常用的场景：开发者在推下一次 amend 前重审自己的 CR。在 Aone 上真正承重的一半是 Request-changes 降级：没有它，一份带 Critical 的自评会给自己作者的 MR 贴上 blocking 头。
评审者测试计划
如何验证
packages/cli下）：npx vitest run src/commands/review/presubmit.test.ts src/commands/review/lib/platform/aone.test.ts src/commands/review/lib/platform/aone-client.test.ts—— 全部通过；新增的 Aone 套件钉住了：仅走 a1（零gh调用）、--hosthint 的接线、大小写不敏感的自评匹配、''对''的退化守卫、fail-soft/fail-closed 两类失败、compare: null的 drift、中立的无支撑切片、pr_number/owner_repo的使用错误分类。packages/core的 SKILL.test.ts 带有 skill 措辞的回退守卫。npm run build && npm run bundle）后，在 origin 位于gitlab.alibaba-inc.com的 clone 内运行node dist/cli.js review presubmit <mr-id> <sha> <owner>/<repo> <out> --host gitlab.alibaba-inc.com。观察到：命令只发起a1调用（auth whoami、一次repo mr view、auth whoami --format json）；当 MR 作者等于认证账号时（已在真实环境只读验证，含大小写不一致的身份），报告为isSelfPr: true、downgradeApprove: true、downgradeRequestChanges: true、downgradeReasons: ["self-PR"]；他人 MR 不触发降级，CI/评论部分为中立；过期的 reviewed sha 报告drifted: true且anchorsAtRisk: true。修复前对照复现：同样的调用全部路由到gh（六次调用、零次a1），对真正的自评 MR 报告isSelfPr: false。gh调用的 GitHub 路径且行为不变——在中性 cwd 与 Aone clone 内都验证过（显式的非 Aone--host优先于 cwd 探测）。证据（前后对比）
N/A（无 TUI 变化）。同一个 Aone 自评 MR 调用在报告层面的前后对比——修复前：
"isSelfPr": false, "downgradeApprove": false, "downgradeRequestChanges": false, "downgradeReasons": []（由 GitHub 身份经gh计算）；修复后："isSelfPr": true, "downgradeApprove": true, "downgradeRequestChanges": true, "downgradeReasons": ["self-PR"]（由 Aone 身份经a1计算）。测试平台
环境（可选）
npm run build && npm run bundle后用node dist/cli.js（打包后的 CLI）跑端到端——既用了带日志的a1/ghPATH shim（确定性的路由证明），也做了对真实 Aone 基础设施的只读冒烟；单元测试经packages/cli/packages/core下的npx vitest run。风险与范围
presubmit，每次 Aone 评审增加两次a1调用（一次mr viewfetch + 一次 whoami；认证门禁的裸 whoami 是既有的）。报告形状不变，因此 Step 7 的报告应用规则与 compose-review 的降级字段无需分支；Aone 写路径中强制 context-unavailable 的上限未动（那里只改了注释），所以本阶段 verdict 仍封顶在 Comment——self-PR 降级可观察的效果是 Request-changes 降级与被披露的原因。--comment轮次的 dedup 声明继续保留在文档中）；Aone 写路径本身未变；测试期间任何地方都没有发生写操作（真实探针均为只读）。关联 Issue
Fixes #9616