diff --git a/docs/design/review-cpu-for-tokens.md b/docs/design/review-cpu-for-tokens.md new file mode 100644 index 00000000000..7b21e608cc1 --- /dev/null +++ b/docs/design/review-cpu-for-tokens.md @@ -0,0 +1,228 @@ +# /review: trade CPU for tokens (prompt-cache baseline + glue sinking) + +**Status:** implemented & verified (2026-08-07) — workstream A closed by +measurement (no client change); workstream B shipped as `match-remote`, +E2E-verified through the skill (see `.qwen/e2e-tests/review-cpu-for-tokens.md` +— an untracked, machine-local run archive, not committed) +**Date:** 2026-08-07 +**Scope:** `/review` token economy — two workstreams, one measurement-gated, +one deterministic. + +## Problem + +A high-effort PR review launches 17-23 model calls. DESIGN.md's token-cost +analysis prices that at ~880K-1.2M input tokens, dominated by a ~50K shared +prefix (system prompt + tool declarations + startup prelude) re-delivered once +per agent. Two ways to spend less without spending recall: + +1. **Shared prefix / prompt caching.** Every review agent is a fresh + `general-purpose` subagent. If the prefix they all share is byte-identical + and the provider caches it, agents 2..N pay for the prefix once. The + question is whether that is already happening, and if not, why. +2. **Glue sinking.** The pipeline's established direction (DESIGN.md — + "deterministic halves as subcommands"; #8642) moves deterministic + orchestrator judgments out of model turns and into tested subcommands. + Most judgment points are already sunk; an inventory of what remains found + one candidate worth taking. + +Both workstreams keep the recall-first contract: nothing here changes what a +review covers or what evidence a verdict requires. + +## Investigation findings + +### The caching chain is already built end to end + +- **Wire layer.** The DashScope provider already applies cache control + (`addDashScopeCacheControl` in + `packages/core/src/core/openaiContentGenerator/provider/dashscope.ts`): + `cache_control: {type: 'ephemeral'}` on the system message, on the last + message for streaming requests, and on the last tool declaration. Enabled + by default (`enableCacheControl !== false`). +- **Response parsing.** The OpenAI-compatible converter reads both + `usage.prompt_tokens_details.cached_tokens` and the top-level + `cached_tokens` shape into `cachedContentTokenCount`. +- **Measurement.** `qwen review cost-ledger` already aggregates per-agent + `input / cached / output / thinking` token counts from the harness's own + transcript records — the same records `check-coverage` trusts. Step 8 + archives the ledger beside every saved report. No new instrumentation is + needed to answer "is the cache hitting". + +### The prefix should already be byte-identical across a fan-out + +A review agent's request prefix, in wire order: + +| Part | Source | Varies across agents in one run? | +| ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| System instruction | builtin `general-purpose` template (static text) + non-interactive suffix + user memory + auto-memory (`buildChatSystemPrompt` in `agents/runtime/agent-core.ts`) | No — same template, same parent session | +| Tool declarations | Parent registry copied into the per-agent override (`rebuildToolRegistryOnOverride`) | No — same tool set, deterministic registry order | +| Startup prelude | `getInitialChatHistory` → date + platform + cwd + folder structure | No — all agents pinned to the same worktree (`working_dir`) or the same main checkout; folder structure is capped at 20 items, alphabetically sorted, and skips `node_modules`/`.git`/`dist` | +| Task prompt | Per-agent block from `agent-prompt --roster` | **Yes — this is the tail, after the shared prefix** | + +The codebase already optimizes the prelude order for prefix caching ("Stable +parts first ... so prefix-caching servers retain the KV-cache", comment in +`getInitialChatHistory`), and forks already exist specifically to share a +byte-identical cache prefix. Review agents cannot be forks (they must return +findings inline), but they do not need to be: their prefix is structurally +identical already. + +**Consequence:** the client side has no identified drift to fix. The open +question is empirical — does DashScope actually serve cache hits for this +shape on qwen3.8-max? That is a measurement, not a code change, and the +measurement exists (cost-ledger `cached` column). Candidate causes to +investigate if the baseline shows misses: server-side minimum cacheable size, +cache TTL vs. agent wall clock, concurrent first-write races across the +fan-out, or the `ephemeral` marker semantics on this endpoint. Several of +these live server-side and may need the model-service team's input. + +### Glue inventory: one candidate survives + +Walked SKILL.md step by step for remaining model-turn judgment points. Most +are already subcommands (parse-args, capture-local, fetch-pr, plan-diff, +load-rules, repo-context, agent-prompt, check-coverage, findings, +resolve-anchors, compose-review, presubmit, submit, script-lint, test-plan, +base-tree, test-delta, extract-step, save-artifact, cost-ledger, cleanup). +What remains: + +| Residual judgment | Disposition | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Step 1 remote matching** (pr-url: parse `git remote -v`, match host + owner/repo by exact segments; bare pr-number: pick the remote whose URL is the derived owner/repo) | **Sink.** Deterministic parsing with two shipped bug classes: a substring match once matched `shao/qwen-code` against a `wenshao/qwen-code` remote (reviewed one repo, posted to another), and a guessed owner/repo once stopped a review before it read any code. Prose rules with bug histories are exactly the class DESIGN.md moves into code. | +| Incremental-cache check (compare two JSON fields, three branches) | Keep as prose — ~3 lines; sinking grows the prompt more than it saves (DESIGN.md: every subcommand is part of the prompt cost). | +| Step 3C angles, whiff checks, Agent 8 selection, dedup/pattern aggregation, open-Critical re-check, skipped-CI-check ruling | Keep — semantic judgments; the pipeline's discipline is that heuristics feed agents, never rule for them. | +| Step 8 report rendering | Keep — #8642 already batched the tail into four responses; the remaining prose is genuine prose (summary, output-language headings). | +| Step 5 cumulative-findings merge | Keep — mechanical-looking, but its inputs are verifier verdicts the orchestrator extracts from natural language; the mechanical half is one file edit per round. | + +## Proposal + +### Workstream A — cache-hit baseline (measurement first, gates any code) + +Run one same-repo PR review at medium effort with the current build, read the +cost-ledger, and record per-agent `input` vs `cached`: + +- **Hit rate high** (agents 2..N read most of the shared prefix from cache): + no client change. Document the result in this design doc's history and in + the DESIGN.md cost table, and close the workstream. +- **Hit rate low:** diagnose before changing anything. Order of checks: + 1. Confirm prefix identity empirically (capture two agents' first request + bodies via the mock provider / HTTP trace; diff the prefix). + 2. If prefixes differ, find the drift and fix it (the table above is the + suspect list). + 3. If prefixes are identical but no hits land, the question moves to the + DashScope side (marker semantics, minimum size, TTL, concurrency) — + raise with the model-service team before adding client workarounds. + +No code is written for this workstream until the baseline picks a branch. + +**Baseline result (2026-08-07, PR #8651, medium effort, frozen bundle of this +commit):** the hit-rate-high branch, decisively. Aggregated from the harness +transcripts (the same records `cost-ledger` reads): 12 agents, 184 model +calls, 12.87M input tokens, 12.0M cached — **93.3% of input served from +cache**. Two structural observations: + +- **The prefix is byte-identical across the fan-out, empirically.** Every + agent's first request prices at 34,250 ± 7 input tokens; three agents + whose first request landed after an earlier agent's write read 30,311 of + those tokens (88-89%) from cache. The shared prefix is ~34K tokens, and + it is shared. +- **The cross-agent miss is a concurrent first-write race, not drift.** 11 + agents launch in one response; 8 of their first requests raced the cache + write and paid full price for the prefix once each. Everything after the + first call of every agent hits (the multi-turn history anchors). The race + costs ~2% of the run's input tokens. A warm-up request before the fan-out + could close it, but that is one extra model call and new mechanism for a + ~2% token delta on an already-93% hit rate — rejected under + simplicity-first. The fan-out's wall clock is worth more than the prefix + write race. + +**Workstream A closes with no client change.** The chain that produces this +(wire-level cache control, prefix-friendly prelude ordering, byte-identical +subagent prefixes) is already in place; the DESIGN.md "~880K-1.2M input +tokens" figure is raw re-delivered tokens, not billed cost — the cache +serves the bulk of it. + +### Workstream B — `qwen review match-remote` + +One new read-only subcommand owning the remote-resolution rule SKILL.md +currently carries as prose in two places (pr-url matching in Step 1, and +remote selection for bare pr-numbers). + +**Interface:** + +```bash +qwen review match-remote --owner --repo [--host ] +# prints the matched remote name on stdout; exits 6 when no remote matches +``` + +- Reads `git remote -v` and parses each URL structurally — the two shapes + `git@:/.git` and `https:////(.git)`. +- A remote matches only when its host equals `--host` (default + `github.com`) AND its owner/repo (`.git` suffix stripped) equals the + arguments, both compared case-insensitively as whole segments. Substring + containment is exactly what shipped the wrong-repo bug; the parser never + does it. +- Exactly one match → print its name, exit 0. Zero → exit 6 with `none` on + stdout (the lightweight-mode signal). Multiple → print all, exit 7 with a + `warning:` line; the orchestrator stops rather than picks (same rule as + today's prose). +- Not a git repository / git unavailable → exit 1 (fail-closed, like the + other gates). + +**SKILL.md delta:** Step 1's two prose paragraphs (exact-segment parse, fork +layout, guessing prohibition) collapse to "run `match-remote`; a printed +name means worktree flow, exit 6 means lightweight mode". Net prompt size +is roughly neutral (measured from this PR's SKILL.md hunks: ~720 chars +added net) — the bash invocation and exit-code prose offset the removed +rule text; the win is determinism and tests, not size. `fetch-pr`'s +interface is unchanged (still takes `--remote`), and the lightweight-mode +branch keeps its current shape, so no other step moves. + +**Tests:** table-driven, mirroring parse-args' suite style — the two URL +shapes, `.git` suffix, case-insensitivity, the `shao/qwen-code` vs +`wenshao/qwen-code` regression row, fork layout (origin = fork, upstream = +target), GHE host mismatch, multiple-match, zero-match, malformed remote +URLs. The bug history supplies the first rows. + +**Host resolution for bare PR numbers:** a bare number has no URL to take +a host from, so Step 1 asks `gh repo view` for the repo's URL as well and +passes its authority as `--host`. `gh` resolves that URL through its own +default-host resolution — an operator-exported `GH_HOST` or, without one, +its auth config — so the matcher compares against exactly the host the +rest of the pipeline routes at. (The first cut omitted `--host` and let +the matcher inherit `GH_HOST` itself, assuming `gh`'s routing always came +from that env; `gh` also resolves a GHE host from its auth config alone, +and an operator using only that was compared against github.com and +hard-stopped at exit 6 — caught by this PR's own review.) With `--host` +omitted, the matcher's fallback is unchanged: an explicit flag wins, else +an operator-exported `GH_HOST`, else github.com. + +### Files affected + +- `packages/cli/src/commands/review/match-remote.ts` (new) + collocated + test; pure parsing/matching core in `review/lib/remote-match.ts` + its + table-driven test. +- `packages/cli/src/commands/review.ts` — registration + demand message; + `review.test.ts` — the registration list. +- `packages/core/src/skills/bundled/review/SKILL.md` — Step 1 shrinks. +- `docs/design/review-cpu-for-tokens.md` — baseline results appended. + +### Scope boundaries (explicitly out) + +- No change to agent fan-out, rosters, briefs, verification, reverse audit, + verdict computation, or posting. +- No incremental-cache subcommand, no report-rendering subcommand (see + inventory). +- No per-agent effort or per-agent model overrides — a separate proposal + (directions A/B in the token-economy discussion), not this one. +- No AST-level diff pre-digestion — needs its own A/B evidence first. + +## Open questions + +1. ~~**DashScope cache behavior on qwen3.8-max**~~ — answered by the + baseline: yes, cross-request prefix hits are served within one API key's + concurrent requests (93.3% of the run's input cached); the only gap is + the concurrent first-write race on the fan-out's first requests, judged + not worth a mechanism. +2. **Exit-code numbering** — the review subcommands already use 3 + (gate refusal / not covered), 4 (budget), 5 (converged) for + structured outcomes; this doc claims 6 for "no matching remote" and 7 + for "several match", keeping 1/2 for error/misuse. Conflicts checked + against the current suite; the implementation pins them. diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 40a2ecc4cd1..8da040a788e 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -41,6 +41,7 @@ describe('reviewCommand', () => { expect(registeredSubcommands()).toEqual([ 'run', 'parse-args', + 'match-remote', 'fetch-pr', 'capture-local', 'plan-diff', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 7099d729ad1..e027ae9459d 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -10,6 +10,7 @@ import type { Argv, CommandModule } from 'yargs'; import { parseArgsCommand } from './review/parse-args.js'; +import { matchRemoteCommand } from './review/match-remote.js'; import { composeReviewCommand } from './review/compose-review.js'; import { findingsCommand } from './review/findings.js'; import { fetchPrCommand } from './review/fetch-pr.js'; @@ -47,6 +48,7 @@ export const reviewCommand: CommandModule = { yargs .command(runCommand) .command(parseArgsCommand) + .command(matchRemoteCommand) .command(fetchPrCommand) .command(captureLocalCommand) .command(planDiffCommand) @@ -76,7 +78,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, match-remote, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/lib/gh.test.ts b/packages/cli/src/commands/review/lib/gh.test.ts index b3d06ca977b..350c7bea599 100644 --- a/packages/cli/src/commands/review/lib/gh.test.ts +++ b/packages/cli/src/commands/review/lib/gh.test.ts @@ -17,6 +17,7 @@ vi.mock('node:child_process', () => ({ import { ghEnv, + resolveGhHost, setGhHost, parseNdjson, gh, @@ -61,6 +62,22 @@ describe('setGhHost / ghEnv', () => { }); }); +describe('resolveGhHost precedence', () => { + it('an explicit --host wins over an operator-exported GH_HOST', () => { + // Load-bearing for the gates: a GHE operator who exports GH_HOST and + // reviews a github.com PR must resolve to github.com, or the matcher + // exits 6 and the write-side gates bind the wrong host. + const savedGhHost = process.env['GH_HOST']; + process.env['GH_HOST'] = 'ghe.example.com'; + try { + expect(resolveGhHost('github.com')).toBe('github.com'); + } finally { + if (savedGhHost === undefined) delete process.env['GH_HOST']; + else process.env['GH_HOST'] = savedGhHost; + } + }); +}); + describe('parseNdjson (the paginated check-runs decode)', () => { it('parses one JSON value per non-blank line', () => { // `gh api --paginate --jq '.check_runs[]'` applies the jq per page diff --git a/packages/cli/src/commands/review/lib/gh.ts b/packages/cli/src/commands/review/lib/gh.ts index 07580f0b34e..80558628ce8 100644 --- a/packages/cli/src/commands/review/lib/gh.ts +++ b/packages/cli/src/commands/review/lib/gh.ts @@ -103,6 +103,26 @@ export function getGhHost(): string | undefined { return ghHost; } +/** + * The effective GitHub host for a command invocation: an explicit `--host` + * flag wins, else an operator-exported GH_HOST, else `undefined` — the + * caller applies its own default (`gh`'s github.com, or the matcher's + * comparison host). Every call site that needs the effective host as a + * value — the matcher and the two write-side authorisation gates — + * resolves through this one helper so they cannot disagree; routing + * sites go through `setGhHost` and inherit an operator-exported GH_HOST + * via the child env. + * + * `|| undefined`, not `??`: an exported-but-empty GH_HOST ("" survives + * `??`, being non-nullish) must read as "no host", not as a host named "" + * that fails every comparison. + */ +export function resolveGhHost( + flagHost: string | undefined, +): string | undefined { + return flagHost ?? (process.env['GH_HOST']?.trim() || undefined); +} + /** * Environment for `gh` child processes. `undefined` means "inherit the * parent env untouched"; with a host set, the inherited env is extended diff --git a/packages/cli/src/commands/review/lib/remote-match.test.ts b/packages/cli/src/commands/review/lib/remote-match.test.ts new file mode 100644 index 00000000000..f3393202d63 --- /dev/null +++ b/packages/cli/src/commands/review/lib/remote-match.test.ts @@ -0,0 +1,280 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { + parseRemoteUrl, + matchRemotes, + normalizeSegment, +} from './remote-match.js'; + +describe('parseRemoteUrl', () => { + interface ParseCase { + name: string; + url: string; + want: { host: string; owner: string; repo: string } | null; + } + + const cases: ParseCase[] = [ + { + name: 'scp shape', + url: 'git@github.com:QwenLM/qwen-code.git', + want: { host: 'github.com', owner: 'qwenlm', repo: 'qwen-code' }, + }, + { + name: 'scp shape without .git', + url: 'git@github.com:QwenLM/qwen-code', + want: { host: 'github.com', owner: 'qwenlm', repo: 'qwen-code' }, + }, + { + name: 'https shape', + url: 'https://github.com/wenshao/qwen-code.git', + want: { host: 'github.com', owner: 'wenshao', repo: 'qwen-code' }, + }, + { + name: 'https shape with trailing slash', + url: 'https://github.com/wenshao/qwen-code/', + want: { host: 'github.com', owner: 'wenshao', repo: 'qwen-code' }, + }, + { + name: 'https shape with userinfo', + url: 'https://user@github.com/wenshao/qwen-code.git', + want: { host: 'github.com', owner: 'wenshao', repo: 'qwen-code' }, + }, + { + name: 'ssh scheme with port', + url: 'ssh://git@ghe.example.com:22/team/tool.git', + want: { host: 'ghe.example.com', owner: 'team', repo: 'tool' }, + }, + { + name: 'host case is normalised', + url: 'git@GitHub.COM:Owner/Repo.git', + want: { host: 'github.com', owner: 'owner', repo: 'repo' }, + }, + { + name: 'extra path segment is not an owner/repo', + url: 'https://github.com/a/b/c.git', + want: null, + }, + { + name: 'bare local path', + url: '/srv/git/qwen-code.git', + want: null, + }, + { + name: 'file scheme has no host', + url: 'file:///srv/git/qwen-code.git', + want: null, + }, + { + name: 'colon without slash is not the scp shape', + url: 'weird:thing', + want: null, + }, + { + name: 'empty string', + url: '', + want: null, + }, + { + name: 'owner missing', + url: 'https://github.com/qwen-code.git', + want: null, + }, + ]; + + it.each(cases)('$name', ({ url, want }) => { + expect(parseRemoteUrl(url)).toEqual(want); + }); +}); + +describe('normalizeSegment', () => { + it('lowercases and strips one trailing .git', () => { + expect(normalizeSegment('QwenLM')).toBe('qwenlm'); + expect(normalizeSegment('qwen-code.git')).toBe('qwen-code'); + // Uppercase .GIT pins the lowercase-THEN-strip order: strip-before- + // lowercase would leave the suffix behind and fail every comparison. + expect(normalizeSegment('QWEN-CODE.GIT')).toBe('qwen-code'); + expect(normalizeSegment('qwen-code.git.git')).toBe('qwen-code.git'); + }); +}); + +describe('matchRemotes', () => { + const FORK_LAYOUT = [ + 'origin\tgit@github.com:QwenLM/qwen-code.git (fetch)', + 'origin\tgit@github.com:QwenLM/qwen-code.git (push)', + 'wenshao\tgit@github.com:wenshao/qwen-code.git (fetch)', + 'wenshao\tgit@github.com:wenshao/qwen-code.git (push)', + ].join('\n'); + + it('matches the upstream in a fork layout', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual(['origin']); + }); + + it('matches the fork by its own owner', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'wenshao', + repo: 'qwen-code', + }); + expect(matched).toEqual(['wenshao']); + }); + + it('compares case-insensitively', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'QWENLM', + repo: 'QWEN-CODE', + }); + expect(matched).toEqual(['origin']); + }); + + it('tolerates a .git suffix on the input repo', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'QwenLM', + repo: 'qwen-code.git', + }); + expect(matched).toEqual(['origin']); + }); + + // The regression row: a substring comparison matched `shao/qwen-code` + // against the `wenshao` remote and one review read one repository while + // posting to another. Exact segment equality must not. + it('does not substring-match an owner contained in another', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'shao', + repo: 'qwen-code', + }); + expect(matched).toEqual([]); + }); + + it('strips an explicit port from the input host before comparing', () => { + // parse-args' PR_URL_RE keeps `host:port` in the verdict and lib/gh.ts' + // HOSTNAME_RE accepts it, but a parsed remote URL never carries a port — + // without the strip, a port-bearing GHE review could never match its own + // remote and would be demoted to lightweight mode. + const remotes = [ + 'origin\thttps://ghe.example.com/team/repo.git (fetch)', + 'origin\thttps://ghe.example.com/team/repo.git (push)', + ].join('\n'); + expect( + matchRemotes(remotes, { + owner: 'team', + repo: 'repo', + host: 'ghe.example.com:8443', + }).matched, + ).toEqual(['origin']); + }); + + it('does not match a different host', () => { + const { matched } = matchRemotes(FORK_LAYOUT, { + owner: 'QwenLM', + repo: 'qwen-code', + host: 'ghe.example.com', + }); + expect(matched).toEqual([]); + }); + + it('matches a GHE remote only under its own host', () => { + const remotes = [ + 'origin\tgit@github.com:QwenLM/qwen-code.git (fetch)', + 'origin\tgit@github.com:QwenLM/qwen-code.git (push)', + 'ghe\tgit@ghe.example.com:QwenLM/qwen-code.git (fetch)', + 'ghe\tgit@ghe.example.com:QwenLM/qwen-code.git (push)', + ].join('\n'); + expect( + matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + host: 'ghe.example.com', + }).matched, + ).toEqual(['ghe']); + expect( + matchRemotes(remotes, { owner: 'QwenLM', repo: 'qwen-code' }).matched, + ).toEqual(['origin']); + }); + + it('reports every match when several remotes serve the same repo', () => { + const remotes = [ + 'upstream\thttps://github.com/QwenLM/qwen-code.git (fetch)', + 'upstream\thttps://github.com/QwenLM/qwen-code.git (push)', + 'mirror\tgit@github.com:QwenLM/qwen-code.git (fetch)', + 'mirror\tgit@github.com:QwenLM/qwen-code.git (push)', + ].join('\n'); + const { matched } = matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual(['upstream', 'mirror']); + }); + + it('matches on the fetch URL only, and counts each remote once', () => { + // pushurl differs from the fetch URL; only the fetch side serves + // `git fetch pull//head`, so only it can match — and the + // push line must not add a duplicate. + const remotes = [ + 'origin\thttps://github.com/QwenLM/qwen-code.git (fetch)', + 'origin\thttps://github.com/someone-else/push-target.git (push)', + ].join('\n'); + const { matched } = matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual(['origin']); + }); + + it('does not match when only the push URL points at the repo', () => { + const remotes = [ + 'origin\thttps://github.com/someone-else/fetch-side.git (fetch)', + 'origin\thttps://github.com/QwenLM/qwen-code.git (push)', + ].join('\n'); + const { matched } = matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual([]); + }); + + it('matches a partial-clone remote despite the filter annotation', () => { + // `git clone --filter=blob:none` makes `git remote -v` print + // `\t (fetch) [blob:none]` — the annotation sits after the + // marker and must not lose the remote (a silent exit-6 demotion for + // every partial clone). + const remotes = [ + 'origin\thttps://github.com/QwenLM/qwen-code.git (fetch) [blob:none]', + 'origin\thttps://github.com/QwenLM/qwen-code.git (push)', + ].join('\n'); + const { matched } = matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual(['origin']); + }); + + it('skips unparsable remotes', () => { + const remotes = [ + 'local\t/srv/git/qwen-code.git (fetch)', + 'local\t/srv/git/qwen-code.git (push)', + 'origin\tgit@github.com:QwenLM/qwen-code.git (fetch)', + 'origin\tgit@github.com:QwenLM/qwen-code.git (push)', + ].join('\n'); + const { matched } = matchRemotes(remotes, { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual(['origin']); + }); + + it('handles empty output', () => { + const { matched } = matchRemotes('', { + owner: 'QwenLM', + repo: 'qwen-code', + }); + expect(matched).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/review/lib/remote-match.ts b/packages/cli/src/commands/review/lib/remote-match.ts new file mode 100644 index 00000000000..b08f1b91e55 --- /dev/null +++ b/packages/cli/src/commands/review/lib/remote-match.ts @@ -0,0 +1,144 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Matching a PR's owner/repo against `git remote -v`, extracted from the +// /review skill's Step 1 prose and given tests, because the prose shipped +// two bugs: a substring comparison that matched `shao/qwen-code` against a +// `wenshao/qwen-code` remote (review one repository, post to another), and +// hand-guessed remote names that stopped a review before it read any code. +// The rule is exact segment equality, case-insensitive, on the URL's host, +// owner and repo — nothing else is a match. + +export interface RemoteIdentity { + host: string; + owner: string; + repo: string; +} + +/** Lowercase and strip one trailing `.git`, the normal form comparison runs in. */ +export function normalizeSegment(value: string): string { + const v = value.toLowerCase(); + return v.endsWith('.git') ? v.slice(0, -4) : v; +} + +/** + * Parse one remote URL into its host / owner / repo, or null when it is + * neither of the two shapes `git remote -v` prints for a GitHub-style host — + * `git@:/(.git)` and `https:////(.git)` + * — nor the `ssh://` spelling of the first. Anything else (a local path, an + * `http` URL with extra path segments, a bundle file) is not a candidate and + * must never match. + */ +export function parseRemoteUrl(raw: string): RemoteIdentity | null { + const url = raw.trim(); + if (url === '') return null; + + let host: string; + let pathPart: string; + + const schemeIdx = url.indexOf('://'); + if (schemeIdx !== -1) { + // https:////(.git), ssh://git@//.git + let parsed: URL; + try { + parsed = new URL(url); + } catch { + return null; + } + if (parsed.hostname === '') return null; + host = parsed.hostname; + pathPart = parsed.pathname; + } else { + // The scp-like shape `[user@]:/` — the colon must + // come before the first slash, which is also what rejects local paths + // (`/srv/git/x.git`, `C:\repo`) and bare names. + const colonIdx = url.indexOf(':'); + const slashIdx = url.indexOf('/'); + if (colonIdx === -1 || slashIdx === -1 || colonIdx > slashIdx) { + return null; + } + host = url.slice(0, colonIdx); + const atIdx = host.lastIndexOf('@'); + if (atIdx !== -1) host = host.slice(atIdx + 1); + pathPart = url.slice(colonIdx + 1); + } + + const segments = pathPart + .split('/') + .map((s) => s.trim()) + .filter((s) => s !== ''); + if (segments.length !== 2) return null; + if (host === '') return null; + + return { + host: host.toLowerCase(), + owner: normalizeSegment(segments[0]), + repo: normalizeSegment(segments[1]), + }; +} + +export interface RemoteMatchInput { + owner: string; + repo: string; + /** Defaults to `github.com` — a PR URL's host, or github.com for bare numbers. */ + host?: string; +} + +export interface RemoteMatchOutcome { + /** Remote names whose FETCH url is an exact-segment match, in `git remote -v` order. */ + matched: string[]; +} + +/** + * Match an owner/repo/host against the raw output of `git remote -v`. + * + * Only `(fetch)` lines count: `fetch-pr` fetches `pull//head` through the + * remote's fetch URL, and a remote whose push URL alone pointed at the repo + * could not serve it. A remote appears twice (fetch and push); matching the + * fetch lines alone also dedupes. + */ +export function matchRemotes( + remoteVOutput: string, + { owner, repo, host = 'github.com' }: RemoteMatchInput, +): RemoteMatchOutcome { + const wantOwner = normalizeSegment(owner); + const wantRepo = normalizeSegment(repo); + // A PR URL's host can carry an explicit port (parse-args' PR_URL_RE keeps + // it, lib/gh.ts' HOSTNAME_RE accepts it), but a parsed remote host never + // does — compare the hostname part only, or a port-bearing GHE review + // could never match its own remote. + const wantHost = normalizeSegment(host.replace(/:\d+$/, '')); + + const matched: string[] = []; + + for (const line of remoteVOutput.split('\n')) { + const trimmed = line.trim(); + // A partial clone's fetch entry carries git's filter annotation AFTER + // the marker — `\t (fetch) [blob:none]` — so the gate + // cannot anchor on `(fetch)` alone or that remote is silently lost. + if (trimmed === '' || !/\(fetch\)(\s+\[[^\]]*\])?$/.test(trimmed)) { + continue; + } + // `\t (fetch)` plus that optional trailing annotation — the + // name never contains whitespace, so the first run of non-space + // characters is the name and the URL sits between it and the marker. + const nameMatch = trimmed.match( + /^(\S+)\s+(.*)\s+\(fetch\)(\s+\[[^\]]*\])?$/, + ); + if (!nameMatch) continue; + const identity = parseRemoteUrl(nameMatch[2]); + if (identity === null) continue; + if ( + identity.host === wantHost && + identity.owner === wantOwner && + identity.repo === wantRepo + ) { + matched.push(nameMatch[1]); + } + } + + return { matched }; +} diff --git a/packages/cli/src/commands/review/match-remote.test.ts b/packages/cli/src/commands/review/match-remote.test.ts new file mode 100644 index 00000000000..09e1414ab2b --- /dev/null +++ b/packages/cli/src/commands/review/match-remote.test.ts @@ -0,0 +1,216 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real `git` around the pure core (covered by lib/remote-match.test.ts): +// the repository gate, the `git remote -v` read, and the exit-code contract +// the /review skill's Step 1 branches on. A mocked child_process would pass +// while the real invocation breaks — the class of bug the parse-args suite +// exists for. + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import type { Argv, CommandModule } from 'yargs'; +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const stdoutSpy = vi.hoisted(() => vi.fn((_line: string) => {})); +const stderrSpy = vi.hoisted(() => vi.fn((_line: string) => {})); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: stdoutSpy, + writeStderrLineSafe: stderrSpy, +})); + +import { matchRemoteCommand, runMatchRemote } from './match-remote.js'; + +let repo: string; +let savedCwd: string; + +function git(...args: string[]): void { + execFileSync('git', args, { cwd: repo, stdio: 'ignore' }); +} + +function run(overrides: Record = {}): void { + runMatchRemote({ + owner: 'QwenLM', + repo: 'qwen-code', + host: 'github.com', + ...overrides, + } as never); +} + +beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'match-remote-')); + savedCwd = process.cwd(); + execFileSync('git', ['init', '-q', repo]); + stdoutSpy.mockClear(); + stderrSpy.mockClear(); + process.exitCode = undefined; +}); + +afterEach(() => { + process.chdir(savedCwd); + process.exitCode = undefined; + rmSync(repo, { recursive: true, force: true }); +}); + +describe('runMatchRemote (real git)', () => { + it('prints the matching remote and exits 0', () => { + git('remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'); + process.chdir(repo); + run(); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + }); + + it('picks the upstream in a fork layout', () => { + git('remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'); + git('remote', 'add', 'wenshao', 'git@github.com:wenshao/qwen-code.git'); + process.chdir(repo); + run(); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + }); + + it('prints none and exits 6 when no remote matches', () => { + git('remote', 'add', 'wenshao', 'git@github.com:wenshao/qwen-code.git'); + process.chdir(repo); + run({ owner: 'shao' }); // the substring-decoy owner: must NOT match `wenshao` + expect(stdoutSpy).toHaveBeenCalledWith('none'); + expect(process.exitCode).toBe(6); + }); + + it('prints every match and exits 7 when several remotes serve the repo', () => { + git('remote', 'add', 'upstream', 'https://github.com/QwenLM/qwen-code.git'); + git('remote', 'add', 'mirror', 'git@github.com:QwenLM/qwen-code.git'); + process.chdir(repo); + run(); + expect(stdoutSpy).toHaveBeenCalledWith('upstream'); + expect(stdoutSpy).toHaveBeenCalledWith('mirror'); + expect(process.exitCode).toBe(7); + expect(stderrSpy).toHaveBeenCalledWith(expect.stringContaining('warning:')); + }); + + it('does not match across hosts', () => { + git('remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'); + process.chdir(repo); + run({ host: 'ghe.example.com' }); + expect(stdoutSpy).toHaveBeenCalledWith('none'); + expect(process.exitCode).toBe(6); + }); + + it('exits 1 outside a git repository', () => { + const bare = mkdtempSync(join(tmpdir(), 'match-remote-norepo-')); + try { + process.chdir(bare); + run(); + expect(process.exitCode).toBe(1); + expect(stdoutSpy).not.toHaveBeenCalled(); + // git's own fatal is surfaced, not swallowed behind a fixed guess — + // container CI's `dubious ownership` refusals must read as what they + // are. + expect(stderrSpy).toHaveBeenCalledWith( + expect.stringContaining('git cannot resolve this repository'), + ); + } finally { + process.chdir(savedCwd); + rmSync(bare, { recursive: true, force: true }); + } + }); + + it('resolves from a bare repository too — a mirror checkout is still a git repo', () => { + // The exit-1 contract is "not a git repository / git unavailable"; a + // bare clone is neither — `git remote -v` and the fetch/worktree steps + // the flow runs next all succeed inside one. + const bareRepo = mkdtempSync(join(tmpdir(), 'match-remote-bare-')); + try { + execFileSync('git', ['init', '-q', '--bare', bareRepo]); + execFileSync( + 'git', + ['remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'], + { cwd: bareRepo, stdio: 'ignore' }, + ); + process.chdir(bareRepo); + run(); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + } finally { + process.chdir(savedCwd); + rmSync(bareRepo, { recursive: true, force: true }); + } + }); + + it('falls through to github.com when GH_HOST is empty or whitespace', () => { + // Intentional fall-through (resolveGhHost): a templated CI export of + // GH_HOST= must read as "no host", not route matching at a host named + // "" and hard-stop. + git('remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'); + process.chdir(repo); + const savedGhHost = process.env['GH_HOST']; + try { + for (const empty of ['', ' ']) { + process.env['GH_HOST'] = empty; + stdoutSpy.mockClear(); + run({ host: undefined }); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + } + } finally { + if (savedGhHost === undefined) delete process.env['GH_HOST']; + else process.env['GH_HOST'] = savedGhHost; + } + }); + + it('inherits an operator-exported GH_HOST when --host is absent', () => { + // The bare-PR-number path omits --host; a GHE operator who exports + // GH_HOST must not be rematched against github.com and hard-stopped. + git('remote', 'add', 'origin', 'git@ghe.example.com:QwenLM/qwen-code.git'); + process.chdir(repo); + const savedGhHost = process.env['GH_HOST']; + process.env['GH_HOST'] = 'ghe.example.com'; + try { + run({ host: undefined }); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + } finally { + if (savedGhHost === undefined) delete process.env['GH_HOST']; + else process.env['GH_HOST'] = savedGhHost; + } + }); + + it('resolves from a subdirectory too — git walks up to the checkout', () => { + // The skill runs every subcommand from the main checkout; a run that + // happens to start in a subdirectory must see the same remotes, exactly + // like `git remote -v` typed there. + git('remote', 'add', 'origin', 'git@github.com:QwenLM/qwen-code.git'); + const sub = join(repo, 'packages', 'core'); + mkdirSync(sub, { recursive: true }); + process.chdir(sub); + run(); + expect(stdoutSpy).toHaveBeenCalledWith('origin'); + expect(process.exitCode).toBeUndefined(); + }); +}); + +describe('matchRemoteCommand builder', () => { + // The demandOption guards are the misuse stop: without them a missing + // --owner becomes String(undefined), matches nothing, exits 6 — and the + // skill reads 6 as "go lightweight", silently degrading the review. + it('demands --owner and --repo; --host stays optional', () => { + const opts: Record = {}; + const stub = { + option: (name: string, config: { demandOption?: boolean }) => { + opts[name] = config; + return stub; + }, + } as unknown as Argv; + ((matchRemoteCommand as CommandModule).builder as (y: Argv) => Argv)(stub); + expect(opts['owner']?.demandOption).toBe(true); + expect(opts['repo']?.demandOption).toBe(true); + expect(opts['host']).toBeDefined(); + expect(opts['host']?.demandOption).toBeFalsy(); + }); +}); diff --git a/packages/cli/src/commands/review/match-remote.ts b/packages/cli/src/commands/review/match-remote.ts new file mode 100644 index 00000000000..0e790ceb37f --- /dev/null +++ b/packages/cli/src/commands/review/match-remote.ts @@ -0,0 +1,137 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review match-remote`: which local git remote serves a PR's +// owner/repo. Step 1 of /review needs this twice — to decide whether a +// `pr-url` target gets the worktree flow or lightweight mode, and to pick +// the fetch remote for a bare PR number — and the rule used to live in the +// prompt as prose, where it shipped a substring match (review one repo, post +// to another) and hand-guessed remotes. The rule is now exact-segment +// equality, tested here, and the orchestrator only relays the outcome. +// +// Outcomes: exactly one matching remote — its name on stdout, exit 0. No +// match — `none` on stdout, exit 6 (the lightweight-mode signal). Several — +// every name on stdout, exit 7; picking among them is not this command's +// call, and the review stops rather than guesses (the same rule the prose +// had). 7, not 2 — 2 stays reserved for shell-level misuse, like run's +// 3-not-2 choice. Not a git repository, or git unavailable — exit 1, fail +// closed like the other gates. + +import type { CommandModule } from 'yargs'; +import { resolveGhHost } from './lib/gh.js'; +import { git } from './lib/git.js'; +import { matchRemotes } from './lib/remote-match.js'; +import { + writeStdoutLine, + writeStderrLineSafe, +} from '../../utils/stdioHelpers.js'; + +interface MatchRemoteArgs { + owner: string; + repo: string; + /** Absent means inherit an operator-exported GH_HOST, else github.com. */ + host?: string; +} + +export function runMatchRemote(args: MatchRemoteArgs): void { + // The gate is "git works here", not "this is a work tree": a bare clone + // (mirror/CI-style checkout) serves the whole flow — `git remote -v`, + // fetch-pr's fetch, and `git worktree add` all succeed inside one — so + // `--is-inside-work-tree` printing `false` must not stop the review. + // Failure means git itself refused the repository — carry its fatal to + // stderr (a fixed two-cause guess misreports container CI's `dubious + // ownership` refusals) and fail closed. + try { + git('rev-parse', '--is-inside-work-tree'); + } catch (err) { + writeStderrLineSafe( + `match-remote: git cannot resolve this repository: ${ + (err as Error).message + }`, + ); + process.exitCode = 1; + return; + } + + // resolveGhHost leaves the default to the caller; the matcher's + // comparison needs a concrete host. + const host = resolveGhHost(args.host) ?? 'github.com'; + + let remoteV: string; + try { + remoteV = git('remote', '-v'); + } catch (err) { + writeStderrLineSafe( + `match-remote: \`git remote -v\` failed: ${(err as Error).message}`, + ); + process.exitCode = 1; + return; + } + + const { matched } = matchRemotes(remoteV, { + owner: args.owner, + repo: args.repo, + host, + }); + + // Loud `writeStdoutLine`, not the `*Safe` variant: this line is the + // command's load-bearing result. If the write fails, the orchestrator + // must see a non-zero exit (fail-closed), not exit 0 with empty output. + if (matched.length === 1) { + writeStdoutLine(matched[0]); + return; + } + + if (matched.length === 0) { + writeStdoutLine('none'); + writeStderrLineSafe( + `match-remote: no remote matches ${host}/${args.owner}/${args.repo} ` + + 'by exact host + owner/repo equality — the PR is not served by any ' + + 'remote of this repository.', + ); + process.exitCode = 6; + return; + } + + for (const name of matched) { + writeStdoutLine(name); + } + writeStderrLineSafe( + `warning: ${matched.length} remotes match ${host}/${args.owner}/${args.repo} ` + + `(${matched.join(', ')}); refusing to pick one — the review stops here.`, + ); + process.exitCode = 7; +} + +export const matchRemoteCommand: CommandModule = { + command: 'match-remote', + describe: + 'Print the git remote whose URL matches an owner/repo by exact host + owner/repo equality (exit 6 when none, exit 7 when several)', + builder: (yargs) => + yargs + .option('owner', { + type: 'string', + demandOption: true, + describe: 'The repository owner (from the PR URL, or `gh repo view`)', + }) + .option('repo', { + type: 'string', + demandOption: true, + describe: 'The repository name', + }) + .option('host', { + type: 'string', + describe: + "The PR's host — from its URL, or from `gh repo view` for a bare number (omitted: inherit an operator-exported GH_HOST, else github.com)", + }), + handler: (argv) => { + runMatchRemote({ + owner: String(argv['owner']), + repo: String(argv['repo']), + host: (argv as { host?: string }).host, + }); + }, +}; diff --git a/packages/cli/src/commands/review/publish-assets.test.ts b/packages/cli/src/commands/review/publish-assets.test.ts index a769663f017..843f762ae64 100644 --- a/packages/cli/src/commands/review/publish-assets.test.ts +++ b/packages/cli/src/commands/review/publish-assets.test.ts @@ -24,14 +24,18 @@ const ghWithInputMock = vi.hoisted(() => const ghWithInputPlainMock = vi.hoisted(() => vi.fn((_input: string, ..._rest: string[]) => ''), ); -vi.mock('./lib/gh.js', () => ({ - gh: ghMock, - // Two DISTINCT mocks: aliasing them hid which function a write actually - // used, and the retry-vs-not split is the point of having two. - ghWithInput: ghWithInputPlainMock, - ghWithInputRetried: ghWithInputMock, - setGhHost: setGhHostMock, -})); +vi.mock('./lib/gh.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + gh: ghMock, + // Two DISTINCT mocks: aliasing them hid which function a write actually + // used, and the retry-vs-not split is the point of having two. + ghWithInput: ghWithInputPlainMock, + ghWithInputRetried: ghWithInputMock, + setGhHost: setGhHostMock, + }; +}); const setGhHostMock = vi.hoisted(() => vi.fn((_h: string) => {})); diff --git a/packages/cli/src/commands/review/publish-assets.ts b/packages/cli/src/commands/review/publish-assets.ts index 6f137f9bc53..90ef5f49dd4 100644 --- a/packages/cli/src/commands/review/publish-assets.ts +++ b/packages/cli/src/commands/review/publish-assets.ts @@ -37,7 +37,7 @@ import { createHash } from 'node:crypto'; import { readFileSync, statSync, writeFileSync, mkdirSync } from 'node:fs'; import { basename, dirname, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { gh, ghWithInputRetried, setGhHost } from './lib/gh.js'; +import { gh, ghWithInputRetried, resolveGhHost, setGhHost } from './lib/gh.js'; import { reviewWriteAuthorization } from './lib/authorization.js'; import { ASSET_HEADER_BYTES, @@ -234,14 +234,10 @@ export function runPublishAssets(args: PublishAssetsArgs): void { const repo = repoResult.repo; // ONE effective host, resolved BEFORE the gate: the gate must bind the - // host the write will actually route at — --host, else an operator-exported - // GH_HOST, else github.com — not merely the flag. Binding args.host while - // routing at effectiveHost let a GH_HOST-driven Enterprise write pass a - // github.com authorisation; caught by this skill's own review. - // `|| undefined`, not `??`: an exported-but-empty GH_HOST ("" survives - // `??`, being non-nullish) must read as "no host". - const effectiveHost = - args.host ?? (process.env['GH_HOST']?.trim() || undefined); + // host the write will actually route at, not merely the flag. Binding + // args.host while routing at effectiveHost let a GH_HOST-driven Enterprise + // write pass a github.com authorisation; caught by this skill's own review. + const effectiveHost = resolveGhHost(args.host); // ── Gate 2: an authorised run — the same gate as `submit` ───────────────── // The PR identity used for authorisation binding is the PR the evidence is diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index a667a11c431..49c6cd90e2a 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -29,11 +29,15 @@ const ghMock = vi.hoisted(() => vi.fn((_payload: string, ..._rest: string[]) => ''), ); const ghViewMock = vi.hoisted(() => vi.fn((..._args: string[]) => '')); -vi.mock('./lib/gh.js', () => ({ - ghWithInput: ghMock, - gh: ghViewMock, - setGhHost: vi.fn(), -})); +vi.mock('./lib/gh.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + ghWithInput: ghMock, + gh: ghViewMock, + setGhHost: vi.fn(), + }; +}); const writeStdoutSpy = vi.hoisted(() => vi.fn((_line: string) => {})); vi.mock('../../utils/stdioHelpers.js', () => ({ diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index d3d96b7e761..2da1b6f1e5b 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -48,7 +48,7 @@ import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; import { mkdirSync, readFileSync } from 'node:fs'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { getCliVersion } from '../../utils/version.js'; -import { ghWithInput, setGhHost } from './lib/gh.js'; +import { ghWithInput, resolveGhHost, setGhHost } from './lib/gh.js'; import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; import { parseReceiptIds } from './lib/receipt.js'; import { composeReview, type ComposeReviewInput } from './compose-review.js'; @@ -170,12 +170,8 @@ function authorization(args: SubmitArgs): { ok: boolean; why: string } { repo: args.repo, // The EFFECTIVE host, not merely the flag: with --host absent the gh // child inherits an operator-exported GH_HOST, so that is where this - // write would route — and what the gate must bind. Same resolution as - // publish-assets. - // `|| undefined`, not `??`: an exported-but-empty GH_HOST ("" survives - // `??`, being non-nullish) must read as "no host", not as a host named - // "" that fails every comparison. - host: args.host ?? (process.env['GH_HOST']?.trim() || undefined), + // write would route — and what the gate must bind. + host: resolveGhHost(args.host), }); } diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b13779cbaf2..6ff28dcbefa 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -80,7 +80,15 @@ At every effort level, the mechanics of obtaining the diff — worktree flow, di The parser already classified the target, so there is nothing to disambiguate by hand. For a `pr-url` target, determine if the local repo can access this PR: -1. Check if any git remote matches the URL's **host and owner/repo — by exact segment equality, never substring**: run `git remote -v` and parse each remote URL structurally (`git@:/.git` and `https:////(.git)` are the two shapes). A remote matches only when its host equals the verdict's `host` AND its `/` (with any `.git` suffix stripped) equals the verdict's `owner/repo`, both compared case-insensitively as whole segments — `shao/qwen-code` does NOT match a `wenshao/qwen-code` remote, and a `github.com` PR does not match a same-named repo on another host. Substring "contains" matching once allowed exactly those, which is reviewing one repository and posting to another. This still handles forks — a local clone with an `upstream` remote pointing to the target repository matches that repository's PRs exactly. +1. Run the remote matcher — it applies the exact host + owner/repo segment-equality rule in code, and you do not re-derive it (a substring comparison once matched `shao/qwen-code` against a `wenshao/qwen-code` remote — one review read one repository and posted to another; a `github.com` PR matching a same-named repo on another host is the same bug wearing a host): + + ```bash + "${QWEN_CODE_CLI:-qwen}" review match-remote \ + --owner --repo --host + ``` + + Exit 0 prints the matching remote's name — forks included: a clone whose `upstream` points to the target repository matches that repository's PRs exactly. Exit 6 means no remote matches — go to item 3. Exit 7 means several match; tell the user and stop rather than picking one. Any other exit is fail-closed like the other gates: report it and stop. + 2. If a matching remote is found, proceed with the **normal worktree flow** — use that remote name (instead of hardcoded `origin`) for `git fetch pull//head:qwen-review/pr-`. In Step 7, use the owner/repo from the URL for posting comments. For a `pr-url` whose `host` is not `github.com` (GitHub Enterprise), **pass `--host ` to every review subcommand that talks to GitHub — `fetch-pr`, `pr-context`, `comment-status`, `presubmit`, and `compose-review`** — which routes all of their `gh` calls via GH_HOST in code; a forgotten host cannot silently retarget them at github.com. The `gh` commands you run directly are still yours to route: prefix Agent 0's `gh pr view`/`gh issue view`, Step 6's residual body fetch, and the Step 7 submission with `GH_HOST= ` (e.g. `GH_HOST=github.example.com gh api ...`). `gh` defaults to `github.com`, so a dropped host makes a call read from and post to the wrong site's `owner/repo`. @@ -114,12 +122,21 @@ Based on the parsed `target.type`: **Where `/` and `` come from — do not guess either.** For a `pr-url` target both are already decided: the URL carries the owner/repo, and the remote is the one matched against it above. For a bare **`pr-number`** there is no URL, and a PR number alone says nothing about which repository it belongs to. Derive it: ```bash - gh repo view --json owner,name --jq '"\(.owner.login)/\(.name)"' + gh repo view --json owner,name,url \ + --jq '"\(.owner.login)/\(.name) \(.url | sub("^[a-z]+://"; "") | split("/")[0])"' + ``` + + That is the same `gh repo view` resolution Step 7 already uses to decide where to post — it resolves through `gh`'s default-repo, which in a fork clone is the **upstream**, where the PR actually lives — and the second token is the host `gh` resolved that repo at (the URL's authority; an explicit port survives, and the matcher strips it). Pass that host to the matcher: `gh` also resolves a host through its own auth config (`gh auth login --hostname …`, no GH_HOST exported), which the matcher cannot see, so omitting `--host` would compare such a GHE repo against the github.com default and stop at exit 6 even though every later `gh` call routes at the GHE host. Then resolve the remote with the same matcher Step 1's pr-url path uses — same rule, same exit codes: + + ```bash + "${QWEN_CODE_CLI:-qwen}" review match-remote \ + --owner --repo \ + --host ``` - That is the same command Step 7 already uses to decide where to post, and it resolves through `gh`'s default-repo — which in a fork clone is the **upstream**, where the PR actually lives. Then pick the remote **whose URL is that owner/repo**, by the same exact-segment parse of `git remote -v` described above. Do not default to `origin`: in the standard fork layout `origin` is the _fork_, which has no `pull//head` ref for an upstream PR, and `fetch-pr` fails. In an upstream-as-`origin` clone the same rule lands on `origin` anyway, so one procedure is correct for both. + Do not default to `origin`: in the standard fork layout `origin` is the _fork_, which has no `pull//head` ref for an upstream PR, and `fetch-pr` fails. In an upstream-as-`origin` clone the matcher lands on `origin` anyway, so one procedure is correct for both. - Guessing the owner/repo here is not a recoverable mistake — a guessed repo has already stopped a review before it read a line of code (measured; DESIGN.md — The guessed fork repo). If `gh repo view` and the remote scan disagree, or no remote matches, say so and stop rather than picking one. + Guessing the owner/repo here is not a recoverable mistake — a guessed repo has already stopped a review before it read a line of code (measured; DESIGN.md — The guessed fork repo). If `gh repo view` fails, or the matcher exits 6 (no remote matches) or 7 (several do), say so and stop rather than picking one. Read `.qwen/tmp/qwen-review-pr--fetch.json` for: `worktreePath`, `baseRefName`, `headRefName`, `fetchedSha` (use as the **HEAD commit SHA** for Step 7), `isCrossRepository`, `diffStat` (files / additions / deletions), `emptyDiff` (**stop here**: the branch tree is byte-identical to its merge base — the work already landed or was superseded; tell the user and recommend close-as-superseded instead of fanning out agents over zero hunks), `collapsedFromUpstream` (disclose in the summary: overlapping merged PRs have collapsed this one to a residual — the review scope is the recomputed diff, and body claims about the rest are description-of-history, which Agent 0 should read accordingly), and `prDescriptionHasHan` (the PR description contains Chinese — every posted inline comment must then be bilingual; see Step 7). If the command fails (auth, network, PR not found), inform the user and stop. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index e4d059ff660..20767227316 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -80,4 +80,21 @@ describe('bundled review skill', () => { expect(body).toContain('`fetch-pr` before all of them'); expect(body).toContain('`agent-prompt --roster` after the rules load'); }); + + it('routes both remote-resolution paths through match-remote', () => { + // The pr-url path (Step 1) and the bare-PR-number path both resolve the + // remote via the deterministic matcher. A later edit reverting either + // hunk to the old model-prose rule must fail a test, not slip through. + const body = skillBody(); + const invocations = + body.match(/"\$\{QWEN_CODE_CLI:-qwen\}" review match-remote/g) ?? []; + expect(invocations).toHaveLength(2); + // The bare-number path threads the host `gh repo view` resolved at — + // dropping it rematches auth-config-only GHE clones against github.com. + expect(body).toContain('--host '); + expect(body).toContain('Exit 6 means no remote matches'); + expect(body).toContain( + 'the matcher exits 6 (no remote matches) or 7 (several do)', + ); + }); });