Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 51 additions & 9 deletions docs/design/2026-08-13-review-platform-provider-abstraction.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ Everything below was confirmed by running the commands, not from docs.
| Whoami | `gh api user --jq .login` | `a1 auth whoami -f json` → `account` |
| Repo identity for bare numbers | `gh repo view --json owner,name,url` | remote URL path (`group/repo`) + `a1 repo view`; `a1 repo link` binding if present |

Post-publication addendum (2026-08-21): the `Inline comment (write)` row
above creates comments that read back `isAiComment: false` — there is no
auto-marking for the posting identity and a1 exposes no flag to request it
(Q4, resolved by a controlled probe — see the open questions section).
Created comments join the `discussion` gate only, never the `ai_comment`
gate.

## Goals / non-goals

**Goals**
Expand Down Expand Up @@ -187,9 +194,13 @@ id directly; no id↔iid mapping is needed anywhere in the pipeline.
The merge gate already blocks on unresolved discussions, so inline Critical
comments left unresolved carry the blocking semantics. This is a semantic
difference from GitHub and is called out in the terminal report.
- AI-comment marking: probe whether `comment create` sets `isAiComment`
automatically or needs a flag; qwen-posted comments SHOULD carry it, because
Aone has a dedicated `ai_comment` merge gate. (Open question Q4.)
- AI-comment marking: **probed 2026-08-21 (Q4 resolved — see the open
questions section).** `comment create` does NOT auto-set `isAiComment` for
the posting identity, and a1 (v0.1.90) has no flag to request it, so
qwen-posted comments join the generic `discussion` gate only — the
dedicated `ai_comment` merge gate does not track them. Until a1 ships a
marking flag, `submit`'s REQUEST_CHANGES note discloses the gate split;
the marking itself is blocked on the a1 feature request.

### D7 — One-commit CRs and the incremental cache

Expand Down Expand Up @@ -357,6 +368,22 @@ Enterprise paragraph.
documented for the user. Still open: dedup/self-PR backing for Aone,
`composeUrl`, cleanup audit, AI-comment marking (Q4), the
render-adjudication carve-out.
- **AI-gate probe (2026-08-21, issue #9614):** Q4 was resolved by a
controlled write probe on a scratch CR — `comment create` auto-sets
NOTHING (both a general and an inline probe read back
`isAiComment: false`, re-checked against an async classifier), and
v0.1.90 exposes no marking flag — and Q3 was re-confirmed (still no
native reject; `mr comment resolve` and `mr cr list` are new on the
surface). Since the marking cannot be requested today, the write path
DISCLOSES the gate split instead of silently implying participation:
the REQUEST_CHANGES note names the posted comments as unflagged, joins
them to the discussion gate only, and says the repo's `ai_comment`
gate does not track them; SKILL.md's Aone paragraph carries the same
fact for the relay. Marking stays open as an a1 feature request; when
the flag ships it wires at `createMrComment` (the sole write seam).
Still open: dedup/self-PR backing for Aone, `composeUrl`, cleanup
audit, the ai_comment marking flag (a1-side), the render-adjudication
carve-out.
- **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test
repo-config escape hatch, publish-assets gating polish, generic-GitLab
(glab) evaluation.
Expand Down Expand Up @@ -384,12 +411,27 @@ create --file/--line` and `-f json` stability? Provider version floor TBD.
2. **Q2 — Inline anchor semantics.** Does `--line` accept only new-side lines?
How are removed-line (`side: left`) comments posted? Needs a controlled
experiment on a scratch CR.
3. **Q3 — REQUEST_CHANGES.** Confirm no native reject/unapprove API exists
(a1 surface + platform docs); if one exists, prefer it over the blocking
header.
4. **Q4 — AI-comment marking.** Does `comment create` auto-set `isAiComment`
for bot/token identities, or is there a flag? Determines whether qwen
comments fall under the `ai_comment` merge gate or the `discussion` gate.
3. **Q3 — REQUEST_CHANGES.** ~~Confirm no native reject/unapprove API
exists.~~ **Re-confirmed 2026-08-21 on a1 v0.1.90:** the `repo mr` surface
(approve/close/comment/cr/create/diff/edit/list/merge/remind/reopen/
reviewers/status/view/workitem) still has no reject/request-changes/
unapprove. The blocking header stands. Two surface changes observed:
`mr comment resolve` (inline comments only) and `mr cr list` now exist;
the a1 FAQ documents `mr create --enable-ai-review`, but the v0.1.90
binary refuses it (`unknown flag`).
4. **Q4 — AI-comment marking.** ~~Does `comment create` auto-set
`isAiComment`?~~ **Resolved 2026-08-21 by a controlled probe** (scratch
CR on a scratch repo, posting identity a personal account): one general
and one inline comment posted via `a1 repo mr comment create` both read
back `isAiComment: false` — immediately and ~3 min later (async
classifier ruled out) — and v0.1.90 exposes no marking flag. Read-side
corroboration: across 68 recent MRs on maxcompute/odps_src (a repo whose
gates include `ai_comment`) zero comments carried the flag — CI-bot and
human-posted "AI 评审" comments alike — so it is neither identity- nor
content-derived; it appears to be server-side state only the platform's
own AI-review service sets. Qwen comments therefore fall under the
`discussion` gate only; the remedy is a marking flag requested from the
a1 CLI (feature request), wired at `createMrComment` when it ships.
5. **Q5 — Partial failure in batched submit.** GitHub's Create Review is
atomic; Aone is N+1 calls. Policy: post inline first, summary last (summary
references nothing not yet posted), and on mid-batch failure report exactly
Expand Down
2 changes: 1 addition & 1 deletion docs/users/features/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ The deterministic halves of the pipeline — argument parsing (`qwen review pars

**GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`match-remote`, `meta`, `fetch-pr`, `pr-context`, `comment-status`, `issue-context`, `fetch-diff`, `comment-body`, `plan-diff`, `test-plan`, `presubmit`, `compose-review`, `submit`, `publish-assets`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`.

**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests/<id>/head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status`/`presubmit` have no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: one comment per inline finding, then the summary comment. Aone has no native request-changes state — on that verdict the summary comment carries a blocking header, and any inline Criticals that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.
**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests/<id>/head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status`/`presubmit` have no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: one comment per inline finding, then the summary comment. Aone has no native request-changes state — on that verdict the summary comment carries a blocking header, and any inline Criticals that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The posted comments carry no AI-comment flag — `a1` cannot set one — so a repo's dedicated `ai_comment` merge gate does not track them. The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.

Every run ends with one machine-readable line (`Review complete: <target> — <disposition>`), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match.

Expand Down
12 changes: 11 additions & 1 deletion packages/cli/src/commands/review/lib/platform/aone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,15 @@ function createMrComment(
message: string,
inline?: { path: string; line: number },
): number | undefined {
// No AI-comment marking here, BY PLATFORM CONSTRAINT: probed 2026-08-21
// on a scratch CR (issue #9614) — `comment create` does NOT auto-set
// `isAiComment` for the posting identity (both a general and an inline
// probe read back false, re-checked minutes later against an async
// classifier), and a1 v0.1.90 exposes no flag to request it. Created
// comments therefore sit in the generic discussion gate, never the
// dedicated ai_comment merge gate; submit's REQUEST_CHANGES note
// discloses that. When a1 ships a marking flag, THIS call is where it
// gets passed.
// a1JsonOnce is the tolerant read-back: an exec FAILURE propagates (a real
// post failure — the partial-post path counts what landed before it), but a
// SUCCEEDED exec whose answer does not parse is "accepted, id unknown", not
Expand Down Expand Up @@ -758,7 +767,8 @@ function a1Cause(err: unknown): string {
* lands; COMMENT is the summary alone; REQUEST_CHANGES has NO native
* equivalent — the summary carries an explicit blocking header, and the
* unresolved inline Criticals carry the blocking semantics through the
* discussion merge gate.
* discussion merge gate (NEVER the ai_comment gate: a1 cannot mark a
* comment as AI — see createMrComment).
*
* Throws BEFORE writing when the head drifted (the commit_id check
* GitHub's API performs server-side). Throws AonePartialPostError when
Expand Down
27 changes: 27 additions & 0 deletions packages/cli/src/commands/review/submit-aone.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,18 @@ describe('submit posts an authorised Aone target through a1', () => {
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('1 inline Critical(s) block the merge'),
);
// The Q4 probe outcome (issue #9614) is disclosed in the same note:
// a1 cannot mark a comment as AI, so the posted Criticals join the
// discussion gate only — the repo's ai_comment gate never sees them.
// One contiguous fragment from the blocking clause to the end of the
// disclosure pins subject-to-predicate and clause-to-call — inverted
// gate attribution, split calls, and a disclosure moved onto the
// unconditional `Posted …` line each break it.
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining(
"block the merge while their discussions stay unresolved. They are NOT marked as AI comments — `a1 repo mr comment create` cannot set the flag — so they join the generic discussion gate only; a repo's dedicated ai_comment merge gate does not track them",
),
);
Comment on lines +293 to +297

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The two assertions pin the disclosure only as independent substring-existence checks over stderrMock's whole call history — each passes if ANY recorded stderr call matches, and neither binds subject to predicate or clause to call — so the note this PR exists to pin (documented as provisional: "Until a1 ships a flag, this note is the disclosure", submit.ts:1010-1011) stays unpinned along three probe-verified axes:

  • Content: inverting which gate tracks the comments — "...join the dedicated ai_comment gate only; a repo's generic discussion merge gate does not track them." — keeps both pinned substrings present and this test green while the disclosure tells operators the opposite. Splitting the note across two writeStderrLine calls also passes.
  • Clause coverage: the positive clause ("so they join the generic discussion gate only") and the causal clause ("They are NOT marked as AI comments — a1 repo mr comment create cannot set the flag") are pinned by no assertion — swapping or trimming either stays green, even though the comment above these assertions claims to pin "The Q4 probe outcome", which lives in the unpinned causal clause.
  • Call-binding and emission condition: nothing forces the disclosure to share the REQUEST_CHANGES Note: line (the single writeStderrLine at submit.ts:1015), and no test pins when the note fires — moving the disclosure sentence into the unconditional Posted … line (submit.ts:993) keeps every assertion green (the contiguous-fragment fix round 1 suggested in this thread survives it too) while an APPROVE post also prints a disclosure about nonexistent blocking Criticals, and the SKILL.md relay (SKILL.md:109) keys on a Note: line that lost the gate disclosure.

Each mutant was run at the reviewed commit: the inverted wording, split calls, positive-clause swap and causal-clause trim all pass 29/29, and the moved-disclosure mutant additionally passes a probe assertion printing the disclosure on APPROVE. Pin one contiguous fragment from the blocking clause to the end of the disclosure — content and call-binding in one check — plus an emission-condition negative:

Suggested change
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('ai_comment'),
);
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('does not track them'),
);
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining(
"block the merge while their discussions stay unresolved. They are NOT marked as AI comments — `a1 repo mr comment create` cannot set the flag — so they join the generic discussion gate only; a repo's dedicated ai_comment merge gate does not track them",
),
);

and in a REQUEST_CHANGES with zero inline Criticals says nothing mechanically blocks, add:

expect(stderrMock).not.toHaveBeenCalledWith(
  expect.stringContaining('does not track them'),
);

Both halves of the spanning fragment were separately verified contiguous in the real emitted note; the combined pin passes the committed code (29/29) and fails every mutant above (probe: 1–2 tests failed per mutant).

中文说明

这两条断言仅以独立的子串存在性检查钉住披露——每条都对 stderrMock 的全部调用历史匹配,任一 stderr 调用命中即通过;两者既不把主语与谓词绑定,也不把子句与调用绑定。因此本 PR 要钉住的这条提示(源码注释已注明其为过渡方案:"Until a1 ships a flag, this note is the disclosure",submit.ts:1010-1011)在三个经探测验证的轴向上仍未被钉住:

  • 内容:把两个门禁的主语互换——"…join the dedicated ai_comment gate only; a repo's generic discussion merge gate does not track them."——两个被钉子串依然存在,测试全绿,而披露告知运维的事实恰好相反;把提示拆成两个 writeStderrLine 调用同样通过。
  • 子句覆盖:正向子句("so they join the generic discussion gate only")与因果子句("They are NOT marked as AI comments — a1 repo mr comment create cannot set the flag")没有任何断言钉住——互换或删掉任一子句测试依旧全绿,尽管上方注释声称这里钉住的是 "The Q4 probe outcome"(该结论恰在未钉住的因果子句中)。
  • 调用绑定与触发条件:没有断言强制披露与 REQUEST_CHANGES 的 Note: 行(submit.ts:1015 处唯一的 writeStderrLine)出自同一次调用,也没有测试钉住它的触发条件——把披露句移入无条件的 Posted … 行(submit.ts:993)后所有断言依旧全绿(本线程 round 1 建议的连续片段修法同样拦不住),APPROVE 发布也会打印一条关于不存在的阻塞 Critical 的披露,而 SKILL.md 的转发(SKILL.md:109)会引用一条已失去门禁披露的 Note: 行。

上述每个变异体都在受审提交上实际运行过:取反表述、拆分调用、正向子句互换、因果子句裁剪均 29/29 全绿;移动披露的变异体还会让一条 APPROVE 打印披露的探测断言通过。建议用一个从阻塞子句延伸到披露末尾的连续片段——一次检查同时钉住内容与调用绑定——再加一个触发条件负向断言(代码块见英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

Comment on lines +293 to +297

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] These pins exercise only the attribution-ON shape — no test pins this note under an attribution-OFF post. The note's count source is deliberately the MARKED pre-post payload (criticalsPosted counted off payload.comments, submit.ts:1012-1014), because under attribution: false the posted bodies lose their severity prefixes and severityOf — a leading-marker classifier (lib/inline-counts.ts:63-72) that never reads the trailing <!-- qwen-review critical --> marker — returns null for all of them. A future edit aligning the count with what was "actually posted" — const criticalsPosted = (post.comments ?? []).filter((c) => severityOf(c) === 'critical').length;, exactly the reading the variable name invites — passes all 29 tests (the mutant was run at the reviewed commit: 29/29 green, invisible). Every attribution-OFF Aone REQUEST_CHANGES post with inline Criticals then flips the note to "posted NO inline Critical discussions, so nothing mechanically blocks the merge; the header is advisory" and drops this disclosure entirely, while Critical discussions actually posted and mechanically block the merge — the operator is told the opposite of the platform state. The fix in the adjacent comment does not close this: it lives in attribution-ON tests, where post.comments === payload.comments. Pin the note in the attribution-OFF test ('an attribution-OFF Aone post strips the severity prefix and appends the invisible marker'):

expect(stderrMock).toHaveBeenCalledWith(
  expect.stringContaining('1 inline Critical(s) block the merge'),
);
expect(stderrMock).toHaveBeenCalledWith(
  expect.stringContaining('ai_comment merge gate does not track them'),
);

The suggested assertions pass the committed code (29/29) and fail the count-source mutant (1 failed) — probe-verified at the reviewed commit.

中文说明

这些钉子只覆盖 attribution-ON 形态——没有任何测试在 attribution-OFF 发布下钉住该提示。提示的计数源刻意取自打标后的发布前载荷(criticalsPosted 来自 payload.commentssubmit.ts:1012-1014),因为 attribution: false 时实际发布的正文会被剥掉 severity 前缀,而 severityOf——按首部标记分类(lib/inline-counts.ts:63-72),从不读取尾部的 <!-- qwen-review critical --> 标记——对它们一律返回 null。未来若有人把计数改为"按实际发布的评论"——const criticalsPosted = (post.comments ?? []).filter((c) => severityOf(c) === 'critical').length;,变量名 criticalsPosted 恰好诱导这种读法——全部 29 个测试依旧通过(变异体已在受审提交上运行:29/29 全绿,完全不可见)。此后每一次带 inline Critical 的 attribution-OFF Aone REQUEST_CHANGES 发布都会把提示翻转为 "posted NO inline Critical discussions, so nothing mechanically blocks the merge; the header is advisory",并完全丢掉本披露,而 Critical 讨论实际已发布且在机械上阻塞合并——运维被告知的与平台状态恰好相反。相邻评论中的修法堵不住这个洞:它们在 post.comments === payload.comments 的 attribution-ON 测试里。请在 attribution-OFF 测试中钉住该提示(代码块见英文部分)。

— qwen3.8-max via Qwen Code /review (v0.21.15)

});

it('an UNAUTHORISED Aone run takes the normal auth-refusal path first', () => {
Expand Down Expand Up @@ -929,6 +941,16 @@ describe('submit posts an authorised Aone target through a1', () => {
expect(req.comments).toHaveLength(1);
expect(req.comments[0].body).not.toContain('**[Critical]**');
expect(req.comments[0].body).toContain('<!-- qwen-review critical -->');
// The note counts off the MARKED pre-post payload, not the stripped
// bodies pinned above (severityOf is a leading-marker classifier and
// reads null on them): attribution-OFF still names the posted
// Critical and the gate disclosure.
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('1 inline Critical(s) block the merge'),
);
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('ai_comment merge gate does not track them'),
);
});

it('the Aone success JSON carries NO url key when a1 answered without detailUrl', () => {
Expand Down Expand Up @@ -965,5 +987,10 @@ describe('submit posts an authorised Aone target through a1', () => {
expect(stderrMock).toHaveBeenCalledWith(
expect.stringContaining('NO inline Critical discussions'),
);
// The gate disclosure fires only when inline Criticals actually
// posted — the zero-Critical shape discloses no gate at all.
expect(stderrMock).not.toHaveBeenCalledWith(
expect.stringContaining('does not track them'),
);
});
});
12 changes: 11 additions & 1 deletion packages/cli/src/commands/review/submit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1003,6 +1003,12 @@ export function runSubmit(
// post with ZERO inline Criticals (they were all body-level), and
// then nothing mechanically blocks the merge — say which shape this
// was, counted off the same comments the consistency gate marked.
// The blocking GATE is named too: a1 cannot mark a comment as an AI
// comment (probed 2026-08-21 on a scratch CR — no auto-flag for the
// posting identity, no explicit flag; issue #9614), so the posted
// comments sit in the generic discussion gate only, and a repo's
// dedicated ai_comment merge gate never sees them. Until a1 ships a
// flag, this note is the disclosure.
const criticalsPosted = (payload.comments ?? []).filter(
(c) => severityOf(c) === 'critical',
).length;
Expand All @@ -1011,7 +1017,11 @@ export function runSubmit(
? `Note: Aone Code has no native request-changes state — the ` +
`summary comment carries the blocking header, and the ` +
`${criticalsPosted} inline Critical(s) block the merge ` +
`while their discussions stay unresolved.`
`while their discussions stay unresolved. They are NOT ` +
`marked as AI comments — \`a1 repo mr comment create\` ` +
`cannot set the flag — so they join the generic ` +
`discussion gate only; a repo's dedicated ai_comment ` +
`merge gate does not track them.`
: `Note: Aone Code has no native request-changes state — the ` +
`summary comment carries the blocking header, but this ` +
`review posted NO inline Critical discussions, so nothing ` +
Expand Down
Loading
Loading