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
228 changes: 228 additions & 0 deletions docs/design/review-cpu-for-tokens.md

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/cli/src/commands/review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ describe('reviewCommand', () => {
expect(registeredSubcommands()).toEqual([
'run',
'parse-args',
'match-remote',
'fetch-pr',
'capture-local',
'plan-diff',
Expand Down
4 changes: 3 additions & 1 deletion packages/cli/src/commands/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -47,6 +48,7 @@ export const reviewCommand: CommandModule = {
yargs
.command(runCommand)
.command(parseArgsCommand)
.command(matchRemoteCommand)
.command(fetchPrCommand)
.command(captureLocalCommand)
.command(planDiffCommand)
Expand Down Expand Up @@ -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: () => {
Expand Down
17 changes: 17 additions & 0 deletions packages/cli/src/commands/review/lib/gh.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ vi.mock('node:child_process', () => ({

import {
ghEnv,
resolveGhHost,
setGhHost,
parseNdjson,
gh,
Expand Down Expand Up @@ -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 <path> --jq '.check_runs[]'` applies the jq per page
Expand Down
20 changes: 20 additions & 0 deletions packages/cli/src/commands/review/lib/gh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

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 documented precedence of resolveGhHost — an explicit --host flag wins over an operator-exported GH_HOST — is pinned by no test. Verified by mutation at this commit: flipping the expression to (process.env['GH_HOST']?.trim() || undefined) ?? flagHost lets all 143 tests across the five affected suites (match-remote, remote-match, gh, publish-assets, submit) pass; the suggested conflict test passes on shipped code and fails under the mutant. No test sets both GH_HOST and a different explicit --host. — Failure scenario: a GHE operator exports GH_HOST and reviews a github.com PR URL; match-remote receives --host github.com from the verdict; a flipped resolver compares against the GHE host instead, finds no match, exits 6 — the worktree flow silently demotes to lightweight mode; on the submit/publish-assets side the authorisation gate would bind the wrong host.

Add one conflict test (e.g. in gh.test.ts):

process.env['GH_HOST'] = 'ghe.example.com';
expect(resolveGhHost('github.com')).toBe('github.com');
中文说明

resolveGhHost 文档中声明的优先级——显式 --host 标志优先于操作者导出的 GH_HOST——没有任何测试钉住。已在本 commit 上通过变异验证:把表达式翻转为 (process.env['GH_HOST']?.trim() || undefined) ?? flagHost 后,五个相关测试套件(match-remote、remote-match、gh、publish-assets、submit)的全部 143 个测试依然通过;而建议的冲突测试在现有代码上通过、在该变异体下失败。没有任何测试同时设置 GH_HOST 与一个不同的显式 --host。故障场景:GHE 操作者导出了 GH_HOST 并审查一个 github.com 的 PR URL;match-remote 从 verdict 收到 --host github.com;被翻转的解析器改为与 GHE host 比较,匹配不到,退出 6——worktree 流程被静默降级为 lightweight 模式;在 submit/publish-assets 一侧,授权门会绑定错误的 host。

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

}

/**
* Environment for `gh` child processes. `undefined` means "inherit the
* parent env untouched"; with a host set, the inherited env is extended
Expand Down
280 changes: 280 additions & 0 deletions packages/cli/src/commands/review/lib/remote-match.test.ts
Original file line number Diff line number Diff line change
@@ -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');
Comment on lines +95 to +97

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] normalizeSegment's lowercase-then-strip ordering is unpinned — every test input uses a lowercase .git suffix, so a strip-before-lowercase mutant survives (verified by mutation at this commit: 39/39 tests pass across both match suites with the mutant). The discriminating input is any segment ending in uppercase .GIT: normalizeSegment('Repo.GIT') yields 'repo' under the shipped code but 'repo.git' under the mutant. — Failure scenario: a hand-added remote URL or --repo value ending in uppercase .GIT (e.g. git@github.com:Owner/Repo.GIT); the current code handles it correctly, but if a future refactor flipped the order, the suffix would survive normalization, the exact-segment comparison would fail, and match-remote would exit 6 — a silent demotion to lightweight mode.

Suggested change
it('lowercases and strips one trailing .git', () => {
expect(normalizeSegment('QwenLM')).toBe('qwenlm');
expect(normalizeSegment('qwen-code.git')).toBe('qwen-code');
it('lowercases and strips one trailing .git', () => {
expect(normalizeSegment('QwenLM')).toBe('qwenlm');
expect(normalizeSegment('qwen-code.git')).toBe('qwen-code');
expect(normalizeSegment('QWEN-CODE.GIT')).toBe('qwen-code');
中文说明

normalizeSegment 先转小写、后剥 .git 的顺序没有被测试钉住——所有测试输入都使用小写的 .git 后缀,因此"先剥后缀再转小写"的变异体可以存活(已在本 commit 上变异验证:两个匹配测试套件的 39/39 个测试在该变异体下仍全部通过)。区分性输入是任何以大写 .GIT 结尾的段:normalizeSegment('Repo.GIT') 在现有代码下得到 'repo',在该变异体下得到 'repo.git'。故障场景:手工添加的 remote URL 或以大写 .GIT 结尾的 --repo 值(如 git@github.com:Owner/Repo.GIT);当前代码处理正确,但若未来重构翻转了顺序,后缀会残留穿过归一化,导致精确分段比较失败、match-remote 退出 6——静默降级为 lightweight 模式。

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

// 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 <remote> pull/<n>/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
// `<name>\t<url> (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([]);
});
});
Loading
Loading