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
4 changes: 2 additions & 2 deletions docs/design/review-repository-context.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ A repository may provide strict JSON at `.qwen/review-context.json`:
}
```

The top-level fields are exactly `version`, `label`, and `rules`. Each rule requires `paths`; all other rule fields are optional. Unknown or missing required fields, comments, unsupported versions, oversized values, control characters, and duplicate array entries are rejected. Arrays are human-authored and may be written in any order; values from all matching rules are merged, deduplicated, and returned sorted and unique (the internal wire format keeps the strict sorted-and-unique check). Rule order is preserved. The total `paths` globs across all rules, the merged `relatedPaths` glob list, and every merged field are capped at the wire bounds and rejected fail-closed, so a matching burst cannot stall the step or outgrow the contract. Note the example's `relatedPaths` wildcard is scoped to one subsystem on purpose: wildcard `relatedPaths` are subject to the 128 resolved-file bound below, and a repository-wide scope like `packages/*/src/**` exceeds it on a repository this size.
The top-level fields are exactly `version`, `label`, and `rules`. Each rule requires `paths`; all other rule fields are optional. Unknown or missing required fields, comments, unsupported versions, oversized values, control characters, and duplicate array entries are rejected. Arrays are human-authored and may be written in any order; values from all matching rules are merged, deduplicated, and returned sorted and unique (the internal wire format keeps the strict sorted-and-unique check). Rule order is preserved. The total `paths` globs across all rules, the merged `relatedPaths` glob list, and every merged field are capped at the wire bounds and rejected fail-closed, so a matching burst cannot stall the step or outgrow the contract. Note the example's `relatedPaths` wildcard is scoped to one subsystem on purpose: wildcard `relatedPaths` are subject to the 256 resolved-file bound below, and a repository-wide scope like `packages/*/src/**` exceeds it on a repository this size.

`paths` and `relatedPaths` use repository-relative `/`-separated globs. Matching is case-sensitive on every platform and `?` consumes one UTF-16 code unit. The supported metacharacters are `*`, `?`, and a complete `**` path segment. Absolute paths, backslashes, empty or `.`/`..` segments, negation, brace expansion, character classes, and extended glob syntax are rejected.

A rule matches when any changed path matches one of its `paths` globs. If no rule matches, the provider returns no context. A matching rule's deduplicated `relatedPaths` globs are expanded from the worktree with dot files enabled, directory results disabled, symlink traversal disabled, and case-sensitive matching. Related globs containing wildcards must start with a non-wildcard directory segment so expansion cannot begin with a repository-wide wildcard; a completely static entry resolves to itself when it exists as a regular file. Globs whose path enters a dependency or build-output directory at any depth are rejected at validation (compared case-insensitively, on every platform), so the never-descend invariant holds for scan roots as well as recursion. Changed paths are removed from the result. Resolved files must remain inside the worktree. Expansion never descends into dependency and build-output trees (`node_modules`, `dist`, and the other conventional names the scan skips) and fails closed when any limit is exceeded: 16384 visited entries across the scan (files and directories, matching or not — calibrated on this repository's installed checkout, so a honestly scoped subtree, including all of `packages/`, never trips it), 128 resolved files in the result, and a matching-work budget charged per attempted pattern match (pattern length times path length) in both the rule filter and the expansion, which reports the matching-work limit and keeps a matching burst from stalling the step.
A rule matches when any changed path matches one of its `paths` globs. If no rule matches, the provider returns no context. A matching rule's deduplicated `relatedPaths` globs are expanded from the worktree with dot files enabled, directory results disabled, symlink traversal disabled, and case-sensitive matching. Related globs containing wildcards must start with a non-wildcard directory segment so expansion cannot begin with a repository-wide wildcard; a completely static entry resolves to itself when it exists as a regular file. Globs whose path enters a dependency or build-output directory at any depth are rejected at validation (compared case-insensitively, on every platform), so the never-descend invariant holds for scan roots as well as recursion. Changed paths are removed from the result. Resolved files must remain inside the worktree. Expansion never descends into dependency and build-output trees (`node_modules`, `dist`, and the other conventional names the scan skips) and fails closed when any limit is exceeded: 16384 visited entries across the scan (files and directories, matching or not — calibrated on this repository's installed checkout, so a honestly scoped subtree, including all of `packages/`, never trips it), 256 resolved files in the result, and a matching-work budget charged per attempted pattern match (pattern length times path length) in both the rule filter and the expansion, which reports the matching-work limit and keeps a matching burst from stalling the step.

## Trust boundary

Expand Down
10 changes: 10 additions & 0 deletions docs/users/configuration/settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,16 @@ Settings are organized into categories. Most settings should be placed within th
| `output.format` | string | The format of the CLI output. | `"text"` | `"text"`, `"json"` |
| `output.showTimestamps` | boolean | Show an `[HH:MM:SS]` timestamp before each assistant response. | `false` | |

#### review

| Setting | Type | Description | Default |
| -------------------- | ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- |
| `review.attribution` | boolean | Append the attribution footer naming the model and CLI version (e.g. `_— qwen3-coder via Qwen Code /review (v0.21.2)_`) to review bodies and inline comments posted by `/review`. Disable to post reviews without AI attribution. With the footer off, presubmit duplicate detection still recognizes earlier posts by the same GitHub account, but footer-less posts from other accounts escape it. | `true` |

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] R3-4: The new review.* settings documentation omits that all three settings are read from operator scopes only — a workspace .qwen/settings.json is silently ignored (skipWorkspaceSettings: true in lib/review-settings.ts) — even though this page documents scope restrictions for other settings (e.g. security.allowedInsecureVoiceBaseUrls: "Only User, System, and SystemDefaults scopes are honored"). — Failure scenario: an operator follows this page and places review.attribution: false (or review.comment: true, review.effort) in the repository's .qwen/settings.json — the natural location for per-repo review policy, and a scope the page's own file-type table lists as valid. The value is silently ignored with no warning anywhere: the attribution footer keeps posting, auto-commenting never activates, and nothing tells the user the scope they chose does not count. The same omission is on docs/users/features/code-review.md:186 ("in settings.json", unqualified).

Suggested fix: add the scope caveat to the review table (e.g. "Read from operator scopes only (system/user); a repository's .qwen/settings.json cannot set these") and qualify the code-review.md sentence to the user-level file.

中文说明

问题: 新增的 review.* 配置文档未说明三个配置都只从操作者作用域读取——工作区 .qwen/settings.json 会被静默忽略(lib/review-settings.ts 中的 skipWorkspaceSettings: true)——尽管本页对其他配置已有作用域限制的说明(如 security.allowedInsecureVoiceBaseUrls:"Only User, System, and SystemDefaults scopes are honored")。

失败场景: 运维者按本页文档把 review.attribution: false(或 review.comment: truereview.effort)写进仓库的 .qwen/settings.json——这是 per-repo review 策略的自然位置,且本页的文件类型表也把它列为有效作用域。该值被静默忽略、无任何告警:署名 footer 继续发布、自动评论永不生效,没有任何提示告知用户所选作用域无效。docs/users/features/code-review.md:186 存在同样遗漏("in settings.json"未加限定)。

建议修复:review 表格补充作用域说明(如"仅从操作者作用域(system/user)读取;仓库的 .qwen/settings.json 无法设置这些配置"),并把 code-review.md 的表述限定到用户级文件。

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

| `review.effort` | enum | Default effort for `/review` when `--effort` is not given: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit `--effort` wins; an effective `--comment` still forces high and `--fix` still floors at medium. | `"auto"` |
| `review.comment` | boolean | Treat every PR `/review` as if `--comment` was passed: findings are posted to the pull request without the flag. The post still binds to the PR named in the invocation. Enable only if you always want reviews published. | `false` |

These settings are read from operator scopes only (User, System, and SystemDefaults); values in a workspace `.qwen/settings.json` are ignored, so a repository cannot set review policy for its reviewers.

#### ui

| Setting | Type | Description | Default |
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 @@ -183,7 +183,7 @@ Or, after running `/review 123`, type `post comments` to publish findings withou
- Where the fix is a single localized edit, a ` ```suggestion ` block you can apply in one click
- For Approve/Request changes verdicts: a review summary with the verdict
- For Comment verdict with all inline comments posted: no separate summary (inline comments are sufficient)
- Model and CLI version attribution footer on each comment (e.g., _— qwen3-coder via Qwen Code /review (v0.21.2)_)
- Model and CLI version attribution footer on each comment (e.g., _— qwen3-coder via Qwen Code /review (v0.21.2)_); set `review.attribution` to `false` in your user or system `settings.json` (the workspace `.qwen/settings.json` is ignored for `review.*` settings) to post without it

**What stays terminal-only:**

Expand Down
96 changes: 96 additions & 0 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,36 @@ vi.mock('../../utils/stdioHelpers.js', () => ({
vi.mock('../../utils/version.js', () => ({
getCliVersion: vi.fn().mockResolvedValue('0.21.2'),
}));
// The handler reads `review.attribution` from the operator's real
// settings.json — pin it, or a developer running with the switch off
// reddens every handler-level footer assertion below.
const reviewSettingsMock = vi.hoisted(() =>
vi.fn((): Record<string, unknown> => ({})),
);
vi.mock('../../config/settings.js', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../../config/settings.js')>();
return {
...actual,
// The production call carries `{ skipWorkspaceSettings: true }` — the
// attribution switch resolves from operator scopes only. A caller that
// forgets the flag reads the workspace-polluted view below instead, and
// the handler assertions redden: a repository's `.qwen/settings.json`
// must not control it.
loadSettings: vi.fn((...callArgs: unknown[]) => {
const opts = callArgs[1] as
| { skipWorkspaceSettings?: boolean }
| undefined;
return {
merged: {
review: opts?.skipWorkspaceSettings
? reviewSettingsMock()
: { attribution: false, comment: true, effort: 'low' },
},
};
}),
};
});
import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';

const runComposeReviewCommand = (argv: unknown): Promise<void> =>
Expand Down Expand Up @@ -71,6 +101,7 @@ let DIFF: string;
let DIFF_HASH: string;

beforeEach(() => {
reviewSettingsMock.mockReturnValue({});
dir = mkdtempSync(join(tmpdir(), 'compose-cov-'));
ENV = { QWEN_CODE_PROJECT_DIR: dir, QWEN_CODE_SESSION_ID: 'S1' };
mkdirSync(join(dir, 'subagents', 'S1'), { recursive: true });
Expand Down Expand Up @@ -424,6 +455,40 @@ describe('composeReview — the C/S table', () => {
).toBe(true);
});

it('omits the footer entirely when attribution is off', () => {
const r = composeReview(base({}), '0.21.2', false);
expect(r.body).toBe('No issues found. LGTM! ✅');
expect(r.body).not.toContain(MODEL);
});

it('attribution off: a missing modelId is no error — its only consumer is gated off', () => {
// Before the gate, an attribution-off run still died over the field the
// footer — provably never rendered — names.
const r = composeReview(base({ modelId: '' }), '0.21.2', false);
expect(r.body).toBe('No issues found. LGTM! ✅');
});

it('attribution off: a footer-unsafe modelId composes — nothing renders it', () => {
const r = composeReview(
base({ modelId: 'evil\nvia Qwen Code /review' }),
'0.21.2',
false,
);
expect(r.body).toBe('No issues found. LGTM! ✅');
});

it('attribution on: a missing modelId is still refused', () => {
expect(() => composeReview(base({ modelId: '' }), '0.21.2')).toThrow(
/modelId is required/,
);
});

it('attribution on: a footer-unsafe modelId is still refused', () => {
expect(() =>
composeReview(base({ modelId: 'evil\nmodel' }), '0.21.2'),
).toThrow(/single line/);
});

it('C=0, S≥1 → COMMENT with the no-blockers opener', () => {
const r = composeReview(base({ suggestionsInline: 2 }));
expect(r.event).toBe('COMMENT');
Expand Down Expand Up @@ -1663,6 +1728,37 @@ describe('composeReviewCommand handler (the CLI glue)', () => {
).toBe(true);
});

it('honours review.attribution=false through the handler (wiring)', async () => {
// Third wiring leg: deleting the attribution argument from the
// composeReviewCommand call leaves the direct composeReview test and the
// submit handler test green, while the persisted/terminal verdict still
// carries the footer the setting exists to remove.
const dir = mkdtempSync(join(tmpdir(), 'compose-attribution-'));
const inputPath = join(dir, 'compose.json');
const commentsPath = join(dir, 'comments.json');
const outPath = join(dir, 'composed.json');
writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8');
writeFileSync(commentsPath, '[]', 'utf8');
reviewSettingsMock.mockReturnValue({ attribution: false });
try {
await runComposeReviewCommand({
input: inputPath,
comments: commentsPath,
out: outPath,
});
const written = JSON.parse(
readFileSync(outPath, 'utf8'),
) as ComposeReviewResult;
// No plan in this minimal state, so the coverage gate caps the body —
// the assertion is on what the wiring leg controls: the footer.
expect(written.body).not.toBe('');
expect(written.body).not.toContain('via Qwen Code /review');
expect(written.body).not.toContain(MODEL);
} finally {
rmSync(dir, { recursive: true, force: true });
}
});

it('pins the persisted footer to the inherited startup version, not the resolved one', async () => {
// Same pin as `submit`: a shared runner rewrites installs under running
// processes, so the version resolved at compose time can disagree with
Expand Down
49 changes: 22 additions & 27 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,12 @@ import {
type DraftedComment,
} from './lib/inline-counts.js';
import {
FOOTER_MARKER,
REVIEW_FOOTER_RE,
footerVersion,
isFooterSafeModelId,
reviewFooter,
stripReviewFooter,
} from './lib/review-footer.js';
import { operatorReviewSettings } from './lib/review-settings.js';

export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT';

Expand Down Expand Up @@ -456,17 +456,6 @@ function toStringList(value: unknown, field: string): string[] {
return [...(value as string[])];
}

function stripReviewFooter(entry: string): string {
// Guarded on the marker: the strip regex opens `\s*` under an unanchored
// search, which scans quadratically on a long whitespace run in an entry
// that carries no footer at all — and these entries are model-written
// with no length cap (measured ~20 s at 80k characters). An entry
// without the marker has nothing to strip.
return entry.includes(FOOTER_MARKER)
? entry.replace(REVIEW_FOOTER_RE, '')
: entry;
}

// Booleans get the same boundary treatment as the counts: the JSON is
// model-written, and a stringified `"false"` is truthy — it once stood to
// fire the downgrade sentence on a review that was never downgraded, and to
Expand All @@ -484,8 +473,9 @@ function toBool(value: unknown, field: string): boolean {
export function composeReview(
input: ComposeReviewInput,
cliVersion = 'unknown',
attribution = true,
): ComposeReviewResult {
const result = composeReviewBody(input, cliVersion);
const result = composeReviewBody(input, cliVersion, attribution);
// The ledger marker rides the body THIS function returns, because this — not
// the CLI handler — is what `submit` calls and posts. Appending it in the
// handler left the feature inert end to end: the marker reached only the
Expand Down Expand Up @@ -549,6 +539,7 @@ function ledgerMarkerFor(input: ComposeReviewInput): string | null {
function composeReviewBody(
input: ComposeReviewInput,
cliVersion: string,
attribution: boolean,
): ComposeReviewResult {
const criticalsInline = toCount(input.criticalsInline, 'criticalsInline');
const suggestionsInline = toCount(
Expand Down Expand Up @@ -1031,17 +1022,21 @@ function composeReviewBody(
'presubmit.downgradeReasons',
);
const modelId: unknown = input.modelId;
if (typeof modelId !== 'string' || modelId.trim() === '') {
throw new TypeError(
'compose-review: modelId is required (the public footer names the reviewing model)',
);
}
if (!isFooterSafeModelId(modelId)) {
throw new TypeError(
'compose-review: modelId is interpolated into the public footer ' +
'verbatim — it must be a single line that does not contain the ' +
'footer marker',
);
let footer = '';
if (attribution) {
if (typeof modelId !== 'string' || modelId.trim() === '') {
throw new TypeError(
'compose-review: modelId is required (the public footer names the reviewing model)',
);
}
if (!isFooterSafeModelId(modelId)) {
throw new TypeError(
'compose-review: modelId is interpolated into the public footer ' +
'verbatim — it must be a single line that does not contain the ' +
'footer marker',
);
}
footer = reviewFooter(modelId, cliVersion);
}

// `C` counts every Critical the review posts anywhere — inline or body.
Expand Down Expand Up @@ -1160,7 +1155,6 @@ function composeReviewBody(
}
}

const footer = reviewFooter(modelId, cliVersion);
// Bilingual rendering: when the plan (fetch-pr's report) says the PR
// description contains Han characters, the posted body carries the complete
// Chinese version collapsed under the English one — the shape this repo's
Expand All @@ -1180,7 +1174,7 @@ function composeReviewBody(
bilingual && zh !== en
? `${en}\n\n<details>\n<summary>中文说明</summary>\n\n${zh}\n\n</details>`
: en;
return `${text}\n\n${footer}`;
return footer === '' ? text : `${text}\n\n${footer}`;
};

// Clause 6 — scope nobody reviewed. Legal on COMMENT and (alongside body
Expand Down Expand Up @@ -2317,6 +2311,7 @@ export const composeReviewCommand: CommandModule = {
// compose time — a shared runner can rewrite the install mid-session.
footerVersion(process.env['QWEN_CODE_STARTUP_VERSION']) ??
(await getCliVersion()),
operatorReviewSettings().attribution,
);
// The exact terminal verdict, persisted beside the fields it is computed
// from. `event` + `cappedBy` alone cannot reconstruct it — a presubmit
Expand Down
Loading
Loading