From 8bc006d1077c3f46c4d685f709bd9057a87f25ce Mon Sep 17 00:00:00 2001 From: tly Date: Mon, 24 Aug 2026 15:35:08 +0800 Subject: [PATCH 1/5] feat(review): remember explicit effort per project --- docs/users/configuration/settings.md | 2 +- .../cli/src/commands/review/lib/paths.test.ts | 17 ++ packages/cli/src/commands/review/lib/paths.ts | 21 +- .../src/commands/review/parse-args.test.ts | 204 +++++++++++++++++- .../cli/src/commands/review/parse-args.ts | 126 ++++++++--- packages/cli/src/config/settingsSchema.ts | 2 +- .../core/src/skills/bundled/review/SKILL.md | 8 +- .../src/skills/bundled/review/SKILL.test.ts | 10 + 8 files changed, 348 insertions(+), 42 deletions(-) diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index 080541d0d19..d3cdb072355 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -112,7 +112,7 @@ Settings are organized into categories. Most settings should be placed within th | 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 visible AI attribution: the footer is omitted and posted comments and body lists lose their `**[Critical]**`/`**[Suggestion]**` markers. The posts stay identifiable in the raw source: each carries an invisible severity marker (``) and the review body carries a ledger marker (``) — anything reading comment bodies (GitHub API automation, the workflows this setting couples to) still recognizes a `/review` artifact, and presubmit duplicate detection recognizes the reviewing account's earlier posts by the severity marker, though unattributed posts from other accounts escape it. Another consequence: qwen-autofix's Critical-only mode (engaged after round 5, or earlier when a counting window's diff-growth budget trips) no longer recognizes the posted findings as Critical and defers them. Disabling also withholds the model from the machine-ledger marker embedded in the review body, so in a fresh environment (CI, another clone — anywhere without a review cache) the incremental anchor recovered from the last posted review fails the same-model check and the re-review falls back to full-range. | `true` | -| `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.effort` | enum | Default effort for `/review` when neither `--effort` nor a project-remembered explicitly typed level applies: `"low"`, `"medium"`, `"high"`, or `"auto"` (the built-in rule: high for PRs, medium for local changes). An explicit or remembered level 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` | | `review.severityFloor` | enum | The lowest severity a PR `/review` posts when `--severity-floor` is not given: `"auto"` (the round-adaptive default — Suggestions post through round 5, only Criticals from round 6, with otherwise-postable high-confidence Suggestions recorded and deferred, and rounds 2–5 deferring new Suggestions on code unchanged since the previous round; low-confidence and Nice-to-have findings stay terminal-only), `"critical"` (that posture from round 1), or `"suggestion"` (Suggestions post at every round; turns the convergence posture off). Non-PR targets have no rounds and ignore this. | `"auto"` | | `review.reverseAuditRounds` | number | Lower the reverse-audit loop's round cap for every high-effort review. The cap otherwise follows the diff topology (10 small / 5 chunked; a huge diff is 3 with a review deadline and 5 without). This can only **lower** whichever tier applies: a value below 3, above the tier, or not a whole number above zero is ignored. Cutting the cap does not make reviews converge sooner — the loop ends on two consecutive dry rounds — it makes them stop before converging more often, and every such stop caps the verdict at Comment. | `0` (unset) | diff --git a/packages/cli/src/commands/review/lib/paths.test.ts b/packages/cli/src/commands/review/lib/paths.test.ts index e70aedf74ce..804a27bf945 100644 --- a/packages/cli/src/commands/review/lib/paths.test.ts +++ b/packages/cli/src/commands/review/lib/paths.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect } from 'vitest'; import { basename, dirname, join, resolve } from 'node:path'; import { inertPath, + lastReviewEffortPath, tmpFile, probeWorktreePath, scratchLabel, @@ -17,6 +18,22 @@ import { PARSE_ARGS_REPORT, } from './paths.js'; +describe('lastReviewEffortPath', () => { + it('uses the project storage owner exported by the parent session', () => { + expect( + lastReviewEffortPath('/workspace/repo', '/runtime/projects/repo'), + ).toBe(resolve('/runtime/projects/repo/review-last-effort')); + }); + + it('keeps fallback storage private and project-scoped', () => { + const first = lastReviewEffortPath('/workspace/one'); + const second = lastReviewEffortPath('/workspace/two'); + expect(first).not.toBe(second); + expect(first).not.toContain('/workspace/one/.qwen'); + expect(second).not.toContain('/workspace/two/.qwen'); + }); +}); + describe('PARSE_ARGS_REPORT', () => { it('is the literal path the skill tees to in Step 0', () => { // The skill's Step 0 hard-codes `.qwen/tmp/qwen-review-parse-args.json` in diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 0474f391b58..023614d501a 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -5,12 +5,13 @@ */ // Centralised path constants and helpers for the `qwen review` subcommands. -// All paths are relative to the project root (the current working directory -// when the command is invoked). Use `path.join` rather than string -// concatenation so Windows backslashes are produced when needed. +// Review artifacts are relative to the project root; user-private runtime +// preferences resolve under Storage's project directory. Use `path.join` +// rather than string concatenation so Windows backslashes are produced. import { existsSync, statSync } from 'node:fs'; import { join, resolve } from 'node:path'; +import { Storage } from '@qwen-code/qwen-code-core'; import { safeTarget } from '../../../utils/paths.js'; /** @@ -60,6 +61,17 @@ export const PARSE_ARGS_REPORT = join( 'qwen-review-parse-args.json', ); +/** User-private path for the last effort explicitly typed in this project. */ +export function lastReviewEffortPath( + projectRoot: string, + sessionProjectDir?: string, +): string { + const owner = sessionProjectDir?.trim() + ? resolve(sessionProjectDir) + : new Storage(projectRoot).getProjectDir(); + return join(owner, 'review-last-effort'); +} + /** Worktree path for a given PR review session. */ export function worktreePath(prNumber: string | number): string { return join(REVIEW_TMP_DIR, `review-pr-${prNumber}`); @@ -69,8 +81,7 @@ export function worktreePath(prNumber: string | number): string { * The disposable worktree the test-efficacy probe runs in — a sibling of the * shared review worktree, discarded wholesale when the probe finishes (#6832). * - * The one exception to this file's "paths are relative to the project root" - * rule: this returns an ABSOLUTE path. The probe drives `git worktree add`/ + * This returns an ABSOLUTE path. The probe drives `git worktree add`/ * `remove` with the shared worktree as cwd, so a relative path would resolve * against that worktree, not the repo root, and land the probe tree nested * inside the tree it is meant to sit beside. Both call sites — the probe and diff --git a/packages/cli/src/commands/review/parse-args.test.ts b/packages/cli/src/commands/review/parse-args.test.ts index f2cac5aae37..afd0ec08130 100644 --- a/packages/cli/src/commands/review/parse-args.test.ts +++ b/packages/cli/src/commands/review/parse-args.test.ts @@ -15,6 +15,7 @@ import { } from 'vitest'; import yargs from 'yargs'; import { join } from 'node:path'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; import { parseArgsCommand, parseReviewArgs, @@ -23,6 +24,7 @@ import { } from './parse-args.js'; import { reviewCommand } from '../review.js'; import { reviewSourceRoots, reviewSourcesDigest } from './lib/stale-bundle.js'; +import { lastReviewEffortPath } from './lib/paths.js'; import { FOREIGN_DIGEST, makeStaleBundleFixture, @@ -45,11 +47,23 @@ vi.mock('node:fs', async (importOriginal) => { const real = (await importOriginal()) as Record; const mock = { ...real, - readFileSync: vi.fn((path: unknown, ...rest: unknown[]) => - path === 0 - ? fsState.stdin - : (real['readFileSync'] as (...a: unknown[]) => unknown)(path, ...rest), - ), + readFileSync: vi.fn((path: unknown, ...rest: unknown[]) => { + if (path === 0) return fsState.stdin; + const key = String(path); + if (key.endsWith('review-last-effort')) { + return fsState.written.get(key); + } + return (real['readFileSync'] as (...a: unknown[]) => unknown)( + path, + ...rest, + ); + }), + existsSync: vi.fn((path: unknown) => { + const key = String(path); + return key.endsWith('review-last-effort') + ? fsState.written.has(key) + : (real['existsSync'] as (path: unknown) => boolean)(path); + }), writeFileSync: vi.fn((path: unknown, data: unknown) => { fsState.written.set(String(path), String(data)); }), @@ -58,6 +72,17 @@ vi.mock('node:fs', async (importOriginal) => { return { ...mock, default: mock }; }); +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + atomicWriteFileSync: vi.fn((path: unknown, data: unknown) => { + fsState.written.set(String(path), String(data)); + }), + }; +}); + vi.mock('../../utils/stdioHelpers.js', () => ({ writeStdoutLine: vi.fn(), writeStderrLineSafe: vi.fn(), @@ -1118,6 +1143,91 @@ describe('parseReviewArgs — settings-provided defaults', () => { }); }); +describe('parseReviewArgs — remembered effort', () => { + it('reuses the last explicitly typed level when no flag or setting applies', () => { + const got = parseReviewArgs('src/foo.ts', { lastUsedEffort: 'high' }); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('last_used'); + expect(got.warnings).toContain( + 'No effort level given — reusing high, the level you typed last time. Type a level like `/review --effort medium` to change it.', + ); + }); + + it('keeps the precedence explicit > remembered > configured > target default', () => { + expect( + parseReviewArgs('6711 --effort low', { + effort: 'medium', + lastUsedEffort: 'high', + }), + ).toMatchObject({ effort: 'low', effortSource: 'explicit' }); + expect( + parseReviewArgs('6711', { + effort: 'medium', + lastUsedEffort: 'high', + }), + ).toMatchObject({ effort: 'high', effortSource: 'last_used' }); + expect(parseReviewArgs('6711', { lastUsedEffort: 'medium' })).toMatchObject( + { effort: 'medium', effortSource: 'last_used' }, + ); + expect(parseReviewArgs('6711')).toMatchObject({ + effort: 'high', + effortSource: 'default', + }); + }); + + it('records an explicitly typed target default without re-recording memory', () => { + const remember = vi.fn(); + parseReviewArgs('6711 --effort high', { lastUsedEffort: 'low' }, remember); + expect(remember).toHaveBeenCalledOnce(); + expect(remember).toHaveBeenCalledWith('high'); + + remember.mockClear(); + parseReviewArgs('6711', { lastUsedEffort: 'high' }, remember); + expect(remember).not.toHaveBeenCalled(); + }); + + it('falls back to memory without recording an invalid explicit value', () => { + const remember = vi.fn(); + const got = parseReviewArgs( + '6711 --effort bogus', + { lastUsedEffort: 'medium' }, + remember, + ); + expect(got.effort).toBe('medium'); + expect(got.effortSource).toBe('last_used'); + expect(got.warnings.some((warning) => warning.includes('"bogus"'))).toBe( + true, + ); + expect(remember).not.toHaveBeenCalled(); + }); + + it('lets the posting and fix safety floors override a remembered level', () => { + expect( + parseReviewArgs('6711 --comment', { lastUsedEffort: 'low' }), + ).toMatchObject({ effort: 'high', effortSource: 'forced-by-comment' }); + expect(parseReviewArgs('--fix', { lastUsedEffort: 'low' })).toMatchObject({ + effort: 'medium', + effortSource: 'forced-by-fix', + }); + }); + + it('keeps remembered effort across effective and ignored --resume shapes', () => { + for (const raw of [ + '6711 --resume', + 'https://github.com/QwenLM/qwen-code/pull/6711 --resume', + '--resume', + 'src/foo.ts --resume', + ]) { + const got = parseReviewArgs(raw, { lastUsedEffort: 'high' }); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('last_used'); + expect(got.warnings.some((warning) => warning.includes('reusing'))).toBe( + true, + ); + } + }); +}); + describe('parseReviewArgs — `--fix` is `--comment` reflected: it needs a tree, not a PR', () => { // The two flags are gated on opposite targets, and each is *ignored with a // warning* on the other's. A PR review's tree is the ephemeral worktree Step 9 @@ -1407,6 +1517,7 @@ describe('parseArgsCommand — configured defaults wiring', () => { fsState.written.clear(); vi.mocked(writeStdoutLine).mockClear(); reviewSettingsMock.mockReturnValue({}); + vi.mocked(atomicWriteFileSync).mockClear(); }); async function verdictFor(stdin: string): Promise { @@ -1523,6 +1634,89 @@ describe('parseArgsCommand — configured defaults wiring', () => { expect(got.effort).toBe('high'); expect(got.effortSource).toBe('default'); }); + + it('persists an explicit effort and reuses it on the next invocation', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + const first = await verdictFor('src/foo.ts --effort high\n'); + expect(first.effortSource).toBe('explicit'); + expect(fsState.written.get(storedEffort)).toBe('high\n'); + expect(atomicWriteFileSync).toHaveBeenCalledWith(storedEffort, 'high\n', { + mode: 0o600, + forceMode: true, + noFollow: true, + }); + + const second = await verdictFor('src/foo.ts\n'); + expect(second.effort).toBe('high'); + expect(second.effortSource).toBe('last_used'); + expect(second.warnings).toContain( + 'No effort level given — reusing high, the level you typed last time. Type a level like `/review --effort medium` to change it.', + ); + }); + + it('lets remembered effort outrank configured effort before comment forcing', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'high\n'); + reviewSettingsMock.mockReturnValue({ effort: 'low', comment: true }); + + const got = await verdictFor('6711\n'); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('last_used'); + expect( + got.warnings.some((warning) => warning.includes('reusing high')), + ).toBe(true); + }); + + it('lets an explicit effort replace malformed remembered state', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'not-an-effort\n'); + + const got = await verdictFor('src/foo.ts --effort medium\n'); + expect(got.effort).toBe('medium'); + expect(got.effortSource).toBe('explicit'); + expect(fsState.written.get(storedEffort)).toBe('medium\n'); + }); + + it('uses the project storage owner exported by the parent session', async () => { + const previous = process.env['QWEN_CODE_PROJECT_DIR']; + const owner = '/runtime/session-project-owner'; + process.env['QWEN_CODE_PROJECT_DIR'] = owner; + try { + const got = await verdictFor('src/foo.ts --effort high\n'); + expect(got.effortSource).toBe('explicit'); + expect( + fsState.written.get(lastReviewEffortPath(process.cwd(), owner)), + ).toBe('high\n'); + } finally { + if (previous === undefined) { + delete process.env['QWEN_CODE_PROJECT_DIR']; + } else { + process.env['QWEN_CODE_PROJECT_DIR'] = previous; + } + } + }); + + it('does not persist a target default', async () => { + const got = await verdictFor('6711\n'); + expect(got.effortSource).toBe('default'); + expect( + fsState.written.has( + lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ), + ), + ).toBe(false); + }); }); describe('parse-args warns when the bundle is not built from these sources', () => { diff --git a/packages/cli/src/commands/review/parse-args.ts b/packages/cli/src/commands/review/parse-args.ts index 17b7a77f5e5..c2c60a5b013 100644 --- a/packages/cli/src/commands/review/parse-args.ts +++ b/packages/cli/src/commands/review/parse-args.ts @@ -17,7 +17,8 @@ // file path exists — stays with the caller. import type { CommandModule } from 'yargs'; -import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; import { writeStdoutLine, @@ -27,6 +28,7 @@ import { tokenizeArgs } from '../../utils/shell-args.js'; import { operatorReviewSettings } from './lib/review-settings.js'; import { bundleStalenessNotices } from './lib/stale-bundle.js'; import { isAoneCanonicalHost } from './lib/remote-match.js'; +import { lastReviewEffortPath } from './lib/paths.js'; export type ReviewEffort = 'low' | 'medium' | 'high'; @@ -84,6 +86,7 @@ export interface ParsedReviewArgs { effortSource: | 'explicit' | 'configured' + | 'last_used' | 'default' | 'forced-by-comment' | 'forced-by-fix'; @@ -354,31 +357,35 @@ function classifyToken(token: string): ReviewTarget | 'invalid-url' | null { return { type: 'file', path: token }; } +interface ReviewArgsDefaults { + /** + * The standing default from `review.effort`, raw (`auto` already mapped + * to undefined by the caller), applied when neither an explicit nor a + * remembered effort is present. Validated case-insensitively exactly like + * an explicit flag — an invalid value warns and falls back instead of + * dropping silently. The `--comment`/`--fix` forcings still override it. + */ + effort?: string; + /** The last valid effort explicitly typed for this project. */ + lastUsedEffort?: ReviewEffort; + /** + * The standing `review.comment` setting: treat a PR review as if + * `--comment` was passed. The target binding is untouched — the run still + * authorises only the PR the arguments name. + */ + comment?: boolean; + /** + * The standing `review.severityFloor` setting, raw (`auto` already mapped + * to undefined by the caller). Validated exactly like the flag — a typo + * warns and falls back to the round-adaptive default. + */ + severityFloor?: string; +} + export function parseReviewArgs( raw: string, - defaults: { - /** - * The standing default from `review.effort`, raw (`auto` already mapped - * to undefined by the caller), applied when no `--effort` flag is - * present. Validated case-insensitively exactly like an explicit flag — - * an invalid value warns and falls back instead of dropping silently. - * An explicit flag still wins; the `--comment`/`--fix` forcings still - * override it. - */ - effort?: string; - /** - * The standing `review.comment` setting: treat a PR review as if - * `--comment` was passed. The target binding is untouched — the run still - * authorises only the PR the arguments name. - */ - comment?: boolean; - /** - * The standing `review.severityFloor` setting, raw (`auto` already mapped - * to undefined by the caller). Validated exactly like the flag — a typo - * warns and falls back to the round-adaptive default. - */ - severityFloor?: string; - } = {}, + defaults: ReviewArgsDefaults = {}, + rememberExplicitEffort?: (effort: ReviewEffort) => void, ): ParsedReviewArgs { const tokens = tokenizeArgs(raw); const warnings: string[] = []; @@ -896,6 +903,9 @@ export function parseReviewArgs( if (explicitEffort !== null) { effort = explicitEffort; effortSource = 'explicit'; + } else if (defaults.lastUsedEffort !== undefined) { + effort = defaults.lastUsedEffort; + effortSource = 'last_used'; } else if (configuredEffort !== undefined) { effort = configuredEffort; effortSource = 'configured'; @@ -932,6 +942,13 @@ export function parseReviewArgs( ); } + if (effortSource === 'last_used') { + const example = effort === 'medium' ? 'high' : 'medium'; + warnings.push( + `No effort level given — reusing ${effort}, the level you typed last time. Type a level like \`/review --effort ${example}\` to change it.`, + ); + } + // Now the resolution is final; compose the deferred effort warnings so // each states what is actually in effect. const resolution = @@ -945,7 +962,9 @@ export function parseReviewArgs( ? '`--fix` forces at least medium effort' : effortSource === 'configured' ? 'using the configured review.effort' - : 'using the default effort'; + : effortSource === 'last_used' + ? 'using the last explicitly typed effort' + : 'using the default effort'; for (const issue of effortIssues) { switch (issue.kind) { case 'invalid-eq': @@ -1063,6 +1082,10 @@ export function parseReviewArgs( } } + if (explicitEffort !== null) { + rememberExplicitEffort?.(explicitEffort); + } + return { target, effort, @@ -1116,6 +1139,48 @@ function reviewDefaultsFromSettings(): { }; } +function readLastReviewEffort(path: string): ReviewEffort | undefined { + if (!existsSync(path)) return undefined; + const value = readFileSync(path, 'utf8').trim(); + const effort = asEffort(value); + if (effort === null) { + throw new Error( + `${path} must contain low, medium, or high; got ${JSON.stringify(value)}`, + ); + } + return effort; +} + +function writeLastReviewEffort(path: string, effort: ReviewEffort): void { + mkdirSync(dirname(path), { recursive: true, mode: 0o700 }); + atomicWriteFileSync(path, `${effort}\n`, { + mode: 0o600, + forceMode: true, + noFollow: true, + }); +} + +function parseReviewArgsWithMemory( + raw: string, + defaults: ReviewArgsDefaults, + effortPath: string, +): ParsedReviewArgs { + let explicitEffort: ReviewEffort | undefined; + const initial = parseReviewArgs(raw, defaults, (effort) => { + explicitEffort = effort; + }); + + if (explicitEffort !== undefined) { + writeLastReviewEffort(effortPath, explicitEffort); + return initial; + } + + const lastUsedEffort = readLastReviewEffort(effortPath); + return lastUsedEffort === undefined + ? initial + : parseReviewArgs(raw, { ...defaults, lastUsedEffort }); +} + export const parseArgsCommand: CommandModule = { command: 'parse-args [raw]', describe: @@ -1181,7 +1246,16 @@ export const parseArgsCommand: CommandModule = { writeStderrLineSafe(bundleNotice); } - const parsed = parseReviewArgs(rawStr, reviewDefaultsFromSettings()); + const projectRoot = process.cwd(); + const effortPath = lastReviewEffortPath( + projectRoot, + process.env['QWEN_CODE_PROJECT_DIR'], + ); + const parsed = parseReviewArgsWithMemory( + rawStr, + reviewDefaultsFromSettings(), + effortPath, + ); const json = JSON.stringify(parsed, null, 2); if (out) { mkdirSync(dirname(out), { recursive: true }); diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index fb5c83a9ffe..f14ad74b8e4 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -749,7 +749,7 @@ const SETTINGS_SCHEMA = { requiresRestart: false, default: 'auto', description: - 'Default effort for /review when --effort is not given. "auto" keeps the built-in rule (high for PRs, medium for local changes). An explicit --effort still wins; an effective --comment still forces high and --fix still floors at medium. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.', + 'Default effort for /review when neither --effort nor a project-remembered explicitly typed level applies. "auto" keeps the built-in rule (high for PRs, medium for local changes). An explicit or remembered level wins; an effective --comment still forces high and --fix still floors at medium. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers.', showInDialog: true, options: [ { value: 'auto', label: 'Auto (high for PRs, medium for local)' }, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index a2c2aa44398..9ac0c20dac5 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -65,13 +65,13 @@ You cannot fix this yourself: the skill you are reading comes from that same bun It prints a JSON verdict; use it **verbatim**: - `target` — `{type: "pr-number", number}` | `{type: "pr-url", url, host, owner, repo, number}` | `{type: "file", path}` | `{type: "local"}`. A `pr-url` arrives validated and canonicalized (scheme/host lowercased, query and fragment dropped, the number required to end its path segment — `/pull/42oops` is not PR 42) with host/owner/repo/number extracted; do not re-classify tokens by hand. A token that merely looks like a URL is refused with a warning and reported in `extraTokens`, never guessed into a target. -- `effort` + `effortSource` — the resolved level after defaults (**high** for PR targets, **medium** for local/file) and the `--comment` override (an **effective** `--comment` forces `high`; an ignored one on a non-PR target changes nothing). Two `settings.json` keys feed the defaults: `review.effort` replaces the built-in default when `--effort` is absent (`effortSource: "configured"`), and `review.comment: true` makes every PR review behave as if `--comment` was passed — the forcings above still apply. Both resolve from operator scopes only (system/user); a repository's `.qwen/settings.json` cannot set them. Do not re-derive it. +- `effort` + `effortSource` — the resolved level after remembered/configured defaults (**high** for PR targets, **medium** for local/file) and the `--comment` override (an **effective** `--comment` forces `high`; an ignored one on a non-PR target changes nothing). `last_used` means the project reused the last level the user explicitly typed, and it outranks `review.effort`. Two `settings.json` keys feed the configured defaults: `review.effort` replaces the built-in target default when neither an explicit nor remembered level applies (`effortSource: "configured"`), and `review.comment: true` makes every PR review behave as if `--comment` was passed — the forcings above still apply. Both resolve from operator scopes only (system/user); a repository's `.qwen/settings.json` cannot set them. Do not re-derive it. - `comment.requested` / `comment.effective` — `effective` is what gates Step 7 (true also when only the `review.comment` setting is on); `requested && !effective` means the user asked on a non-PR target, and the warning for that is already in `warnings`. - `fix.requested` / `fix.effective` — `--fix` is `--comment` reflected, and gated on the opposite target. `--comment` writes to a **pull request**, so it needs one; `--fix` writes to a **working tree**, so it needs one that outlives the review. A PR review's tree is the ephemeral worktree `fetch-pr` creates and Step 9 deletes, so `--fix` on a PR target is ignored with a warning — edits there are discarded minutes later, and reporting findings as "fixed" into a directory that no longer exists is worse than not fixing them. `effective` is what gates Step 6B. An effective `--fix` also floors the effort at **medium**: it edits the user's files, and low runs no verification, so applying an unverified finding is the same mistake as posting one, aimed at their working tree instead of a pull request. It does not force **high** — medium's findings are verified, and the reverse audit high adds hunts for findings that are _missing_, which is not what deciding whether to apply one turns on. - `severityFloor` + `severityFloorSource` — the posting floor for a PR review: `critical` posts only Criticals (otherwise-postable high-confidence Suggestions are recorded and deferred — Step 6's convergence posture; low-confidence and Nice-to-have findings stay terminal-only as ever), `suggestion` posts Criticals and Suggestions at every round, and `auto` — the default — is the **round-adaptive rule you resolve in Step 6**, where the round is known: `suggestion` through round 5, `critical` from round 6 — **or `critical` from any round once the recovered ledger's `flatRounds` streak has reached its bar** (Step 6's signal-driven trigger: the first-time-finding rate has not fallen for that many consecutive rounds, so the loop is re-deriving the same set and the floor stems it early). The parser cannot resolve `auto` itself (the round comes from the previous posted round's ledger, not fetched yet), so carry the verdict's value forward and resolve it there. Explicit flag beats the `review.severityFloor` setting beats `auto`; a non-PR target has no rounds, so the flag warns and is ignored there. The floor governs what the review **posts**, never what it finds, verifies, or reports in the terminal. - `topology` + `topologySource` — the shape of the run. `auto` (the default) runs the standing effort-driven pipeline described below. `minimal` runs the single-pass A/B comparison arm (Step 3M) instead — and when it is set, it OVERRIDES the effort dispatch entirely. In this step you run `parse-args` and the **diff capture only** (`fetch-pr` for a same-repo PR, the lightweight `fetch-diff` for a cross-repo PR, or the local capture for a local/file target — exactly as below), then jump straight to **Step 3M**. You SKIP the rest of Step 1's setup — the rules load, `pr-context`, `comment-status`, and the incremental-cache check — and you skip the fan-out, verification, reverse audit, and posting. `minimal` is terminal-only; the parser has already forced `comment.effective`, `fix.effective`, and `resume.effective` to false, and its warnings for that are in `warnings`. There is no configured topology — it is only ever an explicit flag. -- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, or `--topology minimal` (a fresh single pass neither continues nor consumes an interrupted run), already warned in `warnings`. It never changes the effort: a continuation is pinned to the interrupted run's recorded level, and an explicitly different `--effort` makes `fetch-pr` refuse the resume and run fresh at the requested one. -- `warnings` — surface every entry to the user, word for word. +- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, or `--topology minimal` (a fresh single pass neither continues nor consumes an interrupted run), already warned in `warnings`. The resolved effort source controls continuity: a target default is omitted so the interrupted run stays pinned to its recorded level; an explicit, remembered, configured, or comment-forced level is passed through, and a mismatch makes `fetch-pr` refuse the resume and run fresh at that required level. +- `warnings` — surface every entry to the user, word for word. When a warning says the last explicitly typed effort was reused, relay it as the opening line before starting the review. - `extraTokens` / `unknownFlags` — leftover input the parser refused to guess about; mention them to the user rather than silently dropping them. **Reference files, gated by this verdict.** This skill's conditional territory lives in `references/` beside it, and the verdict above already decides which of them this run needs — read each applicable one with `read_file` from this skill's base directory before the step that owns it: @@ -171,7 +171,7 @@ Based on the parsed `target.type`: - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `pr-context` writes it into the side file `qwen-review-pr--prev-ledger.json` with the rest of the ledger. So when the cache had no anchor to pass — including the case where it HELD one that the cache-path gate withheld, because `lastModelId` was another model's: the marker may carry an anchor THIS model certified, and a round that stops at the cache would never look — **or the anchor it passed was refused** (`incremental.effective: false` — a rebase or force-push retires a cached anchor exactly when another environment may have posted a newer round whose marker still holds a valid one): proceed with the setup batch as usual, and when the side file lands with a `sha` — **different from the one already refused, OR the same sha when the refusal was infrastructure** (`base-untrusted`, `capture-failed`: the anchor was never ruled invalid, and the component that failed — a base fetch, a merge-base resolution, a capture — is re-run by the re-run. One shape of `capture-failed` retries ONCE, not forever: a base-less refusal (a null `mergeBaseSha`) means the base fetch failed (`baseFetchFailed: true`) and no local base ref remained, or `git merge-base` itself failed on a non-answer exit. The failed component IS re-run by the re-run, but the exit status cannot split the members — git exits 128 identically for a transient fetch fault and for a deterministic refusal (the base branch deleted on the remote — the refspec fetch fails every time), and the merge-base probe folds its surface failures the same way — so a second refusal of the same shape on the same sha is the deterministic member. Retry that one, once. Every other reason is deterministic for the same sha and must NOT be retried: a validity refusal re-refuses; a planless `partition-failed` always carries a `mergeBaseSha` — with no base nothing is captured and an empty diff cannot fail to tile — so both ranges were in hand and both refused to tile, which the re-run reproduces exactly, do not retry it; `nothing-to-narrow` re-narrows identically: the same two captures select the same hunks, and a capture that failed a UTF-8 round trip fails it again — and its base-less shape (a null `mergeBaseSha` with `baseFetchFailed: false`) is NOT retryable: the fetch succeeded and `git merge-base` found no common ancestor at all (a cross-fork PR with unrelated history), which a re-run reproduces exactly) —, **re-run the `fetch-pr` command from above with `--since ` — REPLACING any `--since` it already carries, never appending a second one** (a repeated flag is one flag with two values; the CLI takes the last, but a command that reads as two anchors is a command nobody can check) — the PR ref is already fetched so the re-run is cheap, and it rebuilds the worktree, diff and chunk plan scoped to the delta, with the validation the old flow asked you to hand-run (`cat-file`, `merge-base --is-ancestor`) inside the command where it cannot be skipped. Then act on the new report's `incremental` field exactly as the cache path above does (**the same-model gate on this path is RULED FOR YOU, not left to you to apply**: the marker carries `model` beside its `sha` — the identity that certified the range — and `pr-context`'s ledger section states the verdict outright, either "the same-model contract HOLDS" or "**Do NOT pass the anchor above as `--since`**". Obey that sentence and do not compare the two identities yourself: the marker's `model` is a PROVIDER-QUALIFIED identity (`@`) while `{{model}}` above is the bare model id, so they are not the same kind of string — comparing them by hand either never matches, which throws away this whole recovery path, or matches loosely, which accepts another provider's same-named model and scopes past code it never reviewed. A ledger section that states no verdict — because the side file survived from an earlier round the recovery could not re-vouch — is a mismatch: review the full range. The ledger's round is used only for precedence, and an `upToDate` anchor from the side file stops only when `comment.effective` is false **and the side file carries no `anchorFromRound`** — a grafted anchor that resolves to the head means the round it was carried for closed at a head its source had already certified, so `sha..HEAD` re-covers nothing, and the stop would abandon that round's owed work list without a ruling, with every later round at the same head repeating the same stop: proceed instead as when `comment.effective` is true (the re-run report already holds the full-range diff and plan) and rule every ledger entry). The decision lands AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). An anchor that fails validation falls back to the full diff with the reason in the report, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted; and a side file with no `sha` field means no anchor is recoverable. When the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions) and its work list survived whole, `pr-context` grafts the anchor forward from the most recent EARLIER own marker that carries one — the withhold is about the fail-closed round's own range, while the earlier round's "clean up to `sha`" stays true, and scoping `sha..HEAD` re-covers the gap (the ledger section says "anchoring at", never "reviewed at", when the anchor was carried forward this way, and names the round it was carried from). So a missing `sha` means a shape the graft refuses or cannot reach — the winning work list was truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the grafted scope and retire silently), the only anchored own marker is the winner's own round (one round cannot both certify and withhold), the winner ran at the same head the candidate sha certifies (grafting it would hand Step 1 a same-sha stop that abandons the work list the winner still owes), every own round on the PR closed without an anchor, the only markers are other accounts' (the sha never crosses accounts), or the markers predate the field — and the review is full-range. (The side file may also carry `commitId` — the previous review's own `commit_id`. That is Step 6's **age reference** for the convergence posture, present even on fail-closed rounds; it is never an anchor, and scoping the diff to it would skip exactly the range a fail-closed round could not certify.) - - **Resuming an interrupted run (`--resume`)**: when `parse-args` reported `resume.effective: true`, append `--resume` to the `fetch-pr` command above, and decide `--effort` off `effortSource`, not off whether the word `--effort` was typed. Pass the resolved level whenever `effortSource` is `explicit` **or `forced-by-comment`** (the `--comment` flag or the `review.comment` setting forces high — parse-args announces "running at high effort"); omit it ONLY when `effortSource` is `default`. `fetch-pr` cannot tell a passed-through default from a chosen level: the interrupted run may have recorded a different one, and handing it the resolved default refuses the resume (`effort-mismatch`) whose fresh fall-through discards the very state `--resume` exists to save — blaming an effort nobody asked for. Omitted, the continuation pins to the recorded level. A level this invocation actually requires — a user's explicit `--effort`, or the high that `--comment` forces — that differs from the recorded one is NOT a passed-through default: pass it, so a recorded lower level refuses (`effort-mismatch`) and runs fresh at the level this invocation needs. That is right — different effort is different work, and posting authority raising the required depth is different work too, never a silent pin. Omitting a `forced-by-comment` high is the trap: `fetch-pr` has no `--comment` input and reads `requestedEffort` only from `--effort`, so the null would pin the continuation at the recorded sub-high level while `--comment` stays effective — the "effective comment at medium effort" state the medium-tier rules call impossible, posting nothing (medium skips posting) or posting from a pipeline missing the high-only passes the forcing exists to guarantee. `fetch-pr` rules on the interrupted attempt's on-disk state itself (worktree still at `fetchedSha` and clean, diff bytes unchanged, PR head unmoved, resume cap unspent — every probe is a fact it gathers, none is yours to assert) and prints one JSON line on stdout. Branch on it: + - **Resuming an interrupted run (`--resume`)**: when `parse-args` reported `resume.effective: true`, append `--resume` to the `fetch-pr` command above, and decide `--effort` off `effortSource`, not off whether the word `--effort` was typed. Pass the resolved level whenever `effortSource` is `explicit`, `last_used`, `configured`, or `forced-by-comment` (the `--comment` flag or the `review.comment` setting forces high — parse-args announces "running at high effort"); omit it ONLY when `effortSource` is `default`. `fetch-pr` cannot tell a passed-through default from a chosen level: the interrupted run may have recorded a different one, and handing it the resolved default refuses the resume (`effort-mismatch`) whose fresh fall-through discards the very state `--resume` exists to save — blaming an effort nobody asked for. Omitted, the continuation pins to the recorded level. A level this invocation actually requires — a user's explicit `--effort`, the project's remembered level, a configured `review.effort`, or the high that `--comment` forces — that differs from the recorded one is NOT a passed-through default: pass it, so a mismatch refuses the resume (`effort-mismatch`) and runs fresh at the level this invocation needs. That is right — different effort is different work, and posting authority raising the required depth is different work too, never a silent pin. Omitting a `forced-by-comment` high is the trap: `fetch-pr` has no `--comment` input and reads `requestedEffort` only from `--effort`, so the null would pin the continuation at the recorded sub-high level while `--comment` stays effective — the "effective comment at medium effort" state the medium-tier rules call impossible, posting nothing (medium skips posting) or posting from a pipeline missing the high-only passes the forcing exists to guarantee. `fetch-pr` rules on the interrupted attempt's on-disk state itself (worktree still at `fetchedSha` and clean, diff bytes unchanged, PR head unmoved, resume cap unspent — every probe is a fact it gathers, none is yours to assert) and prints one JSON line on stdout. Branch on it: - **`{"resumed": true, ...}`** — this run continues the interrupted one. The report at the `--out` path is the PREVIOUS attempt's, deliberately left untouched (its mtime is the run epoch every downstream fence keys on); read it for the worktree, plan and diff, which are all reused. The report's `incremental` field is now HISTORY, not a decision to re-take: a resumed run proceeds on the reused plan and does NOT re-enter the incremental check above — in particular it never takes the `upToDate: true` stop/cleanup branch, which runs `cleanup pr-` and would destroy the exact worktree and lease `--resume` just saved (the interrupted attempt was a `--comment` full review of an up-to-date PR; resuming it without `--comment` effective in THIS invocation would otherwise route it straight into "No new changes since last review" and abandon it). Then rebuild your working state from disk before launching anything: ```bash diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index d9aabab1b8b..9ea2b94acf2 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -584,6 +584,9 @@ describe('bundled review skill', () => { // refuses and runs fresh at high) rather than silently pinned — dropping // the `forced-by-comment` arm re-creates the "comment at medium" state. expect(body).toContain('`forced-by-comment`'); + expect(body).toContain( + '`explicit`, `last_used`, `configured`, or `forced-by-comment`', + ); // R15-11: a resumed run must NOT re-take the incremental decision — the // previous attempt's `incremental` field is history, so the continuation // never enters the `upToDate` stop/cleanup branch that would destroy the @@ -599,6 +602,13 @@ describe('bundled review skill', () => { ); }); + it('relays a remembered effort notice before the review starts', () => { + const body = skillBody(); + expect(body).toContain( + 'When a warning says the last explicitly typed effort was reused, relay it as the opening line before starting the review.', + ); + }); + 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 From 6976967687c77e4f8862ad1220e94380fb7fadad Mon Sep 17 00:00:00 2001 From: tly Date: Mon, 24 Aug 2026 16:31:16 +0800 Subject: [PATCH 2/5] chore(vscode): regenerate settings schema --- packages/vscode-ide-companion/schemas/settings.schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index ed7a586280f..3c21a58ad36 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -241,7 +241,7 @@ "default": "off" }, "effort": { - "description": "Default effort for /review when --effort is not given. \"auto\" keeps the built-in rule (high for PRs, medium for local changes). An explicit --effort still wins; an effective --comment still forces high and --fix still floors at medium. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers. Options: auto, low, medium, high", + "description": "Default effort for /review when neither --effort nor a project-remembered explicitly typed level applies. \"auto\" keeps the built-in rule (high for PRs, medium for local changes). An explicit or remembered level wins; an effective --comment still forces high and --fix still floors at medium. Only honored from User, System, and SystemDefaults settings scopes; values set in Workspace settings are ignored, so a repository cannot set review policy for its reviewers. Options: auto, low, medium, high", "enum": [ "auto", "low", From c880bcb33cc7ed2d914018f2abd80ec2600718f3 Mon Sep 17 00:00:00 2001 From: tly Date: Mon, 24 Aug 2026 19:13:16 +0800 Subject: [PATCH 3/5] fix(review): align remembered effort guidance --- docs/users/features/code-review.md | 6 +++--- .../src/commands/review/parse-args.test.ts | 20 +++++++++++++++++++ .../core/src/skills/bundled/review/SKILL.md | 7 +++---- .../src/skills/bundled/review/SKILL.test.ts | 3 +++ 4 files changed, 29 insertions(+), 7 deletions(-) diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index 35bd8baa94e..c28a6859334 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -41,7 +41,7 @@ If there are no uncommitted changes, `/review` will let you know and stop — no | `medium` | The high pipeline minus its most expensive passes: the parallel finder fan-out over a reduced dimension set, plus build/test and a single verification pass | Uncapped (verified) | Approve capped at Comment | Never | | `high` | Full pipeline: up to 16 parallel agents → sharded verification → iterative reverse audit | Uncapped (verified) | Approve / Request changes / Comment | With `--comment` | -Defaults: **high** for PR reviews, **medium** for local and file reviews. An effective `--comment` forces high (posted comments must survive verification) — on a non-PR target `--comment` is ignored with a warning and does **not** change the effort. Medium keeps the security and test-coverage agents and build/test, and drops the adversarial personas, the language-pitfall and wrapper/proxy specialists (Agents 1d/1e), the diff-specialist finders and the reverse audit — so a subtle Critical only the second look would surface can slip; use `--effort high` for security-sensitive or pre-release reviews. Only `low` is unverified. Worktree isolation applies to same-repo PR reviews; cross-repo PRs run in lightweight mode (diff-only, no worktree or build/test). The low pass is labeled unverified, emits no verdict, and never writes the incremental review cache, so a later `--effort high` run is never skipped as "already reviewed"; medium is verified but its Approve is capped at Comment, because nothing looked twice for what the first pass missed. The diff-obtaining mechanics are identical at every level — PR reviews always use the isolated worktree and the same base resolution, so the review is never against the wrong base. One scope difference remains: the incremental cache is high-only, so a high re-review may cover just the new commits (`lastCommitSha..HEAD`) while low/medium always review the full PR diff. +`/review` resolves effort in this order: an explicit `--effort`, the last level explicitly typed for this project, the operator `review.effort` setting, then the built-in target default (**high** for PR reviews, **medium** for local and file reviews). When a remembered level applies, `/review` announces it before work begins; type a new `--effort` to replace it. An effective `--comment` forces high (posted comments must survive verification) — on a non-PR target `--comment` is ignored with a warning and does **not** change the effort. Medium keeps the security and test-coverage agents and build/test, and drops the adversarial personas, the language-pitfall and wrapper/proxy specialists (Agents 1d/1e), the diff-specialist finders and the reverse audit — so a subtle Critical only the second look would surface can slip; use `--effort high` for security-sensitive or pre-release reviews. Only `low` is unverified. Worktree isolation applies to same-repo PR reviews; cross-repo PRs run in lightweight mode (diff-only, no worktree or build/test). The low pass is labeled unverified, emits no verdict, and never writes the incremental review cache, so a later `--effort high` run is never skipped as "already reviewed"; medium is verified but its Approve is capped at Comment, because nothing looked twice for what the first pass missed. The diff-obtaining mechanics are identical at every level — PR reviews always use the isolated worktree and the same base resolution, so the review is never against the wrong base. One scope difference remains: the incremental cache is high-only, so a high re-review may cover just the new commits (`lastCommitSha..HEAD`) while low/medium always review the full PR diff. ## How It Works @@ -240,7 +240,7 @@ A long review that dies part-way — a dropped connection, a timeout, a killed t It applies to **PR targets only** (a local review's diff comes from a live working tree, which has no stable interrupted state to continue), and it is safe to pass whenever you are unsure: the review rules on the on-disk state itself — the worktree still at the fetched commit and clean, the captured diff unchanged byte for byte, the PR head unmoved, the resume limit unspent — and silently starts fresh whenever anything no longer matches, telling you which check refused. A continuation reuses the earlier attempt's certified agent results, so the report says how many were recovered; it is disclosed, never a coverage gap. -Two things to know. A continuation keeps the interrupted run's **effort**: passing a different `--effort` refuses the resume and runs fresh at the level you asked for, because different effort is different work. And if the PR head moved while the review was down, the resume refuses (`head-moved`) and the fresh run reviews the new commits — which is what you want, and it counts as this review's one restart. +Two things to know. With only the built-in target default, a continuation keeps the interrupted run's recorded **effort**. An explicit `--effort`, a project-remembered level, the operator `review.effort` setting, or an effective `--comment` supplies a required level; if it differs from the interrupted run, resume is refused and a fresh run starts at that level, because different effort is different work. And if the PR head moved while the review was down, the resume refuses (`head-moved`) and the fresh run reviews the new commits — which is what you want, and it counts as this review's one restart. ## Findings as Data @@ -410,7 +410,7 @@ The exit code is the contract a gate should read: `3` (not `2`) lets a gate distinguish "the review is blocking" from "the tool broke" — yargs already uses `1` for usage errors — without parsing any output. `--timeout-minutes` (default 120, floored at 1) terminates a hung review and exits `1`, and cancelling the command (Ctrl+C / SIGTERM) terminates the review's process group rather than orphaning it. -`--resume` continues an interrupted review of the same PR instead of starting over — when a long local run dies part-way (a dropped connection, a timeout, a killed terminal), the retry would otherwise re-fetch, re-chunk and re-launch agents whose work is already on disk. It is safe to pass unconditionally on a retry: `fetch-pr` rules on the on-disk state itself (worktree still at the fetched SHA and clean, diff bytes unchanged, PR head unmoved, resume cap unspent) and silently falls back to a fresh review whenever anything no longer matches, so the flag never fails a run that could start over. A continuation is pinned to the interrupted run's recorded effort — an explicitly different `--effort` refuses the resume and runs fresh at the requested level. PR targets only (a local review's diff is captured from a live working tree, which has no stable interrupted state to continue). Resume is a **local convenience**: the repository's own CI review workflow does **not** resume — each retry re-runs fresh, because a CI attempt runs no-sandbox and its worktree is deleted on exit, leaving no interrupted state to continue. +`--resume` continues an interrupted review of the same PR instead of starting over — when a long local run dies part-way (a dropped connection, a timeout, a killed terminal), the retry would otherwise re-fetch, re-chunk and re-launch agents whose work is already on disk. It is safe to pass unconditionally on a retry: `fetch-pr` rules on the on-disk state itself (worktree still at the fetched SHA and clean, diff bytes unchanged, PR head unmoved, resume cap unspent) and silently falls back to a fresh review whenever anything no longer matches, so the flag never fails a run that could start over. When the current invocation has only the built-in target default, a continuation stays pinned to the interrupted run's recorded effort. An explicit `--effort`, a project-remembered level, the operator `review.effort` setting, or an effective `--comment` supplies a required level; a mismatch refuses the resume and runs fresh at that level. PR targets only (a local review's diff is captured from a live working tree, which has no stable interrupted state to continue). Resume is a **local convenience**: the repository's own CI review workflow does **not** resume — each retry re-runs fresh, because a CI attempt runs no-sandbox and its worktree is deleted on exit, leaving no interrupted state to continue. A time-budgeted run can also export a **soft** deadline so the review stops its open-ended reverse-audit loop while there is still time to verify, compose and post: `QWEN_REVIEW_DEADLINE_EPOCH` is the Unix-seconds moment the run will be killed, and `QWEN_REVIEW_DEADLINE_RESERVE_SECONDS` (default 3600; `0` keeps only the round estimate) is the tail that must remain for the last round's verification, `compose-review` and submission. When the remaining budget no longer fits another round plus that tail, the round builder refuses to build it, and the composed verdict discloses the truncated audit (an otherwise-Approve verdict is capped at Comment). A missing or malformed deadline leaves the review ungated — the outer timeout still bounds the run. diff --git a/packages/cli/src/commands/review/parse-args.test.ts b/packages/cli/src/commands/review/parse-args.test.ts index afd0ec08130..5769cc849e9 100644 --- a/packages/cli/src/commands/review/parse-args.test.ts +++ b/packages/cli/src/commands/review/parse-args.test.ts @@ -1673,6 +1673,26 @@ describe('parseArgsCommand — configured defaults wiring', () => { ).toBe(true); }); + it('keeps remembered effort when an explicit value is invalid', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'medium\n'); + + const got = await verdictFor('6711 --effort bogus\n'); + expect(got.effort).toBe('medium'); + expect(got.effortSource).toBe('last_used'); + expect(got.warnings).toContain( + 'No effort level given — reusing medium, the level you typed last time. Type a level like `/review --effort high` to change it.', + ); + expect(got.warnings).toContain( + 'Invalid --effort value "bogus" discarded; using the last explicitly typed effort.', + ); + expect(fsState.written.get(storedEffort)).toBe('medium\n'); + expect(atomicWriteFileSync).not.toHaveBeenCalled(); + }); + it('lets an explicit effort replace malformed remembered state', async () => { const storedEffort = lastReviewEffortPath( process.cwd(), diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 9ac0c20dac5..10773adb4b6 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -129,10 +129,9 @@ Based on the parsed `target.type`: # compose-review's own coverage recomputation — reads it from there, so they # cannot disagree about which agents a medium review owed. Omit it only if # the parser resolved the default high. On a FRESH run passing it always - # is harmless; on a RESUME it is not — the ruling cannot tell a passed- - # through default from a user's explicit choice, so follow the resume - # bullet below: pass --effort only when the user chose a level in THIS - # invocation. + # is harmless; on a RESUME it is not — pass it for explicit, last_used, + # configured, or forced-by-comment and omit it only for default, as + # detailed in the resume bullet below. # High-effort re-review with a cached anchor: append --since # (the incremental check below) — the CLI validates the anchor and scopes # the diff and plan; never run git against an anchor yourself. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 9ea2b94acf2..6a5eea2777b 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -587,6 +587,9 @@ describe('bundled review skill', () => { expect(body).toContain( '`explicit`, `last_used`, `configured`, or `forced-by-comment`', ); + expect(body).not.toContain( + 'pass --effort only when the user chose a level in THIS invocation', + ); // R15-11: a resumed run must NOT re-take the incremental decision — the // previous attempt's `incremental` field is history, so the continuation // never enters the `upToDate` stop/cleanup branch that would destroy the From 8c4f0f7d84bcdac272e46f277369ac5c5142d67a Mon Sep 17 00:00:00 2001 From: tly Date: Wed, 26 Aug 2026 10:45:16 +0800 Subject: [PATCH 4/5] fix(review): tolerate remembered effort I/O failures --- .../src/commands/review/parse-args.test.ts | 39 +++++++++++++++++++ .../cli/src/commands/review/parse-args.ts | 39 ++++++++++++++----- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/commands/review/parse-args.test.ts b/packages/cli/src/commands/review/parse-args.test.ts index 5769cc849e9..299de11fd8f 100644 --- a/packages/cli/src/commands/review/parse-args.test.ts +++ b/packages/cli/src/commands/review/parse-args.test.ts @@ -1516,6 +1516,7 @@ describe('parseArgsCommand — configured defaults wiring', () => { fsState.stdin = ''; fsState.written.clear(); vi.mocked(writeStdoutLine).mockClear(); + vi.mocked(writeStderrLineSafe).mockClear(); reviewSettingsMock.mockReturnValue({}); vi.mocked(atomicWriteFileSync).mockClear(); }); @@ -1706,6 +1707,44 @@ describe('parseArgsCommand — configured defaults wiring', () => { expect(fsState.written.get(storedEffort)).toBe('medium\n'); }); + it('ignores malformed remembered state when no explicit effort replaces it', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'not-an-effort\n'); + + const got = await verdictFor('6711\n'); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('default'); + expect(fsState.written.get(storedEffort)).toBe('not-an-effort\n'); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + `${storedEffort} must contain low, medium, or high`, + ); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + 'resolving from review.effort and the target default instead', + ); + }); + + it('uses an explicit effort when remembering it fails', async () => { + vi.mocked(atomicWriteFileSync).mockImplementationOnce(() => { + throw new Error('ENOSPC: no space left on device'); + }); + + const got = await verdictFor('6711 --effort low\n'); + expect(got.effort).toBe('low'); + expect(got.effortSource).toBe('explicit'); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + 'could not be remembered', + ); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + 'ENOSPC: no space left on device', + ); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + 'this review still uses low', + ); + }); + it('uses the project storage owner exported by the parent session', async () => { const previous = process.env['QWEN_CODE_PROJECT_DIR']; const owner = '/runtime/session-project-owner'; diff --git a/packages/cli/src/commands/review/parse-args.ts b/packages/cli/src/commands/review/parse-args.ts index c2c60a5b013..f89db1b27f6 100644 --- a/packages/cli/src/commands/review/parse-args.ts +++ b/packages/cli/src/commands/review/parse-args.ts @@ -1140,24 +1140,43 @@ function reviewDefaultsFromSettings(): { } function readLastReviewEffort(path: string): ReviewEffort | undefined { - if (!existsSync(path)) return undefined; - const value = readFileSync(path, 'utf8').trim(); + let value: string; + try { + if (!existsSync(path)) return undefined; + value = readFileSync(path, 'utf8').trim(); + } catch (error) { + writeStderrLineSafe( + `NOTE: the remembered review effort at ${path} could not be read (${ + error instanceof Error ? error.message.split('\n')[0] : String(error) + }); resolving from review.effort and the target default instead. Type \`--effort \` to record a new one.`, + ); + return undefined; + } const effort = asEffort(value); if (effort === null) { - throw new Error( - `${path} must contain low, medium, or high; got ${JSON.stringify(value)}`, + writeStderrLineSafe( + `NOTE: ${path} must contain low, medium, or high; got ${JSON.stringify(value)}. Ignoring it; resolving from review.effort and the target default instead. Type \`--effort \` to record a new one.`, ); + return undefined; } return effort; } function writeLastReviewEffort(path: string, effort: ReviewEffort): void { - mkdirSync(dirname(path), { recursive: true, mode: 0o700 }); - atomicWriteFileSync(path, `${effort}\n`, { - mode: 0o600, - forceMode: true, - noFollow: true, - }); + try { + mkdirSync(dirname(path), { recursive: true, mode: 0o700 }); + atomicWriteFileSync(path, `${effort}\n`, { + mode: 0o600, + forceMode: true, + noFollow: true, + }); + } catch (error) { + writeStderrLineSafe( + `NOTE: the explicit review effort ${effort} could not be remembered at ${path} (${ + error instanceof Error ? error.message.split('\n')[0] : String(error) + }); this review still uses ${effort}.`, + ); + } } function parseReviewArgsWithMemory( From 215006780990dfd9223f15aa29e0a545df780af1 Mon Sep 17 00:00:00 2001 From: tly Date: Thu, 27 Aug 2026 15:03:00 +0800 Subject: [PATCH 5/5] fix(review): close effort memory review gaps --- .../src/commands/review/parse-args.test.ts | 48 +++++++++++++++++-- .../cli/src/commands/review/parse-args.ts | 14 ++++-- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/commands/review/parse-args.test.ts b/packages/cli/src/commands/review/parse-args.test.ts index 299de11fd8f..addaae5030f 100644 --- a/packages/cli/src/commands/review/parse-args.test.ts +++ b/packages/cli/src/commands/review/parse-args.test.ts @@ -41,6 +41,7 @@ import { const fsState = vi.hoisted(() => ({ stdin: '', written: new Map(), + effortReadError: undefined as Error | undefined, })); vi.mock('node:fs', async (importOriginal) => { @@ -51,6 +52,9 @@ vi.mock('node:fs', async (importOriginal) => { if (path === 0) return fsState.stdin; const key = String(path); if (key.endsWith('review-last-effort')) { + if (fsState.effortReadError !== undefined) { + throw fsState.effortReadError; + } return fsState.written.get(key); } return (real['readFileSync'] as (...a: unknown[]) => unknown)( @@ -1385,6 +1389,7 @@ describe('parseArgsCommand wiring', () => { beforeEach(() => { fsState.stdin = ''; fsState.written.clear(); + fsState.effortReadError = undefined; vi.mocked(writeStdoutLine).mockClear(); }); @@ -1515,6 +1520,7 @@ describe('parseArgsCommand — configured defaults wiring', () => { beforeEach(() => { fsState.stdin = ''; fsState.written.clear(); + fsState.effortReadError = undefined; vi.mocked(writeStdoutLine).mockClear(); vi.mocked(writeStderrLineSafe).mockClear(); reviewSettingsMock.mockReturnValue({}); @@ -1707,6 +1713,19 @@ describe('parseArgsCommand — configured defaults wiring', () => { expect(fsState.written.get(storedEffort)).toBe('medium\n'); }); + it('lets an explicit effort replace valid remembered state', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'high\n'); + + const got = await verdictFor('src/foo.ts --effort low\n'); + expect(got.effort).toBe('low'); + expect(got.effortSource).toBe('explicit'); + expect(fsState.written.get(storedEffort)).toBe('low\n'); + }); + it('ignores malformed remembered state when no explicit effort replaces it', async () => { const storedEffort = lastReviewEffortPath( process.cwd(), @@ -1726,14 +1745,33 @@ describe('parseArgsCommand — configured defaults wiring', () => { ); }); - it('uses an explicit effort when remembering it fails', async () => { + it('ignores unreadable remembered state', async () => { + const storedEffort = lastReviewEffortPath( + process.cwd(), + process.env['QWEN_CODE_PROJECT_DIR'], + ); + fsState.written.set(storedEffort, 'low\n'); + fsState.effortReadError = new Error('EACCES: permission denied'); + + const got = await verdictFor('6711\n'); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('default'); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + `${storedEffort} could not be read`, + ); + expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( + 'EACCES: permission denied', + ); + }); + + it('reports the resolved effort when remembering an explicit effort fails', async () => { vi.mocked(atomicWriteFileSync).mockImplementationOnce(() => { throw new Error('ENOSPC: no space left on device'); }); - const got = await verdictFor('6711 --effort low\n'); - expect(got.effort).toBe('low'); - expect(got.effortSource).toBe('explicit'); + const got = await verdictFor('6711 --comment --effort low\n'); + expect(got.effort).toBe('high'); + expect(got.effortSource).toBe('forced-by-comment'); expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( 'could not be remembered', ); @@ -1741,7 +1779,7 @@ describe('parseArgsCommand — configured defaults wiring', () => { 'ENOSPC: no space left on device', ); expect(vi.mocked(writeStderrLineSafe).mock.calls[0]?.[0]).toContain( - 'this review still uses low', + 'this review still uses high', ); }); diff --git a/packages/cli/src/commands/review/parse-args.ts b/packages/cli/src/commands/review/parse-args.ts index f89db1b27f6..b0ca34b9aff 100644 --- a/packages/cli/src/commands/review/parse-args.ts +++ b/packages/cli/src/commands/review/parse-args.ts @@ -1162,19 +1162,23 @@ function readLastReviewEffort(path: string): ReviewEffort | undefined { return effort; } -function writeLastReviewEffort(path: string, effort: ReviewEffort): void { +function writeLastReviewEffort( + path: string, + explicitEffort: ReviewEffort, + resolvedEffort: ReviewEffort, +): void { try { mkdirSync(dirname(path), { recursive: true, mode: 0o700 }); - atomicWriteFileSync(path, `${effort}\n`, { + atomicWriteFileSync(path, `${explicitEffort}\n`, { mode: 0o600, forceMode: true, noFollow: true, }); } catch (error) { writeStderrLineSafe( - `NOTE: the explicit review effort ${effort} could not be remembered at ${path} (${ + `NOTE: the explicit review effort ${explicitEffort} could not be remembered at ${path} (${ error instanceof Error ? error.message.split('\n')[0] : String(error) - }); this review still uses ${effort}.`, + }); this review still uses ${resolvedEffort}.`, ); } } @@ -1190,7 +1194,7 @@ function parseReviewArgsWithMemory( }); if (explicitEffort !== undefined) { - writeLastReviewEffort(effortPath, explicitEffort); + writeLastReviewEffort(effortPath, explicitEffort, initial.effort); return initial; }