diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 05840470d8d..ab4b7e8c235 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', + 'rescope', 'repo-context', 'pr-context', 'comment-status', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index b7b015962df..a17ab7b21bc 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -16,6 +16,7 @@ import { findingsCommand } from './review/findings.js'; import { fetchPrCommand } from './review/fetch-pr.js'; import { captureLocalCommand } from './review/capture-local.js'; import { planDiffCommand } from './review/plan-diff.js'; +import { rescopeCommand } from './review/rescope.js'; import { repoContextCommand } from './review/repo-context.js'; import { prContextCommand } from './review/pr-context.js'; import { commentStatusCommand } from './review/comment-status.js'; @@ -60,6 +61,7 @@ export const reviewCommand: CommandModule = { .command(fetchPrCommand) .command(captureLocalCommand) .command(planDiffCommand) + .command(rescopeCommand) .command(repoContextCommand) .command(prContextCommand) .command(commentStatusCommand) @@ -86,7 +88,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, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, match-remote, meta, issue-context, fetch-diff, comment-body, fetch-pr, capture-local, plan-diff, rescope, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index a08e98c5d27..1631ccd07a2 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -5611,3 +5611,197 @@ describe('--all-chunks topology anomaly note (#9242)', () => { } }); }); + +describe('incremental-scope briefs', () => { + // A rescoped plan carries two scopes in one diff. The chunk brief must say + // which scope each of ITS files is in — an interaction file re-reviewed from + // scratch re-reports what the previous round already ruled on — and the + // whole-diff readers must be told the rest of the PR is absent on purpose, + // or they go find it in the worktree. + const chunk = (id: number, path: string, start: number) => ({ + id, + startLine: start, + endLine: start + 9, + lines: 10, + chars: 400, + maxLineChars: 80, + oversized: false, + files: [{ path, newStart: 1, newEnd: 10 }], + }); + const INCREMENTAL_PLAN = { + diffPathAbsolute: '/abs/.qwen/tmp/qwen-review-pr-7-diff-incremental.txt', + chunks: [chunk(1, 'src/changed.ts', 1), chunk(2, 'src/caller.ts', 11)], + incremental: { + anchor: 'abc1234def5678900000', + deltaFiles: ['src/changed.ts'], + interaction: [ + { path: 'src/caller.ts', importsChanged: ['src/changed.ts'] }, + ], + contextFileCount: 1, + fullDiffPath: '.qwen/tmp/qwen-review-pr-7-diff.txt', + }, + }; + + it('a delta chunk is briefed to review in full, an interaction chunk at the seam', () => { + const delta = buildChunkAgentPrompt(INCREMENTAL_PLAN, 1); + expect(delta).toContain('INCREMENTAL round'); + expect(delta).toContain('abc1234def56'); + expect(delta).toContain('changed since the last round'); + expect(delta).not.toContain('INTERACTION only'); + + const seam = buildChunkAgentPrompt(INCREMENTAL_PLAN, 2); + expect(seam).toContain('INCREMENTAL round'); + expect(seam).toContain('cleared by the previous round'); + expect(seam).toContain('INTERACTION only'); + expect(seam).toContain('src/changed.ts'); + }); + + it('whole-diff role briefs carry the frame once, up front', () => { + const p = buildRoleBrief(INCREMENTAL_PLAN, '2'); + expect(p).toContain('Incremental round'); + expect(p).toContain('deliberately absent'); + }); + + it('a chunk-scoped ROLE brief lists its OWN files uncapped', () => { + // The reverse auditors are the sole reviewers of their territory; the + // globally capped list can elide their own files past entry 30, leaving + // no way to learn the class or recover the tail. + const wide = { + ...INCREMENTAL_PLAN, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: Array.from({ length: 40 }, (_, i) => `src/d${i}.ts`).concat( + ['src/changed.ts'], + ), + interaction: [ + { path: 'src/caller.ts', importsChanged: ['src/changed.ts'] }, + ], + }, + }; + const brief = buildRoleBrief(wide, 'reverse-audit', { chunk: 2 }); + expect(brief).toContain("Your territory's files, by scope class:"); + expect(brief).toContain('src/caller.ts — **interaction only**'); + // Chunk 1's delta file is named in ITS brief, not elided by the cap. + expect(buildRoleBrief(wide, 'reverse-audit', { chunk: 1 })).toContain( + 'src/changed.ts — **changed since the last round**', + ); + }); + + it('a full-range plan renders no incremental framing at all', () => { + expect(buildChunkAgentPrompt(PLAN, 13)).not.toContain('INCREMENTAL'); + expect(buildRoleBrief(PLAN, '2')).not.toContain('Incremental round'); + }); + + it('a malformed incremental block degrades to full-scope briefs — chunk AND role', () => { + for (const bad of [ + { anchor: 42 }, + // Valid anchor, but no scope list survives validation: rendering the + // frame with zero bullets is not a degrade, it is a confusion. + { anchor: 'abc1234def567890', deltaFiles: [], interaction: [] }, + // An interaction entry whose edges were all invalid names a seam + // pointing at nothing ("because it imports , which changed"). + { + anchor: 'abc1234def567890', + deltaFiles: [], + interaction: [{ path: 'src/caller.ts', importsChanged: [42] }], + }, + ]) { + const mangled = { ...INCREMENTAL_PLAN, incremental: bad }; + expect(buildChunkAgentPrompt(mangled, 1)).not.toContain('INCREMENTAL'); + expect(buildRoleBrief(mangled, '2')).not.toContain('Incremental round'); + } + }); + + it('a mixed delta+interaction chunk renders BOTH scope bullets', () => { + // rescope's composite is cut on line count, not scope class, so one + // chunk can straddle the two kinds; an else-if between the bullet + // branches would silently drop the seam brief for exactly that chunk. + const mixed = { + ...INCREMENTAL_PLAN, + chunks: [ + { + id: 1, + startLine: 1, + endLine: 20, + lines: 20, + chars: 800, + maxLineChars: 80, + oversized: false, + files: [ + { path: 'src/changed.ts', newStart: 1, newEnd: 10 }, + { path: 'src/caller.ts', newStart: 1, newEnd: 10 }, + ], + }, + ], + }; + const p = buildChunkAgentPrompt(mixed, 1); + expect(p).toContain('changed since the last round'); + expect(p).toContain('INTERACTION only'); + expect(p).toContain('the scope class WINS'); + }); + + it('caps the scope lists at 30 entries and 8 edges per entry', () => { + const wide = { + ...INCREMENTAL_PLAN, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: Array.from({ length: 40 }, (_, i) => `src/d${i}.ts`), + interaction: [ + { + path: 'src/hub.ts', + importsChanged: Array.from( + { length: 20 }, + (_, i) => `src/d${i}.ts`, + ), + }, + ], + }, + }; + const p = buildRoleBrief(wide, '2'); + expect(p).toContain('(+10 more)'); // 40 entries − 30 cap + expect(p).toContain('(+12 more)'); // 20 edges − 8 cap + }); + + it('an interaction entry whose edges are all EMPTY strings degrades away', () => { + const mangled = { + ...INCREMENTAL_PLAN, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: [], + interaction: [{ path: 'src/caller.ts', importsChanged: ['', ''] }], + }, + }; + expect(buildChunkAgentPrompt(mangled, 1)).not.toContain('INCREMENTAL'); + }); + + it('a chunk whose files carry NO scope class gets no incremental frame', () => { + // The block validates globally, but a frame with zero bullets implies + // the chunk is out of scope — say nothing instead. + const foreign = { + ...INCREMENTAL_PLAN, + chunks: [ + { + id: 1, + startLine: 1, + endLine: 10, + lines: 10, + chars: 400, + maxLineChars: 80, + oversized: false, + files: [{ path: 'src/unrelated.ts', newStart: 1, newEnd: 10 }], + }, + ], + }; + expect(buildChunkAgentPrompt(foreign, 1)).not.toContain( + 'INCREMENTAL round', + ); + }); + + it('whole-diff briefs name each file with its scope class', () => { + const p = buildRoleBrief(INCREMENTAL_PLAN, '2'); + expect(p).toContain( + 'Changed since the last round (full review): src/changed.ts.', + ); + expect(p).toContain('src/caller.ts (imports src/changed.ts)'); + }); +}); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 9b2c1cb4bd4..68f4387a828 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -145,7 +145,6 @@ interface PlanReport { worktreePath?: unknown; mergeBaseSha?: unknown; host?: unknown; - incremental?: unknown; repositoryContext?: unknown; /** * The two size fields the topology gate reads (#9242) and the ones @@ -163,6 +162,138 @@ interface PlanReport { srcDiffLines?: unknown; diffLines?: unknown; budget?: { agentToolBudget?: unknown; reverseAuditRounds?: unknown }; + /** Present only on a plan `rescope` rewrote — see incrementalScopeOf. */ + incremental?: unknown; +} + +/** + * The `incremental` block a rescoped plan carries, re-validated field by + * field: the plan is parsed off disk with an unchecked cast, and a malformed + * block must degrade to "not an incremental round" (full-scope briefs, which + * are always safe) rather than render `undefined` into an agent's contract. + */ +interface IncrementalScope { + anchor: string; + deltaFiles: string[]; + interaction: Array<{ path: string; importsChanged: string[] }>; +} + +/** + * The per-file scope lists a whole-diff brief renders under its incremental + * frame. Capped per class: past the cap the tail is counted, not listed — + * the plan's own `incremental` block remains the complete record. + */ +/** + * The per-file scope bullets for ONE chunk's files — uncapped, because the + * agent holding that chunk is the sole reviewer of those files and has no + * other source for their class. + */ +function chunkScopeBullets( + incremental: IncrementalScope, + chunk: DiffChunk | undefined, +): string[] { + if (!chunk) return []; + const paths = new Set( + (Array.isArray(chunk.files) ? chunk.files : []) + .map((f) => f?.path) + .filter((p): p is string => typeof p === 'string'), + ); + const delta = incremental.deltaFiles.filter((p) => paths.has(p)); + const seam = incremental.interaction.filter((e) => paths.has(e.path)); + if (delta.length === 0 && seam.length === 0) return []; + return [ + "Your territory's files, by scope class:", + ...delta.map( + (p) => + `- ${inertPath(p)} — **changed since the last round**: review its hunks in full.`, + ), + ...seam.map( + (e) => + `- ${inertPath(e.path)} — **interaction only**: cleared last round, back in ` + + `scope because it imports ${e.importsChanged.map(inertPath).join(', ')}. ` + + `Review that seam, not the rest of its diff.`, + ), + ]; +} + +const SCOPE_LIST_CAP = 30; +/** Edge lists are capped per entry in the WHOLE-DIFF frame — the entry cap + * alone still let one interaction row carry hundreds of imports into every + * brief. The chunk-level bullets are uncapped on purpose: that agent is the + * sole reviewer of its files' seams and has nowhere to recover a tail. */ +const SCOPE_EDGE_CAP = 8; +function cappedEdges(edges: readonly string[]): string { + const shown = edges.slice(0, SCOPE_EDGE_CAP).map(inertPath); + const rest = edges.length - SCOPE_EDGE_CAP; + return shown.join(', ') + (rest > 0 ? ` (+${rest} more)` : ''); +} +function scopeFileLists(incremental: IncrementalScope): string[] { + const cap = (items: T[], render: (item: T) => string): string => { + const shown = items.slice(0, SCOPE_LIST_CAP).map(render); + const rest = items.length - SCOPE_LIST_CAP; + return shown.join(', ') + (rest > 0 ? ` (+${rest} more)` : ''); + }; + const out: string[] = []; + if (incremental.deltaFiles.length > 0) { + out.push( + `Changed since the last round (full review): ` + + `${cap(incremental.deltaFiles, inertPath)}.`, + ); + } + if (incremental.interaction.length > 0) { + out.push( + `Interaction only (cleared last round; check the seam with what each ` + + `imports): ${cap( + incremental.interaction, + (e) => + `${inertPath(e.path)} (imports ${cappedEdges(e.importsChanged)})`, + )}.`, + ); + } + return out; +} + +function incrementalScopeOf(report: PlanReport): IncrementalScope | null { + const raw = report.incremental as + | { + anchor?: unknown; + deltaFiles?: unknown; + interaction?: unknown; + } + | undefined + | null; + if (!raw || typeof raw.anchor !== 'string' || raw.anchor === '') return null; + const strings = (v: unknown): string[] => + Array.isArray(v) + ? v.filter((s): s is string => typeof s === 'string' && s.length > 0) + : []; + const interaction = Array.isArray(raw.interaction) + ? raw.interaction + .filter( + (e): e is { path: string; importsChanged?: unknown } => + !!e && + typeof (e as { path?: unknown }).path === 'string' && + (e as { path: string }).path.length > 0 && + // An interaction entry IS its edge: with no surviving + // importsChanged the brief would read "because it imports , + // which changed" — a seam pointing at nothing. + strings((e as { importsChanged?: unknown }).importsChanged).length > + 0, + ) + .map((e) => ({ + path: e.path, + importsChanged: strings(e.importsChanged), + })) + : []; + const deltaFiles = strings(raw.deltaFiles); + // Degrade-to-full-scope means DEGRADE: a block whose lists all failed + // validation must not render an incremental frame with zero scope bullets. + if (deltaFiles.length === 0 && interaction.length === 0) return null; + return { + anchor: raw.anchor, + deltaFiles, + interaction, + }; } /** A heavy file's entry, which is the only kind an invariant agent can be built from. */ @@ -550,6 +681,70 @@ export function buildChunkAgentPrompt( ); } + // Incremental rounds carry two scopes in one diff, and the difference is + // the agent's whole brief for the second kind: an interaction file's diff + // was already reviewed clean once, and re-litigating it from scratch is how + // an incremental round quietly costs what it saved — or worse, re-reports + // findings the previous round already ruled on. + const incremental = incrementalScopeOf(report); + if (incremental) { + const chunkPaths = new Set( + (Array.isArray(chunk.files) ? chunk.files : []) + .map((f) => f?.path) + .filter((p): p is string => typeof p === 'string'), + ); + const deltaHere = incremental.deltaFiles.filter((p) => chunkPaths.has(p)); + const seamHere = incremental.interaction.filter((e) => + chunkPaths.has(e.path), + ); + 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))}\`), ` + + `plus still-clean files one import hop from a change. Your files' scopes:`, + ]; + if (deltaHere.length > 0) { + lines.push( + ...deltaHere.map( + (p) => + `- ${inertPath(p)} — **changed since the last round**: its hunks here are ` + + `its full change against the review's base (the previous round's clean verdict ` + + `no longer covers this file); review them in full, as usual.`, + ), + ); + } + if (seamHere.length > 0) { + lines.push( + ...seamHere.map( + (e) => + `- ${inertPath(e.path)} — **unchanged, cleared by the previous round**, back in ` + + `scope because it imports ${e.importsChanged.map(inertPath).join(', ')}, ` + + `which ` + + `changed. Review the INTERACTION only: do this file's uses of what it imports ` + + `still hold — signatures, argument contracts, invariants, error behaviour — ` + + `now that the imported side moved? Read the changed side from the worktree to ` + + `answer that. Do not re-review the rest of this file's diff from scratch, and ` + + `do not report defects in it that the change it imports does not affect.`, + ), + ); + lines.push( + // The generic duties below this block (the line-by-line walk, the + // deletion audit, every-dimension ownership) predate incremental + // scope and address the ordinary case. Without this sentence an + // agent obeys whichever instruction it read last — measured in + // review: told "interaction only", then told "audit all deletions + // in your territory", it re-opened round-1 findings. + `Where those general duties below conflict with a file's scope class ` + + `above, the scope class WINS: for an interaction file, every duty ` + + `applies only to its interaction surface with what changed.`, + ); + } + // A frame with a header and no scope bullets tells the agent nothing and + // implies its files are out of scope — render it only when at least one + // of this chunk's files actually carries a class. + if (deltaHere.length > 0 || seamHere.length > 0) parts.push(...lines); + } + parts.push( '', 'You may also `read_file` the **full source files** above from the worktree whenever a ' + @@ -852,9 +1047,53 @@ function diffReadingBlock( (c) => c.maxLineChars > READ_FILE_CHAR_CAP, ); + // Whole-diff readers (dimension agents, auditors) get the incremental frame + // once, up front: without it, "the diff" reads as the whole PR, and an agent + // that notices most of the PR is absent invents its own explanation — or + // walks the worktree re-reviewing scope the previous round already cleared. + const incremental = incrementalScopeOf(report); + const parts = [ '## The diff', '', + ...(incremental + ? [ + `**Incremental round.** This diff is scoped to what changed since the previous ` + + `clean review round (anchor \`${inertPath(incremental.anchor.slice(0, 12))}\`), 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. ` + + `A defect in absent code is reportable only when a change IN this diff is ` + + `what makes it wrong now. Where the sweep duties below (walk every ` + + `hunk, audit every deletion, own every dimension) conflict with a ` + + `file's scope class, the scope class WINS: for an interaction file ` + + `every duty applies only to its interaction surface with what ` + + `changed — its other hunks were cleared last round and re-reporting ` + + `them is the cost this scoping exists to prevent.`, + '', + // A whole-diff reader must know WHICH file carries which scope — + // told only that the two classes coexist, it cannot tell the file + // owed a full review from the one owed a seam check. Capped so a + // wide round cannot flood the brief; the chunk briefs always carry + // their own files' classes in full. + ...scopeFileLists(incremental), + '', + ] + : []), + // A CHUNK-scoped role brief (the reverse auditors) owns one territory and + // is its sole reviewer: the capped global list above can elide its own + // files past entry 30, leaving the agent no way to learn their class or + // recover the tail. Its own files are therefore listed in full, exactly + // as the bare chunk agent's brief lists them. + ...(incremental && scoped + ? [ + ...chunkScopeBullets( + incremental, + chunks.find((c) => c.id === chunkId), + ), + '', + ] + : []), scoped ? `Your territory is **chunk ${chunkId}** of the diff. It is a file on disk — ` + 'nothing in this prompt contains the code. Read your chunk:' diff --git a/packages/cli/src/commands/review/fetch-pr.test.ts b/packages/cli/src/commands/review/fetch-pr.test.ts index 6c52d261a17..c4a2a674551 100644 --- a/packages/cli/src/commands/review/fetch-pr.test.ts +++ b/packages/cli/src/commands/review/fetch-pr.test.ts @@ -303,6 +303,21 @@ vi.mock('./lib/gh.js', () => ({ vi.mock('./lib/git.js', () => ({ git: producerMocks.git, gitOpt: producerMocks.gitOpt, + // Moved out of fetch-pr.ts into lib/git.ts (it is `rescope`'s too now), so + // the mock owes it an export. Expressed over the same `gitRaw` fixture the + // real one reads, rather than a constant: a report's line counts decide + // heaviness, and a stub returning 0 everywhere would make every plan light. + fileLineCount: (ref: string, path: string) => { + try { + const buf = producerMocks.gitRaw('show', `${ref}:${path}`); + if (!buf || buf.length === 0) return 0; + let n = 0; + for (const b of buf) if (b === 0x0a) n++; + return buf[buf.length - 1] === 0x0a ? n : n + 1; + } catch { + return 0; + } + }, // The exit-code-aware probe, expressed in terms of the same mock: a null // answer is the DEFINITIVE no (exit 1), which is what these fixtures mean. // A test that wants the git-surface-unavailable shape overrides this. diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 908c6cfa5fb..f7a942fe000 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -41,6 +41,7 @@ import { import { ensureAuthenticated, gh, setGhHost } from './lib/gh.js'; import type { ReviewEffort } from './parse-args.js'; import { + fileLineCount, git, gitOpt, gitProbe as gitExit, @@ -366,20 +367,6 @@ export function resolveIncrementalAnchor( return { incremental: { since, effective: true }, diffBase: resolved }; } -/** Count lines of `:`, or 0 if it does not exist there. */ -function fileLineCount(ref: string, path: string): number { - try { - const buf = gitRaw('show', `${ref}:${path}`); - if (buf.length === 0) return 0; - let n = 0; - for (const b of buf) if (b === 0x0a) n++; - // A final line without a trailing newline still counts. - return buf[buf.length - 1] === 0x0a ? n : n + 1; - } catch { - return 0; // absent at this ref: created by the PR, or deleted by it - } -} - /** * Does every hunk of `inner` fall inside `outer`, per file? * diff --git a/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts b/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts new file mode 100644 index 00000000000..25240921f23 --- /dev/null +++ b/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `sliceDiffByLines` carries two contracts nothing else in the review can +// re-establish: the bytes it keeps are byte-identical to the input's (a +// decode/re-encode rewrites every hunk of a non-UTF-8 file), and the ranges +// it is given are `parseDiff`'s own per-file ranges — so a round-trip through +// parse → slice → parse must reproduce the selected sections exactly. + +import { describe, it, expect } from 'vitest'; +import { parseDiff, sliceDiffByLines } from './diff-plan.js'; + +const DIFF = [ + 'diff --git a/a.ts b/a.ts', + 'index 111..222 100644', + '--- a/a.ts', + '+++ b/a.ts', + '@@ -1,1 +1,1 @@', + '-const a = 1;', + '+const a = 2;', + 'diff --git a/b.ts b/b.ts', + 'index 333..444 100644', + '--- a/b.ts', + '+++ b/b.ts', + '@@ -1,1 +1,1 @@', + '-const b = 1;', + '+const b = 2;', + '', +].join('\n'); + +describe('sliceDiffByLines', () => { + it('keeps exactly the selected file sections, parseable on their own', () => { + const parsed = parseDiff(DIFF); + const keep = parsed.files + .filter((f) => f.path === 'b.ts') + .map((f) => ({ startLine: f.diffStart, endLine: f.diffEnd })); + const out = sliceDiffByLines(Buffer.from(DIFF, 'utf8'), keep); + const text = out.toString('utf8'); + expect(text).toContain('b/b.ts'); + expect(text).not.toContain('a.ts'); + expect(parseDiff(text).files.map((f) => f.path)).toEqual(['b.ts']); + }); + + it('is byte-exact: invalid UTF-8 and lone CR survive verbatim', () => { + const raw = Buffer.concat([ + Buffer.from('keep '), + Buffer.from([0x80, 0x0d, 0x81]), + Buffer.from('\ndrop\n'), + ]); + const out = sliceDiffByLines(raw, [{ startLine: 1, endLine: 1 }]); + expect([...out]).toEqual([...raw.subarray(0, raw.indexOf(0x0a) + 1)]); + }); + + it('re-orders ranges by line and clamps past the last line', () => { + const buf = Buffer.from('l1\nl2\nl3\n', 'utf8'); + expect( + sliceDiffByLines(buf, [ + { startLine: 3, endLine: 9 }, + { startLine: 1, endLine: 1 }, + ]).toString('utf8'), + ).toBe('l1\nl3\n'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/diff-plan.ts b/packages/cli/src/commands/review/lib/diff-plan.ts index 3d078884f9f..d93e8848318 100644 --- a/packages/cli/src/commands/review/lib/diff-plan.ts +++ b/packages/cli/src/commands/review/lib/diff-plan.ts @@ -752,3 +752,33 @@ export function chunksCoverDiff( } return expected === diffLines + 1; } + +/** + * Cut a captured diff down to the file sections named by `keep`, by BYTES. + * + * The capture contract is bytes end to end: a decode/re-encode rewrites the + * content of every hunk touching a file git handed over in a non-UTF-8 + * encoding. The caller passes the 1-based inclusive LINE ranges `parseDiff` + * reported per file (`diffStart`/`diffEnd`), and this maps them to byte + * ranges over the same newline structure `parseDiff` walked. Slicing an + * existing diff — rather than re-capturing with pathspecs — is also what + * keeps RENAME sections intact: a pathspec-scoped `git diff` cannot see the + * rename source, un-pairs the rename, and renders the file as a whole-file + * add whose hunks exist nowhere in the original diff. + */ +export function sliceDiffByLines( + diff: Buffer, + keep: ReadonlyArray<{ startLine: number; endLine: number }>, +): Buffer { + // Byte offset of the start of each 1-based line; sentinel = buffer length. + const starts: number[] = [0]; + for (let i = 0; i < diff.length; i++) { + if (diff[i] === 0x0a) starts.push(i + 1); + } + const offsetOf = (line1: number): number => + line1 - 1 < starts.length ? starts[line1 - 1] : diff.length; + const parts = [...keep] + .sort((a, b) => a.startLine - b.startLine) + .map((r) => diff.subarray(offsetOf(r.startLine), offsetOf(r.endLine + 1))); + return Buffer.concat(parts); +} diff --git a/packages/cli/src/commands/review/lib/git.integration.test.ts b/packages/cli/src/commands/review/lib/git.integration.test.ts index 65a42bfb29a..9d99334291a 100644 --- a/packages/cli/src/commands/review/lib/git.integration.test.ts +++ b/packages/cli/src/commands/review/lib/git.integration.test.ts @@ -15,10 +15,16 @@ import { existsSync, writeFileSync, mkdirSync, + realpathSync, } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { gitProbe, gitRawTolerateDiff, releaseWorktree } from './git.js'; +import { + fileLineCount, + gitProbe, + gitRawTolerateDiff, + releaseWorktree, +} from './git.js'; import { NULL_DEVICE } from './diff-flags.js'; import { isolateHostGitConfig } from './test-utils.js'; @@ -188,6 +194,42 @@ describe('gitRawTolerateDiff', () => { }); }); +describe('fileLineCount', () => { + // The one consumer chain that matters: `buildPlanReport`'s post-image + // resolver. A count that drifts (or an import that silently breaks — the + // move out of fetch-pr was measured unguarded by any test) mis-classifies + // heaviness and silently drops invariant agents from rosters. + it('counts lines at a ref: trailing newline, no trailing newline, absent, empty', () => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'flc-'))); + const prev = process.cwd(); + process.chdir(dir); + const iso = isolateHostGitConfig(); + try { + const g = (...args: string[]) => + execFileSync('git', args, { cwd: dir, encoding: 'utf8' }).trim(); + g('init', '-q', '--template=', '.'); + g('config', 'user.email', 'a@b'); + g('config', 'user.name', 'a'); + g('config', 'commit.gpgsign', 'false'); + writeFileSync(join(dir, 'three.txt'), 'a\nb\nc\n'); + writeFileSync(join(dir, 'no-nl.txt'), 'a\nb'); + writeFileSync(join(dir, 'empty.txt'), ''); + g('add', '-A'); + g('commit', '-q', '--no-verify', '-m', 'one'); + const sha = g('rev-parse', 'HEAD'); + expect(fileLineCount(sha, 'three.txt')).toBe(3); + expect(fileLineCount(sha, 'no-nl.txt')).toBe(2); + expect(fileLineCount(sha, 'empty.txt')).toBe(0); + expect(fileLineCount(sha, 'absent.txt')).toBe(0); + expect(fileLineCount('not-a-ref', 'three.txt')).toBe(0); + } finally { + process.chdir(prev); + iso.dispose(); + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe('gitProbe — the exit status the anchor taxonomy rests on', () => { // Every fetch-pr test mocks this module, so nothing else consumes the real // `status`. A rewrite returning `{status: 1}` for every failure, or diff --git a/packages/cli/src/commands/review/lib/git.ts b/packages/cli/src/commands/review/lib/git.ts index 74a01fc9e45..9cb54347467 100644 --- a/packages/cli/src/commands/review/lib/git.ts +++ b/packages/cli/src/commands/review/lib/git.ts @@ -228,6 +228,33 @@ export function gitRaw(...args: string[]): Buffer { }); } +/** + * Count lines of `:`, or 0 if it does not exist there. + * + * Shared by the plan builders that can name a post-image ref (`fetch-pr`, + * `rescope`): `buildPlanReport` derives heaviness from this count, and two + * counters that disagreed would classify the same file heavy in one plan and + * not the other. + */ +export function fileLineCount( + ref: string, + path: string, + repoRoot?: string, +): number { + try { + const buf = repoRoot + ? gitRaw('-C', repoRoot, 'show', `${ref}:${path}`) + : gitRaw('show', `${ref}:${path}`); + if (buf.length === 0) return 0; + let n = 0; + for (const b of buf) if (b === 0x0a) n++; + // A final line without a trailing newline still counts. + return buf[buf.length - 1] === 0x0a ? n : n + 1; + } catch { + return 0; // absent at this ref: created by the PR, or deleted by it + } +} + /** * Like `gitRaw`, but treats "the inputs differ" — exit 1 **with output** — as * success and returns the diff the child produced anyway. diff --git a/packages/cli/src/commands/review/lib/import-graph.test.ts b/packages/cli/src/commands/review/lib/import-graph.test.ts new file mode 100644 index 00000000000..d45c5d3405c --- /dev/null +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -0,0 +1,306 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The widening heuristic's contract is directional: a false positive costs one +// extra review, a false negative keeps the pre-widening floor. These tests pin +// the resolution rules (ESM-TS `.js` → `.ts`, index forms, workspace names) +// and the fail-quiet misses, because a resolver that silently resolved OUTSIDE +// the membership set would widen the scope with files nobody planned. + +import { describe, it, expect } from 'vitest'; +import { + scanImportSpecifiers, + resolveSpecifier, + dependentsOfChanged, + discoverWorkspacePackages, +} from './import-graph.js'; + +describe('scanImportSpecifiers', () => { + it('finds all four import shapes, deduplicated, order preserved', () => { + const src = ` + import { a } from './a.js'; + import defaultB from "../b.js"; + export { c } from './c.js'; + export * from './d.js'; + import './side-effect.js'; + const e = await import('./e.js'); + const f = require('./f.cjs'); + import { a2 } from './a.js'; + `; + expect(scanImportSpecifiers(src)).toEqual([ + './a.js', + '../b.js', + './c.js', + './d.js', + './side-effect.js', + './e.js', + './f.cjs', + ]); + }); + + it('ignores template-literal and multi-line specifiers', () => { + expect(scanImportSpecifiers('await import(`./x${v}.js`)')).toEqual([]); + expect(scanImportSpecifiers("from '\n./broken.js'")).toEqual([]); + }); + + it('accepts type-only imports — a signature change is an interaction too', () => { + expect( + scanImportSpecifiers("import type { T } from './types.js';"), + ).toEqual(['./types.js']); + }); +}); + +describe('resolveSpecifier', () => { + const files = new Set([ + 'packages/cli/src/a.ts', + 'packages/cli/src/b.tsx', + 'packages/cli/src/dir/index.ts', + 'packages/core/src/index.ts', + 'packages/core/src/util/x.ts', + ]); + const pkgs = [ + { name: '@qwen/core', dir: 'packages/core' }, + { name: '@qwen/cli', dir: 'packages/cli' }, + ]; + + it('maps the emitted-extension specifier back to its source', () => { + expect(resolveSpecifier('packages/cli/src/z.ts', './a.js', files)).toBe( + 'packages/cli/src/a.ts', + ); + expect(resolveSpecifier('packages/cli/src/z.ts', './b.jsx', files)).toBe( + 'packages/cli/src/b.tsx', + ); + }); + + it('walks extensions and index forms for extensionless specifiers', () => { + expect(resolveSpecifier('packages/cli/src/z.ts', './a', files)).toBe( + 'packages/cli/src/a.ts', + ); + expect(resolveSpecifier('packages/cli/src/z.ts', './dir', files)).toBe( + 'packages/cli/src/dir/index.ts', + ); + }); + + it('resolves workspace-package specifiers, entry and subpath alike', () => { + expect( + resolveSpecifier('packages/cli/src/z.ts', '@qwen/core', files, pkgs), + ).toBe('packages/core/src/index.ts'); + expect( + resolveSpecifier( + 'packages/cli/src/z.ts', + '@qwen/core/src/util/x.js', + files, + pkgs, + ), + ).toBe('packages/core/src/util/x.ts'); + // A dist deep-import names build output; the src fallback finds the source. + expect( + resolveSpecifier( + 'packages/cli/src/z.ts', + '@qwen/core/util/x.js', + files, + pkgs, + ), + ).toBe('packages/core/src/util/x.ts'); + }); + + it('maps an emitted .js specifier to a .tsx source — the UI layer convention', () => { + const uiFiles = new Set(['packages/cli/src/ui/App.tsx']); + expect( + resolveSpecifier( + 'packages/cli/src/ui/AppContainer.tsx', + './App.js', + uiFiles, + ), + ).toBe('packages/cli/src/ui/App.tsx'); + }); + + it('resolves .cjs to .cts — every EXT_MAP row has a resolution pin', () => { + expect(resolveSpecifier('a.ts', './f.cjs', new Set(['f.cts']))).toBe( + 'f.cts', + ); + }); + + it('dist deep-imports resolve under BOTH emit layouts', () => { + const pkgs = [{ name: '@qwen/core', dir: 'packages/core' }]; + // dist/src/… layout (this repo): strip dist, keep the rest. + expect( + resolveSpecifier( + 'a.ts', + '@qwen/core/dist/src/utils/x.js', + new Set(['packages/core/src/utils/x.ts']), + pkgs, + ), + ).toBe('packages/core/src/utils/x.ts'); + // flat dist/… layout: strip dist, add src. + expect( + resolveSpecifier( + 'a.ts', + '@qwen/core/dist/utils/x.js', + new Set(['packages/core/src/utils/x.ts']), + pkgs, + ), + ).toBe('packages/core/src/utils/x.ts'); + }); + + it('resolves .mjs to .mts and root-package (dir: "") specifiers', () => { + const rootFiles = new Set(['src/index.ts', 'src/util/x.ts', 'mod.mts']); + const rootPkg = [{ name: 'root', dir: '' }]; + expect(resolveSpecifier('a.ts', './mod.mjs', rootFiles)).toBe('mod.mts'); + expect(resolveSpecifier('a.ts', 'root', rootFiles, rootPkg)).toBe( + 'src/index.ts', + ); + expect( + resolveSpecifier('a.ts', 'root/src/util/x.js', rootFiles, rootPkg), + ).toBe('src/util/x.ts'); + }); + + it('strips the dist/ segment on emitted-tree deep imports', () => { + const distFiles = new Set(['packages/core/src/utils/foo.ts']); + const distPkgs = [{ name: '@qwen/core', dir: 'packages/core' }]; + expect( + resolveSpecifier( + 'a.ts', + '@qwen/core/dist/utils/foo.js', + distFiles, + distPkgs, + ), + ).toBe('packages/core/src/utils/foo.ts'); + }); + + it('maps an emitted .js to a .jsx source, and normalises package subpaths', () => { + expect(resolveSpecifier('a.ts', './W.js', new Set(['W.jsx']))).toBe( + 'W.jsx', + ); + const pkgs = [{ name: '@q/core', dir: 'packages/core' }]; + expect( + resolveSpecifier( + 'a.ts', + '@q/core/src/util/../x.js', + new Set(['packages/core/src/x.ts']), + pkgs, + ), + ).toBe('packages/core/src/x.ts'); + // …and a subpath escaping the package resolves to nothing. + expect( + resolveSpecifier( + 'a.ts', + '@q/core/../outside.js', + new Set(['outside.ts']), + pkgs, + ), + ).toBeNull(); + }); + + it('a directory that merely BEGINS with dots is not a root escape', () => { + const dotFiles = new Set(['..config/mod.ts']); + expect(resolveSpecifier('a.ts', './..config/mod.js', dotFiles)).toBe( + '..config/mod.ts', + ); + // Separating case for the guard itself: membership CONTAINS the escaping + // path, so only repoJoin's refusal produces the null. + expect( + resolveSpecifier('a.ts', '../../x', new Set(['../../x.ts'])), + ).toBeNull(); + }); + + it('resolves a specifier that already names its source extension', () => { + // The literal-form candidate: `./a.ts` from a repo that imports source + // extensions directly (deno-style, or a `.json`/`.css` asset import). + expect( + resolveSpecifier('a.ts', './data.json', new Set(['data.json'])), + ).toBe('data.json'); + expect(resolveSpecifier('a.ts', './b.ts', new Set(['b.ts']))).toBe('b.ts'); + }); + + it('returns null outside membership, above the root, and for unknown packages', () => { + expect( + resolveSpecifier('packages/cli/src/z.ts', './missing', files), + ).toBeNull(); + expect(resolveSpecifier('a.ts', '../../escape', files)).toBeNull(); + expect(resolveSpecifier('a.ts', 'left-pad', files, pkgs)).toBeNull(); + }); +}); + +describe('dependentsOfChanged', () => { + const sources = new Map([ + ['src/caller.ts', "import { f } from './changed.js';"], + ['src/bystander.ts', "import { g } from './other.js';"], + ['src/changed.ts', "import { h } from './caller.js';"], + ]); + const read = (p: string) => sources.get(p) ?? null; + + it('returns only candidates that import a changed file, with the edges named', () => { + const changed = new Set(['src/changed.ts']); + const out = dependentsOfChanged( + changed, + ['src/caller.ts', 'src/bystander.ts', 'src/changed.ts'], + read, + ); + expect([...out.entries()]).toEqual([['src/caller.ts', ['src/changed.ts']]]); + }); + + it('skips candidates already in the changed set — they are in scope on their own account', () => { + const changed = new Set(['src/changed.ts', 'src/caller.ts']); + const out = dependentsOfChanged(changed, ['src/caller.ts'], read); + expect(out.size).toBe(0); + }); + + it('consults the packages argument for cross-package edges', () => { + const out = dependentsOfChanged( + new Set(['packages/core/src/index.ts']), + ['packages/cli/src/z.ts'], + () => "import { x } from '@qwen/core';", + [{ name: '@qwen/core', dir: 'packages/core' }], + ); + expect([...out.entries()]).toEqual([ + ['packages/cli/src/z.ts', ['packages/core/src/index.ts']], + ]); + }); + + it('an unreadable candidate contributes no edge and no crash', () => { + const out = dependentsOfChanged( + new Set(['src/changed.ts']), + ['src/gone.ts'], + () => null, + ); + expect(out.size).toBe(0); + }); +}); + +describe('discoverWorkspacePackages', () => { + const manifests = new Map([ + ['package.json', JSON.stringify({ name: 'root' })], + ['packages/core/package.json', JSON.stringify({ name: '@qwen/core' })], + ['packages/cli/package.json', JSON.stringify({ name: '@qwen/cli' })], + ]); + const read = (p: string) => manifests.get(p) ?? null; + + it('maps each file to its nearest manifest, most specific dir first', () => { + const pkgs = discoverWorkspacePackages( + ['packages/core/src/a.ts', 'packages/cli/src/deep/b.ts', 'scripts/x.js'], + read, + ); + expect(pkgs).toEqual([ + { name: '@qwen/core', dir: 'packages/core' }, + { name: '@qwen/cli', dir: 'packages/cli' }, + { name: 'root', dir: '' }, + ]); + }); + + it('is fail-quiet on malformed or nameless manifests', () => { + for (const manifest of [ + 'not json', + JSON.stringify({ name: '' }), + JSON.stringify({}), + ]) { + const pkgs = discoverWorkspacePackages(['pkg/src/a.ts'], (p) => + p === 'pkg/package.json' ? manifest : null, + ); + expect(pkgs).toEqual([]); + } + }); +}); diff --git a/packages/cli/src/commands/review/lib/import-graph.ts b/packages/cli/src/commands/review/lib/import-graph.ts new file mode 100644 index 00000000000..1f325878eb7 --- /dev/null +++ b/packages/cli/src/commands/review/lib/import-graph.ts @@ -0,0 +1,276 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// One import hop, for incremental review widening. +// +// An incremental round reviews `anchor..HEAD` — the fix — and skips everything +// the previous round already cleared. But "clean" was certified against the +// code as it stood THEN: a fix that changes a function's contract can break an +// unchanged caller two files away, and a scope that never re-opens the caller +// retires that breakage silently. So the incremental scope is widened by one +// import hop: every still-clean file that imports a changed file re-enters the +// review, briefed to check the interaction seam rather than re-reviewed from +// scratch. +// +// This is a HEURISTIC, and its failure directions are chosen deliberately: +// +// - The specifier scan is regex over source text, not a parse. A specifier +// quoted in a comment or a string literal scans as an import; the cost is a +// file reviewed once more than strictly needed. Widening errs toward +// reviewing. +// - Resolution stops at one hop and does not follow re-export chains (a barrel +// `index.ts` between caller and callee hides the edge). A missed edge means +// a file the review skips exactly as the pre-widening scope skipped every +// dependent; the floor never drops below what incremental review shipped +// with. +// - Bare workspace-package imports (`@scope/pkg` with no subpath) resolve only +// to the conventional entry candidates (`src/index.*`, `index.*`). A package +// with an exports map pointing elsewhere contributes no edge, same floor. +// - `tsconfig` path aliases (`@/lib/utils`), `package.json#exports` subpath +// rewrites, and declaration-file references are not consulted — each is a +// per-repo config surface this scanner deliberately does not parse. An +// alias yields a missed edge (the file keeps the pre-widening floor). An +// exports map can also make the conventional-layout guesses below resolve +// a subpath to a file the map actually routes elsewhere — a WRONG edge, +// whose whole cost is one extra widened file; the scope never narrows. +// +// The scan reads files from the review worktree (post-change state), because +// the question is whether the caller AS IT NOW STANDS uses what changed. + +import * as nodePath from 'node:path'; + +/** File-reading seam: rescope passes worktree reads, tests pass a map. */ +export type SourceReader = (repoRelPath: string) => string | null; + +/** + * Every module specifier the source mentions, deduplicated, order preserved. + * + * Four shapes: `import … from 'x'` / `export … from 'x'` (one regex — both + * end in `from ''`), side-effect `import 'x'`, dynamic `import('x')`, + * and CommonJS `require('x')`. Template-literal specifiers are dynamic values + * and are ignored, as is anything spanning a newline. + */ +export function scanImportSpecifiers(source: string): string[] { + const out: string[] = []; + const seen = new Set(); + const push = (spec: string) => { + if (spec && !seen.has(spec)) { + seen.add(spec); + out.push(spec); + } + }; + const patterns = [ + /\bfrom\s*(['"])([^'"\n]+)\1/g, + /\bimport\s*(['"])([^'"\n]+)\1/g, + /\bimport\s*\(\s*(['"])([^'"\n]+)\1\s*\)/g, + /\brequire\s*\(\s*(['"])([^'"\n]+)\1\s*\)/g, + ]; + for (const re of patterns) { + for (const m of source.matchAll(re)) push(m[2]); + } + return out; +} + +/** + * Extension candidates for a specifier, ESM-TS aware. + * + * This repo — like every NodeNext TypeScript workspace — imports `./x.js` + * meaning `x.ts`: the specifier names the EMITTED file. So the mapped form is + * tried first, then the literal, then the bare-specifier extension walk, then + * the directory-index forms. + */ +const EXT_MAP: ReadonlyArray<[RegExp, string]> = [ + // BOTH TS source extensions for an emitted `.js`: under `react-jsx` a + // `.tsx` file also emits `.js`, and this repo's UI layer imports + // `./App.js` while only `App.tsx` exists. Measured before the second row + // was added: 921 of 6,200 relative `.js` specifiers under packages/cli/src + // named `.tsx` targets no edge could ever reach. + [/\.js$/, '.ts'], + [/\.js$/, '.tsx'], + // …and `.jsx`: a JSX source in a JS project emits `.js` under the same + // convention, so the emitted name names it too. + [/\.js$/, '.jsx'], + [/\.jsx$/, '.tsx'], + [/\.mjs$/, '.mts'], + [/\.cjs$/, '.cts'], +]; +const EXT_WALK = ['.ts', '.tsx', '.mts', '.cts', '.js', '.jsx', '.mjs', '.cjs']; + +function candidatesFor(base: string): string[] { + const out: string[] = []; + for (const [re, ts] of EXT_MAP) { + if (re.test(base)) out.push(base.replace(re, ts)); + } + out.push(base); + if (!/\.[a-z]+$/i.test(base)) { + for (const ext of EXT_WALK) out.push(`${base}${ext}`); + for (const ext of EXT_WALK) out.push(`${base}/index${ext}`); + } + return out; +} + +/** POSIX-normalise a joined path and refuse escapes above the repo root. */ +function repoJoin(dir: string, spec: string): string | null { + const joined = nodePath.posix.normalize(nodePath.posix.join(dir, spec)); + // Segment-exact: a legal directory that merely BEGINS with two dots + // (`..config/mod`) is not an escape, and `startsWith('..')` called it one. + return joined === '..' || joined.startsWith('../') ? null : joined; +} + +/** + * A workspace package the resolver may route bare specifiers into: + * `name` from its manifest, `dir` repo-relative (`''` for the root package). + */ +export interface WorkspacePackage { + name: string; + dir: string; +} + +/** + * Resolve one specifier to a repo-relative path, or null. + * + * `membership` is the only truth consulted — resolution never stats the disk. + * The caller passes the set of paths it cares about (the review plan's files), + * so "resolved" means "this specifier names a file in the review", which is + * the exact question widening asks. + */ +export function resolveSpecifier( + fromFile: string, + spec: string, + membership: ReadonlySet, + packages: readonly WorkspacePackage[] = [], +): string | null { + if (spec.startsWith('./') || spec.startsWith('../')) { + const base = repoJoin(nodePath.posix.dirname(fromFile), spec); + if (base === null) return null; + for (const c of candidatesFor(base)) if (membership.has(c)) return c; + return null; + } + for (const pkg of packages) { + if (spec === pkg.name) { + // Bare entry import: conventional entry points only (see header). + const roots = ['src/index', 'index']; + for (const root of roots) { + const base = pkg.dir === '' ? root : `${pkg.dir}/${root}`; + for (const c of candidatesFor(base)) if (membership.has(c)) return c; + } + return null; + } + if (spec.startsWith(`${pkg.name}/`)) { + // Normalised like a relative specifier: a legal subpath carrying `.` + // or `..` segments otherwise builds a candidate string no + // git-normalised membership path can equal, silently dropping the edge. + const subRaw = spec.slice(pkg.name.length + 1); + const subNorm = nodePath.posix.normalize(subRaw); + if (subNorm.startsWith('..')) return null; + const sub = subNorm; + const base = pkg.dir === '' ? sub : `${pkg.dir}/${sub}`; + for (const c of candidatesFor(base)) if (membership.has(c)) return c; + // Deep imports into a package's emitted tree (`dist/…`) name build + // output. Emit layouts differ per package — some emit `src/x.ts` to + // `dist/x.js` (strip dist, add src), this repo's packages emit it to + // `dist/src/x.js` (strip dist, keep the rest) — so try the stripped + // path both as-is and under `src/`. Without the strip at all, the + // remap produced `/src/dist/…`, matching nothing. + const srcSub = sub.startsWith('dist/') ? sub.slice('dist/'.length) : sub; + for (const base2 of [ + pkg.dir === '' ? srcSub : `${pkg.dir}/${srcSub}`, + pkg.dir === '' ? `src/${srcSub}` : `${pkg.dir}/src/${srcSub}`, + ]) { + for (const c of candidatesFor(base2)) if (membership.has(c)) return c; + } + return null; + } + } + return null; +} + +/** + * Discover the workspace packages the plan's files live in. + * + * For each file, the nearest ancestor directory whose `package.json` the + * reader can produce a `name` from is its package; distinct packages are + * returned root-last so `resolveSpecifier`'s first-match loop sees the most + * specific dir first. The reader is a seam (worktree reads in production), + * and every miss is fail-quiet: a file under no readable manifest simply + * contributes no package, which only ever narrows the widening. + */ +export function discoverWorkspacePackages( + files: readonly string[], + readJson: (repoRelPath: string) => string | null, +): WorkspacePackage[] { + const nameByDir = new Map(); + const lookup = (dir: string): string | null => { + const cached = nameByDir.get(dir); + if (cached !== undefined) return cached; + const raw = readJson(dir === '' ? 'package.json' : `${dir}/package.json`); + let name: string | null = null; + if (raw !== null) { + try { + const parsed = JSON.parse(raw) as { name?: unknown }; + if (typeof parsed.name === 'string' && parsed.name !== '') { + name = parsed.name; + } + } catch { + // Not a manifest; keep walking up. + } + } + nameByDir.set(dir, name); + return name; + }; + const out = new Map(); // dir → name, deduped + for (const file of files) { + let dir = nodePath.posix.dirname(file); + if (dir === '.') dir = ''; + for (;;) { + const name = lookup(dir); + if (name !== null) { + if (!out.has(dir)) out.set(dir, name); + break; + } + if (dir === '') break; + const parent = nodePath.posix.dirname(dir); + dir = parent === '.' ? '' : parent; + } + } + return [...out.entries()] + .sort(([a], [b]) => b.length - a.length) + .map(([dir, name]) => ({ name, dir })); +} + +/** + * Which candidates import a changed file — the widening set. + * + * Returns `candidate → the changed files it imports` (non-empty lists only), + * insertion-ordered by the candidates array. Candidates already in `changed` + * are skipped: they are in the scope on their own account. A candidate whose + * source cannot be read (deleted, binary, reader refused) contributes no + * edges — same fail-quiet floor as every other miss here. + */ +export function dependentsOfChanged( + changed: ReadonlySet, + candidates: readonly string[], + read: SourceReader, + packages: readonly WorkspacePackage[] = [], +): Map { + const out = new Map(); + for (const candidate of candidates) { + if (changed.has(candidate)) continue; + const source = read(candidate); + if (source === null) continue; + const hits: string[] = []; + const seen = new Set(); + for (const spec of scanImportSpecifiers(source)) { + const resolved = resolveSpecifier(candidate, spec, changed, packages); + if (resolved !== null && !seen.has(resolved)) { + seen.add(resolved); + hits.push(resolved); + } + } + if (hits.length > 0) out.set(candidate, hits); + } + return out; +} diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index d4c3e892ea6..8a64599ba6d 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -412,6 +412,63 @@ describe('requiredAgents — Step 3B', () => { ); expect(keys(heavy)).not.toContain('invariant-a--src/small.ts'); }); + + it('a heavy INTERACTION file in an incremental plan gets no invariant agents', () => { + // `heavy` is computed from the file's full-range slice, which for an + // interaction file is the code the previous round already cleared — + // three from-scratch whole-file walks are the re-review incremental + // scope exists to avoid. A heavy DELTA file keeps them; a malformed + // block widens (both keep them), never narrows. + const base = { + ...BIG, + files: [ + { path: 'src/delta.ts', kind: 'source', removedLines: 9, heavy: true }, + { path: 'src/seam.ts', kind: 'source', removedLines: 9, heavy: true }, + ], + }; + const incremental = { + ...base, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: ['src/delta.ts'], + interaction: [ + { path: 'src/seam.ts', importsChanged: ['src/delta.ts'] }, + ], + }, + }; + const k = keys(incremental as typeof base); + expect(k).toContain('invariant-a--src/delta.ts'); + expect(k).not.toContain('invariant-a--src/seam.ts'); + // An array of junk passes Array.isArray but names nothing to subtract. + const junkDelta = { + ...base, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: [null, 42], + interaction: [ + { path: 'src/seam.ts', importsChanged: ['src/delta.ts'] }, + ], + }, + }; + expect(keys(junkDelta as typeof base)).toContain( + 'invariant-a--src/seam.ts', + ); + const mangled = { ...base, incremental: { interaction: 'nope' } }; + expect(keys(mangled as typeof base)).toContain('invariant-a--src/seam.ts'); + // A path in BOTH lists is a DELTA file (its change is live): the delta + // classification wins and the invariant agents stay. + const both = { + ...base, + incremental: { + anchor: 'abc1234def567890', + deltaFiles: ['src/seam.ts'], + interaction: [ + { path: 'src/seam.ts', importsChanged: ['src/delta.ts'] }, + ], + }, + }; + expect(keys(both as typeof base)).toContain('invariant-a--src/seam.ts'); + }); }); describe('a heavy file in a Step-3A-sized diff', () => { diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index fba18f84897..6d0f240be68 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -68,6 +68,8 @@ export interface RosterPlan { worktreePath?: unknown; prNumber?: unknown; untrackedFiles?: unknown; + /** Present only on a rescoped plan — see incrementalInteractionPaths. */ + incremental?: unknown; /** * The review's effort, as the capturing command recorded it (`--effort`). * `'medium'` is the balanced tier and drops the adversarial personas; anything @@ -157,6 +159,57 @@ function heavyFiles(plan: RosterPlan): string[] { .map((f) => f.path as string); } +/** + * The interaction-file paths of a rescoped plan, defensively parsed — the + * plan is disk JSON, and a malformed block must widen the roster (invariant + * agents run), never narrow it. + */ +function incrementalInteractionPaths(plan: RosterPlan): Set { + const raw = plan.incremental as + | { interaction?: unknown; deltaFiles?: unknown } + | null + | undefined; + const out = new Set(); + // Same validation the brief renderer applies (`incrementalScopeOf`): an + // anchor-less block is not an incremental plan, and an entry with no + // surviving edge is not an interaction — treating either as one NARROWS + // the roster, and every malformation here must widen instead. + if ( + !raw || + typeof (raw as { anchor?: unknown }).anchor !== 'string' || + (raw as { anchor: string }).anchor === '' || + !Array.isArray(raw.interaction) + ) { + return out; + } + // A malformed `deltaFiles` disables the delta-wins subtraction below, so + // it must disable the narrowing entirely: with no trustworthy delta list + // there is no way to tell a seam-only file from a live one. + if ( + !Array.isArray(raw.deltaFiles) || + raw.deltaFiles.some((p) => typeof p !== 'string') + ) { + // An array of junk passes `Array.isArray` but leaves the delta-wins + // subtraction unable to name what it must subtract — the same reason a + // missing list disables the narrowing. + return out; + } + for (const e of raw.interaction) { + const path = (e as { path?: unknown } | null)?.path; + const edges = (e as { importsChanged?: unknown } | null)?.importsChanged; + const hasEdge = + Array.isArray(edges) && + edges.some((x) => typeof x === 'string' && x.length > 0); + if (typeof path === 'string' && path.length > 0 && hasEdge) out.add(path); + } + // A path in BOTH lists is a DELTA file (its change is live): the delta + // classification wins and its invariant agents stay. + for (const p of raw.deltaFiles) { + if (typeof p === 'string') out.delete(p); + } + return out; +} + /** * Every agent this plan requires, and the key each one's prompt is recorded under. * @@ -258,8 +311,16 @@ export function requiredAgents(plan: RosterPlan): RequiredAgent[] { // them. Requiring them there demanded agents the review was never meant to launch, // and `check-coverage` then exit-3'd an otherwise-complete small PR. Gate the loop // on the topology that actually runs them. + // An incremental plan's INTERACTION files get no invariant agents even when + // heavy: `heavy` is computed from the file's full-range slice, which for an + // interaction file is exactly the code the previous round already cleared — + // three whole-file agents re-walking it from scratch is the re-review the + // incremental scope exists to avoid, and the chunk agent for the same file + // is briefed for the seam only. Delta files keep them: their change is live. + const interactionPaths = incrementalInteractionPaths(plan); if (isTerritoryFanOut(plan)) { for (const file of heavyFiles(plan)) { + if (interactionPaths.has(file)) continue; add('invariant-a', file); add('invariant-b', file); add('invariant-c', file); diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts new file mode 100644 index 00000000000..a84434f2a7a --- /dev/null +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -0,0 +1,728 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real `git`, real repo. The command's whole job is to turn shas into a +// correctly-scoped diff, and the failure this feature must never have is a +// range that reviews the wrong code — a property of git's behaviour, not of a +// mock's. The exit-code contract is pinned hard because the skill branches on +// it: 2 falls back to the FULL diff, 3 stops as "nothing new", and only 0 may +// touch the plan. + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + rmSync, + writeFileSync, + mkdirSync, + readFileSync, + realpathSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + rescopeCommand, + RESCOPE_EXIT_FULL_RANGE, + RESCOPE_EXIT_NOTHING_NEW, + type IncrementalScope, +} from './rescope.js'; +import { buildDiffPlan } from './lib/diff-plan.js'; +import { buildPlanReport } from './lib/report.js'; +import { isolateHostGitConfig } from './lib/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-rescope-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + process.exitCode = undefined; +}); + +afterEach(() => { + process.exitCode = undefined; + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +const CHANGED = 'packages/app/src/changed.ts'; +const CALLER = 'packages/app/src/caller.ts'; +const BYSTANDER = 'packages/app/src/bystander.ts'; + +/** + * base — the PR's merge base; + * anchor — round 1's head (PR touches all three files); + * head — round 2's head (the fix touches only `changed.ts`). + */ +function seedHistory(): { base: string; anchor: string; head: string } { + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'commit.gpgsign', 'false'); + git('config', 'core.hooksPath', join(repo, '.no-such-hooks')); + write('packages/app/package.json', JSON.stringify({ name: '@t/app' })); + write(CHANGED, 'export const v = 1;\n'); + write(CALLER, "import { v } from './changed.js';\nexport const c = v;\n"); + write(BYSTANDER, 'export const b = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + const base = git('rev-parse', 'HEAD'); + + write(CHANGED, 'export const v = 2;\n'); + write(CALLER, "import { v } from './changed.js';\nexport const c = v + 1;\n"); + write(BYSTANDER, 'export const b = 2;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'round 1 (the PR)'); + const anchor = git('rev-parse', 'HEAD'); + + write(CHANGED, 'export const v = 3;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'round 2 (the fix)'); + const head = git('rev-parse', 'HEAD'); + return { base, anchor, head }; +} + +/** The plan `fetch-pr` would have written for base..head, plus identity. */ +function writeFetchedPlan(base: string, head: string): string { + const diffText = execFileSync( + 'git', + ['diff', '--no-color', `${base}..${head}`], + { cwd: repo, encoding: 'utf8' }, + ); + mkdirSync(join(repo, '.qwen/tmp'), { recursive: true }); + const diffPath = '.qwen/tmp/qwen-review-pr-7-diff.txt'; + writeFileSync(join(repo, diffPath), diffText); + const plan = { + prNumber: '7', + ownerRepo: 'o/r', + worktreePath: repo, + fetchedSha: head, + mergeBaseSha: base, + diffPath, + diffPathAbsolute: join(repo, diffPath), + effort: 'high', + carriedThrough: 'untouched', + ...buildPlanReport(buildDiffPlan(diffText, 400), null, { + operatorRoundCap: undefined, + hasDeadline: false, + }), + }; + const planPath = join(repo, 'plan.json'); + writeFileSync(planPath, JSON.stringify(plan, null, 2)); + return planPath; +} + +function run(planPath: string, anchor: string): void { + (rescopeCommand.handler as (argv: unknown) => void)({ + plan: planPath, + anchor, + maxChunkLines: 400, + }); +} + +type RescopedPlan = Record & { + incremental: IncrementalScope; + diffPath: string; + chunks: Array<{ id: number }>; + files: Array<{ path: string }>; +}; + +describe('rescope', () => { + it('scopes to the interdiff, widens one import hop, and preserves plan identity', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + // Identity and provenance ride through untouched. + expect(plan['prNumber']).toBe('7'); + expect(plan['worktreePath']).toBe(repo); + expect(plan['fetchedSha']).toBe(head); + expect(plan['mergeBaseSha']).toBe(base); + expect(plan['carriedThrough']).toBe('untouched'); + + // The scope: the fix's file on its incremental hunks, its importer pulled + // back in by the widening, the bystander left out. + expect(plan.incremental.anchor).toBe(anchor); + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + expect(plan.incremental.interaction).toEqual([ + { path: CALLER, importsChanged: [CHANGED] }, + ]); + expect(plan.incremental.contextFileCount).toBe(1); + // ABSOLUTE: a later step reaching for the superseded diff need not + // share this command's cwd. + expect(plan.incremental.fullDiffPath).toBe( + join(repo, '.qwen/tmp/qwen-review-pr-7-diff.txt'), + ); + expect(plan['diffPathAbsolute']).toBe( + join(repo, '.qwen/tmp/qwen-review-pr-7-diff-incremental.txt'), + ); + + // The composite diff: BOTH scoped files carry their full-range hunks — + // the interdiff only chose the delta file NAMES. changed.ts therefore + // shows v1 -> v3 (not the round-2 v2 -> v3 slice: an interdiff hunk that + // restores earlier lines exists in no hunk of the PR diff and 422s + // comment anchoring), and the bystander appears nowhere. + const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); + expect(diff).toContain('+export const v = 3;'); + expect(diff).toContain('-export const v = 1;'); + expect(diff).not.toContain('const v = 2;'); + expect(diff).toContain('+export const c = v + 1;'); + expect(diff).not.toContain('bystander'); + // The superseded full-range diff file stays intact for `fullDiffPath` + // readers — a successful rescope must not consume what it supersedes. + expect(readFileSync(plan.incremental.fullDiffPath!, 'utf8')).toContain( + 'bystander', + ); + + // The plan's chunks/files were rebuilt from the composite by the shared + // builders — the same shapes every downstream reader already parses. + expect(plan.files.map((f) => f.path).sort()).toEqual([CALLER, CHANGED]); + expect(plan.chunks.length).toBeGreaterThan(0); + }); + + it('exit 2 on an unknown or non-ancestor anchor, plan untouched', () => { + const { base, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const before = readFileSync(planPath, 'utf8'); + + run(planPath, 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef'); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + + // A commit from a DIFFERENT history: real, resolvable, not an ancestor. + process.exitCode = undefined; + git('checkout', '-q', '--orphan', 'stray'); + write('stray.ts', 'export {};\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'stray'); + const stray = git('rev-parse', 'HEAD'); + run(planPath, stray); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('exit 3 when the anchor IS the head, and when the interdiff is empty', () => { + const { base, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + + run(planPath, head); + expect(process.exitCode).toBe(RESCOPE_EXIT_NOTHING_NEW); + + // An empty commit on top: new sha, identical tree — nothing to review. + process.exitCode = undefined; + git('commit', '-q', '--no-verify', '--allow-empty', '-m', 'empty'); + const emptyHead = git('rev-parse', 'HEAD'); + const planPath2 = writeFetchedPlan(base, emptyHead); + const before = readFileSync(planPath2, 'utf8'); + run(planPath2, head); + expect(process.exitCode).toBe(RESCOPE_EXIT_NOTHING_NEW); + expect(readFileSync(planPath2, 'utf8')).toBe(before); + }); + + it('exit 2 on a plan with no worktree flow — local and lightweight plans', () => { + seedHistory(); + const planPath = join(repo, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: '7' })); + const before = readFileSync(planPath, 'utf8'); + run(planPath, 'HEAD'); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('a delta file nobody imports widens nothing', () => { + const { base, head } = seedHistory(); + // Round 3 touches only the bystander, which nobody imports. + write(BYSTANDER, 'export const b = 3;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'round 3'); + const head3 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head3); + run(planPath, head); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([BYSTANDER]); + expect(plan.incremental.interaction).toEqual([]); + expect(plan.files.map((f) => f.path)).toEqual([BYSTANDER]); + }); +}); + +describe('rescope — contract pins from review findings', () => { + it('pins the exit-code LITERALS the skill prose branches on', () => { + // The skill hardcodes 3 = "nothing new, stop" and any-other = "full + // range". A swap of the two constants keeps every symbolic assertion + // green while refusals start STOPPING the round — the skip-instead-of- + // fallback failure the module header forbids. + expect(RESCOPE_EXIT_FULL_RANGE).toBe(2); + expect(RESCOPE_EXIT_NOTHING_NEW).toBe(3); + }); + + it('honours --out: the rescoped plan lands there, the input stays byte-identical', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const before = readFileSync(planPath, 'utf8'); + const outPath = join(repo, 'rescoped.json'); + (rescopeCommand.handler as (argv: unknown) => void)({ + plan: planPath, + anchor, + out: outPath, + maxChunkLines: 400, + }); + expect(process.exitCode ?? 0).toBe(0); + expect(readFileSync(planPath, 'utf8')).toBe(before); + const out = JSON.parse(readFileSync(outPath, 'utf8')) as RescopedPlan; + expect(out.incremental.deltaFiles).toEqual([CHANGED]); + }); + + it('same-sha exit 3 leaves the plan byte-identical', () => { + const { base, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const before = readFileSync(planPath, 'utf8'); + run(planPath, head); + expect(process.exitCode).toBe(RESCOPE_EXIT_NOTHING_NEW); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('widens exactly ONE hop — a two-link chain does not flood-fill', () => { + const { base } = seedHistory(); + // deep.ts imports caller.ts (which imports changed.ts). Both links are in + // the PR (touched in a follow-up commit) so both are plan candidates. + write( + 'packages/app/src/deep.ts', + "import { c } from './caller.js';\nexport const d = c;\n", + ); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'add deep'); + const anchor2 = git('rev-parse', 'HEAD'); + write(CHANGED, 'export const v = 9;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'fix again'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor2); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + expect(plan.incremental.interaction.map((e) => e.path)).toEqual([CALLER]); + // deep.ts is two hops out: context, not interaction — and the count is + // EXACT (bystander + deep), so a flood-fill or a dropped exclusion + // cannot hide inside a `> 0`. + expect(plan.incremental.contextFileCount).toBe(2); + }); + + it('a test-file dependent stays OUT of the interaction set', () => { + const { base } = seedHistory(); + write( + 'packages/app/src/caller.test.ts', + "import { v } from './changed.js';\nexport const t = v;\n", + ); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'add test dependent'); + const anchor2 = git('rev-parse', 'HEAD'); + write(CHANGED, 'export const v = 9;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'fix'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor2); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.interaction.map((e) => e.path)).toEqual([CALLER]); + }); + + it('widens across workspace packages through a bare package specifier', () => { + const { base } = seedHistory(); + write('packages/app/src/index.ts', 'export const entry = 1;\n'); + write('packages/lib/package.json', JSON.stringify({ name: '@t/lib' })); + write( + 'packages/lib/src/user.ts', + "import { entry } from '@t/app';\nexport const u = entry;\n", + ); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'add cross-package user'); + const anchor2 = git('rev-parse', 'HEAD'); + write('packages/app/src/index.ts', 'export const entry = 2;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'change entry'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor2); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.interaction).toEqual([ + { + path: 'packages/lib/src/user.ts', + importsChanged: ['packages/app/src/index.ts'], + }, + ]); + }); + + it('preserves post-image line counts and heaviness in the rescoped plan', () => { + seedHistory(); + // A large file whose round-1 change rewrites most of it: heavy by the + // rewrite-ratio branch. The fix touches it again so it is delta. + const bigV1 = + Array.from({ length: 600 }, (_, i) => `export const a${i} = 1;`).join( + '\n', + ) + '\n'; + write('packages/app/src/big.ts', bigV1); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'seed big'); + const base2 = git('rev-parse', 'HEAD'); + const bigV2 = + Array.from({ length: 600 }, (_, i) => `export const a${i} = 2;`).join( + '\n', + ) + '\n'; + write('packages/app/src/big.ts', bigV2); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'round 1 rewrites big'); + const anchor2 = git('rev-parse', 'HEAD'); + write('packages/app/src/big.ts', bigV2 + 'export const extra = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'fix big'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base2, head2); + run(planPath, anchor2); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + const big = ( + plan.files as Array<{ + path: string; + fileLines?: number; + heavy?: boolean; + addedRanges?: unknown; + }> + ).find((f) => f.path === 'packages/app/src/big.ts')!; + // The degradation rescope exists to prevent: a null post-image resolver + // zeroes fileLines and heavy never fires — invariant agents vanish. + expect(big.fileLines).toBe(601); // the HEAD count, not the base or anchor + expect(big.heavy).toBe(true); + expect(big.addedRanges).toBeDefined(); + }); + + it('refuses an already-rescoped plan and a plan with unusable files[]', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const after = readFileSync(planPath, 'utf8'); + process.exitCode = undefined; + run(planPath, anchor); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(after); + + process.exitCode = undefined; + const planPath2 = writeFetchedPlan(base, head); + const mangled = JSON.parse(readFileSync(planPath2, 'utf8')) as Record< + string, + unknown + >; + mangled['files'] = 'nope'; + writeFileSync(planPath2, JSON.stringify(mangled)); + const before = readFileSync(planPath2, 'utf8'); + run(planPath2, anchor); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath2, 'utf8')).toBe(before); + }); + + it('an unreadable plan path exits 2, not a throw', () => { + seedHistory(); + run(join(repo, 'no-such-plan.json'), 'HEAD'); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + }); + + it('a plan that parses to JSON null exits 2, not a TypeError', () => { + seedHistory(); + const nullPlan = join(repo, 'null-plan.json'); + writeFileSync(nullPlan, 'null'); + run(nullPlan, 'HEAD'); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + }); +}); + +describe('rescope — round-2 findings', () => { + it('a rename in the fix round keeps its RENAME section — hunks stay a subset of the PR diff', () => { + const { base, anchor } = seedHistory(); + // The fix round renames caller.ts (routine `git mv` on review feedback). + git('mv', CALLER, 'packages/app/src/renamed-caller.ts'); + git('commit', '-q', '--no-verify', '-m', 'rename in fix round'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); + // A pathspec-scoped re-capture cannot see the rename source and renders + // a whole-file ADD ('new file mode'); the byte-slice keeps the pairing. + expect(diff).toContain('rename to packages/app/src/renamed-caller.ts'); + expect(diff).not.toContain('new file mode'); + }); + + it('empty and zero-usable files[] both refuse with the plan untouched', () => { + const { base, anchor, head } = seedHistory(); + for (const files of [[], [{}, { path: 42 }]]) { + process.exitCode = undefined; + const planPath = writeFetchedPlan(base, head); + const mangled = JSON.parse(readFileSync(planPath, 'utf8')) as Record< + string, + unknown + >; + mangled['files'] = files; + writeFileSync(planPath, JSON.stringify(mangled)); + const before = readFileSync(planPath, 'utf8'); + run(planPath, anchor); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + } + }); + + it('a delta file restored to its merge-base state is reconciled out of deltaFiles', () => { + const { base, anchor } = seedHistory(); + // The fix round restores changed.ts to its merge-base content: it is in + // the interdiff, but the PR's own diff has no section for it. + write(CHANGED, 'export const v = 1;\n'); + write(BYSTANDER, 'export const b = 9;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'restore + touch bystander'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + // bystander changed for real; the restored file names no phantom scope — + // but its importer still re-enters through the widening. + expect(plan.incremental.deltaFiles).toEqual([BYSTANDER]); + expect(plan.incremental.interaction.map((e) => e.path)).toContain(CALLER); + const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); + expect(diff).not.toContain('changed.ts'); + }); + + it('runs correctly from a cwd OUTSIDE the worktree — the documented production shape', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + // Rewrite the plan's diff path to absolute (fetch-pr always records it). + const plan0 = JSON.parse(readFileSync(planPath, 'utf8')) as Record< + string, + unknown + >; + plan0['diffPathAbsolute'] = join(repo, plan0['diffPath'] as string); + writeFileSync(planPath, JSON.stringify(plan0)); + const checkout = realpathSync( + mkdtempSync(join(tmpdir(), 'main-checkout-')), + ); + const prev = process.cwd(); + process.chdir(checkout); + try { + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + // The interaction hunks are present — the -C pin, not the cwd, decides. + const diff = readFileSync(join(checkout, plan.diffPath), 'utf8'); + expect(diff).toContain('caller.ts'); + } finally { + process.chdir(prev); + rmSync(checkout, { recursive: true, force: true }); + } + }); + + it('an unwritable --out exits 2 instead of throwing', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const blocked = join(repo, 'blocked'); + writeFileSync(blocked, 'a plain file where a directory must go'); + (rescopeCommand.handler as (argv: unknown) => void)({ + plan: planPath, + anchor, + out: join(blocked, 'nested', 'plan.json'), + maxChunkLines: 400, + }); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + }); +}); + +describe('rescope — round-3 findings', () => { + it('a rename-before-anchor then delete refuses rather than losing the lineage', () => { + // parseDiff labels a deletion with its LEFT-side path, so the PR diff + // calls this file `old.ts` while the interdiff calls it `new.ts`; the + // section carrying its unreviewed hunks matches no scoped name. Dropping + // it would narrow BELOW the un-widened interdiff floor. + seedHistory(); + write('packages/app/src/old.ts', 'export const o = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'seed old.ts'); + const base2 = git('rev-parse', 'HEAD'); + git('mv', 'packages/app/src/old.ts', 'packages/app/src/new.ts'); + git('commit', '-q', '--no-verify', '-m', 'round 1 renames it'); + const anchor2 = git('rev-parse', 'HEAD'); + git('rm', '-q', 'packages/app/src/new.ts'); + git('commit', '-q', '--no-verify', '-m', 'fix round deletes it'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base2, head2); + const before = readFileSync(planPath, 'utf8'); + run(planPath, anchor2); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('exit 3 when every delta file was restored to its merge-base state', () => { + const { base, head } = seedHistory(); + // Anchor at the LAST reviewed head, then restore the ONE file this round + // touches (the bystander, which nobody imports, so the widening adds + // nothing): the interdiff is non-empty, yet no scoped file carries a + // section of the PR's own diff. The rest of the PR still does — this is + // not the empty-diff case. + const anchor = head; + write(BYSTANDER, 'export const b = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'restore the bystander'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + const before = readFileSync(planPath, 'utf8'); + run(planPath, anchor); + expect(process.exitCode).toBe(RESCOPE_EXIT_NOTHING_NEW); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('the unwritable --out refusal leaves the input plan byte-identical', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const before = readFileSync(planPath, 'utf8'); + const blocked = join(repo, 'blocked-2'); + writeFileSync(blocked, 'a plain file where a directory must go'); + (rescopeCommand.handler as (argv: unknown) => void)({ + plan: planPath, + anchor, + out: join(blocked, 'nested', 'plan.json'), + maxChunkLines: 400, + }); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); +}); + +describe('rescope — round-5/6 findings', () => { + it('a RESTORED importer still gets a reader when what it imports keeps changing', () => { + // R6-10: the restored file has no PR-diff section (nothing left to + // review in it) but its imports of a still-changing file are live seams + // — judged after the fact it fell between both reader classes and got + // zero readers. + const { base } = seedHistory(); + // Round 1 (anchor) changed both; the fix round REVERTS caller.ts to its + // merge-base content and changes changed.ts again. + const anchor = git('rev-parse', 'HEAD'); + write(CALLER, "import { v } from './changed.js';\nexport const c = v;\n"); + write(CHANGED, 'export const v = 9;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'revert caller, change callee'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + expect(plan.incremental.interaction.map((e) => e.path)).toContain(CALLER); + }); + + it('a plan naming a symbolic ref instead of a sha refuses', () => { + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const plan0 = JSON.parse(readFileSync(planPath, 'utf8')) as Record< + string, + unknown + >; + plan0['fetchedSha'] = 'HEAD'; + writeFileSync(planPath, JSON.stringify(plan0)); + const before = readFileSync(planPath, 'utf8'); + run(planPath, anchor); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + expect(readFileSync(planPath, 'utf8')).toBe(before); + }); + + it('a dead stdout after the plan write does not turn exit 0 into exit 1', () => { + // "Only exit 0 rewrites the plan" needs its contrapositive: a non-zero + // exit must mean the plan is untouched, so nothing past the write may + // throw (EPIPE from `qwen … | head`). + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + const spy = vi.spyOn(process.stdout, 'write').mockImplementation(() => { + throw Object.assign(new Error('write EPIPE'), { code: 'EPIPE' }); + }); + try { + expect(() => run(planPath, anchor)).not.toThrow(); + } finally { + spy.mockRestore(); + } + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + }); +}); + +describe('rescope — round-6/7 Criticals', () => { + it.skipIf(process.platform === 'win32')( + 'a content-revert that KEEPS chmod +x is not "restored" — the mode change is scope', + () => { + // `rev-parse :` yields the blob alone, so a revert that + // keeps the exec bit compared equal and was dropped — yet its + // mode-only section IS in the PR's own diff, so the incremental scope + // narrowed below the full-range floor and exited 3 over a real change. + const { base } = seedHistory(); + const anchor = git('rev-parse', 'HEAD'); + write(CHANGED, 'export const v = 1;\n'); // content back to base + // A REAL exec bit on disk: `update-index --chmod` alone is undone by a + // later `commit -a`, which re-stages from the worktree. + execFileSync('chmod', ['+x', join(repo, CHANGED)]); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'revert content, keep +x'); + const head2 = git('rev-parse', 'HEAD'); + const planPath = writeFetchedPlan(base, head2); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toContain(CHANGED); + expect(readFileSync(join(repo, plan.diffPath), 'utf8')).toContain( + 'new mode 100755', + ); + }, + ); + + it('an ASYNC stdout error after the plan write still exits 0', () => { + // EPIPE reaches the process as an unhandled 'error' EVENT, which no + // try/catch around the write can intercept; unhandled, it exits 1 over + // an already-rewritten plan. + const { base, anchor, head } = seedHistory(); + const planPath = writeFetchedPlan(base, head); + run(planPath, anchor); + expect(process.exitCode ?? 0).toBe(0); + // The listener the command installs is what makes the async shape inert. + expect(process.stdout.listenerCount('error')).toBeGreaterThan(0); + expect(process.stderr.listenerCount('error')).toBeGreaterThan(0); + expect(() => + process.stdout.emit('error', new Error('EPIPE')), + ).not.toThrow(); + expect(() => + process.stderr.emit('error', new Error('EPIPE')), + ).not.toThrow(); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as RescopedPlan; + expect(plan.incremental.deltaFiles).toEqual([CHANGED]); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts new file mode 100644 index 00000000000..ede6ff085a8 --- /dev/null +++ b/packages/cli/src/commands/review/rescope.ts @@ -0,0 +1,612 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review rescope`: shrink a fetched PR plan to the incremental range +// since a previous clean round's anchor, widened by one import hop. +// +// Incremental review existed only as prose: Step 1 told the orchestrator to +// "compute `git diff ..HEAD` and use it as the review scope", +// and the mechanics were left to improvisation. The improvised route — re-run +// `plan-diff` over a hand-captured interdiff — silently degrades the plan: a +// `plan-diff` plan has no `worktreePath`, no `prNumber`/`ownerRepo` unless +// re-supplied, no per-file line counts and so no `heavy` classification, which +// drops Agent 0, the modeled-system lens and every invariant agent from the +// roster with nothing to say so. This command produces the incremental plan +// the right way round: same builders as `fetch-pr`, identity fields carried +// over, post-image line counts from the fetched head. +// +// It also WIDENS the range. The previous round's "clean" was certified against +// the code as it stood then; the fix under review now can change a contract an +// unchanged file depends on. Every still-clean source file that imports a +// changed file re-enters the scope with its full-range diff, and the plan +// records why (`incremental.interaction[]`), so its chunk brief can direct the +// agent at the seam instead of a from-scratch re-review. +// +// Failure is directional ON PURPOSE. Exit 2 means "could not scope" — the +// caller falls back to the FULL diff, never to a skip: the plan file is left +// untouched, so the fetched full-range plan simply remains the plan of record. +// Exit 3 means "nothing new to review": the interdiff is empty (the anchor's +// tree and the head's are identical, the same outcome as the same-SHA +// shortcut), or every file it named turned out to be restored to its +// merge-base state and so carries no section of the PR's own diff. Only +// exit 0 rewrites the plan, atomically. + +import type { CommandModule } from 'yargs'; +import { mkdirSync, readFileSync } 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 { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; +import { fileLineCount, gitOpt, gitRaw } from './lib/git.js'; +import { operatorReviewSettings } from './lib/review-settings.js'; +import { hasReviewDeadline } from './lib/deadline.js'; +import { + LITERAL_PATHSPECS, + PINNED_DIFF_CONFIG, + PINNED_DIFF_FLAGS, +} from './lib/diff-flags.js'; +import { + buildDiffPlan, + parseDiff, + sliceDiffByLines, + DEFAULT_MAX_CHUNK_LINES, + READ_FILE_CHAR_CAP, +} from './lib/diff-plan.js'; +import { + buildPlanReport, + warnOnReportSize, + stringifyPlanReport, +} from './lib/report.js'; +import { + dependentsOfChanged, + discoverWorkspacePackages, +} from './lib/import-graph.js'; + +/** Exit codes the skill branches on. Named so the prose and the code agree. */ +export const RESCOPE_EXIT_FULL_RANGE = 2; +export const RESCOPE_EXIT_NOTHING_NEW = 3; + +interface RescopeArgs { + plan: string; + anchor: string; + out?: string; + maxChunkLines: number; +} + +/** The fields rescope reads off the fetched plan. Parsed off disk — guard everything. */ +interface FetchedPlan { + prNumber?: unknown; + worktreePath?: unknown; + fetchedSha?: unknown; + mergeBaseSha?: unknown; + diffPath?: unknown; + diffPathAbsolute?: unknown; + files?: unknown; + incremental?: unknown; +} + +/** + * The block the rescoped plan carries. Chunk briefs and the roster read it; + * absence means a full-range plan, which is every plan rescope did not write. + */ +export interface IncrementalScope { + /** Full sha of the previous clean round's head — the range's left side. */ + anchor: string; + /** + * Files changed in `anchor..head`. The interdiff decides only WHICH files + * these are; their hunks in the composite are the full `mergeBase..head` + * change. Since-anchor hunks were tried first and reverted: a fix round + * that RESTORES lines the previous round changed produces interdiff hunks + * that exist in no hunk of the PR's own diff, and an inline comment + * anchored on one 422s the whole posted review, all-or-nothing. Full-range + * hunks are a subset of the PR diff by construction, so every anchor a + * chunk agent produces stays anchorable. + */ + deltaFiles: string[]; + /** + * Still-clean files pulled back in by the one-hop widening, each with the + * changed files it imports — the seam its brief directs the agent at. + */ + interaction: Array<{ path: string; importsChanged: string[] }>; + /** + * How many still-clean source files this scope leaves out. A count, not a + * list: nothing downstream reads the names, and on a large PR the list + * alone measured 23 KB against the plan's one-read budget. + */ + contextFileCount: number; + /** Where the full-range diff still is, for a reader who needs the whole PR. */ + fullDiffPath: string | null; +} + +function fail(code: number, message: string): void { + writeStderrLine(message); + process.exitCode = code; +} + +function runRescope(args: RescopeArgs): void { + let plan: FetchedPlan; + try { + const parsed: unknown = JSON.parse(readFileSync(args.plan, 'utf8')); + // `JSON.parse('null')` succeeds; dereferencing it does not. A truncated + // or clobbered plan must land on the refusal, not on a TypeError with an + // exit code the skill has no branch for. + if ( + typeof parsed !== 'object' || + parsed === null || + Array.isArray(parsed) + ) { + throw new Error('the plan is not a JSON object'); + } + plan = parsed as FetchedPlan; + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: cannot read plan ${args.plan}: ${(err as Error).message}. ` + + `Continue with the full-range plan.`, + ); + return; + } + + const worktreePath = + typeof plan.worktreePath === 'string' ? plan.worktreePath : null; + const fetchedSha = + typeof plan.fetchedSha === 'string' ? plan.fetchedSha : null; + const mergeBaseSha = + typeof plan.mergeBaseSha === 'string' ? plan.mergeBaseSha : null; + // Both ends of the PR range must be OBJECT IDS, not refs: `fetch-pr` writes + // full shas, and a clobbered plan naming a moving ref (`HEAD`, a branch) + // would resolve at call time — the interdiff describing one tree, the + // worktree reads another, while the exit-0 plan claims a scope that is not + // the one under review. Anchor validation below covers only the anchor. + const SHA_RE = /^[0-9a-f]{40,64}$/i; + if ( + (fetchedSha !== null && !SHA_RE.test(fetchedSha)) || + (mergeBaseSha !== null && !SHA_RE.test(mergeBaseSha)) + ) { + fail( + RESCOPE_EXIT_FULL_RANGE, + "rescope: the plan's fetchedSha/mergeBaseSha are not object ids — a " + + 'symbolic or clobbered ref would scope this round to a range that is ' + + 'not the one under review. Continue with the full-range plan.', + ); + return; + } + if (!worktreePath || !fetchedSha || !mergeBaseSha) { + // Local and lightweight plans have no worktree and no fetched range — + // there is nothing to scope an anchor against. + fail( + RESCOPE_EXIT_FULL_RANGE, + 'rescope: the plan carries no worktreePath/fetchedSha/mergeBaseSha — ' + + 'incremental rescoping serves the fetched-PR flow only. ' + + 'Continue with the full-range plan.', + ); + return; + } + if (typeof plan.incremental === 'object' && plan.incremental !== null) { + // A second rescope of an already-rescoped plan would derive candidates + // from the already-shrunk file list (files widened out in round N can + // never re-enter) and repoint `fullDiffPath` at the incremental diff it + // is about to overwrite. The plan of record for a rescope is always a + // fresh fetch. + fail( + RESCOPE_EXIT_FULL_RANGE, + 'rescope: the plan is already rescoped — re-run fetch-pr to restore ' + + 'the full-range plan first. Continue with the full-range review.', + ); + return; + } + // `plan.files` is the candidate universe for the widening AND the proof the + // fetch captured anything at all. Normalising a malformed list to [] would + // silently drop every interaction candidate and still exit 0 — a truncated + // plan must keep the safe full review instead. + const planFilesRaw = plan.files; + if (!Array.isArray(planFilesRaw) || planFilesRaw.length === 0) { + fail( + RESCOPE_EXIT_FULL_RANGE, + 'rescope: the plan carries no usable `files[]` (missing, malformed, or ' + + 'empty) — cannot widen safely. Continue with the full-range plan.', + ); + return; + } + const planFiles = planFilesRaw as Array<{ + path?: unknown; + kind?: unknown; + binary?: unknown; + }>; + const allPaths = planFiles + .filter((f): f is { path: string } => !!f && typeof f?.path === 'string') + .map((f) => f.path); + if (allPaths.length === 0) { + // A NON-EMPTY files[] whose entries carry no usable path is the same + // truncated-plan shape as an empty one — and worse downstream, where + // "zero files compared" must never read as "nothing changed". + fail( + RESCOPE_EXIT_FULL_RANGE, + "rescope: the plan's `files[]` has no entry with a usable path — " + + 'cannot widen safely. Continue with the full-range plan.', + ); + return; + } + + // Anchor validation is re-done HERE, not trusted from the caller: a sha that + // is not in this history would hand `git diff` a range that reviews the + // wrong code, which is the one failure mode this feature must never have. + // Every git call is pinned to the plan's worktree with `-C`: pathspecs + // resolve against git's cwd, and from a subdirectory an unmatched pathspec + // exits 0 with EMPTY output — hunks silently vanish instead of failing. + const anchorFull = gitOpt( + '-C', + worktreePath, + 'rev-parse', + `${args.anchor}^{commit}`, + ); + if (anchorFull === null) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: anchor ${args.anchor} is not a commit in this repository ` + + `(rebased away, or from another history). Continue with the ` + + `full-range plan.`, + ); + return; + } + if (anchorFull === fetchedSha) { + fail( + RESCOPE_EXIT_NOTHING_NEW, + `rescope: anchor ${args.anchor} IS the fetched head — no new commits ` + + `since the last clean round.`, + ); + return; + } + if ( + gitOpt( + '-C', + worktreePath, + 'merge-base', + '--is-ancestor', + anchorFull, + fetchedSha, + ) === null + ) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: anchor ${args.anchor} is not an ancestor of the fetched head ` + + `${fetchedSha} (force-push or rebase). Continue with the full-range ` + + `plan.`, + ); + return; + } + + // The interdiff decides only WHICH files changed since the anchor — see + // `IncrementalScope.deltaFiles` for why their hunks are captured full-range + // instead of from this diff. + let interdiff: Buffer; + try { + interdiff = gitRaw( + '-C', + worktreePath, + ...PINNED_DIFF_CONFIG, + 'diff', + ...PINNED_DIFF_FLAGS, + `${anchorFull}..${fetchedSha}`, + ); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: could not capture ${args.anchor}..head: ` + + `${(err as Error).message}. Continue with the full-range plan.`, + ); + return; + } + const deltaFiles = parseDiff(interdiff.toString('utf8')).files.map( + (f) => f.path, + ); + if (deltaFiles.length === 0) { + fail( + RESCOPE_EXIT_NOTHING_NEW, + `rescope: ${args.anchor}..head is an empty diff — the tree is ` + + `identical to the last clean round's.`, + ); + return; + } + // The restoration probe runs BEFORE the widening, not after it. A delta + // file the fix round restored to its merge-base state has no PR-diff + // section (nothing left to review in it) — but it is also, by definition, + // a file whose CURRENT content is the base content, so if it imports a + // file that IS still changing, that seam is exactly what the widening + // exists to catch. Judged after the fact it fell between both classes: + // excluded from the delta readers for having no section, and excluded from + // the widening candidates for being in `delta`. It is a CANDIDATE. + const restored = (p: string): boolean => { + // The whole TREE ENTRY, not the blob: `rev-parse :` yields the + // oid alone, so a fix round that reverts the content and keeps `chmod +x` + // — or swaps a file for a symlink with the same text — compared equal and + // was dropped as "restored". Its mode-only section IS in the PR's diff + // (parseDiff emits one, planChunks gives it a chunk), so dropping it + // narrowed the incremental scope BELOW the full-range floor and exited 3 + // "nothing new" over a change nobody reviewed. + const at = (ref: string) => { + const line = gitOpt( + '-C', + worktreePath, + LITERAL_PATHSPECS, + 'ls-tree', + ref, + '--', + p, + ); + if (line === null || line === '') return null; + const tab = line.indexOf('\t'); + const meta = (tab < 0 ? line : line.slice(0, tab)).split(' '); + return meta.length >= 3 ? `${meta[0]} ${meta[2]}` : null; + }; + const b = at(mergeBaseSha); + const h = at(fetchedSha); + // Absent on BOTH sides is deliberately NOT a restoration. Two shapes + // produce it and this layer cannot tell them apart: a file the PR added + // and this round deleted (net-zero — safe), and a file renamed before + // the anchor and deleted now, whose unreviewed deletion hunks sit in the + // PR diff under its pre-rename name (dropping it loses them). Refusing + // costs a full review on the first shape; dropping loses scope on the + // second, so the refusal wins. + return b !== null && h !== null && b === h; + }; + const restoredDelta = new Set(deltaFiles.filter(restored)); + // Two sets, because a restored file plays both parts. As a CHANGE it still + // pulls its importers in: round 1 cleared them against the pre-revert + // callee, and (importer@head × callee@base) is a pairing no round has + // seen. As a FILE it has nothing left to review — its content is the base + // content — so it owes no full review and instead becomes a widening + // candidate in its own right, for the still-changing files IT imports. + const delta = new Set(deltaFiles); + const deltaLive = new Set(deltaFiles.filter((p) => !restoredDelta.has(p))); + + // One import hop over the plan's still-clean SOURCE files. Test and docs + // dependents stay out: re-running tests is `build-test`'s job, and prose + // does not call functions. Reads come from the worktree — the post-change + // state is the state whose interactions are in question. + const candidates = planFiles + .filter( + (f): f is { path: string; kind: string; binary?: boolean } => + !!f && + typeof f.path === 'string' && + f.kind === 'source' && + f.binary !== true && + // Keyed on the LIVE delta: a restored file reads as base content, so + // it owes no review of its own and is a candidate like any other. + !deltaLive.has(f.path), + ) + .map((f) => f.path); + const readWorktree = (rel: string): string | null => { + try { + return readFileSync(join(worktreePath, rel), 'utf8'); + } catch { + return null; + } + }; + const packages = discoverWorkspacePackages( + [...deltaFiles, ...candidates], + readWorktree, + ); + const interaction = dependentsOfChanged( + delta, + candidates, + readWorktree, + packages, + ); + // A restored file is inside `delta`, so the pass above skips it as a + // candidate by construction (`dependentsOfChanged` never scans a file that + // is itself changed). It still needs one: its own imports of files that + // are STILL changing are live seams no other reader covers. Keyed on + // `deltaLive`, because a restored file importing another restored file has + // no moving side to check. + for (const [path, edges] of dependentsOfChanged( + deltaLive, + [...restoredDelta], + readWorktree, + packages, + )) { + if (!interaction.has(path)) interaction.set(path, edges); + } + + // The composite is a BYTE-SLICE of the fetched full-range diff, not a + // pathspec-scoped re-capture. Two invariants ride on that: every hunk is + // byte-identical to a hunk of the PR's own diff (comment anchoring can + // never produce a line GitHub refuses), and RENAME sections stay paired — + // a pathspec-scoped `git diff` cannot see the rename source, un-pairs the + // rename, and renders the file as a whole-file add whose hunks exist + // nowhere in the original diff. + const origDiffPath = + typeof plan.diffPathAbsolute === 'string' + ? plan.diffPathAbsolute + : typeof plan.diffPath === 'string' + ? resolve(plan.diffPath) + : null; + let origDiff: Buffer; + try { + if (origDiffPath === null) throw new Error('the plan names no diff file'); + origDiff = readFileSync(origDiffPath); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: cannot read the fetched diff (${(err as Error).message}). ` + + `Continue with the full-range plan.`, + ); + return; + } + const scoped = new Set([...deltaLive, ...interaction.keys()]); + const sections = parseDiff(origDiff.toString('utf8')).files.filter((f) => + scoped.has(f.path), + ); + const composite = sliceDiffByLines( + origDiff, + sections.map((f) => ({ startLine: f.diffStart, endLine: f.diffEnd })), + ); + // Reconcile the reported delta with what the composite actually holds: a + // file restored to its merge-base state is in the interdiff but has no + // hunks here, and a plan naming delta files with zero hunks sends agents + // hunting for scope that does not exist. (The WIDENING above still used + // the full changed set — a restoration still moves its importers' seams.) + const sectionPaths = new Set(sections.map((f) => f.path)); + // Every LIVE delta file must carry a section of the PR's own diff. One + // that does not is a lineage break — a file renamed before the anchor and + // deleted now is `new.ts` in the interdiff but `old.ts` on the PR diff's + // deletion section (parseDiff labels a deletion with its left-side path), + // so the section holding its unreviewed hunks is scoped out under a name + // nothing matched. Restored files are already out of `delta` above. + const lineageLost = [...deltaLive].filter((p) => !sectionPaths.has(p)); + if (lineageLost.length > 0) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: ${lineageLost.length} file(s) changed since ${args.anchor} ` + + `carry no section of the PR's own diff under that name ` + + `(${lineageLost.slice(0, 3).join(', ')}${lineageLost.length > 3 ? ', …' : ''}) ` + + `— a rename or lineage change the scoped slice cannot follow. ` + + `Continue with the full-range plan.`, + ); + return; + } + if (sections.length === 0) { + // Nothing of the PR's own diff is in scope: every changed file was + // restored to its merge-base state and nothing imports them. There is + // nothing left to re-review. + fail( + RESCOPE_EXIT_NOTHING_NEW, + `rescope: the files changed since ${args.anchor} carry no section of ` + + `the PR's own diff (restored to the merge-base state) — nothing new ` + + `to review.`, + ); + return; + } + const deltaReported = [...deltaLive].filter((p) => sectionPaths.has(p)); + let diffPlan; + try { + diffPlan = buildDiffPlan(composite.toString('utf8'), args.maxChunkLines); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: could not partition the incremental diff ` + + `(${(err as Error).message}). Continue with the full-range plan.`, + ); + return; + } + + const target = + typeof plan.prNumber === 'string' || typeof plan.prNumber === 'number' + ? `pr-${plan.prNumber}` + : 'rescope'; + const diffRel = tmpFile(target, 'diff-incremental.txt'); + const incremental: IncrementalScope = { + anchor: anchorFull, + deltaFiles: deltaReported, + interaction: [...interaction.entries()].map(([path, importsChanged]) => ({ + path, + importsChanged, + })), + contextFileCount: candidates.filter((p) => !interaction.has(p)).length, + // Absolute: the field's whole job is to let a later step reach the + // superseded diff, and that step need not share this command's cwd. + fullDiffPath: origDiffPath, + }; + + // The rescoped plan is the fetched plan with its diff swapped: identity and + // provenance fields ride through the spread untouched (worktreePath, + // prNumber, ownerRepo, shas, repositoryContext when repo-context already + // ran, effort), while everything the diff determines — chunks, files, + // topology counts, budget — is recomputed from the incremental diff by the + // SAME builders `fetch-pr` used, post-image line counts included, so the + // heaviness classification and the roster it drives cannot drift from what + // a full-range plan of this diff would have said. + const result = { + ...(plan as Record), + diffPath: diffRel, + diffPathAbsolute: resolve(diffRel), + // `-C`-pinned like every other git call here: an unpinned `git show` + // resolves `:` against the process cwd's repository. + ...buildPlanReport( + diffPlan, + (path) => fileLineCount(fetchedSha, path, worktreePath), + { + operatorRoundCap: operatorReviewSettings().reverseAuditRounds, + hasDeadline: hasReviewDeadline(process.env), + }, + ), + incremental, + }; + + const out = args.out ?? args.plan; + try { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + atomicWriteFileSync(diffRel, composite); + mkdirSync(dirname(resolve(out)), { recursive: true }); + atomicWriteFileSync(out, stringifyPlanReport(result)); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: could not write the rescoped plan (${(err as Error).message}). ` + + `Continue with the full-range plan.`, + ); + return; + } + // NOTHING past the plan write may end the process. Two shapes, and the + // try/catch below only ever caught the first: `write` can throw + // synchronously, and it can also surface EPIPE as an ASYNC 'error' event on + // the stream — unhandled, that terminates the process with exit 1 no + // catch block can intercept. A persistent no-op listener is what makes the + // async shape inert (measured: it flips the dead-pipe arm back to exit 0). + const swallow = () => {}; + process.stdout.on('error', swallow); + process.stderr.on('error', swallow); + // NOTHING past the plan write may throw. "Only exit 0 rewrites the plan" is + // the invariant the skill branches on, and its contrapositive has to hold + // too: a non-zero exit must mean the plan is untouched. A dead stdout (the + // command piped into `head`, a daemon redirect) makes `write` raise EPIPE, + // which would exit 1 over an already-rewritten plan and send the caller + // down the "full-range plan untouched" branch against an incremental one. + // The reporting is a courtesy; the write is the result. + try { + writeStdoutLine(`Wrote incremental plan to ${out}`); + writeStderrLine( + `Incremental scope since ${anchorFull.slice(0, 12)}: ` + + `${deltaReported.length} changed file(s), ${interaction.size} ` + + `interaction file(s) (one import hop), ` + + `${incremental.contextFileCount} clean file(s) left out of scope; ` + + `${diffPlan.diffLines} diff line(s) -> ${diffPlan.chunks.length} chunk(s).`, + ); + warnOnReportSize(out, READ_FILE_CHAR_CAP); + } catch { + // A reader that went away cannot un-write the plan. + } +} + +export const rescopeCommand: CommandModule = { + command: 'rescope', + describe: + 'Rescope a fetched PR plan to the incremental diff since a previous ' + + 'clean round, widened by one import hop', + builder: (y) => + y + .option('plan', { + type: 'string', + demandOption: true, + describe: 'The fetch-pr plan report to rescope', + }) + .option('anchor', { + type: 'string', + demandOption: true, + describe: "The previous clean round's reviewed head sha", + }) + .option('out', { + type: 'string', + describe: 'Where to write the rescoped plan (default: in place)', + }) + .option('max-chunk-lines', { + type: 'number', + default: DEFAULT_MAX_CHUNK_LINES, + describe: 'Target chunk size in diff lines', + }) + .strict(), + handler: (argv) => runRescope(argv as unknown as RescopeArgs), +}; diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index fe7692f92c7..06f96a44fe6 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -499,6 +499,14 @@ So the authoritative ledger now travels in the posted body as an HTML-comment ma Two consequences of those boundaries are worth naming rather than discovering. Ids are **carried, not renumbered**: a still-standing finding is re-reported under the id it already has, that id is written into the comment right after the severity marker, and `buildLedger` reads it back — because a ledger that renumbered by position would key the next round's work list to ids the report riding beside it never used, and `R1-2 names the same claim in every round` is the entire payoff. And own-account recovery means a PR reviewed from **two** accounts — a maintainer locally, a bot in CI — keeps two independent ledgers, each with its own round counter and its own `R2-1`; that is the honest reading of "only this account's reviews can claim what this account stood behind", but it does mean the ids are scoped to the account that wrote them, not to the PR. +## Why incremental scoping is a command, and why it widens by one import hop + +Incremental review shipped as prose: Step 1 told the orchestrator to "compute `git diff ..HEAD` and use it as the review scope", and left the mechanics to improvisation. The only improvisable route — capture the interdiff by hand and re-run `plan-diff` over it — silently degrades the plan: a `plan-diff` plan carries no `worktreePath`, no PR identity unless re-supplied, and no per-file line counts, so no `heavy` classification — which drops Agent 0, the modeled-system lens and every invariant agent from the roster, with nothing anywhere saying so. `rescope` closes that hole the way `check-coverage` closed the receipt hole: the scope decision moves from prose into a subcommand that validates its own inputs (the anchor is re-checked against the history — `rev-parse`, `--is-ancestor` — because a mis-scoped range reviews the wrong code, the one failure this feature must never have) and rebuilds the plan with the same builders `fetch-pr` used, identity fields riding through and post-image line counts intact. Its failure direction is pinned: any refusal leaves the plan file untouched, so the fallback is always the full-range review, never a skip. + +Scoped files carry their FULL-RANGE hunks; the interdiff only chooses which files are in scope. The first cut gave delta files their since-anchor hunks — tighter, and wrong: a fix round that RESTORES lines the previous round changed produces interdiff hunks that exist in no hunk of the PR's own `mergeBase..head` diff, and an inline comment anchored on such a line 422s the whole Create Review call, all-or-nothing — the review's entire inline output lost to one anchor. Full-range hunks are a subset of the PR diff by construction, so every anchor stays anchorable; the savings that matter were always file-level (the files skipped), not hunk-level. + +The widening exists because "clean" is a verdict about the code as it stood. The previous round cleared a caller against the callee it imported THEN; the fix under review moves the callee, and a scope that holds only the interdiff never re-opens the caller — the breakage retires silently, permanently, because the next clean round re-anchors past it. So every still-clean source file one import hop from a changed file re-enters the scope with its full-range hunks, and the plan records why (`incremental.interaction[]`), so the chunk brief can direct its agent at the seam — "do your uses of what changed still hold" — instead of a from-scratch re-review that re-reports what round 1 already ruled on. One hop, dependents only, source files only: the callee-side risk lives in the changed file's own chunk (its agent reads callees from the worktree), test dependents are `build-test`'s job, and a barrel re-export between caller and callee hides the edge — a documented miss that leaves exactly the floor incremental review had before widening existed. The specifier scan is a regex heuristic on purpose, and its error directions are chosen: a false positive reviews a file once more than needed, a false negative never drops below the unwidened floor. + ## 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 1615b278ec4..a7fa26c27fa 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -756,7 +756,7 @@ The ledger has two sources, in priority order: **the PR itself** — `pr-context **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. **The class finding carries one demonstrated entrance as its witness** — the concrete input and the line(s) producing the wrong outcome — so it clears Step 4's high-confidence bar and posts (a shape with no concrete corner confirms only low, and low-confidence findings are terminal-only — they never post and never reach the ledger this backstop reads); the entrance is the class's evidence, not a separate finding. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by ` (the disposition above), fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round's **ledger marker** recovers one entry, not N, and a prior sibling that resurfaces on the PR as its own thread is ruled `superseded`, not re-posted. **A brand-new sibling found in the current round** — by a Step 3 finder or Step 5 auditor over the incremental diff, while the class finding is already on the ledger and open — folds the same way: into the class finding's re-report as evidence under the class id at Step 6 rendering, never filed under its own id. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. **Supersession preserves the strongest evidence**: collapse a family only when the class finding is filed at **at least the highest severity AND confidence any absorbed sibling demonstrated** — a proven high-confidence Critical entrance must not be retired behind a low-confidence or non-Critical class finding (which never posts, so nothing carries the block and the defect stays live at a zero-Critical verdict). If the class finding cannot carry that strength, keep the prior Critical open until an equally-strong verified class finding replaces it. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) -Render the rulings as a short table at the top of the Findings section — id, one-line title, this round's status — so the report reads as a continuation, the way a human reviewer's round-2 comment opens with "M1 is fixed". The incremental scope rule does not conflict with this: the _diff_ reviewed is `lastCommitSha..HEAD`, but a ledger ruling reads the code at HEAD, which every agent already has. +Render the rulings as a short table at the top of the Findings section — id, one-line title, this round's status — so the report reads as a continuation, the way a human reviewer's round-2 comment opens with "M1 is fixed". The incremental scope rule does not conflict with this: the _files_ reviewed are those changed in `lastCommitSha..HEAD` (plus the one-hop interaction files `rescope` widened the scope with), but a ledger ruling reads the code at HEAD, which every agent already has. ### The convergence posture (round-aware posting, PR re-reviews only)