diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index df85be82281..dff729505c7 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -49,6 +49,7 @@ describe('reviewCommand', () => { 'fetch-pr', 'capture-local', 'plan-diff', + 'cache-commit', 'repo-context', 'pr-context', 'comment-status', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 0f02bf251f8..b1e81e23bdd 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -17,6 +17,7 @@ import { recoverFindingsCommand } from './review/recover-findings.js'; import { fetchPrCommand } from './review/fetch-pr.js'; import { captureLocalCommand } from './review/capture-local.js'; import { planDiffCommand } from './review/plan-diff.js'; +import { cacheCommitCommand } from './review/cache-commit.js'; import { repoContextCommand } from './review/repo-context.js'; import { prContextCommand } from './review/pr-context.js'; import { commentStatusCommand } from './review/comment-status.js'; @@ -62,6 +63,7 @@ export const reviewCommand: CommandModule = { .command(fetchPrCommand) .command(captureLocalCommand) .command(planDiffCommand) + .command(cacheCommitCommand) .command(repoContextCommand) .command(prContextCommand) .command(commentStatusCommand) @@ -90,7 +92,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, match-remote, meta, issue-context, fetch-diff, comment-body, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, scratch-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, recover-findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, match-remote, meta, issue-context, fetch-diff, comment-body, fetch-pr, capture-local, plan-diff, cache-commit, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, scratch-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, recover-findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index f3d93588b47..87bc58f6cf7 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -38,6 +38,7 @@ // remember. import type { CommandModule } from 'yargs'; +import { displayAnchor } from './lib/report.js'; import { createHash } from 'node:crypto'; import { readFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; @@ -733,7 +734,7 @@ export function buildChunkAgentPrompt( const lines = [ '', `**This is an INCREMENTAL round** — the diff holds only what changed since the ` + - `previous clean review round (anchor \`${inertPath(incremental.anchor.slice(0, 12))}\`), ` + + `previous clean review round (anchor \`${inertPath(displayAnchor(incremental.anchor))}\`), ` + `plus still-clean files one import hop from a change. Your files' scopes:`, ]; if (deltaHere.length > 0) { @@ -1103,7 +1104,7 @@ function diffReadingBlock( ...(incremental ? [ `**Incremental round.** This diff is scoped to what changed since the previous ` + - `clean review round (anchor \`${inertPath(incremental.anchor.slice(0, 12))}\`), plus ` + + `clean review round (anchor \`${inertPath(displayAnchor(incremental.anchor))}\`), plus ` + `still-clean files one import hop from a change — each of those is in scope ` + `only for its interaction with what it imports. The rest of the change was ` + `reviewed clean last round and is deliberately absent; do not go find it. ` + diff --git a/packages/cli/src/commands/review/cache-commit.test.ts b/packages/cli/src/commands/review/cache-commit.test.ts new file mode 100644 index 00000000000..bcc0c92eb4b --- /dev/null +++ b/packages/cli/src/commands/review/cache-commit.test.ts @@ -0,0 +1,341 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The one property that matters here is precedence: the candidate's anchor +// fields must win every key collision, because the ledger half is +// model-written and a mis-copied anchor is exactly the defect that moved this +// merge out of prose. The rest is boundary manners: refuse loudly on inputs +// that would write a cache no next round could trust. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + mkdtempSync, + rmSync, + writeFileSync, + readFileSync, + existsSync, + mkdirSync, + symlinkSync, + lstatSync, + realpathSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { cacheCommitCommand } from './cache-commit.js'; + +let dir: string; + +beforeEach(() => { + // `realpathSync`, like every sibling fixture here: the handler refuses to + // write through a symlinked parent (`assertUnredirectedParent`), and + // `tmpdir()` IS a symlink on macOS (`/var/folders/…` → + // `/private/var/folders/…`). Without the wrap every success-path test here + // fails on a developer's Mac while CI stays green on its real-path TMPDIR. + dir = realpathSync(mkdtempSync(join(tmpdir(), 'cache-commit-'))); +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +function run(argv: Record): void { + (cacheCommitCommand.handler as (argv: unknown) => void)(argv); +} + +function seed(candidate: unknown, ledger: unknown): Record { + const candidatePath = join(dir, 'candidate.json'); + const ledgerPath = join(dir, 'ledger.json'); + // Every real candidate carries the identity that certified the round — both + // captures record it — so a fixture that omits it is testing something else + // and gets the default. A test about the field itself passes its own (or + // `null` to leave it out). + const withModel = + candidate !== null && + typeof candidate === 'object' && + !('lastModelId' in candidate) + ? { ...candidate, lastModelId: 'candidate-model@aaaaaaaa' } + : candidate; + writeFileSync(candidatePath, JSON.stringify(withModel)); + writeFileSync(ledgerPath, JSON.stringify(ledger)); + return { + candidate: candidatePath, + ledger: ledgerPath, + out: join(dir, 'cache/pr-7.json'), + }; +} + +describe('cache-commit', () => { + it('merges candidate + ledger, candidate winning every collision', () => { + const argv = seed( + { + v: 1, + target: 'pr-7', + lastCommitSha: 'real-sha', + lastModelId: 'm1', + fileVerdicts: { 'a.ts': { base: 'b', head: 'h' } }, + }, + { + // The bare token an orchestrator could type. The candidate's + // provider-qualified one must win it, like every other anchor field. + lastModelId: 'bare-name', + round: 2, + verdict: 'Approve', + findings: [], + findingsCount: 0, + lastCommitSha: 'forged-sha', // the collision that must lose + }, + ); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect(cache['lastCommitSha']).toBe('real-sha'); + // The candidate's, not the bare token the ledger carried. + expect(cache['lastModelId']).toBe('m1'); + expect(cache['round']).toBe(2); + expect(cache['fileVerdicts']).toEqual({ 'a.ts': { base: 'b', head: 'h' } }); + expect(typeof cache['lastReviewDate']).toBe('string'); + }); + + it('refuses a CANDIDATE without lastModelId — the capture records who certified', () => { + // Not the ledger: a ledger value is one the orchestrator typed, and the + // only token it can type is the bare `{{model}}`, which two providers + // exposing one model name share. + const argv = seed({ v: 1, target: 'pr-7', lastModelId: '' }, { round: 1 }); + expect(() => run(argv)).toThrow(/lastModelId/); + expect(existsSync(argv['out'])).toBe(false); + }); + + it('refuses unreadable or non-object inputs by name', () => { + const argv = seed({ v: 1 }, { lastModelId: 'm' }); + writeFileSync(argv['candidate'], '[1,2]'); + expect(() => run(argv)).toThrow(/not a JSON object/); + expect(() => + run({ ...argv, candidate: join(dir, 'missing.json') }), + ).toThrow(/cannot read the cache candidate/); + }); + + it('works for a LOCAL candidate too — the merge is shape-agnostic', () => { + const argv = seed( + { + v: 1, + target: 'local', + headSha: 'h', + files: { 'a.ts': 'x' }, + stateId: 's', + }, + { round: 1, verdict: 'Comment', findings: [] }, + ); + argv['out'] = join(dir, 'cache/local.json'); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect(cache['stateId']).toBe('s'); + expect(cache['lastModelId']).toBe('candidate-model@aaaaaaaa'); + }); + + it('refuses a candidate whose target does not match --out', () => { + // pr-7's candidate committed to pr-8.json erases pr-8's ledger under + // pr-7's anchor. + const argv = seed( + { v: 1, target: 'pr-7' }, + { lastModelId: 'm1', round: 1 }, + ); + expect(() => run({ ...argv, out: join(dir, 'cache/pr-8.json') })).toThrow( + /refusing to promote across targets/, + ); + }); + + it('a candidate cannot smuggle ledger-owned fields, and a ledger cannot backdate the stamp', () => { + const argv = seed( + { + v: 1, + target: 'pr-7', + lastCommitSha: 'real-sha', + // Tampered candidate keys OUTSIDE the anchor allowlist: must be + // ignored, or a wrong candidate erases unresolved review state. + round: 99, + verdict: 'Approve', + findings: [], + lastModelId: 'candidate-model@aaaaaaaa', + }, + { + lastModelId: 'm1', + round: 2, + verdict: 'Request changes', + findings: [{ id: 'R1-1' }], + lastReviewDate: '1999-01-01T00:00:00Z', // must not survive + }, + ); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect(cache['round']).toBe(2); + expect(cache['verdict']).toBe('Request changes'); + expect(cache['findings']).toEqual([{ id: 'R1-1' }]); + expect(cache['lastModelId']).toBe('candidate-model@aaaaaaaa'); + expect(cache['lastCommitSha']).toBe('real-sha'); + expect(cache['lastReviewDate']).not.toBe('1999-01-01T00:00:00Z'); + }); + + it('an allowlist key present only in the LEDGER is scrubbed from the merge', () => { + // The delete branch: a ledger smuggling `fileVerdicts` (an anchor field + // the candidate does not carry) must not have it survive into the cache. + const argv = seed( + { v: 1, target: 'pr-7' }, + { + lastModelId: 'm1', + round: 1, + fileVerdicts: { 'a.ts': { base: 'x', head: 'y' } }, + }, + ); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect('fileVerdicts' in cache).toBe(false); + }); + + it('every candidate-owned anchor field survives the merge intact', () => { + const argv = seed( + { + v: 1, + target: 'local', + headSha: 'h1', + files: { 'a.ts': '100644:x' }, + stateId: 's1', + }, + { lastModelId: 'm1', round: 3, verdict: 'Approve', findings: [] }, + ); + argv['out'] = join(dir, 'cache/local.json'); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect(cache['v']).toBe(1); + expect(cache['target']).toBe('local'); + expect(cache['headSha']).toBe('h1'); + expect(cache['files']).toEqual({ 'a.ts': '100644:x' }); + expect(cache['stateId']).toBe('s1'); + }); + + it('a slashed target names the flattened-token contract in its refusal', () => { + const argv = seed({ v: 1, target: 'src/foo.ts' }, { lastModelId: 'm1' }); + expect(() => + run({ ...argv, out: join(dir, 'cache/src_foo.ts.json') }), + ).toThrow(/FLATTENED repo-relative path/); + }); + + it('refuses control characters in the ledger model AND in any candidate anchor field', () => { + // The command's stated posture: refuse at the writing end, where a human + // is present, rather than escaping at every reader. Policing one field of + // a tampered candidate is policing none — the next round hands + // lastCommitSha to git as an argument. + const esc = String.fromCharCode(0x1b); + const bad1 = seed({ v: 1, target: 'pr-7', lastModelId: `m${esc}[31m` }, {}); + expect(() => run(bad1)).toThrow(/lastModelId. carries control/); + expect(existsSync(bad1['out'])).toBe(false); + const bad2 = seed( + { v: 1, target: 'pr-7', lastCommitSha: `abc${esc}[2J` }, + {}, + ); + expect(() => run(bad2)).toThrow(/lastCommitSha. carries control/); + expect(existsSync(bad2['out'])).toBe(false); + }); + + it('every refusal leaves NO cache file behind', () => { + const argv = seed({ v: 1, target: 'pr-7' }, { lastModelId: 'm1' }); + expect(() => run({ ...argv, out: join(dir, 'cache/pr-8.json') })).toThrow(); + expect(existsSync(join(dir, 'cache/pr-8.json'))).toBe(false); + }); + + it('preserves an explicitly-null candidate field (unborn HEAD)', () => { + const argv = seed( + { v: 1, target: 'local', headSha: null, files: {}, stateId: 's' }, + { lastModelId: 'm1' }, + ); + argv['out'] = join(dir, 'cache/local.json'); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect('headSha' in cache).toBe(true); + expect(cache['headSha']).toBeNull(); + }); + + it('carries mergeBaseSha and map-valued anchor fields through the merge', () => { + const argv = seed( + { + v: 1, + target: 'pr-7', + lastCommitSha: 'head-sha', + mergeBaseSha: 'base-sha', + fileVerdicts: { 'a.ts': { base: '100644 b', head: '100644 h' } }, + }, + { lastModelId: 'm1', round: 1 }, + ); + run(argv); + const cache = JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >; + expect(cache['mergeBaseSha']).toBe('base-sha'); + expect(cache['fileVerdicts']).toEqual({ + 'a.ts': { base: '100644 b', head: '100644 h' }, + }); + }); + + it.skipIf(process.platform === 'win32')( + 'replaces a planted symlink instead of writing through it', + () => { + // The cache path is deterministic and inside the repo: a contributor + // branch can commit a symlink there, and a maintainer's review would + // otherwise clobber the link's target with merged-cache JSON. + const victim = join(dir, 'victim.txt'); + writeFileSync(victim, 'ORIGINAL'); + const argv = seed({ v: 1, target: 'pr-7' }, { lastModelId: 'm1' }); + mkdirSync(join(dir, 'cache'), { recursive: true }); + symlinkSync(victim, argv['out']); + run(argv); + expect(readFileSync(victim, 'utf8')).toBe('ORIGINAL'); + expect(lstatSync(argv['out']).isSymbolicLink()).toBe(false); + expect( + JSON.parse(readFileSync(argv['out'], 'utf8')) as Record< + string, + unknown + >, + ).toMatchObject({ target: 'pr-7' }); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'refuses when a symlink sits at the cache DIRECTORY, not just the file', + () => { + // `noFollow` guards the final element only; planting the link one + // layer up needs no guess at the file name (`local.json` is fixed) + // and lands the merged cache in the attacker's directory. + const elsewhere = join(dir, 'elsewhere'); + mkdirSync(elsewhere, { recursive: true }); + symlinkSync(elsewhere, join(dir, 'cache')); + const argv = seed({ v: 1, target: 'pr-7' }, { lastModelId: 'm1' }); + expect(() => run(argv)).toThrow(/resolves to .* Refusing/s); + expect(existsSync(join(elsewhere, 'pr-7.json'))).toBe(false); + }, + ); + + it('refuses an EMPTY lastModelId, not just a missing one', () => { + const argv = seed({ v: 1, target: 'pr-7', lastModelId: '' }, {}); + expect(() => run(argv)).toThrow(/lastModelId/); + }); +}); diff --git a/packages/cli/src/commands/review/cache-commit.ts b/packages/cli/src/commands/review/cache-commit.ts new file mode 100644 index 00000000000..bf89d04fc3b --- /dev/null +++ b/packages/cli/src/commands/review/cache-commit.ts @@ -0,0 +1,212 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review cache-commit`: promote a capture's cache candidate into +// `.qwen/review-cache/`, merged with the round's model-written ledger. +// +// The cache used to be hand-written by the model from a JSON template, which +// was fine while it held six scalar fields and a findings list. The content +// anchors changed that: a candidate now carries a per-file map — a hundred +// blob pairs on a real PR — and routing that through model output is a copy +// job models get wrong in ways nobody sees (a dropped entry reads exactly +// like a file that was never captured, and the next round silently +// full-reviews or — worse — trusts a pair that was mangled in transit). +// +// So the merge is mechanical. The candidate (deterministic, written by +// `fetch-pr` or `capture-local` at capture time) provides the anchor fields; +// the ledger file (small, model-written under Step 8's prose rules) provides +// the round's verdict and findings; this command validates both and writes +// the union atomically. Precedence is the security posture: the candidate's +// fields WIN on any key collision, so a ledger cannot overwrite the anchor it +// rides beside — a mis-copied `lastCommitSha` was exactly the class of defect +// the hand-written template invited. +// +// WHETHER to promote stays the model's call, exactly as before: Step 8's +// fail-closed and effort gates decide whether this command runs at all. What +// stops being the model's call is the bytes. + +import type { CommandModule } from 'yargs'; +import { readFileSync, mkdirSync } from 'node:fs'; +import { basename, dirname, resolve } from 'node:path'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; +import { writeStdoutLine } from '../../utils/stdioHelpers.js'; +import { inertText } from './lib/inert-text.js'; +import { assertUnredirectedParent } from './lib/paths.js'; + +interface CacheCommitArgs { + candidate: string; + ledger: string; + out: string; +} + +function readJsonObject(path: string, what: string): Record { + let raw: unknown; + try { + raw = JSON.parse(readFileSync(path, 'utf8')); + } catch (err) { + throw new Error( + `cache-commit: cannot read ${what} at ${inertText(path)}: ` + + // A JSON.parse failure embeds a snippet of the offending bytes, + // control characters included, and this error reaches the terminal + // through the CLI's raw stderr write. + inertText((err as Error).message), + ); + } + if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { + throw new Error( + `cache-commit: ${what} at ${inertText(path)} is not a JSON object`, + ); + } + return raw as Record; +} + +/** The candidate-owned fields — the ONLY keys a candidate may contribute. + * Everything else in a candidate file is ignored: an allowlist, so a + * tampered candidate cannot smuggle `round`, `verdict` or `findings` past + * the ledger, exactly as the ledger cannot overwrite the anchor. + * + * This list is the one place that decides what survives promotion, so a + * field added on the producer side has to be added HERE too — a candidate + * field the local flow wrote and this list omitted was silently dropped, + * and the next round's gate then refused its own anchor for ever, blaming + * a stale cache format. */ +const CANDIDATE_FIELDS = [ + 'v', + 'target', + 'headSha', + 'files', + 'stateId', + 'lastCommitSha', + 'mergeBaseSha', + 'fileVerdicts', + // The identity that certified the round, and an ANCHOR field like the rest + // of this list even though it names a model rather than a tree. The capture + // records it from what the runtime published — provider-qualified, + // `@` — because the alternative is a token routed through + // the orchestrator's output, and `{{model}}` interpolates the BARE model + // id: two provider configurations exposing one model name write the same + // string and pass each other's same-model gate, which is the whole contract + // the anchor rests on. Left out of this list, a hand-written ledger key + // would win the collision and put that bare token back in the cache. + 'lastModelId', +] as const; + +function runCacheCommit(args: CacheCommitArgs): void { + const candidate = readJsonObject(args.candidate, 'the cache candidate'); + const ledger = readJsonObject(args.ledger, 'the round ledger'); + + // The two fields every incremental check reads are non-negotiable: a cache + // without a model has no same-model contract to enforce, and Step 1 would + // fail-open it into a full review forever; better to refuse loudly now. + // + // Read off the CANDIDATE, which is where both captures record it, and never + // off the ledger. A ledger value is one the orchestrator typed, and the only + // token it can type is `{{model}}` — the BARE model id, which two provider + // configurations exposing one model name share. The captures record what the + // runtime published instead, provider-qualified, so the string that gets + // compared is the one that tells those two apart. + const candidateModel = candidate['lastModelId']; + if (typeof candidateModel !== 'string' || candidateModel === '') { + throw new Error( + 'cache-commit: the candidate must carry a non-empty `lastModelId` — ' + + 'the incremental anchor is a same-model contract, and the capture is ' + + 'what records who certified it.', + ); + } + // The promoted cache is read back by commands that print these values on a + // refusal, and the next round hands `lastCommitSha` to git as an argument; + // a control-charactered value would make the cache the intake for a forged + // terminal line. Refuse at the writing end, where a human is present, + // rather than escaping it at every reader. Every persisted STRING is + // checked — the ledger's model id and each candidate-owned anchor field — + // because the threat this command polices is a tampered candidate, and + // policing one field of it is policing none. + const controlled = (v: unknown): boolean => + // eslint-disable-next-line no-control-regex + typeof v === 'string' && /[\u0000-\u001f\u007f]/.test(v); + for (const key of CANDIDATE_FIELDS) { + if (controlled(candidate[key])) { + throw new Error( + `cache-commit: the candidate's \`${key}\` carries control ` + + 'characters — refusing to persist a value that forges terminal ' + + 'output (or a git argument) when read back.', + ); + } + } + + // Bind the promotion to its target: `pr-7`'s candidate committed to + // `pr-8.json` erases pr-8's ledger under pr-7's anchor. The out path's + // basename IS the target by the cache's naming contract. + const outTarget = basename(args.out).replace(/\.json$/, ''); + if (candidate['target'] !== outTarget) { + const hint = + typeof candidate['target'] === 'string' && + candidate['target'].includes('/') + ? ' The target contains "/": file-path reviews must pass the ' + + 'FLATTENED repo-relative path (src/foo.ts -> src_foo.ts) as ' + + '--target and name the cache file the same way.' + : ''; + throw new Error( + `cache-commit: the candidate belongs to target ` + + `${inertText(String(candidate['target']), 80)}, but --out names ` + + `${inertText(outTarget, 80)} — refusing to promote across targets.` + + hint, + ); + } + + const merged: Record = { ...ledger }; + for (const key of CANDIDATE_FIELDS) { + // Candidate fields LAST: the anchor must win any collision (see header). + if (key in candidate) merged[key] = candidate[key]; + else delete merged[key]; + } + // Command-owned, stamped at promotion: spread FIRST it was silently + // overridable by a ledger key — the precedence inversion this command + // exists to prevent, in its own output. + merged['lastReviewDate'] = new Date().toISOString(); + + mkdirSync(dirname(resolve(args.out)), { recursive: true }); + assertUnredirectedParent(args.out, 'cache', 'cache-commit'); + // `noFollow`: the target path is deterministic and lives in the repo, so a + // contributor branch can commit a SYMLINK there and a maintainer's review + // would write merged-cache JSON onto the link's target — an arbitrary-file + // clobber inside the reviewer's permissions, invisible in the reviewed + // diff. The default resolves the chain and renames onto the resolved file; + // this replaces the link itself. + atomicWriteFileSync(args.out, `${JSON.stringify(merged, null, 2)}\n`, { + noFollow: true, + }); + writeStdoutLine(`Committed review cache to ${args.out}`); +} + +export const cacheCommitCommand: CommandModule = { + command: 'cache-commit', + describe: + "Promote a capture's cache candidate into .qwen/review-cache/, merged " + + "with the round's ledger", + builder: (y) => + y + .option('candidate', { + type: 'string', + demandOption: true, + describe: + "The capture's cache candidate (the plan's `cacheCandidatePath`)", + }) + .option('ledger', { + type: 'string', + demandOption: true, + describe: + 'A small JSON file with the round ledger: lastModelId, round, ' + + 'verdict, findingsCount, findings[]', + }) + .option('out', { + type: 'string', + demandOption: true, + describe: 'The cache file to write (.qwen/review-cache/.json)', + }) + .strict(), + handler: (argv) => runCacheCommit(argv as unknown as CacheCommitArgs), +}; diff --git a/packages/cli/src/commands/review/capture-local.test.ts b/packages/cli/src/commands/review/capture-local.test.ts index 0646bc33f44..176521fa6ac 100644 --- a/packages/cli/src/commands/review/capture-local.test.ts +++ b/packages/cli/src/commands/review/capture-local.test.ts @@ -11,7 +11,15 @@ // command that reports it stopped saying a file was skipped. import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { mkdtempSync, rmSync, readFileSync, existsSync } from 'node:fs'; +import { + mkdtempSync, + mkdirSync, + rmSync, + readFileSync, + existsSync, + realpathSync, + symlinkSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { seedParseArgs } from './lib/test-utils.js'; @@ -65,7 +73,12 @@ function capture(over: Record = {}) { } beforeEach(() => { - dir = mkdtempSync(join(tmpdir(), 'capture-local-')); + // `realpathSync`: the candidate write now refuses a parent chain that + // traverses a symlink, and `tmpdir()` IS one on macOS + // (`/var/folders/…` → `/private/var/folders/…`). Without the wrap every + // test here would fail on a developer's Mac while CI stayed green on its + // real-path TMPDIR — the same trap this directory's sibling suites hit. + dir = realpathSync(mkdtempSync(join(tmpdir(), 'capture-local-'))); cwd = process.cwd(); process.chdir(dir); errs = []; @@ -95,6 +108,42 @@ describe('capture-local (command boundary)', () => { expect(readFileSync(plan.diffPathAbsolute, 'utf8')).toBe(DIFF); }); + it('drops the candidate — not the round — on a symlinked `.qwen/tmp`', () => { + // `noFollow` guards only the final element, and this path is + // deterministic and in-repo: a contributor branch can commit `.qwen/tmp` + // as a link (gitignore does not stop `git add -f`), and the atomic + // tmp+rename then lands the candidate wherever it points. That is worse + // than a clobbered file — the plan advertises the path as + // `cacheCandidatePath`, and `cache-commit` reads back a candidate the + // attacker wrote, promoting forged anchors past validation that is only + // shape-deep. + const elsewhere = realpathSync(mkdtempSync(join(tmpdir(), 'victim-'))); + mkdirSync(join(dir, '.qwen'), { recursive: true }); + symlinkSync(elsewhere, join(dir, '.qwen', 'tmp')); + try { + capture(); + // The round COMPLETES — the guard costs the anchor, not the review. + // Letting it throw exited non-zero with no plan, no report and no diff + // after the capture, the hashing and the plan were all already done. + run(join(dir, 'plan.json')); + const plan = JSON.parse( + readFileSync(join(dir, 'plan.json'), 'utf8'), + ) as Record; + expect(plan['diffPath']).toBeTruthy(); + // …and says so, with the field ABSENT rather than naming a file this + // run refused to write: Step 8 branches on its presence, so a silent + // drop would send it promoting an earlier round's candidate. + expect(plan['cacheCandidatePath']).toBeUndefined(); + expect(errs.join('\n')).toContain('Could not write the cache candidate'); + // Nothing was written through the link. + expect( + existsSync(join(elsewhere, 'qwen-review-local-cache-candidate.json')), + ).toBe(false); + } finally { + rmSync(elsewhere, { recursive: true, force: true }); + } + }); + it('creates the output directory the caller chose', () => { // It created `.qwen/tmp` — its own — and then wrote to the caller's path, // which may be elsewhere. `--out reports/plan.json` answered with ENOENT. diff --git a/packages/cli/src/commands/review/capture-local.toctou.test.ts b/packages/cli/src/commands/review/capture-local.toctou.test.ts index 1b9ac9c6900..d7c038d0147 100644 --- a/packages/cli/src/commands/review/capture-local.toctou.test.ts +++ b/packages/cli/src/commands/review/capture-local.toctou.test.ts @@ -17,6 +17,7 @@ import { writeFileSync, readFileSync, existsSync, + mkdirSync, realpathSync, } from 'node:fs'; import { execFileSync } from 'node:child_process'; @@ -254,3 +255,38 @@ describe('capture-local — TOCTOU candidate withholding', () => { expect(stderrLines.join('\n')).not.toContain('candidate is withheld'); }); }); + +describe('capture-local — the withheld candidate is not announced', () => { + it('omits cacheCandidatePath from the plan and removes a stale file', () => { + // Step 8 branches on the field's presence; announcing a path to a file + // this run deliberately withheld sends it promoting an earlier round's + // candidate. + const stale = join( + repo, + '.qwen/tmp/qwen-review-local-cache-candidate.json', + ); + // PLANT an earlier round's candidate: without it the removal assertion + // passes even when the removal itself is deleted. + mkdirSync(join(repo, '.qwen/tmp'), { recursive: true }); + writeFileSync( + stale, + JSON.stringify({ v: 1, target: 'local', stale: true }), + ); + captures.push({ diff: DIFF_A }, { diff: Buffer.from('moved mid-hash\n') }); + run(); + const plan = JSON.parse( + readFileSync(join(repo, 'plan.json'), 'utf8'), + ) as Record; + expect('cacheCandidatePath' in plan).toBe(false); + expect(existsSync(stale)).toBe(false); + }); + + it('announces the path when the candidate IS written', () => { + captures.push({ diff: DIFF_A }, { diff: Buffer.from(DIFF_A) }); + run(); + const plan = JSON.parse( + readFileSync(join(repo, 'plan.json'), 'utf8'), + ) as Record; + expect(plan['cacheCandidatePath']).toContain('cache-candidate.json'); + }); +}); diff --git a/packages/cli/src/commands/review/capture-local.ts b/packages/cli/src/commands/review/capture-local.ts index 1613a9b29a6..b62960c70df 100644 --- a/packages/cli/src/commands/review/capture-local.ts +++ b/packages/cli/src/commands/review/capture-local.ts @@ -21,12 +21,19 @@ import { existsSync, mkdirSync, readFileSync, + rmSync, statSync, writeFileSync, } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { repoRelativeOf, REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; +import { + assertUnredirectedParent, + repoRelativeOf, + REVIEW_TMP_DIR, + tmpFile, +} from './lib/paths.js'; import { safeTarget } from '../../utils/paths.js'; import { planEffortField } from './lib/effort.js'; import type { ReviewEffort } from './parse-args.js'; @@ -87,8 +94,12 @@ type CaptureLocalResult = PlanReport & { skippedFiles: SkippedFile[]; /** Present only when `--cache` scoped this capture incrementally. */ incremental?: IncrementalBlock; - /** Where this round's content anchor landed — Step 8 promotes it on a clean run. */ - cacheCandidatePath: string; + /** + * Where this round's content anchor landed — Step 8 promotes it on a clean + * run. ABSENT when the capture withheld the candidate (a mid-capture tree + * change), because Step 8 branches on this field's presence. + */ + cacheCandidatePath?: string; }; /** @@ -357,10 +368,54 @@ function runCaptureLocal(args: CaptureLocalArgs): void { // erased the first file's anchor and its open findings. ...(sourcePath !== undefined ? { source: sourcePath } : {}), }; - const cacheCandidatePath = tmpFile(target, 'cache-candidate.json'); + const candidatePath = tmpFile(target, 'cache-candidate.json'); + // The field rides the plan ONLY when a candidate exists to promote: Step 8 + // keys its cache-commit-vs-hand-write branch on the field's presence, and + // announcing a path to a file this run deliberately withheld would send it + // promoting a stale candidate from an earlier round. + let cacheCandidatePath: string | undefined; if (treeHeldStill) { - writeFileSync(cacheCandidatePath, JSON.stringify(candidate, null, 2)); + // Guarded as a whole, like the PR flow's candidate write and for the + // reason that one states: a convenience artefact must never take the + // round with it. The refusal below arrives AFTER the capture, the + // hashing and the plan are all done; letting it escape `runCaptureLocal` + // — which the yargs handler does not catch — exited non-zero with no + // plan, no report and no diff, over a check whose whole cost is supposed + // to be the next round's anchor. The pre-guard code wrote here with a + // plain `writeFileSync` and could not fail this way at all. + try { + // The PARENT chain first: `noFollow` below guards only the final + // element, and `.qwen/tmp` committed as a symlink redirects the write + // just as well — the plan then advertises that path as + // `cacheCandidatePath` and `cache-commit` reads a candidate the + // attacker wrote. Same guard the promoted cache gets. + assertUnredirectedParent( + candidatePath, + 'cache candidate', + 'capture-local', + ); + // noFollow: a planted symlink at this deterministic path would redirect + // the candidate write onto its target (see cache-commit's note). + atomicWriteFileSync(candidatePath, JSON.stringify(candidate, null, 2), { + noFollow: true, + }); + cacheCandidatePath = candidatePath; + } catch (err) { + // Said out loud, and the field stays absent: Step 8 branches on its + // presence, so a silent drop would send it promoting an earlier + // round's candidate. + writeStderrLine( + `Could not write the cache candidate ` + + `(${(err as Error).message}); this round cannot anchor the next ` + + `one, but the review itself is unaffected.`, + ); + } } else { + try { + rmSync(candidatePath, { force: true }); + } catch { + // The absent field above is the load-bearing half. + } writeStderrLine( 'The working tree changed while the capture was being hashed — the ' + 'cache candidate is withheld, so the next round cannot anchor on ' + @@ -575,7 +630,7 @@ function runCaptureLocal(args: CaptureLocalArgs): void { skippedFiles: capture.skipped, ...(incremental ? { incremental } : {}), ...(nothingToReview ? { nothingToReview } : {}), - cacheCandidatePath, + ...(cacheCandidatePath ? { cacheCandidatePath } : {}), ...planEffortField(args.effort), }; diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 706f174ac31..5acf5e7ab16 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -26,6 +26,7 @@ // LLM reads to drive the rest of Step 1. import type { CommandModule } from 'yargs'; +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; import { execFileSync } from 'node:child_process'; import { createHash } from 'node:crypto'; import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; @@ -61,6 +62,7 @@ import { widenScope } from './lib/incremental-scope.js'; import { containedWorktreeReader } from './lib/worktree-reader.js'; import { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from './lib/diff-flags.js'; import { + assertUnredirectedParent, REVIEW_TMP_DIR, reviewBranch, tmpFile, @@ -81,6 +83,7 @@ import { import { resolveMergeBase, type GitProbe } from './lib/merge-base.js'; import { operatorReviewSettings } from './lib/review-settings.js'; import { SHA_RE } from './lib/ledger.js'; +import { blobPairs } from './lib/file-verdicts.js'; import { appendRunSession, ledgerResumeCount, @@ -245,6 +248,23 @@ type FetchPrResult = PlanReport & { * pair is certified by the same declared fallback as before this field. */ reviewModelId?: string; + /** + * Where this round's content-verdict candidate landed — the per-file + * `(base, head)` blob pairs of everything the plan covers, plus the commit + * anchor. Step 8 promotes it into the review cache on a clean high-effort + * end (via `cache-commit`). Absent when the capture had no diff to + * describe. + * + * PRODUCER SIDE ONLY at this commit, and the distinction matters because + * the docs used to read as though the feature had shipped. Nothing reads + * `fileVerdicts` back yet: the transfer was to be `rescope --cache`, and + * `rescope` is gone — its scoping moved into `fetch-pr --since`. So a + * rebase still degrades to a full review today; what the pairs buy is that + * the record exists and is sound (mode-aware, `.gitattributes`-aware, + * refusing an added-file pair) when the consumer lands on the `--since` + * path. Until then this field is groundwork, not rebase survival. + */ + cacheCandidatePath?: string; /** * Present when `--since ` was passed: the incremental-review scoping * decision, validated HERE so the orchestrator never hand-runs git against @@ -1524,6 +1544,81 @@ async function runFetchPr(args: FetchPrArgs): Promise { ); } } + // The content-verdict candidate: what a clean end of THIS round would let a + // post-rebase round transfer. Computed here because this is the moment the + // reviewed pairs are defined — plan files at `mergeBaseSha..fetchedSha` — + // and written beside the plan unconditionally: promotion into the cache is + // Step 8's clean-high-effort decision, not the capture's. + let cacheCandidatePath: string | undefined; + if (diffPath !== null && mergeBaseSha) { + // Pinned to the repo root: the pathspec-scoped ls-tree inside resolves + // paths against git's cwd, and a fetch started from a subdirectory would + // otherwise record every pair as (absent, absent) — a candidate that + // later transfers clean verdicts over anything. + const pairs = blobPairs( + gitOpt('rev-parse', '--show-toplevel') ?? '.', + mergeBaseSha, + fetchedSha, + plan.files.map((f) => f.path), + ); + if (pairs !== null) { + // Guarded for the reason the plan-partition step above is: this runs + // after the worktree exists and before the report is written, and a + // convenience artifact must never take the whole fetch with it. + try { + cacheCandidatePath = tmpFile( + `pr-${prNumber}`, + 'cache-candidate.json', + ); + // `noFollow` below guards the final element only, and this path is + // deterministic and in-repo: `.qwen/tmp` committed as a symlink + // (gitignore does not stop `git add -f`) redirects the write + // through the chain — and the plan then advertises that + // attacker-chosen path as `cacheCandidatePath` for `cache-commit` + // to read back, so a swapped candidate promotes forged anchors into + // the review cache, where every validation is shape-based. The same + // guard `cache-commit` already applies to its own `--out`, for the + // same reason its header gives. + assertUnredirectedParent( + cacheCandidatePath, + 'cache candidate', + 'fetch-pr', + ); + atomicWriteFileSync( + cacheCandidatePath, + JSON.stringify( + { + v: 1, + target: `pr-${prNumber}`, + lastCommitSha: fetchedSha, + mergeBaseSha, + fileVerdicts: pairs, + // WHO certified this anchor, recorded HERE rather than merged + // in by Step 8 from `{{model}}`. That interpolates the BARE + // model id, while every identity this CLI compares is + // provider-qualified — two provider configurations exposing + // one model name wrote the same token and passed each other's + // same-model gate, which is the contract the anchor rests on. + // Empty when the runtime published nothing, which every + // consumer reads as a mismatch. + lastModelId: roundModelIdFrom(process.env), + }, + null, + 2, + ), + { noFollow: true }, + ); + } catch (err) { + cacheCandidatePath = undefined; + writeStderrLine( + `WARNING: could not write the cache candidate ` + + `(${(err as Error).message}); this round cannot anchor the next ` + + `one's rebase survival, but the review itself is unaffected.`, + ); + } + } + } + const result: FetchPrResult = { prNumber, ownerRepo, @@ -1614,6 +1709,7 @@ async function runFetchPr(args: FetchPrArgs): Promise { diffSha256, prDescriptionHasHan: /\p{Script=Han}/u.test(meta.body ?? ''), ...(roundModelId ? { reviewModelId: roundModelId } : {}), + ...(cacheCandidatePath ? { cacheCandidatePath } : {}), ...(anchor ? { incremental: anchor.incremental } : {}), ...buildPlanReport(plan, (path) => fileLineCount(fetchedSha, path), { operatorRoundCap: operatorReviewSettings().reverseAuditRounds, diff --git a/packages/cli/src/commands/review/lib/file-verdicts.test.ts b/packages/cli/src/commands/review/lib/file-verdicts.test.ts new file mode 100644 index 00000000000..26f0424b88a --- /dev/null +++ b/packages/cli/src/commands/review/lib/file-verdicts.test.ts @@ -0,0 +1,298 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real git for the blob listings — content addressing IS git's behaviour, and +// the property under test (a rebase that preserves a file's pair preserves +// its verdict) only means anything against real object ids. The parsing half +// is the usual untrusted-boundary posture: the map lives in a model-promoted +// cache, so malformed → null, never a throw. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + rmSync, + writeFileSync, + mkdirSync, + realpathSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + NO_BLOB, + blobsAt, + blobPairs, + changedPairs, + readFileVerdicts, +} from './file-verdicts.js'; +import { isolateHostGitConfig } from './test-utils.js'; + +let repo: string; +let cwd: string; +let gitIsolation: ReturnType; + +function git(...args: string[]): string { + return execFileSync('git', args, { cwd: repo, encoding: 'utf8' }).trim(); +} + +function write(rel: string, content: string): void { + const abs = join(repo, rel); + mkdirSync(join(abs, '..'), { recursive: true }); + writeFileSync(abs, content); +} + +beforeEach(() => { + repo = realpathSync(mkdtempSync(join(tmpdir(), 'review-fv-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'commit.gpgsign', 'false'); +}); + +afterEach(() => { + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +describe('blobsAt / blobPairs', () => { + it('lists mode+oid identities at a ref, absent paths as NO_BLOB', () => { + write('a.ts', 'A\n'); + write('dir/b.ts', 'B\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'one'); + const sha = git('rev-parse', 'HEAD'); + const blobs = blobsAt(repo, sha, ['a.ts', 'dir/b.ts', 'missing.ts'])!; + expect(blobs['a.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/); + expect(blobs['dir/b.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/); + expect(blobs['missing.ts']).toBe(NO_BLOB); + // Content-addressed: the oid half equals what hash-object computes. + expect(blobs['a.ts']).toBe(`100644 ${git('hash-object', '--', 'a.ts')}`); + }); + + it('the identity carries the MODE: an exec-bit flip alone changes the pair', () => { + write('run.sh', 'echo hi\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + const base = git('rev-parse', 'HEAD'); + git('update-index', '--chmod=+x', 'run.sh'); + git('commit', '-q', '--no-verify', '-m', 'chmod only'); + const head = git('rev-parse', 'HEAD'); + const recorded = blobPairs(repo, base, base, ['run.sh'])!; + const current = blobPairs(repo, base, head, ['run.sh'])!; + // Same bytes both sides — git diff still prints old/new mode lines, and + // the pair must move with them. + expect(changedPairs(recorded, current, ['run.sh'])).toEqual(['run.sh']); + }); + + it('an attribute change retires the verdict, blobs byte-identical or not', () => { + // What a round REVIEWS is the rendering, and `.gitattributes` decides it: + // `binary` turns hunks into "Binary files … differ" for the same bytes. + // A ` ` identity cannot see that, so the clean verdict + // transferred over a diff no round ever read. + write('data.txt', 'line1\n'); + write('.gitattributes', 'data.txt binary\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + const base = git('rev-parse', 'HEAD'); + // The attribute goes away; `data.txt` itself is untouched. + write('.gitattributes', '\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'attributes only'); + const head = git('rev-parse', 'HEAD'); + + const recorded = blobPairs(repo, base, base, ['data.txt'])!; + const current = blobPairs(repo, base, head, ['data.txt'])!; + // The pair itself did NOT move — which is exactly the hole. + expect(recorded['data.txt']).toEqual(current['data.txt']); + // …and the attributes file was recorded even though the caller never + // named it, so the consumer can see the move. + expect(recorded['.gitattributes']).toBeDefined(); + expect(changedPairs(recorded, current, ['data.txt'])).toEqual(['data.txt']); + }); + + it('records the governing .gitattributes of every directory on the path', () => { + // One `.gitattributes` governs a subtree, so the set that applies to a + // path is every ancestor's. Recording only the root's would let a nested + // one change unseen. + write('pkg/deep/a.ts', 'A\n'); + write('pkg/.gitattributes', '*.ts text\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'nested attributes'); + const sha = git('rev-parse', 'HEAD'); + const pairs = blobPairs(repo, sha, sha, ['pkg/deep/a.ts'])!; + expect(pairs['pkg/.gitattributes']).toBeDefined(); + expect(pairs['pkg/.gitattributes'].base).not.toBe(NO_BLOB); + // The ones that do not exist are recorded inert rather than omitted, so + // their later APPEARANCE is a move the consumer sees. + expect(pairs['.gitattributes']).toEqual({ base: NO_BLOB, head: NO_BLOB }); + expect(pairs['pkg/deep/.gitattributes']).toEqual({ + base: NO_BLOB, + head: NO_BLOB, + }); + }); + + it('is cwd-independent: identical listings from the root and a subdirectory', () => { + write('pkg/deep/a.ts', 'A\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'one'); + const sha = git('rev-parse', 'HEAD'); + const fromRoot = blobsAt(repo, sha, ['pkg/deep/a.ts']); + const prev = process.cwd(); + process.chdir(join(repo, 'pkg')); + try { + // Unpinned, the pathspec would miss from here and read NO_BLOB — the + // silent everything-absent shape that converts a fallback into a skip. + expect(blobsAt(repo, sha, ['pkg/deep/a.ts'])).toEqual(fromRoot); + } finally { + process.chdir(prev); + } + }); + + it('returns null — unusable, not "everything absent" — on a bad ref, and on ONE bad side of a pair', () => { + write('a.ts', 'A\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'one'); + const sha = git('rev-parse', 'HEAD'); + expect(blobsAt(repo, 'deadbeef', ['a.ts'])).toBeNull(); + expect(blobPairs(repo, 'deadbeef', sha, ['a.ts'])).toBeNull(); + expect(blobPairs(repo, sha, 'deadbeef', ['a.ts'])).toBeNull(); + }); + + it('pairs survive a history rewrite that preserves content', () => { + write('a.ts', 'A\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + const base1 = git('rev-parse', 'HEAD'); + write('a.ts', 'A2\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'change'); + const head1 = git('rev-parse', 'HEAD'); + // Rewrite history: same tree contents, brand-new shas. + git('commit', '--amend', '-q', '--no-verify', '-m', 'change (amended)'); + const head2 = git('rev-parse', 'HEAD'); + expect(head2).not.toBe(head1); + expect(blobPairs(repo, base1, head1, ['a.ts'])).toEqual( + blobPairs(repo, base1, head2, ['a.ts']), + ); + }); + + it('a batch past 200 paths still maps every file', () => { + const many: string[] = []; + for (let i = 0; i < 201; i++) { + const p = `many/f${String(i).padStart(3, '0')}.txt`; + write(p, String(i)); + many.push(p); + } + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'many'); + const sha = git('rev-parse', 'HEAD'); + const blobs = blobsAt(repo, sha, many)!; + for (const p of many) expect(blobs[p]).toMatch(/^100644 [0-9a-f]{40,64}$/); + }); +}); + +describe('blobsAt — pathspec magic', () => { + // ':' is a reserved NTFS character (drive / ADS separator): the write + // itself throws on Windows, where the merge-queue leg runs this suite. + it.skipIf(process.platform === 'win32')( + 'a colon-prefixed filename lists literally under --literal-pathspecs', + () => { + write(':weird.ts', 'W\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'colon'); + const sha = git('rev-parse', 'HEAD'); + const blobs = blobsAt(repo, sha, [':weird.ts'])!; + expect(blobs[':weird.ts']).toMatch(/^100644 [0-9a-f]{40,64}$/); + }, + ); +}); + +describe('blobsAt — decode aliasing', () => { + it.skipIf(process.platform === 'win32')( + 'refuses the whole listing when a path cannot decode faithfully', + () => { + // U+FFFD in a decoded path is ambiguous by construction: it is either + // a filename literally containing the replacement character (this + // fixture) or a filename whose invalid byte the decode destroyed. The + // two collapse to one key, so a plan path could resolve to the OTHER + // file's tree entry on both sides and carry its clean verdict over an + // edited file. Unusable beats wrong — the caller degrades to full. + const mangled = 'a\uFFFD.ts'; + writeFileSync(join(repo, mangled), 'A\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'replacement-char name'); + const sha = git('rev-parse', 'HEAD'); + expect(blobsAt(repo, sha, [mangled])).toBeNull(); + expect(blobPairs(repo, sha, sha, [mangled])).toBeNull(); + }, + ); +}); + +describe('readFileVerdicts', () => { + it('round-trips a valid map and rejects every malformation', () => { + const good = { 'a.ts': { base: 'b1', head: 'h1' } }; + expect(readFileVerdicts(good)).toEqual(good); + for (const bad of [ + null, + 'nope', + { 'a.ts': { base: 'b1' } }, + { 'a.ts': { base: 1, head: 'h' } }, + { 'a.ts': null }, + ]) { + expect(readFileVerdicts(bad)).toBeNull(); + } + }); +}); + +describe('changedPairs', () => { + const recorded = { + 'same.ts': { base: 'b1', head: 'h1' }, + 'moved-base.ts': { base: 'b2', head: 'h2' }, + 'moved-head.ts': { base: 'b3', head: 'h3' }, + }; + it('flags a moved base, a moved head, and an unrecorded path; keeps identical pairs', () => { + const current = { + 'same.ts': { base: 'b1', head: 'h1' }, + 'moved-base.ts': { base: 'bX', head: 'h2' }, + 'moved-head.ts': { base: 'b3', head: 'hX' }, + 'new.ts': { base: NO_BLOB, head: 'h4' }, + }; + expect( + changedPairs(recorded, current, [ + 'same.ts', + 'moved-base.ts', + 'moved-head.ts', + 'new.ts', + ]), + ).toEqual(['moved-base.ts', 'moved-head.ts', 'new.ts']); + }); + + it('an ABSENT-BASE pair never transfers, identical or not', () => { + // The rename hole: a pure rename records (absent, blob) for its + // destination, and a keep-both restructure reproduces the same pair + // while the file became an all-new addition no round ever read. + const rec = { 'added.ts': { base: NO_BLOB, head: 'h1' } }; + const cur = { 'added.ts': { base: NO_BLOB, head: 'h1' } }; + expect(changedPairs(rec, cur, ['added.ts'])).toEqual(['added.ts']); + }); + + it('an identical DELETION pair (blob, NO_BLOB) transfers — only absent-BASE never does', () => { + const rec = { 'gone.ts': { base: '100644 b1', head: NO_BLOB } }; + const cur = { 'gone.ts': { base: '100644 b1', head: NO_BLOB } }; + expect(changedPairs(rec, cur, ['gone.ts'])).toEqual([]); + }); + + it('a path named __proto__ compares as an ordinary key', () => { + const rec = JSON.parse( + '{"__proto__": {"base": "b1", "head": "h1"}}', + ) as Record; + expect(changedPairs(rec, {}, ['__proto__'])).toEqual(['__proto__']); + }); +}); diff --git a/packages/cli/src/commands/review/lib/file-verdicts.ts b/packages/cli/src/commands/review/lib/file-verdicts.ts new file mode 100644 index 00000000000..2e73ad343ac --- /dev/null +++ b/packages/cli/src/commands/review/lib/file-verdicts.ts @@ -0,0 +1,275 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Content-addressed per-file verdicts: the PR flow's rebase survival. +// +// The commit anchor dies with its history: one rebase, and `rescope` refuses +// the sha (correctly — the range would review the wrong code) and the whole +// incremental saving degrades to a full review. But "what the previous round +// reviewed" was never really a commit; it was, per file, a PAIR of tree +// entries — the base side and the head side, whose difference is exactly the +// diff the round read. Tree entries are content-addressed and indifferent to +// history: after a rebase that changed nothing about a file's diff, its +// `(base, head)` pair is byte-for-byte the pair the clean round certified, +// and the verdict transfers. A file whose pair moved — its own change +// amended, or the merge-base slid under it — re-enters the scope in full. +// +// The identity is ` `, not the oid alone: an exec-bit flip or a +// file↔symlink typechange is its own lines in `git diff`, so identical +// content under a different mode is NOT an identical change. +// +// The pairs are recorded at capture time by `fetch-pr` (they describe what +// this round is about to review) and promoted into the review cache only by +// Step 8's clean-high-effort gate, exactly like the local flow's content +// candidate. They deliberately do NOT ride the posted-review marker: a +// hundred-file map does not fit a footnote (`LEDGER_MAX_BYTES`), so a fresh +// environment keeps the commit anchor and only the machine that reviewed +// keeps rebase survival — the same graceful degradation the cache has always +// had. + +import { gitRaw } from './git.js'; +import { LITERAL_PATHSPECS } from './diff-flags.js'; + +/** A file absent on one side: created by the PR, or deleted by it. */ +export const NO_BLOB = 'absent'; + +export interface BlobPair { + base: string; + head: string; +} + +export type FileVerdicts = Record; + +/** A verdicts map that keys arbitrary file paths safely — `__proto__` + * included: on a plain object that assignment is a silent no-op. */ +function nullProtoMap(): Record { + return Object.create(null) as Record; +} + +/** + * Tree-entry identities (` `) of `paths` at `ref`, batched — one + * `ls-tree` per 200 paths. Paths absent at the ref map to `NO_BLOB`. + * + * `repoRoot` pins the git cwd: pathspecs resolve against it, and from a + * subdirectory an unmatched pathspec exits 0 with EMPTY output — every path + * would read `NO_BLOB` at every ref, pairs would compare stable, and the + * fallback would silently convert into a skip. + * + * The listing is read as BYTES (`gitRaw`), not through the CRLF-normalising + * text helpers: `-z` exists precisely to keep paths byte-faithful. (Even a + * mangled lookup would only ever fail SAFE now — an unmatched path stays + * `NO_BLOB`, and `changedPairs` never transfers an absent-base pair — but a + * byte-faithful read keeps the identity, so a CRLF filename costs nothing + * instead of a permanent re-review.) + * + * A ref that cannot be listed at all returns null: the caller must treat the + * whole lookup as unusable rather than reading "everything absent". + */ +export function blobsAt( + repoRoot: string, + ref: string, + paths: readonly string[], +): Record | null { + const out = nullProtoMap(); + for (const p of paths) out[p] = NO_BLOB; + const BATCH = 200; + for (let i = 0; i < paths.length; i += BATCH) { + const batch = paths.slice(i, i + BATCH); + let raw: Buffer; + try { + raw = gitRaw( + '-C', + repoRoot, + LITERAL_PATHSPECS, + 'ls-tree', + '-r', + '-z', + ref, + '--', + ...batch, + ); + } catch { + return null; + } + // ` \t` records, NUL-terminated. `-z` also turns + // off the C-style quoting that would otherwise mangle non-ASCII paths. + // Decoding is where a byte-faithful listing can still betray us: an + // invalid UTF-8 byte in a filename decodes to U+FFFD, and a SIBLING + // literally named with U+FFFD then shares the decoded key. The verdict + // recorded under that key would be the sibling's pair on both sides, so + // an edited file compares unchanged and its clean verdict transfers. + // The whole lookup goes unusable rather than resolving the wrong file — + // the caller degrades to the full-range review, which is the direction + // every other failure here takes. + const text = raw.toString('utf8'); + if (text.includes('\uFFFD')) return null; + const seen = new Set(); + for (const record of text.split('\0')) { + if (record === '') continue; + const tab = record.indexOf('\t'); + if (tab < 0) continue; + const meta = record.slice(0, tab).split(' '); + const path = record.slice(tab + 1); + // Two records decoding to one key is the same aliasing by another + // route (a repo can hold both spellings). + if (seen.has(path)) return null; + seen.add(path); + if (meta.length >= 3 && Object.hasOwn(out, path)) { + out[path] = `${meta[0]} ${meta[2]}`; + } + } + } + return out; +} + +/** + * The recorded pairs for `paths` across a base and a head. + * + * The `.gitattributes` that GOVERN those paths are recorded too, whether or + * not the round touched them. They decide how a blob is rendered — `binary`, + * `-diff`, `text` — and a round reviews the rendering, so a pair identity + * that cannot see them lets a clean verdict transfer over a diff nobody read. + * The consumer (`changedPairs`) rules on them; it can only rule on what the + * producer wrote, which is why they are added HERE and not left to a caller + * to remember. They carry `NO_BLOB` on a side where they do not exist, like + * any other path. + */ +export function blobPairs( + repoRoot: string, + baseSha: string, + headSha: string, + paths: readonly string[], +): FileVerdicts | null { + const all = [...new Set([...paths, ...governingAttributePaths(paths)])]; + const base = blobsAt(repoRoot, baseSha, all); + const head = blobsAt(repoRoot, headSha, all); + if (base === null || head === null) return null; + const out = nullProtoMap(); + for (const p of all) out[p] = { base: base[p], head: head[p] }; + return out; +} + +/** + * Every `.gitattributes` that could apply to `paths`: the repository root's, + * and one in each ancestor directory of each path. + * + * Derived from the path strings rather than probed on disk — a file that does + * not exist records `NO_BLOB` on both sides and is inert, which costs one + * `ls-tree` entry and needs no filesystem walk. Git also reads + * `.git/info/attributes` and the user's global file; neither is in the tree, + * so neither travels with the PR, and a round cannot be made to disagree with + * itself through them. + */ +function governingAttributePaths(paths: readonly string[]): string[] { + const out = new Set([GITATTRIBUTES]); + for (const p of paths) { + const parts = p.split('/'); + // The last element is the filename, so stop before it. + for (let i = 1; i < parts.length; i++) { + out.add(`${parts.slice(0, i).join('/')}/${GITATTRIBUTES}`); + } + } + return [...out]; +} + +/** + * Validate a `fileVerdicts` map read from the (model-promoted) cache. + * Malformed → null, and the caller degrades to the full review — the same + * untrusted-boundary posture as every other cache read. + */ +export function readFileVerdicts(raw: unknown): FileVerdicts | null { + if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { + return null; + } + const out = nullProtoMap(); + for (const [path, pair] of Object.entries(raw as Record)) { + const p = pair as { base?: unknown; head?: unknown }; + if (!p || typeof p.base !== 'string' || typeof p.head !== 'string') { + return null; + } + out[path] = { base: p.base, head: p.head }; + } + return out; +} + +/** Every `.gitattributes` path either side recorded, in one set. */ +function attributePaths(...sides: FileVerdicts[]): string[] { + const out = new Set(); + for (const side of sides) { + for (const p of Object.keys(side)) { + if (p === GITATTRIBUTES || p.endsWith(`/${GITATTRIBUTES}`)) out.add(p); + } + } + return [...out]; +} + +/** + * Did any `.gitattributes` this record covers move between the two states? + * + * Only files the record CARRIES are visible here — `blobPairs` is given the + * plan's file list — so an attributes file outside it cannot be ruled on. The + * producer is what has to include them; this is the consumer's half, and it + * fails safe on what it can see. + */ +function attributesMoved( + recorded: FileVerdicts, + current: FileVerdicts, +): boolean { + return attributePaths(recorded, current).some((p) => { + const rec = Object.hasOwn(recorded, p) ? recorded[p] : undefined; + const cur = Object.hasOwn(current, p) ? current[p] : undefined; + // Present on one side only is a move: added, deleted, or newly in scope. + if (!rec || !cur) return true; + return rec.base !== cur.base || rec.head !== cur.head; + }); +} + +const GITATTRIBUTES = '.gitattributes'; + +/** + * The paths whose pair moved — plus every path the record never saw, which + * has no verdict to transfer. `paths` is the CURRENT plan's file list: a file + * the record knows but the current diff no longer touches simply has nothing + * to review, so it contributes nothing here. + * + * A pair whose BASE side is `NO_BLOB` never transfers, identical or not. + * Such a pair says "this path did not exist at the merge base" — which is + * also what a pure RENAME records for its destination, with the rename + * source never consulted. After a keep-both restructure (the old path + * restored, the new path a plain copy of the same content) the pair is + * byte-identical while the file's true diff became an all-new addition no + * round ever read. Added files re-enter every round; their incremental + * saving is the one this identity cannot carry soundly. + */ +export function changedPairs( + recorded: FileVerdicts, + current: FileVerdicts, + paths: readonly string[], +): string[] { + // `.gitattributes` decides how a blob is RENDERED, and a round reviews the + // rendering, not the blob. `binary`, `-diff` and `text` all change what + // `git diff` emits for byte-identical content — one history shows + // "Binary files … differ" where the other shows hunks — and those files are + // in the tree, so a PR can change them. A pair identity built from + // ` ` cannot see it, and the verdict would transfer over a diff + // no round ever read, in either direction: hunks appearing where none were + // reviewed, or content vanishing behind a binary marker. + // + // Ruled here rather than folded into each pair, because the attributes are + // not per-file state: one `.gitattributes` governs a subtree, and the set + // that applies to a path is itself a function of the tree. Any move in any + // of them retires every transferable verdict for this round — coarse, and + // the fail-safe direction: a full review costs tokens, a transferred + // verdict over an unread rendering costs the review. + if (attributesMoved(recorded, current)) return [...paths]; + return paths.filter((p) => { + const rec = Object.hasOwn(recorded, p) ? recorded[p] : undefined; + const cur = Object.hasOwn(current, p) ? current[p] : undefined; + if (!rec || !cur) return true; + if (rec.base === NO_BLOB || cur.base === NO_BLOB) return true; + return rec.base !== cur.base || rec.head !== cur.head; + }); +} diff --git a/packages/cli/src/commands/review/lib/inert-text.test.ts b/packages/cli/src/commands/review/lib/inert-text.test.ts new file mode 100644 index 00000000000..dada33e9f6f Binary files /dev/null and b/packages/cli/src/commands/review/lib/inert-text.test.ts differ diff --git a/packages/cli/src/commands/review/lib/inert-text.ts b/packages/cli/src/commands/review/lib/inert-text.ts new file mode 100644 index 00000000000..c7579ce1562 --- /dev/null +++ b/packages/cli/src/commands/review/lib/inert-text.ts @@ -0,0 +1,54 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// One escaper for every workspace-controlled string this command family +// prints. +// +// The review reads three classes of attacker-or-model-written text and writes +// them to a terminal: filenames (git permits almost any byte in one), the +// review cache's own fields (`lastModelId`, `stateId`, `target` — written by +// a model under prose rules, or planted on disk), and `JSON.parse` failure +// messages, which embed a snippet of the offending file's bytes verbatim, +// control characters included. Printed raw, any of them can forge a second +// warning line — SKILL.md tells the orchestrator to repeat stderr lines back +// to the user, so a forged line reaches the model's context too — or emit +// OSC/CSI sequences at the operator's terminal. +// +// `capture-local` has escaped filenames this way since its own review found +// the hole; this module is that rule, extracted, so the newer sinks cannot +// each re-derive it (and re-forget it). + +/** Control characters, including DEL — the forgery and escape-sequence set. */ +// eslint-disable-next-line no-control-regex +const CONTROL = /[\u0000-\u001f\u007f]/; + +/** + * Render untrusted text inert for a terminal: quoted-and-escaped when it + * carries control characters, verbatim otherwise (the overwhelming case, and + * quoting every ordinary string would make every message harder to read). + * + * `maxChars` caps the result BEFORE escaping so a hostile value cannot flood + * the line; the cap counts source characters, and the truncation marker is + * added outside the quoting so it can never be mistaken for content. + */ +export function inertText(value: string, maxChars = 200): string { + const clipped = value.length > maxChars ? value.slice(0, maxChars) : value; + // `JSON.stringify` alone is not enough: it escapes the familiar control + // characters but passes DEL (U+007F) through verbatim, and DEL is a + // terminal control code like any other. Every control character is + // replaced explicitly, then the whole value is quoted so it cannot be + // read as prose. + const rendered = CONTROL.test(clipped) + ? JSON.stringify( + // eslint-disable-next-line no-control-regex + clipped.replace(/[\u0000-\u001f\u007f]/g, (c) => { + const hex = c.charCodeAt(0).toString(16).padStart(4, '0'); + return `\\u${hex}`; + }), + ) + : clipped; + return clipped.length < value.length ? `${rendered}…` : rendered; +} diff --git a/packages/cli/src/commands/review/lib/paths.test.ts b/packages/cli/src/commands/review/lib/paths.test.ts index e70aedf74ce..a636926f6a7 100644 --- a/packages/cli/src/commands/review/lib/paths.test.ts +++ b/packages/cli/src/commands/review/lib/paths.test.ts @@ -7,6 +7,15 @@ import { describe, it, expect } from 'vitest'; import { basename, dirname, join, resolve } from 'node:path'; import { + mkdtempSync, + mkdirSync, + realpathSync, + rmSync, + symlinkSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { + assertUnredirectedParent, inertPath, tmpFile, probeWorktreePath, @@ -180,3 +189,63 @@ describe('inertPath', () => { expect(inertPath('my probe.ts')).toBe('my probe.ts'); }); }); + +describe('assertUnredirectedParent', () => { + it('refuses a write whose parent chain traverses a symlink', () => { + // `noFollow` protects the FINAL element only, and the threat is satisfied + // one layer up: plant `.qwen/tmp` (or `.qwen/review-cache`) as a link — + // gitignore does not stop `git add -f` — and `mkdirSync(…, + // {recursive:true})` succeeds through it while the atomic tmp+rename + // lands the file wherever the link points. Worse for a candidate: the + // plan then advertises that path, and `cache-commit` reads back a + // candidate the attacker wrote, promoting forged anchors past validation + // that is only shape-deep. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'redirect-'))); + try { + const victim = join(root, 'victim'); + mkdirSync(victim); + const link = join(root, 'linked'); + symlinkSync(victim, link); + expect(() => + assertUnredirectedParent( + join(link, 'candidate.json'), + 'cache candidate', + 'fetch-pr', + ), + ).toThrow(/resolves to .*Refusing/s); + // The command name rides the message: three writers share this guard, + // and a refusal that names none of them is one nobody can act on. + expect(() => + assertUnredirectedParent(join(link, 'x.json'), 'cache', 'cache-commit'), + ).toThrow(/^cache-commit:/); + + // A real directory passes. + expect(() => + assertUnredirectedParent( + join(victim, 'candidate.json'), + 'cache candidate', + 'fetch-pr', + ), + ).not.toThrow(); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + it('refuses a parent that cannot be resolved at all', () => { + // Unresolvable is not "fine": the write would create it, and what it + // creates through is exactly what this cannot see. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'redirect-'))); + try { + expect(() => + assertUnredirectedParent( + join(root, 'nope', 'candidate.json'), + 'cache candidate', + 'capture-local', + ), + ).toThrow(/cannot resolve the cache candidate directory/); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 8c50068d4ab..ff370c88e02 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -10,8 +10,9 @@ // concatenation so Windows backslashes are produced when needed. import { existsSync, realpathSync, statSync } from 'node:fs'; -import { isAbsolute, join, relative, resolve, sep } from 'node:path'; +import { dirname, isAbsolute, join, relative, resolve, sep } from 'node:path'; import { safeTarget } from '../../../utils/paths.js'; +import { inertText } from './inert-text.js'; /** * Classify a `--out` target BEFORE the command fetches anything: an empty / @@ -304,3 +305,45 @@ export function repoRelativeOf( rel === '' || rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel); return { rel, abs, escapes }; } + +/** + * Refuse a write whose PARENT CHAIN is redirected. + * + * `noFollow` protects the final path element only, and the threat these + * commands police — a contributor branch committing a symlink at a + * deterministic in-repo path — is satisfied one layer up just as well: plant + * `.qwen/tmp` or `.qwen/review-cache` itself as a link (gitignore does not + * stop `git add -f`), and `mkdirSync(…, {recursive:true})` succeeds through + * it while the atomic tmp+rename lands the file in the attacker's directory, + * over whatever of that name lives there. Comparing the resolved parent + * against its lexical form catches a link ANYWHERE in the chain, which is + * what the final-element guard cannot do. + * + * Shared, because the three writers that need it are the three that write a + * deterministic in-repo path — the promoted cache and both candidate files — + * and the one that had it while the others did not is how a candidate whose + * path the plan then advertised became an attacker-chosen file. + */ +export function assertUnredirectedParent( + target: string, + what: string, + command: string, +): void { + const parent = resolve(dirname(target)); + let real: string; + try { + real = realpathSync(parent); + } catch (err) { + throw new Error( + `${command}: cannot resolve the ${what} directory ${inertText(parent)}: ` + + inertText((err as Error).message), + ); + } + if (real !== parent) { + throw new Error( + `${command}: the ${what} directory ${inertText(parent)} resolves to ` + + `${inertText(real)} — a symlink in the path would redirect this ` + + `write outside the tree it names. Refusing.`, + ); + } +} diff --git a/packages/cli/src/commands/review/lib/report.test.ts b/packages/cli/src/commands/review/lib/report.test.ts index 94ab30e8c75..a033927db32 100644 --- a/packages/cli/src/commands/review/lib/report.test.ts +++ b/packages/cli/src/commands/review/lib/report.test.ts @@ -6,7 +6,11 @@ import { describe, it, expect } from 'vitest'; import { buildDiffPlan } from './diff-plan.js'; -import { buildPlanReport, stringifyPlanReport } from './report.js'; +import { + buildPlanReport, + displayAnchor, + stringifyPlanReport, +} from './report.js'; import { makeDiff } from './test-utils.js'; /** A diff that edits an existing file: `ctx` context lines then `add` new ones. */ @@ -212,3 +216,18 @@ describe('stringifyPlanReport', () => { expect(parsed.files[0].path).toBe(weird); }); }); + +describe('displayAnchor', () => { + it('truncates sha-shaped labels and renders every other label whole', () => { + const sha40 = 'a'.repeat(40); + expect(displayAnchor(sha40)).toBe('a'.repeat(12)); + expect(displayAnchor('A'.repeat(64))).toHaveLength(12); + // The regression it exists for: a 12-char slice printed `content-verd` + // in the summary line and in every brief. + expect(displayAnchor('content-verdicts')).toBe('content-verdicts'); + // A local round's state id is a 64-hex sha256 — truncating it is right. + expect(displayAnchor('f'.repeat(64))).toHaveLength(12); + // Too short to be an object id: rendered whole rather than mangled. + expect(displayAnchor('abc123')).toBe('abc123'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/report.ts b/packages/cli/src/commands/review/lib/report.ts index 7f4fcd0fb85..7523e4b1023 100644 --- a/packages/cli/src/commands/review/lib/report.ts +++ b/packages/cli/src/commands/review/lib/report.ts @@ -301,3 +301,14 @@ export interface IncrementalScope { /** Where the full-range diff still is, for a reader who needs all of it. */ fullDiffPath: string | null; } + +/** + * Render an incremental anchor for humans: truncate only sha-shaped labels. + * The label space holds 40-64-hex commit shas AND the literal + * `content-verdicts`; a blind 12-char slice printed `content-verd` into the + * summary line and every brief. One copy, because its two renderers + * (`rescope`'s summary, `agent-prompt`'s frames) must never drift. + */ +export function displayAnchor(label: string): string { + return /^[0-9a-f]{40,64}$/i.test(label) ? label.slice(0, 12) : label; +} diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index b0903ea45b1..b92f2963d28 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -558,6 +558,8 @@ It cannot be folded into the narrowing, because the file it adds is one the delt The local flow anchors on content, not on a commit, because it has no commit to anchor on and is forbidden from making one: the reviewed state is a dirty working tree, and `local-diff.ts`'s standing constraint — nothing on the capture path writes to the index, the worktree, or any ref — rules out snapshot commits and stashes. `git hash-object` without `-w` computes the blob id of the current bytes and writes nothing, so the anchor is the hashed per-file state of exactly what the plan covered, plus the HEAD the diff was measured against. The identity is `:`, not the blob alone — an exec-bit flip or a file↔symlink typechange is its own diff lines, so identical bytes under a different mode are not an identical change; symlinks hash their link text at 120000, exactly what `git diff` renders, never the resolved target's bytes. Whatever cannot be captured faithfully — a submodule gitlink (the pinned diff flags deliberately keep those visible), a FIFO, a path git C-quoted out of an invalid-UTF-8 filename — is marked `unhashable`, which never compares equal, not even to itself: "could not capture it twice" is not "unchanged", and each of those shapes was measured comparing stable under the naive scheme, silently leaving incremental scope forever. The capture also re-snapshots the diff after hashing and withholds the candidate unless the two captures are byte-identical — the one race where the anchor could certify bytes no round reviewed, closed by refusing to anchor rather than by pretending the window is empty. HEAD is hashed into the state id AND checked as a separate hard gate — the redundancy is for legibility's sake — "HEAD moved since the last local round" is a reason a user can act on, where a mere state-id mismatch is not — and it is load-bearing: the captured diff is HEAD-vs-worktree, so under a moved HEAD identical worktree bytes describe a different change under review (a reset exposes commits no round ever read). The candidate/cache split mirrors the PR flow's marker rules: the capture writes this round's anchor deterministically on every run, and only Step 8's clean-high-effort gate promotes it to `.qwen/review-cache/`, so a fail-closed round can never anchor the next round's skip past scope nobody reviewed. +The commit anchor's one blind spot is history rewrites, and the answer is that the anchor was never really a commit. What the round certified, per file, is a PAIR of tree entries — base side and head side, mode included, whose difference is exactly the diff it read — and tree entries are content-addressed: after a rebase that changed nothing about a file's change, its `(base, head)` pair is byte-for-byte the certified pair, and the verdict transfers; a pair that moved (the change amended, the merge-base slid under it, an exec bit flipped, a file swapped for a symlink) re-enters in full. Two pair classes never transfer at all: an absent-base pair (an added file — which is also what a pure rename records for its destination, and a keep-both restructure reproduces the pair while the file's true diff became an all-new addition no round read), and any pair the listing could not produce (the whole lookup goes unusable rather than reading "everything absent"). The pairs also apply while the commit anchor is ALIVE: an upstream-moved merge base changes a file's diff-under-review without one new commit past the anchor, so the scope is the union of the interdiff's files and the pair-moved files, and "empty interdiff" alone never certifies "nothing new" when verdicts are available to check. `fetch-pr` records the pairs at capture time, `cache-commit` promotes them mechanically on a clean high-effort end — the merge moved out of prose the day the cache grew a per-file map, because a model-transcribed pair that drops an entry reads exactly like a file that was never captured — and the CONSUMER is not landed: the transfer was to be `rescope --cache`, and `rescope` is gone — its scoping moved into `fetch-pr --since` when that command grew anchor validation. So a rebase still degrades to a full review today. What this half buys is a record that is sound when the consumer arrives — mode-aware, `.gitattributes`-aware (a round reviews the RENDERING, and an attribute change moves it while the blobs stand still), and refusing an absent-base pair outright. The terms the consumer must honour are unchanged: transfer only under the model that certified the pairs, and only when at least one pair actually transfers, since an "incremental" plan of everything would be a full review wearing the wrong label. The pairs deliberately do not ride the posted-review marker: a hundred-file map does not fit a footnote, so a fresh environment keeps the commit anchor and only the machine that reviewed keeps rebase survival — the cache's original degradation, unchanged. + ## Why three more mutation operators, and why each is shaped the way it is Statement deletion with a safety-verb filter was the first operator because it has the cleanest survivor semantics. But a live maintainer re-verification produced a survivor list the deletion operator cannot express — and every entry mapped to one of three shapes, each with equally crisp semantics: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 0f07371212f..32e31ab75e6 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -1341,12 +1341,11 @@ The JSON helper is fail-closed because it carries the authoritative review resul If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. -**A local or file-path review at high effort writes its cache the same way, from the capture's candidate.** `capture-local` wrote this round's content anchor to the plan's `cacheCandidatePath` (`.qwen/tmp/qwen-review--cache-candidate.json`): the hashed per-file state and HEAD of exactly what was captured, deterministic, not yours to recompute. Read that file, add the ledger fields — `lastReviewDate`, `round`, `findingsCount`, `verdict`, and `findings[]` under the same id-carrying rules as the PR cache below — and write the merged object to `.qwen/review-cache/local.json` (file-path review: `.qwen/review-cache/.json`). The same fail-closed rule as the PR cache applies unchanged — **and a non-empty `skippedFiles` in the capture is fail-closed for this write**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. A run that ended with unreviewed or undecided scope skips this write and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) Low and medium local reviews must NOT write it, for the PR cache's exact reason. +**The write is one command, for PR and local alike — never a hand-copied JSON.** Both captures write a deterministic candidate beside the plan (`cacheCandidatePath` in the plan report: `fetch-pr` records the per-file `(base, head)` blob pairs plus the commit anchor; `capture-local` the hashed per-file working-tree state plus HEAD). A candidate is not yours to recompute or transcribe — a per-file map routed through your output is a copy job that fails silently, and a dropped or mangled pair reads downstream as a verdict it is not. So: write ONLY the small ledger file `.qwen/tmp/qwen-review--ledger.json` with this round's `round`, `findingsCount`, `verdict`, and `findings[]` (id rules below). **No `lastModelId`**: the capture already recorded the identity the runtime published, provider-qualified, in the candidate — and `cache-commit` lets the candidate win it. A hand-carried one would be the bare `{{model}}`, which two provider configurations exposing a single model name share, so the same-model contract the anchor rests on would pass between them, then run `"${QWEN_CODE_CLI:-qwen}" review cache-commit --candidate --ledger --out .qwen/review-cache/.json` **from the main checkout, with all three paths resolved against it** — every one of them is main-checkout-relative (the candidate is where the capture wrote it, and the cache must outlive the worktree Step 9 deletes; a cache written inside the worktree is thrown away with it, and this machine silently loses rebase survival) (`pr-.json` / `local.json` / the file-review's target). The command merges mechanically, candidate fields winning every collision, and writes atomically. A plan with **no** `cacheCandidatePath` — a degraded capture, or a local round whose capture withheld the candidate because the tree moved mid-capture — falls back to hand-writing the template below — anchor fields only as accurate as what Step 1 recorded. The same fail-closed rule applies to BOTH flows unchanged: a run that ended with unreviewed or undecided scope skips the write entirely and says so, because the candidate would anchor the next round's skip past scope nobody reviewed. Low and medium reviews must NOT write it, for the reason above. **A non-empty `skippedFiles` in the capture is fail-closed for this write, both flows**: skipped content is in no diff and no hash, so a candidate promoted over it would anchor the next round's "no changes" past work this round could not read. (A capture that detected a mid-capture tree change withholds the candidate itself and says so — then there is nothing to promote.) **The cache advances exactly when the marker anchored — read the marker, do not re-derive the net.** `compose-review` already computed whether this round may certify a range: its posted body's ledger marker carries a `sha` on a clean round and withholds it otherwise (unproven coverage, an undecided blocker, any cap other than a depth-only `unreviewed-dimension` — where depth-only means every entry names the build-and-test dimension or is the machine's own relayed stop entry; a whiffed LENS in that field withholds). The cache and the marker must never disagree about what a clean round is, and a hand-copied condition list here is how they drifted once already — the list in this paragraph aged out of sync with the module and told a whiffed-lens round to cache the sha the marker had refused. So the rule is mechanical: **write `lastCommitSha` into the cache only if the composed body's marker carries a `sha`** (check the composed JSON's body for `"sha"` inside the `qwen-review-ledger` comment); when it does not, **skip the cache write entirely and say so in the terminal output**. Caching this SHA would scope the next high-effort run to `lastCommitSha..HEAD` — or, worse, let the same-SHA shortcut report "No new changes since last review" and skip the run outright, Step 6 re-check included: a whiffed Security lens at SHA A followed by an incremental review at SHA B means no run ever reviews A's diff for security, and an existing blocker this run could only mark `cannot tell` would never be re-checked at the same SHA, while the cached verdict reads as full coverage. Leave the previous cache entry in place (or none), so the next high-effort run re-covers the whole range — re-detecting any uncoverable chunk and re-ruling on any undecided blocker, keeping both disclosures alive: -1. Create `.qwen/review-cache/` directory if it doesn't exist -2. Write `.qwen/review-cache/pr-.json` with: +1. Write the ledger file and run `cache-commit` as described above. **Fallback only** — when the plan carries no `cacheCandidatePath` — create `.qwen/review-cache/` and hand-write `.qwen/review-cache/pr-.json` with: ```json { @@ -1370,7 +1369,7 @@ If reviewing a PR **at high effort**, update the review cache for incremental re The cache is the FALLBACK copy of the ledger — the authoritative one rides the posted review body itself: `compose-review` embeds a machine-readable marker (an HTML comment, invisible on the PR page) carrying this round's findings, round number, and — when the run ended clean — the reviewed head `sha`, and the next round's `pr-context` reads it back wherever it runs. The `sha` is what lets a fresh environment recover BOTH halves of incremental review, the work list and the anchor (Step 1's recovered-anchor check), where the cache could only ever serve the machine that wrote it. It is withheld under the fail-closed conditions that skip this cache write **and under every cap `compose-review` computes itself except `unreviewed-dimension`** — `cannotTellCriticals`, `uncoverableChunks`, the context-unavailable state, `scopeUnproven` (coverage the module could not prove — a chunk nobody read, an idle or blind agent), findings still `— [unverified]`, the deterministic gates — because an anchor written past unread scope would let the next round's incremental range skip it forever: a fail-closed round still posts its findings; it just never certifies a range. The wider net is measured, not cautionary: gated on the input fields alone, a round the module itself stamped "could not certify that any of this diff was reviewed" still carried the anchor. **`unreviewedDimensions` is the deliberate exception, and it is measured too**: it is prose about DEPTH — "the integration suite CI skipped did not run locally" is true of every round on a repo whose suites do not fit `build-test`'s whole-call budget — so gating on it closed a loop with no exit, where an untestable dimension capped the verdict, the cap withheld the anchor, and the missing anchor made the next round re-review the full diff of a PR that had not changed a line (measured: PR #9113 round 2, 119 minutes, 34M input tokens). A dimension nobody could run says nothing about WHICH LINES were read, and the anchor's only claim is about lines. A run that posts therefore persists its ledger even when this cache write is skipped; a run that does not post has only this cache, which is exactly why the cache remains. The `findings` ledger is what lets the **next** run open with "R1-2 is fixed" instead of a from-scratch list (see Step 6's previous-round section). Write every **newly confirmed high-confidence** finding under a fresh `R-` id, and carry a still-standing previous entry forward **under the id it already has** — the whole payoff is that `R1-2` names the same claim in every round, so a finding that survives is re-reported, never renumbered — while a finding ruled `fixed` this round leaves the ledger (the report said so; the cache is for what the next round must check, not history). Low-confidence and terminal-only findings stay out: the ledger holds claims this review stands behind, because next round re-asserts each one by id. Findings the convergence posture deferred stay out the same way — carrying them as ledger work would hand the next round the very re-ruling the posture exists to end. Their durable record on the PR is the POSTED deferral list (up to 20 entries; the body's overflow count names how many more) — and it is **not guaranteed**: the list is the first section the body budget trims, so an overflowing body can carry none of it. The findings artifact carries each deferred finding's full content under its `D-` id but no structured deferred marker yet, and the run report is machine-local — so an entry past the rendered cap, or in a list the budget trimmed, has no cross-round record on the PR at all. Keep the deferral list within its cap by collapsing families first (the bounded/unbounded rule) rather than deferring twenty-plus point findings; when the budget trims it, the terminal summary is where the author's copy comes from. -3. Ensure `.qwen/reviews/` and `.qwen/review-cache/` are ignored by `.gitignore` — a broader rule like `.qwen/*` also satisfies this. Only warn the user if those paths are not ignored at all. +2. Ensure `.qwen/reviews/` and `.qwen/review-cache/` are ignored by `.gitignore` — a broader rule like `.qwen/*` also satisfies this. Only warn the user if those paths are not ignored at all. ## Step 9: Clean up