feat(review): Aone Code read path (second review-platform provider) - #9226
Conversation
Adds an Aone Code provider so /review can review a MaxCompute CR locally. The read path works end to end against a real odps_src CR (verified E2E): fetch-pr fetches `refs/merge-requests/<global-id>/head` and builds the worktree + diff (stats computed locally, Aone advertises none); meta / issue-context / fetch-diff resolve identity, Aone workitem evidence, and the diff via the a1 CLI. The four reader-backed subcommands and fetch-pr select the provider from the clone's remote (or an Aone host), so a GitHub clone is unchanged. Read-only this phase: pr-context / comment-status / presubmit have no Aone backing yet (the run degrades to context-unavailable), and --comment is refused on an Aone target. SKILL.md + code-review.md document the Aone target and the degradations. See docs/design/2026-08-15-review-aone-provider.md.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, with a concrete reviewer test plan. Problem: real and observed, not theoretical. Direction: aligned. #9096 built the provider abstraction explicitly for this target, and this PR exercises it. As a direction signal, claude-code's CHANGELOG shows the same area moving (GitLab MR URL support, non-GitHub remote detection, GitLab token handling) — non-GitHub review hosts are a live concern across the tooling, not a one-off internal need. Size: maintainer-authored, so the two-tier core gate is exempt. For the record: ~636 production lines, ~170 test lines, ~104 doc lines. All production logic lives inside the review skill ( Approach: the scope feels right — a vertical read-only slice (a1 transport + provider + detection + Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需段落齐全、双语，并附有具体的 reviewer 验证计划。 问题： 真实且已被观测到，不是理论性问题。 方向： 对齐。#9096 明确为该目标构建了 provider 抽象，本 PR 正是对其的验证。方向信号方面，claude-code 的 CHANGELOG 显示同一领域也在推进（GitLab MR URL 支持、非 GitHub remote 检测、GitLab token 处理）——非 GitHub 评审宿主是跨工具的共性需求，而非一次性的内部诉求。 规模： 作者为 maintainer，两级核心门禁豁免。记录在案：约 636 行生产代码、约 170 行测试、约 104 行文档。全部生产逻辑位于 review skill 内部（ 方案： 范围合理——只读垂直切片（a1 transport + provider + 检测 + 风险： 无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: for "make The GitHub regression surface is essentially nil: No critical blockers. Three non-blocking observations, none worth holding the PR:
sequenceDiagram
participant P1 as fetch-pr
participant P2 as registry
participant P3 as platform reader
participant P4 as git
P1->>P2: getPlatformReader with remote URL and host
P2->>P2: hint, host, remote, cwd origin, else GitHub
P2-->>P1: aone or github reader
P1->>P3: ensureAuthenticated
P1->>P4: fetch refspec from fetchHeadRefSpec
P1->>P3: getFetchMeta for PR metadata
P1->>P1: when not github, computeDiffStats off the captured diff
Test evidence — the PR's own CI (unattended run; no PR code executed here)At fetch time the Linux unit suite is still running; no check has failed. macOS/Windows matrix jobs are skipped for this PR (repo CI gating, not a PR defect), as are the gated bot lanes ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The new unit tests pin detection, URL grammar, and diff-stats counting, but the Aone-side integration (real 中文说明代码审查：先写独立方案再对照 diff——本 PR 与我的独立方案完全一致（a1 transport 作为 gh.ts 的姊妹模块、Aone provider 实现 reader 接口、registry 按 remote host 检测、fetch-pr 拉取 MR ref 并本地计算 diff stats、解析 codereview URL、拒绝 --comment），且有一处更优：检测内置于 getPlatformReader()（显式 hint → --host → remote URL → cwd origin → GitHub 的优先级），四个 reader 子命令零调用点改动即获得平台感知。 GitHub 回归面几乎为零：fetchHeadRefSpec 返回逐字节相同的 pull//head refspec，getFetchMeta 封装了原先内联的 gh pr view --json 调用，测试 mock 也相应放宽。computeDiffStats 计数正确（@@ 之前不计数，跳过反斜杠标记行）。新纯逻辑均有单测锁定。无关键阻塞项；三个非阻塞观察：a1 重试无 stderr 提示（gh.ts 有）、fetchDiff 硬编码 origin 且目标分支不可拉时退化为 ref~1（设计文档已知）、codereview URL 正则不限宿主（与 GHE 的宽松处理一致，最坏只是清晰报错）。 测试证据：无人值守运行，未执行任何 PR 代码。抓取时 Linux 单测套件仍在运行，无失败；macOS/Windows 矩阵与本 PR 无关地被跳过。作者关于 stale-bundle 预存失败的说明是其声明，未在此验证——若套件最终红，finalize 会扣住批准并标记。上方表格为真实 CI 状态。 沙箱验证通道：新单测锁定了检测、URL 文法与 diff 统计，但 Aone 侧集成（真实 a1 调用）只能从内网触达，描述中的 E2E 是作者本人的运行记录。可沙箱验证的一半可用 @qwen-code /verify 收口——A/B 运行可证明 fetch-pr 的 provider 改路由对 GitHub 克隆无行为变化（现有套件 mock 了 gh/git，绿色本身不证明端到端一致）。Aone 一侧没有任何通道可达，maintainer 的 E2E 即为记录在案的证据。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal second provider; only non-blocking nits, approval waiting on the Linux unit suite. Stepping back: this is phase 2 of a plan the repo already signed up for — #9096 merged the seam this morning explicitly so a second provider could prove it, and this PR is exactly that proof, delivered by the same author with a design doc that records the verified platform facts. The approach matches my independent proposal and beats it (registry-baked detection means zero call-site churn in the four reader subcommands). The GitHub path is byte-for-byte untouched in behavior — same refspec string, same The one thing I can't do from here is re-run the Aone E2E — it needs If I'm maintaining this in six months, the provider split plus the design doc's platform-fact table is exactly what I want to find. Approving with the three nits from my review left as follow-ups, none worth blocking. Approval deferred until CI lands green on 中文说明回顾全局：这是仓库已认可计划的第二阶段——#9096 今早合入接缝，明确就是为了让第二个 provider 来验证它；本 PR 正是同一作者交出的验证，且设计文档记录了已核实的平台事实。方案与我的独立提议一致且更优（检测内置于 registry，四个 reader 子命令零调用点改动）。GitHub 路径行为逐字节不变——同样的 refspec 字符串、同样的 gh pr view 调用，只是改经 provider 路由——这正是只读 Aone 路径可以安全落地的原因：新代码路径最坏只是清晰报错，且 --comment 在触碰任何东西之前就拒绝。 我在此无法复跑 Aone E2E——那需要内网的 a1 认证。该证据是作者（maintainer）本人对真实 CR 29295886 的运行记录，我按此权重对待：可信、与设计文档的探测数字一致、且受本 PR 所加只读守卫约束。新单测锁定了网络这侧所有可锁定的部分。 六个月后维护这段代码时，provider 拆分加设计文档的平台事实表正是我希望看到的样子。批准，三个非阻塞建议留作后续。Linux 单测套件仍在运行，批准推迟到 CI 全绿后由 finalize 任务提交（若有检查变红或 head 移动则扣住并标记）。 — 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.
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 8a": verify cleanup.ts auditPrWrites failure handling when a fetch-pr report records an Aone host ( host: code.alibaba-inc.com flows into setGhHost + gh api a….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 8a"：verify cleanup.ts auditPrWrites failure handling when a fetch-pr report records an Aone host ( host: code.alibaba-inc.com flows into setGhHost + gh api a…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Critical:
- match-remote: Aone CR URLs use the WEB host (code.alibaba-inc.com) while
a clone's remote uses the GIT host (gitlab.alibaba-inc.com) — treat them as
one equivalence class (hostsEquivalent) so a codereview URL matches its
clone's remote and the worktree flow is reachable; nested-group remotes
(group/subgroup/project) now collapse to the last two segments instead of
failing to match
- registry: an explicit non-Aone host/remote now beats the cwd probe, so an
explicitly-GitHub subcommand run from an Aone clone is not hijacked to
Aone; hint host is trimmed; the four reader-backed subcommands thread
--host into detection (previously dropped); dropped the unwired --platform
dead switch
- aone parseRemoteUrl: user-less scp remotes (ssh-config/insteadOf), nested
groups, a trailing slash after .git, and empty segments now parse; the
parse-failure message redacts a user:token@ origin (no credential leak)
- aone getCommentBody throws on a missing id (was an indistinguishable empty
string); fetchDiff uses gitRaw (512 MiB buffer, no CRLF rewrite, latin1)
instead of git() (1 MiB ENOBUFS, CJK byte loss)
Suggestions:
- aone-client: 120 s timeout, ENOENT branch in the auth check ("install the
a1 CLI"), TRANSIENT_RE anchored to HTTP 5xx (bare 502/503/504 misfired on
command lines containing those digits), one stderr trace line per retry
- fetch-pr: validate pr_number before Number() coercion (1e3 fetched PR
1000), trim --host before detection; submit guard trims --host;
comment-body --pr help notes the Aone per-MR requirement
- parse-args: nested-group codereview URL grammar; invalid-url warning names
both grammars
- SKILL.md + code-review.md: Aone paragraph corrected (clone-origin trigger,
Agent 0 skipped, test-plan/publish-assets unbacked, pass --host), design
doc updated (detection, Agent 0 gating)
- tests: aone.test.ts, registry cwd-mock + precedence + parseRemoteUrl cases,
remote-match hostsEquivalent + nested collapse, parse-args nested codereview,
submit-aone refusal
Critical: - parse-args: the Aone CR URL grammar is now constrained to Aone hosts (*.alibaba-inc.com) — a /codereview/ URL on any other host hits the fail-closed invalid-url refusal instead of becoming a live PR target (unlike …/pull/<n>, which any GHE host legitimately serves) - aone fetchDiff: merge-bases against a fetched target branch (not a present-but-stale origin/<target>), and the MR-head refspec is now force-fetched (+) so a stale throwaway ref from an interrupted run does not fail the fetch when the head was rewritten (normal AGit-Flow iteration) Suggestions: - fetch-pr: countDiffChangedLines now delegates to the single hunk-state walker in computeDiffStats (the two could not disagree silently); the changedFiles count is pinned on `diff --git` via a binary-file fixture - aone parseRemoteUrl scheme case made explicit + pinned (RFC 3986) - submit.test.ts pins the platform registry to GitHub so the Aone refusal guard neither spawns a real git in the vitest cwd nor couples to the machine's clone origin
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.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Critical: - registry: hostOfRemoteUrl now makes `user@` optional in the scp branch (user-less scp remotes from ssh-config/insteadOf no longer misroute an Aone clone to GitHub); the token-bearing scp userinfo parses to the host, not an owner - parse-args: the Aone CR-URL host group now requires a REAL subdomain dot boundary (`(?:[A-Za-z0-9-]+\.)+alibaba-inc.com`), so lookalikes (`evilalibaba-inc.com`) hit the fail-closed refusal; a `/pull/<n>` URL on an Aone host is refused too (Aone serves no /pull/ pages) - meta: on a non-GitHub platform an explicit `--repo` without `--host` is refused (no default host off GitHub) instead of emitting the contradictory `platform:aone` + `host:github.com` - aone fetchDiff: spreads PINNED_DIFF_CONFIG/PINNED_DIFF_FLAGS (an un-pinned color.diff=always zeroes computeDiffStats), discloses a failed target-branch fetch via a stderr WARNING, and refuses to diff from a clone of a different repo; scp-form userinfo is redacted in the parse-failure message Suggestions handled: - comment-body: the Aone per-MR `--pr` requirement is enforced before the auth gate (usage errors precede auth) - aone-client: the auth-failure diagnostic surfaces a1's real first stderr line (not the execFileSync preamble) and reports a timeout/kill distinctly - remote-match docstring + registry precedence comment updated to the implemented behavior Deferred to follow-up QwenLM#9194: the 16 test-gap patterns, headRefOid dead-field removal, MAX_SAFE_INTEGER digit guard, and the refusal-message host branch.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Critical:
- aone fetchDiff + fetch-pr merge-base fetch: the server-controlled
target/base branch reached `git fetch` bare — a dash-leading branch name
(creatable by full-refname push) parses as an option, so
`--upload-pack=<payload>` executed attacker-named code with the
reviewer's credentials. Pass `--` to end option parsing and refuse
dash-leading values outright on both providers
- meta: the no-default-host guard now gates on the FLAG, not the resolved
value — a GH_HOST export no longer bypasses it (and an empty-string
--host counts as missing); the whole --repo branch's pure resolution
moves above the auth gate (usage errors precede auth)
- skill: pass --host for EVERY pr-url target including github.com — an
omitted hint falls back to the cwd origin probe, which hijacked a
github.com review run from an Aone clone (and vice versa); lightweight
fetch-diff/pr-context carry the host too
- submit: the Aone refusal moves BELOW the authorisation gate and takes
the exit-3 + {"posted": false} shape instead of throwing — an
unauthorised Aone run now ends as the skill's contract defines, and
detection reads the effective host (flag → GH_HOST), so an Aone-pointing
GH_HOST export is refused instead of dying opaque inside gh
Suggestions handled:
- parseRemoteUrl: strip query/fragment (credential channel into repo
identity), fix the cleaning order for two-plus trailing slashes after
.git, and discard an explicit port instead of folding it into the path
- registry: isAoneHost normalizes the trailing-dot FQDN spelling; the cwd
probe delegates to lib/git's gitOpt (shared git policy)
- aone: the MR-head refspec is stated once (mrHeadRefSpec); resolveRepo
quotes git's real error line, not the execFileSync preamble
- aone-client: the auth fall-through message is neutral (covers
non-auth failures the login hint cannot fix)
- fetch-pr: pr_number guard tightened to ^[1-9]\d*$ (no PR zero, no
leading zeros, no side effects before the refusal)
- the five detection-consuming subcommands' --host describes now state
the implemented semantics; SKILL.md/code-review.md read-only phrasing
corrected and the false "detection reads the clone's remote, not the
URL" claim fixed
Tests: dash-leading refusal on both providers, meta guard flip tests
(GH_HOST bypass, empty flag, pre-auth), submit exit-3 shape (authorised,
unauthorised, padded host, GH_HOST), trailing-dot and /pull/-on-Aone
parse refusals, port/query/slash parse cases, fetch-pr zero-number and
base-ref refusals.
Deferred to follow-up QwenLM#9194: the single-branch merge-base disclosure
(R3-9), cleanup audit skip-in-code (R3-13), URL-form --remote hint
(R3-19), publish-assets refusal parity (R3-22), and the data-path
deadline translation (R3-25).
The round-4 fix rewrote the skill's --host notes (pass --host for every pr-url target, github.com included); three revert-guard tests pinned the old 'add --host <host> for Enterprise' phrasing and reddened the core suite in CI. Repin them at the new wording.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 4 and 5 still reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- aone resolveRepo: redactUrl now strips the query/fragment channel too — a ?private_token=… origin carries no @ for the userinfo redaction, so the parse-refusal message echoed the secret the success path strips (test pins the refusal message secret-free) - aone fetchDiff: the merge-base fallback (base = ref~1) DISCLOSES via a stderr WARNING — previously silent, a multi-commit MR got only its last commit served as the complete diff (shallow/single-branch clones hit this; the GitHub path is loud about the same class) - submit: the Aone write-refusal binds the platform in BOTH directions — the authorisation gate now surfaces the recorded target's host, so a recorded Aone host refuses whatever the runtime-effective host resolves to (an ambient GH_HOST export can no longer steer an Aone review into posting at a same-named repo), while a recorded non-Aone pr-url binding is no longer vetoed by the cwd probe from an Aone-origin clone Tests: refusal-message redaction, fallback disclosure (spy calls captured before mockRestore — vitest's restore clears them), bidirectional refusal arms (recorded-Aone + GHE env refuses; recorded-github + Aone cwd posts).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none (finished within budget). No Budget gap line.I've completed my walk of chunk 3. Final assessment:; "agent reverse-audit (round 4)": none — nothing was cut short.; "agent reverse-audit (round 4)": none — no check was cut short..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none (finished within budget). No Budget gap line.I've completed my walk of chunk 3. Final assessment:；"agent reverse-audit (round 4)"：none — nothing was cut short.；"agent reverse-audit (round 4)"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- authorization: the --user-authorized fast path now surfaces the recorded target's host too (best-effort read of the recorded args) — it returned before the args file was read, so recordedHost was always undefined on that path and the 'a recorded Aone host always refuses' invariant leaked: a user-authorised post of a recorded Aone codereview review from a non-Aone cwd with no --host/GH_HOST posted at github.com's same-named repo. Tests pin the fast-path host through the REAL gate and the end-to-end refusal (the witness scenario) - aone: the query/fragment strip now uses [\s\S]* in both redactUrl and parseRemoteUrl — git stores newline-bearing remote URLs, and a plain . stopped at the first \n, letting ?private_token=SECRET\nx smuggle the token past the strip into the parse-refusal message. Tests cover both the parse-success and refusal paths of the smuggle Round-6 is Critical-only per the ~5-round policy (user-confirmed for convergence); the 13 Suggestions are deferred to follow-up QwenLM#9194.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — full packages/cli unit suite exceeded build-test's per-command deadline; the PR's own test files ran green scoped (285/285 review tests + SKILL.test.ts 16/16), full build green in all 18 workspaces.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": empirical two-clone verification that git fetch origin -- <branch> updates refs/remotes/origin/<branch> — the daemon shell guard denied the mutating probe t….
中文说明
未审查：reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — full packages/cli unit suite exceeded build-test's per-command deadline; the PR's own test files ran green scoped (285/285 review tests + SKILL.test.ts 16/16), full build green in all 18 workspaces。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：empirical two-clone verification that git fetch origin -- <branch> updates refs/remotes/origin/<branch> — the daemon shell guard denied the mutating probe t…。
— qwen3.8-max via Qwen Code /review (v0.21.11)
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.
Unresolved, please confirm:
- [Critical] R2-1 (aone.ts:298 — posted by the author mid-run, after this review's blocker baseline): concurrent-fetchDiff throwaway-ref contention — not independently re-verified end-to-end this round
- [Critical] R2-2 (aone.ts:293 — posted mid-run): colon/plus refspec channel through the dash-only target/base-ref guards — independently CONFIRMED by this run's probe (planted/force-updated local refs, flip-verified); unresolved at HEAD
- [Critical] R2-3 (aone.ts:127 — posted mid-run): ?/# inside userinfo truncates redactUrl before the userinfo redaction — not independently re-verified this round
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally).
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none — all checks above ran to completion within budget..
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未决，请确认：共 3 条（原文未翻译，列表见上方英文部分）。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (the same unit suites ran green on Linux locally)。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：none — all checks above ran to completion within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- fetchDiff's throwaway ref now carries a pid suffix — two concurrent runs for the same MR in one clone shared the name: one session's finally- delete killed the other mid-review (unknown revision), and a pre-existing local branch of the reserved name was force-moved then deleted, reflog and all (race probe: 12/60 failures → 0 with the per-run unique name) - the target/base-ref guards close the refspec channel the dash-only check left open after `--`: a leading `+` parses as a force refspec (fetches the wrong head — stale evidence, no WARNING) and a colon as src:dst (force-moves the throwaway ref or a reviewer-local branch). Both providers now refuse '-', '+', and ':' shapes (probe-confirmed on real fetchDiff incl. the served-wrong-diff and local-branch-overwrite witnesses); tests pin the new channels on both guards - redactUrl and parseRemoteUrl clean userinfo BEFORE the query/fragment strip: a userinfo that itself contains '?' or '#' was truncated mid-credential, leaking the username+secret prefix into the refusal message and making parseable origins unparseable (flip-verified on the witness shapes) Round-7 is Critical-only per the convergence directive; the 8 Suggestions (incl. the 4 bot findings) are deferred to QwenLM#9194.
# Conflicts: # packages/cli/src/commands/review/fetch-pr.test.ts # packages/cli/src/commands/review/fetch-pr.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the same unit suites ran green on Linux: packages/cli review tests and packages/core SKILL tests all pass; the failing suites are measured pre-existing on the merge base).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (rounds 3, 4, and 5 still reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the same unit suites ran green on Linux: packages/cli review tests and packages/core SKILL tests all pass; the failing suites are measured pre-existing on the merge base)。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- the server-controlled branch-name guards now validate ALLOWLIST-style on both providers (aone.fetchDiff's target, fetch-pr's baseRefName): the denylist admitted HEAD (silent fetch + merge-base through the stale clone-time symref), rev-parse metasyntax (wrong base under a misdescribing warning), ranges, and the empty string (garbled diff-less fallback) — a plain-branch-name shape closes every channel - parseRemoteUrl/redactUrl consume userinfo GREEDILY up to the last @ of the authority — multi-@ and :-/-bearing token userinfo no longer leaks cleartext residue through the refusal messages or folds into the parsed host (take() fails closed on any surviving @); the scp strip admits only a removal that leaves a host: shape behind - fetch-pr's Aone stats backfill moves AFTER the plan/rescue, where diffText is final — the partition-rescue republishing the full range no longer leaves delta-scoped numbers beside a full-range diffPath — and isCollapsedFromUpstream is skipped when the stats are locally derived (one source, not two: the disclosure needs an independent advertised fact, and a delta-scoped round beside the full-range count fired a false collapse) - remote identity is injective again: Aone nested-group targets carry the full group path (parse-args → match-remote --group-path → matchRemotes compares every segment when both sides have three or more), and fetchDiff's origin guard adds the origin's host (Aone family) — a same-named repo in another group or on another platform can no longer pass either gate; SKILL.md passes --group-path for nested targets - meta's discovery branch drops GH_HOST inheritance off GitHub — an ambient GHE export beside an Aone-origin clone no longer vetoes the valid invocation at HOSTNAME_RE; only an explicit --host steers routing Round-8 is Critical-only per the convergence directive; all five findings fixed, no deferrals this round.
# Conflicts: # packages/cli/src/commands/review/fetch-pr.test.ts # packages/cli/src/commands/review/fetch-pr.ts
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— deepseek-v4-flash via Qwen Code /review (v0.21.11)
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 (the same unit suites ran green on Linux: all 8 changed review test files + SKILL.test.ts 20/20; the 93 failing tests across 14 files were measured pre-existing on the merge base via base-tree + test-delta).
Not explored to full depth (tool budget reached): chunk 4: none — the only check not run was npx vitest run on the file itself (worktree has no node_modules ), which I substituted with verbatim execution of the revie…; "agent reverse-audit (round 1)": none — I completed every check I opened; nothing was cut short at the ceiling..
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the same unit suites ran green on Linux: all 8 changed review test files + SKILL.test.ts 20/20; the 93 failing tests across 14 files were measured pre-existing on the merge base via base-tree + test-delta)。
未探索到全部深度（达到工具调用预算）：chunk 4：none — the only check not run was npx vitest run on the file itself (worktree has no node_modules ), which I substituted with verbatim execution of the revie…；"agent reverse-audit (round 1)"：none — I completed every check I opened; nothing was cut short at the ceiling.。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- redactUrl is fail-closed BY CONSTRUCTION: split at the last @, redact
everything before it — the per-regex redaction kept missing shapes
(round-9: URL userinfo with a / in the secret, scp userinfo with a
newline, residues with no host: shape all leaked verbatim through the
parse-refusal message)
- parseRemoteUrl cleans per form and fails CLOSED: URL-form userinfo is
bounded to the authority (greedy within it — multi-@ and ?/# inside
secrets consumed whole, /-bearing secrets left to fail closed in take),
scheme inputs never fall through to the scp grammar (a malformed
https://user:pa/ss no longer parses host user); the round-8 scp-strip
firing on scheme URLs fabricated coordinates from query-borne and
path-borne @ witnesses — all witnesses now parse correctly or refuse
- registry hostOfRemoteUrl consumes token-bearing userinfo (':' AND '/'
in the secret) on both branches, mirroring aone.parseRemoteUrl —
detection no longer parses the credential prefix as the host and
misroutes Aone clones to GitHub; detectPlatformKind ranks an explicit
--host above the remote-URL hint in BOTH directions (an Aone origin
can no longer hijack an explicitly-GitHub invocation into fetching a
global MR id from the wrong remote)
- nested-group identity is injective in both directions: matchRemotes
compares the full group path exactly whenever the target carries one
(any length — a 3+-segment target no longer matches a two-segment
remote sharing its tail, nor the reverse); Aone CR targets carry the
path even at two segments and the canonicalized URL keeps the full
path; fetchDiff's origin guard compares the origin's full path against
the MR's own detailUrl path (authoritative repo identity, where the
seam's ownerRepo is collapsed); the rescue pool keys on the full path
and same-id cross-group CR URLs are refused as ambiguous
Round-9 is Critical-only per the convergence directive; the 8
Suggestions (R8-6..R8-13) are deferred to follow-up QwenLM#9194.
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the same unit suites ran green on Linux: all changed review test files pass; the failing suites were measured pre-existing on the merge base via base-tree + test-delta).
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/fetch-pr.test.ts:519 — [review] the new pr_number test uses '0' (already rejected by the pre-existing guard); the added ^[1-9]\d*$ guard's unique '01' reach is untested — flip-provenpackages/cli/src/commands/review/lib/remote-match.ts:182 — [review] bare-number Aone flow cannot supply --group-path (MetaResult drops it); bounded fail-closed harm (exit-7 stop / collapsed a1 --repo coordinate)packages/cli/src/commands/review/match-remote.ts:99 — [review] exit-6 stderr still states the pre-PR owner/repo rule when the full-path comparison rejectspackages/cli/src/commands/review/parse-args.ts:256 — [review] two-or-more trailing dots bypass the /pull/-on-Aone refusal and submit isAoneHost(recordedHost) arm (downstream fails loudly; one-line fix flip-verified)packages/cli/src/commands/review/lib/platform/aone.test.ts:501 — [review] fallback-range assertion uses two independent \d+ quantifiers — does not pin base and head as the SAME throwaway ref; backreference fixpackages/cli/src/commands/review/parse-args.ts:535 — [review] targetKey carries case-preserving groupPath and port-bearing host while every identity gate normalizes — identity-equivalent CR spellings trip a factually-wrong ambiguity refusal…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally (the same unit suites ran green on Linux: all changed review test files pass; the failing suites were measured pre-existing on the merge base via base-tree + test-delta)。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
- aone.fetchDiff's host arm keys on the CANONICAL Aone-family predicate (new remote-match isAoneHostFamily: port/trailing-dot/case normalized; registry.isAoneHost now delegates to it) — a trailing-dot FQDN clone that detection accepts as Aone can no longer be refused by the diff gate with a misdirecting remedy - the URL cleaning/redaction class is closed structurally, not per shape (sixth consecutive round a new entrance was found): parseRemoteUrl's URL-form userinfo is consumed whole WITHIN the authority (span between // and the first /), and the scp-form userinfo strip + its lookahead are bounded at ?/# — an @ inside a query or fragment value is the credential's own character and can no longer fabricate coordinates from the query tail; redactUrl fails the DISPLAY closed with a constant when the last @ sits after a ?/# marker — the token tail can no longer reach the refusal message (URL/scp/fragment witnesses all pinned) - isPlainBranchName rejects git's pseudo-ref set (FETCH_HEAD/ORIG_HEAD/ MERGE_HEAD/…) on both guards — FETCH_HEAD resolves to the just-fetched PR head (empty diff beside full-range metadata), ORIG_HEAD to an arbitrary ancestor; both shape-legal, both silently wrong - fetch-pr's merge-base probe requires the fetch to have produced the tracking ref — a tag-only baseRefName exits 0 writing only FETCH_HEAD, and the bare-name fallback once merge-based against the reviewer's local tag with baseFetchFailed falsely false; the tag shape now lands in the disclosed state - parse-args: the repo-qualified CR URL outranks a same-number bare spelling as the target in BOTH the rescue pool and positional order — the bare number carries no host, and letting it win flipped detection onto the cwd fallback (a loud refusal at the merge base had degraded to a silent wrong-platform retarget); bare restatements of the URL target are skipped silently, matching the rescue loop's restatement handling Round-10 is Critical-only per the convergence directive (the bot's own ledger is at its round cap); the 6 convergence-posture deferrals named in the review body join follow-up QwenLM#9194.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI (the same unit suites ran green on Linux; the failing suites were measured pre-existing on the merge base via base-tree + test-delta).
Not reviewed: build-and-test — whole-call budget ran out before packages/vscode-ide-companion, packages/web-shell, packages/webui suites ran.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/platform/aone.ts:38 — [review] six unread interface fields beside the '(the fields we read)' comment (AoneMrView title/author/state, AoneWorkitemRef subject/link, AoneWorkitem id)packages/cli/src/commands/review/lib/platform/aone.ts:378 — [probe] fetchDiff origin guard fallback arm compares case-preserving owner/repo while the detailUrl arm compares lowercased paths (probe-flipped)packages/cli/src/commands/review/lib/platform/aone.ts:464 — [review] ref~1 merge-base fallback dies with 'fatal: unknown revision' on root-commit MR headspackages/cli/src/commands/review/match-remote.ts:148 — [review] --group-path handler→matcher wiring untested; a wiring slip silently degrades nested-group matching to the non-injective last-two collapsepackages/cli/src/commands/review/parse-args.ts:625 — [probe] positional hoist moves a PR URL ahead of ANY preceding positional — file targets demoted and --comment flips effective (executed before/after)packages/cli/src/commands/review/lib/authorization.ts:246 — [probe] write gate binds the recorded host with raw equality — web/git aliases (code ≡ gitlab) produce the wrong refusal class (probe through the real gate, flip via hostsEquivalen…packages/cli/src/commands/review/lib/platform/aone.ts:297 — [review] getPrMeta/getClosingIssues/getIssue never called by any test — workitem argv and field-fallback mutations stay greenpackages/core/src/skills/bundled/review/SKILL.md:103 — [review] Step-9 bypass-audit bullet's '(host null)' parenthetical is false for skill-compliant Aone runs — key the skip on the host family, not nullpackages/cli/src/commands/review/lib/remote-match.ts:40 — [probe] hostsEquivalent diverges from isAoneHostFamily (trailing dot, port, membership set) — dotted-FQDN same-repo origins exit 6 (executed, flip-verified)packages/cli/src/commands/review/meta.test.ts:70 (+4 locations) — [probe] new cwd-origin probe couples four untouched suites (meta/issue-context/fetch-diff/comment-body tests) to the checkout's origin — from Aone-origin clones they die in r…packages/cli/src/commands/review/parse-args.ts:588 — [probe] CR URL discarded as an invalid flag value beside a same-number bare positional loses platform identity — detection flips to the cwd fallback (executed)packages/cli/src/commands/review/parse-args.ts:158 — [probe] AONE_CR_URL_RE refuses trailing-dot FQDN spellings the /pull/ arm and detection normalize (executed asymmetry)packages/cli/src/commands/review/submit.ts:499 — [probe] Aone refusal's env/--host arm suppressed whenever recordedHost is defined — Aone-pointing GH_HOST/--host evades the exit-3 shape (probe arms + flip; severity downgraded from the filed…packages/core/src/skills/bundled/review/SKILL.test.ts:397 — [probe] 422 drift-re-check pin relaxed from exact parenthetical to prefix match — tail truncation mutation stays 20/20 green (executed; sibling pin fails the same mutation)packages/cli/src/commands/review/lib/platform/aone.ts:122 — [probe] case-sensitive .git suffix strip diverges from remote-match normalization — .GIT origins get wrong a1 coordinates and the origin guard refuses the legitimate clone (execute…packages/cli/src/commands/review/lib/platform/aone.test.ts:425 — [probe] latin1 'byte-faithful' test uses an ASCII fixture — a utf8 regression ships 34/34 green and corrupts every non-ASCII diff (mutation executed)packages/cli/src/commands/review/lib/platform/aone.ts:276 — [probe] resolveRepo never binds the origin host to the Aone family — meta can emit platform:'aone' beside host:'github.com' (probe 3/3)packages/cli/src/commands/review/lib/platform/aone.test.ts:355 (+4 locations) — [probe] pattern: mock seams answer regardless of argv — 4 unpinned sites (getCommentBody argv/note precedence, fetchDiff merge-base origin/ prefix, fetch-pr pro…packages/cli/src/commands/review/lib/platform/aone.test.ts:452 (+3 locations) — [probe] pattern: new guard/flow orderings pinned by zero tests (target-fetch-failure disclosure, Aone refusal × dryRun:true, refusal probe host-hint — hint-drop…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI (the same unit suites ran green on Linux; the failing suites were measured pre-existing on the merge base via base-tree + test-delta)。
未审查：build-and-test — whole-call budget ran out before packages/vscode-ide-companion, packages/web-shell, packages/webui suites ran。
收敛姿态下延后（第 10 轮，非阻断）——已记录，本轮不要求修改：共 19 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
- the URL cleaning/redaction surface is closed STRUCTURALLY: one parser, one source of truth — registry.hostOfRemoteUrl now delegates to the canonical aone.parseRemoteUrl (detection and the identity parser can no longer disagree), and the scp branch reads GIT'S OWN grammar (GIT_TRACE-probed: hostinfo ends at the FIRST ':', userinfo carries no ':' or '/') — the last-'@' consumption once parsed a different host than git connects to, letting fetchDiff's same-repo guard pass while git fetched from another server; token-bearing scp shapes now fail closed, and the round-8 detection tests are re-blessed onto shapes git reads that way - the pseudo-ref allowlist is CASE-INSENSITIVE on both twins: on case-insensitive filesystems (macOS/Windows defaults) fetch_head folds onto FETCH_HEAD, resolving the merge-base to the just-fetched MR head (empty diff beside full-range metadata); lowercase spellings refused, pinned - submit.test.ts's file-level setup now saves/clears/restores GH_HOST — the Aone refusal reads the ambient env, and the org's standard intranet export pattern (an Aone-family host) turned 50 of 69 posting tests into refusals - the --user-authorized fast path binds the recorded host to THIS write (same-PR number only — a stale recording of another PR must not supply a host) and scans SIBLING session recordings when the session-scoped args file is absent — the characteristic cross-session publish shape otherwise lost the host and posted a recorded Aone review at github.com's same-named repo (real-gate witness: exit 0, COMMENT filed); tests drive the real gate through a sibling-session fixture - the tracking-ref requirement and both merge-base sites are FULLY QUALIFIED (refs/remotes/…): git resolves unqualified origin/<name> in refs/tags and refs/heads first, so a tag or branch literally named origin/<baseRefName> — a PUSHABLE, server-controlled refname a plain clone auto-carries — shadowed the just-fetched tracking ref and moved the merge base with no disclosure; shadow-tag tests pinned on the resolveMergeBase probe, the fetch-pr seam, and aone.fetchDiff Round-11 is Critical-only per the convergence directive; the bot's own ledger is at its round cap and this round still produced findings — recommend freezing the bot loop and moving to human security 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.
Not reviewed: reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/fetch-pr.ts:803 (+12 locations) — [review] untested new behaviour across the Aone surface (fetch-pr Aone path, a1 transport, reader methods, --host threading, pre-auth guard, --group-path wiring, argv pins, …packages/cli/src/commands/review/lib/remote-match.ts:40 (+5 locations) — [review] host/path normalization diverges between identity gates (hostsEquivalent trailing-dot, new-URL userinfo ?, scp ?# strip, .GIT case, fetchDiff fallback case)packages/cli/src/commands/review/plan-diff.ts:184 (+5 locations) — [review] --host sweep incomplete: stale plan-diff/submit describes, unswept pr-context SKILL block, four rewritten notes unpinned, SKILL.md:99 host spelling vs the raw gate …packages/cli/src/commands/review/parse-args.ts:535 (+2 locations) — [review] dedupe targetKey raw host/case axes split one PR into two -> false ambiguity refusal + silent local-tree fallbackpackages/cli/src/commands/review/fetch-pr.ts:660 — [review] new pr_number guard unreachable for its cited cases; the leading-zero lease-bypass delta is untestedpackages/cli/src/commands/review/lib/platform/aone-client.ts:74 — [review] no non-retrying executor for the future write phase — a retried POST could duplicate the reviewpackages/cli/src/commands/review/submit.ts:502 — [review] defined non-Aone recordedHost suppresses the flag/env refusal arm -> opaque gh failure instead of the exit-3 shapepackages/cli/src/commands/review/lib/authorization.ts:273 — [review] slow-path host binding compares raw strings vs hostsEquivalent -> wrong refusal class for the aliased Aone hostspackages/cli/src/commands/review/lib/platform/aone.ts:481 — [review] ref~1 merge-base fallback crashes on parentless MR heads (orphan/shallow) instead of degradingpackages/cli/src/commands/review/meta.ts:97 — [review] meta discards RepoIdentity.groupPath -> bare-number flow can never pass --group-path (spurious exit 7)packages/cli/src/commands/review/fetch-pr.ts:582 — [review] GIT_PSEUDO_REFS misses stash -> bare fallback can merge-base against the reviewer's refs/stashpackages/cli/src/commands/review/fetch-pr.ts:733 — [review] URL-valued --remote loses the detection hint and can never satisfy the tracking-ref requirementpackages/cli/src/commands/review/lib/merge-base.ts:56 — [review] baseFetchFailed conflates fetch failure with refspec-not-stored (single-branch clones) -> false WARNINGpackages/cli/src/commands/review/lib/platform/aone.ts:187 — [review] redactUrl's path-only-@ class emits the tail contrary to its docstring -> token in the refusal messagepackages/cli/src/commands/review/lib/authorization.ts:249 — [review] write gate binds collapsed owner/repo only; groupPath never threaded (live-wrong once the write phase lands)packages/cli/src/commands/review/parse-args.ts:643 — [review] positional duplicate restatements warn while their flag-value twins are blessed silentlypackages/cli/src/commands/review/lib/platform/aone.ts:490 — [review] throwaway __qwen-review-diff-* branches have no crash-left-behind sweeper (finally-only cleanup)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the round cap of 5 without converging (all five rounds reported new findings)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 11 轮，非阻断）——已记录，本轮不要求修改：共 17 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
- the recorded-args host lookup is HARDENED — the store lives under
.qwen/tmp/ beside review worktrees checked out from the PR's own tree,
so its content is attacker-influenceable: only s-* session directories
are scanned (a malicious PR can no longer plant a root-level args file
that binds a host), symlinks are skipped at both the directory and file
levels (mirroring writeSkillArgs' O_NOFOLLOW write-side policy), reads
are size-bounded, and the host binds only when the recording names the
same PR number AND the same repo
- the canonical Aone invocation shape (bare global MR id, no URL) can no
longer post cross-session without host evidence: a same-number
recording with no host binds the recorded --host flag when present
(parse-args now records it), and without one the write gate FAILS
CLOSED with the exit-3 shape and names the remedy — instead of posting
the review at github.com's same-named repo (the probe-verified witness
once exited 0 and POSTed)
- parse-args: the URL-outranks-bare-number invariant now holds for MIXED
shapes — a positional bare number restating the rescue pool's single
PR is carved out of hasValidCandidate, so --effort <cr-url> 7 (and
both orderings/equals-form) target the CR URL instead of silently
retargeting onto the cwd clone's same-number PR; a different number
still outranks
- aoneReader.resolveRepo refuses an origin outside the Aone host family —
an explicit --host can steer detection onto this reader while the cwd
clone is a GitHub mirror (the dual-remote migration setup), which once
emitted {platform:'aone', host:'github.com'} and queried a1 with the
mirror's coordinates; same predicate fetchDiff's origin guard applies
- isPlainBranchName (both twins) rejects refs/-prefixed names: legal
branch names (check-ref-format --branch) that resolve qualified refs
the server controls as fetch/merge-base arguments (refs/remotes/origin/
HEAD is the clone's default-branch symref — wrong base, misdescribing
WARNING)
- the ref-dwim class is closed at the verified sites: fetch-pr's base
probe fetches an EXPLICIT branch refspec (bare names dwim onto
same-named tags — exit 0, tracking ref untouched, stale base passing
the freshness guard it never refreshed), its fetchedSha/merge-base head
reads are refs/heads-qualified (a planted same-name tag can no longer
shadow the real head), and aone.fetchDiff's target fetch + merge-base +
diff-range reads are qualified the same way
Round-12 is Critical-only per the convergence directive (the bot's own
ledger is past its round cap).
# Conflicts: # packages/cli/src/commands/review/fetch-pr.test.ts
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
|
Released in v0.21.14. |
4 participants
What this PR does
Adds an Aone Code read path to
/review— the second provider behind the review-platform seam introduced in #9096. A clone whose origin is ongitlab.alibaba-inc.com(or a…/codereview/<id>URL) is detected from the remote, and the existing read subcommands (meta,issue-context,fetch-diff) plusfetch-prwork unchanged, backed by a newa1-CLI transport instead ofgh. The target number is the global MR id;fetch-prfetchesrefs/merge-requests/<id>/headinto the review worktree and computes the diff stats locally (Aone does not advertise them). The four reader-backed subcommands andfetch-prselect the provider from the clone's remote (or an Aone host), so a GitHub clone's behavior is untouched.New files:
lib/platform/aone-client.ts(a1 transport),lib/platform/aone.ts(the provider),lib/platform/registry.ts(detection). Modified:fetch-pr.ts(provider-routed refspec + metadata),parse-args.ts(Aone CR URL grammar),submit.ts(refuse--commenton Aone),SKILL.md+code-review.md(Aone target handling).Why it's needed
/reviewwas GitHub/gh-hardcoded. MaxCompute's engine repo (odps_src) is reviewed on Aone Code, an internal AGit-Flow platform, so the skill could not review those CRs at all. #9096 built the seam; this is the second provider (the one the seam was designed for), which both proves the abstraction and makes local review of a MaxCompute CR work.Reviewer Test Plan
How to verify
Inside an
odps_srcclone (authenticateda1):qwen review meta→{"platform":"aone","host":"gitlab.alibaba-inc.com","ownerRepo":"maxcompute/odps_src"}.qwen review meta <global-id>→ the MR's head SHA (matchesa1 repo mr view <id>sourceBranch) and thecodereview/<id>web URL.qwen review issue-context <global-id> --repo maxcompute/odps_src --out <f>→ the linked Aone workitem's title/description in the evidence file.qwen review fetch-diff <global-id> --repo maxcompute/odps_src --out <f>→ the unified diff.qwen review fetch-pr <global-id> maxcompute/odps_src --remote origin --out <f>→ fetches the MR ref, builds the worktree, and reports locally-computed diff stats.qwen review <aone-cr> --commentis refused with a clear read-only message.Evidence (Before & After)
Before: no Aone support — the skill could only target GitHub. After: E2E against real
odps_srcCR 29295886 —metareturnsplatform:aonewith the correct head SHA;issue-contextfetches Aone workitem #83413579;fetch-diffproduces the 51-file diff;fetch-prbuilds the worktree and reports51 files, +2930/-114, 13 review chunks.Tested on
Environment (optional)
Local build (
node dist/cli.js) against a realodps_srcclone witha1authenticated. Unit tests:packages/cli/src/commands/review/lib/platform/registry.test.ts, plus new cases inparse-args.test.tsandfetch-pr.test.ts.Risk & Scope
pr-context,comment-status, andpresubmithave no Aone backing yet; on an Aone target the run degrades to the existing context-unavailable mode (verdict caps atCOMMENT, findings still generated;issue-contextstill fetches the linked workitem evidence standalone, but Agent 0 (issue fidelity) is skipped on Aone this phase).--commentis refused. Documented in SKILL.md + code-review.md.fetchHeadRefSpec,getFetchMeta) thatfetch-prnow routes through.workitem getdoesn't return them); thea1-based diff-stats are computed locally;fetch-diff's byte fidelity is realized only if downstream readers also use a byte-safe read (tracked in chore(review): close the mutation-verified test-pin gaps from PR #9096 review rounds 5-6 #9194).stale-bundle) is pre-existing onmain(verified by stashing this change) and unrelated.Linked Issues
Closes the Aone-adaptation gap tracked in the design doc
docs/design/2026-08-15-review-aone-provider.md; builds on #9096. Follow-up hardening items remain in #9194.中文说明
本 PR 做了什么
为
/review增加 Aone Code 读路径——#9096 引入的 review-platform 接缝后的第二个 provider。origin 在gitlab.alibaba-inc.com的克隆（或…/codereview/<id>URL）会从 remote 检测出平台，现有读子命令（meta/issue-context/fetch-diff）与fetch-pr无需改动即可工作，底层由新的a1CLI transport 替代gh。目标编号是全局 MR id；fetch-pr拉取refs/merge-requests/<id>/head进 review worktree，diff stats 本地计算（Aone 不报）。四个 reader 子命令与fetch-pr从克隆 remote（或 Aone host）选择 provider，GitHub 克隆行为不变。新文件：
lib/platform/aone-client.ts（a1 transport）、lib/platform/aone.ts（provider）、lib/platform/registry.ts（检测）。改动：fetch-pr.ts（provider 路由 refspec + 元数据）、parse-args.ts（Aone CR URL 文法）、submit.ts（Aone 拒绝--comment）、SKILL.md+code-review.md（Aone 目标处理）。为什么需要
/review此前 GitHub/gh硬编码。MaxCompute 引擎仓（odps_src）在 Aone Code（内部 AGit-Flow 平台）上评审，skill 完全无法评审这些 CR。#9096 建了接缝；本 PR 是第二个 provider（正是接缝为之设计的），既验证了抽象，也让本地评审 MaxCompute CR 成为可能。验证
在
odps_src克隆内（a1已认证）：qwen review meta返回platform:aone；meta <全局id>返回正确 head SHA 与 codereview URL；issue-context抓到 Aone workitem；fetch-diff出 unified diff；fetch-pr建 worktree 并报本地算的 diff stats。GitHub 克隆行为不变。--comment在 Aone 上被拒绝。已对真实 CR 29295886 E2E 通过（meta返回platform:aone；issue-context抓到 workitem #83413579；fetch-diff出 51 文件 diff；fetch-pr建 worktree 报51 files, +2930/-114, 13 review chunks）。风险与范围
pr-context/comment-status/presubmit暂无 Aone 支持，Aone 目标降级为既有 context-unavailable 模式（verdict 上限 COMMENT，findings 仍产出；issue-context仍独立抓取关联工作项证据，但 Agent 0（issue fidelity）本阶段在 Aone 上被跳过）；--comment被拒绝。已在 SKILL.md + code-review.md 说明。fetch-pr路由用的方法外未动。fetch-diff字节保真需下游也用字节安全读取才完整（在 chore(review): close the mutation-verified test-pin gaps from PR #9096 review rounds 5-6 #9194 跟踪）。stale-bundle）在main上即失败（已用 stash 验证），与本 PR 无关。关联 Issue
闭合设计文档
docs/design/2026-08-15-review-aone-provider.md跟踪的 Aone 适配缺口；构建于 #9096 之上。后续加固项仍在 #9194。