From 5febdc5631c51ab09c25b768bd44bc3298263ea1 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 15 Aug 2026 01:44:45 +0800 Subject: [PATCH 1/8] =?UTF-8?q?feat(review):=20rescope=20=E2=80=94=20deter?= =?UTF-8?q?ministic=20incremental=20plans,=20widened=20one=20import=20hop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Incremental review existed only as prose: Step 1 said "compute git diff ..HEAD and use it as the review scope" and left the mechanics to improvisation. The improvisable route — re-run plan-diff over a hand-captured interdiff — silently degrades the plan (no worktreePath, no PR identity, no heaviness), dropping Agent 0, the modeled-system lens and every invariant agent from the roster. `qwen review rescope --plan --anchor ` moves the scope decision into code: it re-validates the anchor against the history, captures the interdiff with the pinned flags, widens it by one import hop — every still-clean source file that imports a changed file re-enters the scope with its full-range hunks — and rewrites the plan in place with the same builders fetch-pr used, identity fields riding through and post-image line counts intact. The plan gains an `incremental` block; chunk briefs annotate each file's class (changed = review in full, interaction = review the seam only), and whole-diff briefs carry the frame once. Failure is directional: any refusal leaves the plan untouched, so the fallback is the full-range review, never a skip; an empty interdiff exits 3 and maps to the same-SHA outcomes. The widening exists because "clean" was certified against the code as it stood: a fix that moves a contract can break an unchanged caller, and an interdiff-only scope never re-opens it. Dependents only, source only, one hop; the scan is a documented heuristic whose misses keep exactly the pre-widening floor. --- packages/cli/src/commands/review.test.ts | 1 + packages/cli/src/commands/review.ts | 4 +- .../src/commands/review/agent-prompt.test.ts | 61 +++ .../cli/src/commands/review/agent-prompt.ts | 112 ++++++ packages/cli/src/commands/review/fetch-pr.ts | 23 +- packages/cli/src/commands/review/lib/git.ts | 21 ++ .../commands/review/lib/import-graph.test.ts | 179 +++++++++ .../src/commands/review/lib/import-graph.ts | 243 ++++++++++++ .../cli/src/commands/review/rescope.test.ts | 249 +++++++++++++ packages/cli/src/commands/review/rescope.ts | 349 ++++++++++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 6 + .../core/src/skills/bundled/review/SKILL.md | 4 +- 12 files changed, 1234 insertions(+), 18 deletions(-) create mode 100644 packages/cli/src/commands/review/lib/import-graph.test.ts create mode 100644 packages/cli/src/commands/review/lib/import-graph.ts create mode 100644 packages/cli/src/commands/review/rescope.test.ts create mode 100644 packages/cli/src/commands/review/rescope.ts diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 8da040a788e..1c0139113af 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -45,6 +45,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 e027ae9459d..095ead8f68f 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'; @@ -52,6 +53,7 @@ export const reviewCommand: CommandModule = { .command(fetchPrCommand) .command(captureLocalCommand) .command(planDiffCommand) + .command(rescopeCommand) .command(repoContextCommand) .command(prContextCommand) .command(commentStatusCommand) @@ -78,7 +80,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, match-remote, fetch-pr, capture-local, plan-diff, repo-context, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, cost-ledger, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, match-remote, 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 6eaa45128be..fc01bf98b3f 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -4668,3 +4668,64 @@ describe('the verify gate — compose survives a budget stop', () => { expect(readRecordedPrompts(plan).size).toBe(1); }); }); + +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'] }, + ], + contextFiles: ['src/bystander.ts'], + 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 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', () => { + const mangled = { ...INCREMENTAL_PLAN, incremental: { anchor: 42 } }; + expect(buildChunkAgentPrompt(mangled, 1)).not.toContain('INCREMENTAL'); + }); +}); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index b020bfba9a5..e75cfbf315b 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -136,6 +136,52 @@ interface PlanReport { mergeBaseSha?: unknown; repositoryContext?: unknown; budget?: { agentToolBudget?: 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[] }>; +} + +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') : []; + 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, + ) + .map((e) => ({ + path: e.path, + importsChanged: strings(e.importsChanged), + })) + : []; + return { + anchor: raw.anchor, + deltaFiles: strings(raw.deltaFiles), + interaction, + }; } /** A heavy file's entry, which is the only kind an invariant agent can be built from. */ @@ -521,6 +567,54 @@ 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 \`${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 ` + + `the change under review; 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.`, + ), + ); + } + parts.push(...lines); + } + parts.push( '', 'You may also `read_file` the **full source files** above from the worktree whenever a ' + @@ -823,9 +917,27 @@ 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 \`${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 PR 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.`, + '', + ] + : []), 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.ts b/packages/cli/src/commands/review/fetch-pr.ts index 26c04ae4c27..cab5052307c 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -33,7 +33,14 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { createReviewWorktreeLease } from '../../services/review-worktree-lease.js'; import { ensureAuthenticated, gh, setGhHost } from './lib/gh.js'; import type { ReviewEffort } from './parse-args.js'; -import { git, gitOpt, gitRaw, refExists, releaseWorktree } from './lib/git.js'; +import { + fileLineCount, + git, + gitOpt, + gitRaw, + refExists, + releaseWorktree, +} from './lib/git.js'; import { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from './lib/diff-flags.js'; import { REVIEW_TMP_DIR, @@ -139,20 +146,6 @@ type FetchPrResult = PlanReport & { prDescriptionHasHan: boolean; }; -/** 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 - } -} - /** The real git surface `resolveMergeBase` runs against. */ const gitProbe: GitProbe = { fetch: (remote, ref) => gitOpt('fetch', remote, ref) !== null, diff --git a/packages/cli/src/commands/review/lib/git.ts b/packages/cli/src/commands/review/lib/git.ts index 2f0f2995dfc..a7ee41cdd45 100644 --- a/packages/cli/src/commands/review/lib/git.ts +++ b/packages/cli/src/commands/review/lib/git.ts @@ -201,6 +201,27 @@ 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): 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 + } +} + /** * 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..97a71726f1c --- /dev/null +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -0,0 +1,179 @@ +/** + * @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('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('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', () => { + const pkgs = discoverWorkspacePackages(['pkg/src/a.ts'], (p) => + p === 'pkg/package.json' ? 'not json' : 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..61cd9125d0d --- /dev/null +++ b/packages/cli/src/commands/review/lib/import-graph.ts @@ -0,0 +1,243 @@ +/** + * @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. +// +// 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]> = [ + [/\.js$/, '.ts'], + [/\.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)); + return 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}/`)) { + const sub = spec.slice(pkg.name.length + 1); + 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; try the conventional source root before giving up. + const srcBase = pkg.dir === '' ? `src/${sub}` : `${pkg.dir}/src/${sub}`; + for (const c of candidatesFor(srcBase)) 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/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts new file mode 100644 index 00000000000..a06617ffaec --- /dev/null +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -0,0 +1,249 @@ +/** + * @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 } 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), + }; + 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.contextFiles).toContain(BYSTANDER); + expect(plan.incremental.fullDiffPath).toBe( + '.qwen/tmp/qwen-review-pr-7-diff.txt', + ); + + // The composite diff: changed.ts carries ONLY the round-2 hunk (v2 -> v3, + // not the PR's v1 -> v2), caller.ts carries its full-range hunks, 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 = 2;'); + expect(diff).not.toContain('-export const v = 1;'); + expect(diff).toContain('+export const c = v + 1;'); + expect(diff).not.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' })); + run(planPath, 'HEAD'); + expect(process.exitCode).toBe(RESCOPE_EXIT_FULL_RANGE); + }); + + 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]); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts new file mode 100644 index 00000000000..a65114428d8 --- /dev/null +++ b/packages/cli/src/commands/review/rescope.ts @@ -0,0 +1,349 @@ +/** + * @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 "the interdiff is empty" — the anchor state and the head state +// are identical trees, the same outcome as the same-SHA shortcut. Only exit 0 +// rewrites the plan, atomically. + +import type { CommandModule } from 'yargs'; +import { mkdirSync, readFileSync } from 'node:fs'; +import { 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 { + LITERAL_PATHSPECS, + PINNED_DIFF_CONFIG, + PINNED_DIFF_FLAGS, +} from './lib/diff-flags.js'; +import { + buildDiffPlan, + parseDiff, + 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; + files?: 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` — reviewed on their incremental hunks. */ + 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[] }>; + /** Source files the previous round cleared that this scope leaves out. */ + contextFiles: string[]; + /** 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 { + plan = JSON.parse(readFileSync(args.plan, 'utf8')) 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; + 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; + } + + // 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. + const anchorFull = gitOpt('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('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; + } + + let interdiff: Buffer; + try { + interdiff = gitRaw( + ...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; + } + const delta = new Set(deltaFiles); + + // 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 planFiles = Array.isArray(plan.files) + ? (plan.files as Array<{ + path?: unknown; + kind?: unknown; + binary?: unknown; + }>) + : []; + const candidates = planFiles + .filter( + (f): f is { path: string; kind: string; binary?: boolean } => + !!f && + typeof f.path === 'string' && + f.kind === 'source' && + f.binary !== true && + !delta.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, + ); + + let interactionDiff: Buffer = Buffer.alloc(0); + if (interaction.size > 0) { + try { + interactionDiff = gitRaw( + LITERAL_PATHSPECS, + ...PINNED_DIFF_CONFIG, + 'diff', + ...PINNED_DIFF_FLAGS, + `${mergeBaseSha}..${fetchedSha}`, + '--', + ...interaction.keys(), + ); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: could not capture the interaction files' diff: ` + + `${(err as Error).message}. Continue with the full-range plan.`, + ); + return; + } + } + + const composite = Buffer.concat([interdiff, interactionDiff]); + 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, + interaction: [...interaction.entries()].map(([path, importsChanged]) => ({ + path, + importsChanged, + })), + contextFiles: candidates.filter((p) => !interaction.has(p)), + fullDiffPath: typeof plan.diffPath === 'string' ? plan.diffPath : null, + }; + + // 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), + ...buildPlanReport(diffPlan, (path) => fileLineCount(fetchedSha, path)), + incremental, + }; + + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + atomicWriteFileSync(diffRel, composite); + const out = args.out ?? args.plan; + atomicWriteFileSync(out, stringifyPlanReport(result)); + writeStdoutLine(`Wrote incremental plan to ${out}`); + writeStderrLine( + `Incremental scope since ${anchorFull.slice(0, 12)}: ` + + `${deltaFiles.length} changed file(s), ${interaction.size} ` + + `interaction file(s) (one import hop), ` + + `${incremental.contextFiles.length} clean file(s) left out of scope; ` + + `${diffPlan.diffLines} diff line(s) -> ${diffPlan.chunks.length} chunk(s).`, + ); + warnOnReportSize(out, READ_FILE_CHAR_CAP); +} + +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 931655656dc..9f7a37318c6 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -460,6 +460,12 @@ 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. + +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 53e8fa46844..86086b5eef9 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -143,7 +143,7 @@ Based on the parsed `target.type`: Worktree isolation: all subsequent steps (agents, build/test) operate inside `worktreePath`, not the user's working tree. Cache and reports (Step 8) are written to the **main project directory**, not the worktree. - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-.json` exists, read it **in the same response as the fetch report** — both are `read_file`, genuinely parallel — for `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): - - If SHAs differ **and** model matches → continue with the worktree just created. Compute the incremental diff (`git diff ..HEAD` inside the worktree) and use as the review scope; if the cached commit was rebased away, fall back to the full diff and log a warning. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. + - If SHAs differ **and** model matches → continue with the worktree just created, and rescope the plan to the incremental range: run `"${QWEN_CODE_CLI:-qwen}" review rescope --plan --anchor `. **Do not compute the interdiff by hand and do not re-run `plan-diff` over it** — that produces a plan with no `worktreePath`, no PR identity and no heaviness, which silently drops Agent 0, the modeled-system lens and every invariant agent from the roster. `rescope` rewrites the plan file in place with the same builders `fetch-pr` used: the diff becomes `..HEAD` **widened by one import hop** (every still-clean source file that imports a changed file re-enters the scope, and its chunk brief directs the agent at the interaction seam instead of a from-scratch re-review), identity fields ride through, and the plan gains an `incremental` block naming each file's class. Every later step reads the rewritten plan with no other change. Branch on its exit code: **0** — continue, the plan is now incremental; **3** — the tree at HEAD is identical to the anchor's (empty interdiff): treat exactly as the SHAs-match outcomes below; **any other exit** — it validated the anchor itself and refused (rebased away, not this history's, unreadable plan): continue with the **full-range** plan it left untouched, and repeat its stderr line to the user. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. - If SHAs differ **but** model differs → continue with the worktree, but the scope is the **full diff**, never `..HEAD`: "clean up to `lastCommitSha`" is {cached_model}'s verdict, and an incremental range scoped to another model's anchor leaves everything before it reviewed by no run of `{{model}}` — permanently, because this round's own cache write would re-anchor past it. Inform: "Previous round was reviewed by {cached_model}. Running full review with {{model}}." Still read the cache's `findings` ledger and owe each entry its Step 6 ruling — the work list carries across models (every entry is re-asserted against the code); only the anchor does not. - If SHAs match **and** model matches **and** `comment.effective` is false (no `--comment` flag, and `review.comment` not enabled in settings) → inform the user "No new changes since last review", run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-` to remove the worktree just created, and stop. - If SHAs match **and** model matches **but** `comment.effective` is true (the `--comment` flag or the `review.comment` setting) → run the full review anyway. Inform the user: "No new code changes. Running review to post inline comments." @@ -758,7 +758,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 _diff_ reviewed is `lastCommitSha..HEAD` (plus the one-hop interaction files `rescope` widened it with), but a ledger ruling reads the code at HEAD, which every agent already has. ### Before an Approve or a zero-Critical verdict: re-check the open Criticals From 6dfe7c09bf84b03dfea18bf96ea18a3a867cb361 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 15 Aug 2026 09:07:22 +0800 Subject: [PATCH 2/8] fix(review): harden rescope and the widening against review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the PR's own review rounds, each verified before fixing: - Scoped files now carry FULL-RANGE hunks; the interdiff only chooses which files are in scope. Since-anchor hunks broke inline-comment anchoring: a fix round that restores lines the previous round changed produces hunks that exist nowhere in the PR's own diff, and one such anchor 422s the whole posted review, all-or-nothing. - EXT_MAP maps .js to BOTH .ts and .tsx — under react-jsx a .tsx file emits .js, and 921 of 6,200 relative .js specifiers in this repo named .tsx targets no edge could reach. Root-escape guard is segment-exact (a '..config' directory is not an escape), and the documented dist/ deep-import remap now actually strips the dist/ segment. - rescope refuses an already-rescoped plan (a second pass derived candidates from the shrunk file list and repointed fullDiffPath at the file it was about to overwrite) and a plan with missing or malformed files[] (normalising to [] silently dropped every widening candidate). All git calls are pinned with -C to the plan's worktree: pathspecs resolve against git's cwd, and from a subdirectory an unmatched pathspec exits 0 with empty output instead of failing. - incrementalScopeOf honours its degrade contract: interaction entries whose edges failed validation are dropped, and a block with no surviving scope renders no incremental frame at all. - Whole-diff briefs name each file with its scope class (capped list); chunk briefs state that scope classes override the generic duties for interaction files; heavy INTERACTION files get no invariant agents — their full-range slice is exactly the code the previous round cleared. - incremental.contextFiles (23 KB measured on a 300-file plan, with no reader) is now a count; fullDiffPath is named in the skill prose. SKILL.md states rescope runs from the main checkout, not the worktree. - Test batch from the mutation findings: exit-code literals pinned, --out exercised, diffPathAbsolute asserted, one-hop limit gated, same-sha refusal byte-compared, heaviness preservation asserted, test-file dependents excluded, cross-package widening exercised, fileLineCount covered at the git layer. --- .../src/commands/review/agent-prompt.test.ts | 59 ++++- .../cli/src/commands/review/agent-prompt.ts | 66 +++++- .../review/lib/git.integration.test.ts | 39 +++- .../commands/review/lib/import-graph.test.ts | 74 ++++++- .../src/commands/review/lib/import-graph.ts | 23 +- .../src/commands/review/lib/roster.test.ts | 30 +++ .../cli/src/commands/review/lib/roster.ts | 28 +++ .../cli/src/commands/review/rescope.test.ts | 205 +++++++++++++++++- packages/cli/src/commands/review/rescope.ts | 134 +++++++++--- .../core/src/skills/bundled/review/DESIGN.md | 2 + .../core/src/skills/bundled/review/SKILL.md | 4 +- 11 files changed, 605 insertions(+), 59 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index fc01bf98b3f..8d1fb6816ec 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -4694,7 +4694,7 @@ describe('incremental-scope briefs', () => { interaction: [ { path: 'src/caller.ts', importsChanged: ['src/changed.ts'] }, ], - contextFiles: ['src/bystander.ts'], + contextFileCount: 1, fullDiffPath: '.qwen/tmp/qwen-review-pr-7-diff.txt', }, }; @@ -4724,8 +4724,59 @@ describe('incremental-scope briefs', () => { expect(buildRoleBrief(PLAN, '2')).not.toContain('Incremental round'); }); - it('a malformed incremental block degrades to full-scope briefs', () => { - const mangled = { ...INCREMENTAL_PLAN, incremental: { anchor: 42 } }; - expect(buildChunkAgentPrompt(mangled, 1)).not.toContain('INCREMENTAL'); + 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('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 e75cfbf315b..784f43bb0b0 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -152,6 +152,38 @@ interface IncrementalScope { 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. + */ +const SCOPE_LIST_CAP = 30; +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 ${e.importsChanged.map(inertPath).join(', ')})`, + )}.`, + ); + } + return out; +} + function incrementalScopeOf(report: PlanReport): IncrementalScope | null { const raw = report.incremental as | { @@ -170,16 +202,25 @@ function incrementalScopeOf(report: PlanReport): IncrementalScope | null { (e): e is { path: string; importsChanged?: unknown } => !!e && typeof (e as { path?: unknown }).path === 'string' && - (e as { path: string }).path.length > 0, + (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: strings(raw.deltaFiles), + deltaFiles, interaction, }; } @@ -594,7 +635,8 @@ export function buildChunkAgentPrompt( ...deltaHere.map( (p) => `- ${inertPath(p)} — **changed since the last round**: its hunks here are ` + - `the change under review; review them in full, as usual.`, + `its full change against the PR base (the previous round's clean verdict ` + + `no longer covers this file); review them in full, as usual.`, ), ); } @@ -611,6 +653,17 @@ export function buildChunkAgentPrompt( `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.`, + ); } parts.push(...lines); } @@ -936,6 +989,13 @@ function diffReadingBlock( `A defect in absent code is reportable only when a change IN this diff is ` + `what makes it wrong now.`, '', + // 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), + '', ] : []), scoped 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 8beadb847f7..8e9ba305435 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,11 @@ import { existsSync, writeFileSync, mkdirSync, + realpathSync, } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { gitRawTolerateDiff, releaseWorktree } from './git.js'; +import { fileLineCount, gitRawTolerateDiff, releaseWorktree } from './git.js'; import { NULL_DEVICE } from './diff-flags.js'; import { isolateHostGitConfig } from './test-utils.js'; @@ -187,3 +188,39 @@ describe('gitRawTolerateDiff', () => { ).toThrow(); }); }); + +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 }); + } + }); +}); diff --git a/packages/cli/src/commands/review/lib/import-graph.test.ts b/packages/cli/src/commands/review/lib/import-graph.test.ts index 97a71726f1c..32138eb5a74 100644 --- a/packages/cli/src/commands/review/lib/import-graph.test.ts +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -107,6 +107,54 @@ describe('resolveSpecifier', () => { ).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 .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('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('returns null outside membership, above the root, and for unknown packages', () => { expect( resolveSpecifier('packages/cli/src/z.ts', './missing', files), @@ -140,6 +188,18 @@ describe('dependentsOfChanged', () => { 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']), @@ -171,9 +231,15 @@ describe('discoverWorkspacePackages', () => { }); it('is fail-quiet on malformed or nameless manifests', () => { - const pkgs = discoverWorkspacePackages(['pkg/src/a.ts'], (p) => - p === 'pkg/package.json' ? 'not json' : null, - ); - expect(pkgs).toEqual([]); + 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 index 61cd9125d0d..53974e1567d 100644 --- a/packages/cli/src/commands/review/lib/import-graph.ts +++ b/packages/cli/src/commands/review/lib/import-graph.ts @@ -29,6 +29,11 @@ // - 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. Every +// one of these is a missed edge, never a wrong edge: the file simply keeps +// the pre-widening floor. // // 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. @@ -76,7 +81,13 @@ export function scanImportSpecifiers(source: string): string[] { * 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'], [/\.jsx$/, '.tsx'], [/\.mjs$/, '.mts'], [/\.cjs$/, '.cts'], @@ -99,7 +110,9 @@ function candidatesFor(base: string): string[] { /** 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)); - return joined.startsWith('..') ? null : joined; + // 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; } /** @@ -146,8 +159,12 @@ export function resolveSpecifier( 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; try the conventional source root before giving up. - const srcBase = pkg.dir === '' ? `src/${sub}` : `${pkg.dir}/src/${sub}`; + // output; strip that segment and try the conventional source root. + // Without the strip the remap produced `/src/dist/…`, which + // matches no source path — the branch's one stated purpose was dead. + const srcSub = sub.startsWith('dist/') ? sub.slice('dist/'.length) : sub; + const srcBase = + pkg.dir === '' ? `src/${srcSub}` : `${pkg.dir}/src/${srcSub}`; for (const c of candidatesFor(srcBase)) if (membership.has(c)) return c; return null; } diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index d4c3e892ea6..e260f7e341a 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -412,6 +412,36 @@ 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'); + const mangled = { ...base, incremental: { interaction: 'nope' } }; + expect(keys(mangled 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 3e78adb5794..aee52320296 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -159,6 +159,26 @@ export function hasExecutableScript(plan: RosterPlan): boolean { } /** Source files rewritten heavily enough that the diff is the wrong frame. */ +/** + * 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 as { incremental?: unknown }).incremental as + | { interaction?: unknown } + | null + | undefined; + const out = new Set(); + if (raw && Array.isArray(raw.interaction)) { + for (const e of raw.interaction) { + const path = (e as { path?: unknown } | null)?.path; + if (typeof path === 'string' && path.length > 0) out.add(path); + } + } + return out; +} + function heavyFiles(plan: RosterPlan): string[] { const files = Array.isArray(plan.files) ? plan.files : []; return files @@ -267,8 +287,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 index a06617ffaec..26b225cc8e5 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -163,20 +163,30 @@ describe('rescope', () => { expect(plan.incremental.interaction).toEqual([ { path: CALLER, importsChanged: [CHANGED] }, ]); - expect(plan.incremental.contextFiles).toContain(BYSTANDER); + expect(plan.incremental.contextFileCount).toBe(1); expect(plan.incremental.fullDiffPath).toBe( '.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: changed.ts carries ONLY the round-2 hunk (v2 -> v3, - // not the PR's v1 -> v2), caller.ts carries its full-range hunks, and the - // bystander appears nowhere. + // 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 = 2;'); - expect(diff).not.toContain('-export const v = 1;'); + 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(join(repo, 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. @@ -247,3 +257,186 @@ describe('rescope', () => { 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. + expect(plan.incremental.contextFileCount).toBeGreaterThan(0); + }); + + 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.replace('a0 = 2', 'a0 = 3')); + 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(600); + 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); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts index a65114428d8..343156166b5 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -79,6 +79,7 @@ interface FetchedPlan { mergeBaseSha?: unknown; diffPath?: unknown; files?: unknown; + incremental?: unknown; } /** @@ -88,15 +89,28 @@ interface FetchedPlan { export interface IncrementalScope { /** Full sha of the previous clean round's head — the range's left side. */ anchor: string; - /** Files changed in `anchor..head` — reviewed on their incremental hunks. */ + /** + * 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[] }>; - /** Source files the previous round cleared that this scope leaves out. */ - contextFiles: 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; } @@ -136,11 +150,45 @@ function runRescope(args: RescopeArgs): void { ); 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; + } // 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. - const anchorFull = gitOpt('rev-parse', `${args.anchor}^{commit}`); + // 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, @@ -158,7 +206,16 @@ function runRescope(args: RescopeArgs): void { ); return; } - if (gitOpt('merge-base', '--is-ancestor', anchorFull, fetchedSha) === null) { + 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 ` + @@ -168,9 +225,14 @@ function runRescope(args: RescopeArgs): void { 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, @@ -201,13 +263,11 @@ function runRescope(args: RescopeArgs): void { // 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 planFiles = Array.isArray(plan.files) - ? (plan.files as Array<{ - path?: unknown; - kind?: unknown; - binary?: unknown; - }>) - : []; + const planFiles = planFilesRaw as Array<{ + path?: unknown; + kind?: unknown; + binary?: unknown; + }>; const candidates = planFiles .filter( (f): f is { path: string; kind: string; binary?: boolean } => @@ -236,29 +296,31 @@ function runRescope(args: RescopeArgs): void { packages, ); - let interactionDiff: Buffer = Buffer.alloc(0); - if (interaction.size > 0) { - try { - interactionDiff = gitRaw( - LITERAL_PATHSPECS, - ...PINNED_DIFF_CONFIG, - 'diff', - ...PINNED_DIFF_FLAGS, - `${mergeBaseSha}..${fetchedSha}`, - '--', - ...interaction.keys(), - ); - } catch (err) { - fail( - RESCOPE_EXIT_FULL_RANGE, - `rescope: could not capture the interaction files' diff: ` + - `${(err as Error).message}. Continue with the full-range plan.`, - ); - return; - } + // ONE capture, full-range, scoped to delta ∪ interaction: every hunk in + // the composite is a hunk of the PR's own diff, so downstream comment + // anchoring can never produce a line GitHub refuses. + let composite: Buffer; + try { + composite = gitRaw( + '-C', + worktreePath, + LITERAL_PATHSPECS, + ...PINNED_DIFF_CONFIG, + 'diff', + ...PINNED_DIFF_FLAGS, + `${mergeBaseSha}..${fetchedSha}`, + '--', + ...deltaFiles, + ...interaction.keys(), + ); + } catch (err) { + fail( + RESCOPE_EXIT_FULL_RANGE, + `rescope: could not capture the scoped files' diff: ` + + `${(err as Error).message}. Continue with the full-range plan.`, + ); + return; } - - const composite = Buffer.concat([interdiff, interactionDiff]); let diffPlan; try { diffPlan = buildDiffPlan(composite.toString('utf8'), args.maxChunkLines); @@ -283,7 +345,7 @@ function runRescope(args: RescopeArgs): void { path, importsChanged, })), - contextFiles: candidates.filter((p) => !interaction.has(p)), + contextFileCount: candidates.filter((p) => !interaction.has(p)).length, fullDiffPath: typeof plan.diffPath === 'string' ? plan.diffPath : null, }; @@ -312,7 +374,7 @@ function runRescope(args: RescopeArgs): void { `Incremental scope since ${anchorFull.slice(0, 12)}: ` + `${deltaFiles.length} changed file(s), ${interaction.size} ` + `interaction file(s) (one import hop), ` + - `${incremental.contextFiles.length} clean file(s) left out of scope; ` + + `${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); diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 9f7a37318c6..0ab87140f51 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -464,6 +464,8 @@ Two consequences of those boundaries are worth naming rather than discovering. I 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 diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 86086b5eef9..f5bab59f94d 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -143,7 +143,7 @@ Based on the parsed `target.type`: Worktree isolation: all subsequent steps (agents, build/test) operate inside `worktreePath`, not the user's working tree. Cache and reports (Step 8) are written to the **main project directory**, not the worktree. - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): if `.qwen/review-cache/pr-.json` exists, read it **in the same response as the fetch report** — both are `read_file`, genuinely parallel — for `lastCommitSha` and `lastModelId`. Compare to `fetchedSha` from the fetch report and the current model ID (`{{model}}`): - - If SHAs differ **and** model matches → continue with the worktree just created, and rescope the plan to the incremental range: run `"${QWEN_CODE_CLI:-qwen}" review rescope --plan --anchor `. **Do not compute the interdiff by hand and do not re-run `plan-diff` over it** — that produces a plan with no `worktreePath`, no PR identity and no heaviness, which silently drops Agent 0, the modeled-system lens and every invariant agent from the roster. `rescope` rewrites the plan file in place with the same builders `fetch-pr` used: the diff becomes `..HEAD` **widened by one import hop** (every still-clean source file that imports a changed file re-enters the scope, and its chunk brief directs the agent at the interaction seam instead of a from-scratch re-review), identity fields ride through, and the plan gains an `incremental` block naming each file's class. Every later step reads the rewritten plan with no other change. Branch on its exit code: **0** — continue, the plan is now incremental; **3** — the tree at HEAD is identical to the anchor's (empty interdiff): treat exactly as the SHAs-match outcomes below; **any other exit** — it validated the anchor itself and refused (rebased away, not this history's, unreadable plan): continue with the **full-range** plan it left untouched, and repeat its stderr line to the user. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. + - If SHAs differ **and** model matches → continue with the worktree just created, and rescope the plan to the incremental range: run `"${QWEN_CODE_CLI:-qwen}" review rescope --plan --anchor ` **from the main checkout — the directory `fetch-pr` just ran in, NOT the worktree**: the plan path is relative to it, and the command pins its own git calls to the plan's `worktreePath`, so the worktree cwd rule for review agents does not apply to this call. **Do not compute the interdiff by hand and do not re-run `plan-diff` over it** — that produces a plan with no `worktreePath`, no PR identity and no heaviness, which silently drops Agent 0, the modeled-system lens and every invariant agent from the roster. `rescope` rewrites the plan file in place with the same builders `fetch-pr` used: the interdiff since `` decides **which files** are in scope, **widened by one import hop** (every still-clean source file that imports a changed file re-enters, and its chunk brief directs the agent at the interaction seam instead of a from-scratch re-review), and every scoped file carries its **full-range hunks** — so any inline-comment anchor an agent produces exists in the PR's own diff. Identity fields ride through, the plan gains an `incremental` block naming each file's class, and the superseded full diff stays on disk at `incremental.fullDiffPath` for any later step that needs the whole PR. Every later step reads the rewritten plan with no other change. Branch on its exit code: **0** — continue, the plan is now incremental; **3** — the tree at HEAD is identical to the anchor's (empty interdiff): treat exactly as the SHAs-match outcomes below; **any other exit** — it validated the anchor itself and refused (rebased away, not this history's, unreadable plan): continue with the **full-range** plan it left untouched, and repeat its stderr line to the user. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. - If SHAs differ **but** model differs → continue with the worktree, but the scope is the **full diff**, never `..HEAD`: "clean up to `lastCommitSha`" is {cached_model}'s verdict, and an incremental range scoped to another model's anchor leaves everything before it reviewed by no run of `{{model}}` — permanently, because this round's own cache write would re-anchor past it. Inform: "Previous round was reviewed by {cached_model}. Running full review with {{model}}." Still read the cache's `findings` ledger and owe each entry its Step 6 ruling — the work list carries across models (every entry is re-asserted against the code); only the anchor does not. - If SHAs match **and** model matches **and** `comment.effective` is false (no `--comment` flag, and `review.comment` not enabled in settings) → inform the user "No new changes since last review", run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-` to remove the worktree just created, and stop. - If SHAs match **and** model matches **but** `comment.effective` is true (the `--comment` flag or the `review.comment` setting) → run the full review anyway. Inform the user: "No new code changes. Running review to post inline comments." @@ -758,7 +758,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` (plus the one-hop interaction files `rescope` widened it with), 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. ### Before an Approve or a zero-Critical verdict: re-check the open Criticals From 2f59b09ab76ab424db6f624a6534e55d51a3eb85 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 15 Aug 2026 12:54:18 +0800 Subject: [PATCH 3/8] =?UTF-8?q?fix(review):=20round-2=20findings=20?= =?UTF-8?q?=E2=80=94=20slice=20the=20fetched=20diff,=20cap=20and=20reconci?= =?UTF-8?q?le=20the=20frames?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review findings, each reproduced before fixing: - The composite is now a BYTE-SLICE of the fetched full-range diff, not a pathspec-scoped re-capture: a scoped re-capture cannot see a rename source, un-pairs the rename, and renders a whole-file add whose hunks exist nowhere in the PR's own diff — the second entrance of the same 422 anchor class the round-1 redesign closed. Slicing also keeps the subset invariant byte-exact. sliceDiffByLines moves to lib/diff-plan. - deltaFiles is reconciled with the sections the composite actually holds (a file restored to its merge-base state names no phantom scope; its importers still widen), a files[] whose entries carry no usable path refuses like an empty one (zero-compared must never read as nothing-changed), and an unwritable --out exits 2 instead of throwing. - The whole-diff frame carries the same scope-class-WINS reconciliation as chunk briefs (agents 1a/1b sweep duties re-opened round-1 findings over interaction hunks), scope lists cap edges per entry (8) as well as entries (30), anchors render inertly, empty-string edges degrade, a chunk with no classed files gets no frame, and the frame wording is flow-neutral (review's base, not PR base). - Roster: interaction paths subtract deltaFiles (a path in both lists is live delta — widening wins), the field is declared on RosterPlan, and heavyFiles' doc is re-attached. - import-graph: dist deep-imports resolve under BOTH emit layouts (dist/src/… and flat dist/…), and the header now states the honest wrong-edge cost of unparsed exports maps (one extra widened file, never a narrowed scope). - Tests: rename-preserving slice, restored-file reconciliation, empty and zero-usable files[], out-of-worktree cwd run, unwritable --out, exact contextFileCount, head-distinct heaviness oracle, .cjs resolution, both dist layouts, list/edge caps, both-lists roster widening, no-frame-for-unclassed-chunks. --- .../src/commands/review/agent-prompt.test.ts | 57 +++++++++ .../cli/src/commands/review/agent-prompt.ts | 36 ++++-- .../cli/src/commands/review/lib/diff-plan.ts | 30 +++++ .../commands/review/lib/import-graph.test.ts | 28 +++++ .../src/commands/review/lib/import-graph.ts | 25 ++-- .../src/commands/review/lib/roster.test.ts | 13 ++ .../cli/src/commands/review/lib/roster.ts | 27 ++-- .../cli/src/commands/review/rescope.test.ts | 113 ++++++++++++++++- packages/cli/src/commands/review/rescope.ts | 117 +++++++++++++----- 9 files changed, 381 insertions(+), 65 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 8d1fb6816ec..1ff48e243de 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -4772,6 +4772,63 @@ describe('incremental-scope briefs', () => { 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( diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 784f43bb0b0..66ad7cc8570 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -158,6 +158,14 @@ interface IncrementalScope { * the plan's own `incremental` block remains the complete record. */ const SCOPE_LIST_CAP = 30; +/** Edge lists are capped per entry too — the entry cap alone still let one + * interaction row carry hundreds of imports into every brief. */ +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); @@ -177,7 +185,7 @@ function scopeFileLists(incremental: IncrementalScope): string[] { `imports): ${cap( incremental.interaction, (e) => - `${inertPath(e.path)} (imports ${e.importsChanged.map(inertPath).join(', ')})`, + `${inertPath(e.path)} (imports ${cappedEdges(e.importsChanged)})`, )}.`, ); } @@ -195,7 +203,9 @@ function incrementalScopeOf(report: PlanReport): IncrementalScope | null { | 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') : []; + Array.isArray(v) + ? v.filter((s): s is string => typeof s === 'string' && s.length > 0) + : []; const interaction = Array.isArray(raw.interaction) ? raw.interaction .filter( @@ -627,7 +637,7 @@ export function buildChunkAgentPrompt( const lines = [ '', `**This is an INCREMENTAL round** — the diff holds only what changed since the ` + - `previous clean review round (anchor \`${incremental.anchor.slice(0, 12)}\`), ` + + `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) { @@ -635,7 +645,7 @@ export function buildChunkAgentPrompt( ...deltaHere.map( (p) => `- ${inertPath(p)} — **changed since the last round**: its hunks here are ` + - `its full change against the PR base (the previous round's clean verdict ` + + `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.`, ), ); @@ -645,7 +655,7 @@ export function buildChunkAgentPrompt( ...seamHere.map( (e) => `- ${inertPath(e.path)} — **unchanged, cleared by the previous round**, back in ` + - `scope because it imports ${e.importsChanged.map(inertPath).join(', ')}, which ` + + `scope because it imports ${cappedEdges(e.importsChanged)}, 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 ` + @@ -665,7 +675,10 @@ export function buildChunkAgentPrompt( `applies only to its interaction surface with what changed.`, ); } - parts.push(...lines); + // 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( @@ -982,12 +995,17 @@ function diffReadingBlock( ...(incremental ? [ `**Incremental round.** This diff is scoped to what changed since the previous ` + - `clean review round (anchor \`${incremental.anchor.slice(0, 12)}\`), plus ` + + `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 PR was ` + + `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.`, + `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 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/import-graph.test.ts b/packages/cli/src/commands/review/lib/import-graph.test.ts index 32138eb5a74..a5583157d17 100644 --- a/packages/cli/src/commands/review/lib/import-graph.test.ts +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -118,6 +118,34 @@ describe('resolveSpecifier', () => { ).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: '' }]; diff --git a/packages/cli/src/commands/review/lib/import-graph.ts b/packages/cli/src/commands/review/lib/import-graph.ts index 53974e1567d..701125d4af4 100644 --- a/packages/cli/src/commands/review/lib/import-graph.ts +++ b/packages/cli/src/commands/review/lib/import-graph.ts @@ -31,9 +31,11 @@ // 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. Every -// one of these is a missed edge, never a wrong edge: the file simply keeps -// the pre-widening floor. +// 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. @@ -159,13 +161,18 @@ export function resolveSpecifier( 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; strip that segment and try the conventional source root. - // Without the strip the remap produced `/src/dist/…`, which - // matches no source path — the branch's one stated purpose was dead. + // 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; - const srcBase = - pkg.dir === '' ? `src/${srcSub}` : `${pkg.dir}/src/${srcSub}`; - for (const c of candidatesFor(srcBase)) if (membership.has(c)) return c; + 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; } } diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index e260f7e341a..70ffc249387 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -441,6 +441,19 @@ describe('requiredAgents — Step 3B', () => { expect(k).not.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'); }); }); diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index aee52320296..54238042313 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -63,6 +63,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 @@ -159,14 +161,21 @@ export function hasExecutableScript(plan: RosterPlan): boolean { } /** Source files rewritten heavily enough that the diff is the wrong frame. */ +function heavyFiles(plan: RosterPlan): string[] { + const files = Array.isArray(plan.files) ? plan.files : []; + return files + .filter((f) => f?.heavy === true && typeof f.path === '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 as { incremental?: unknown }).incremental as - | { interaction?: unknown } + const raw = plan.incremental as + | { interaction?: unknown; deltaFiles?: unknown } | null | undefined; const out = new Set(); @@ -175,17 +184,17 @@ function incrementalInteractionPaths(plan: RosterPlan): Set { const path = (e as { path?: unknown } | null)?.path; if (typeof path === 'string' && path.length > 0) out.add(path); } + // A malformed block naming one path in BOTH lists must widen (the delta + // classification wins — its change is live), never narrow. + if (Array.isArray(raw.deltaFiles)) { + for (const p of raw.deltaFiles) { + if (typeof p === 'string') out.delete(p); + } + } } return out; } -function heavyFiles(plan: RosterPlan): string[] { - const files = Array.isArray(plan.files) ? plan.files : []; - return files - .filter((f) => f?.heavy === true && typeof f.path === 'string') - .map((f) => f.path as string); -} - /** * Every agent this plan requires, and the key each one's prompt is recorded under. * diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts index 26b225cc8e5..faf482ee1ed 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -237,8 +237,10 @@ describe('rescope', () => { 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', () => { @@ -315,8 +317,10 @@ describe('rescope — contract pins from review findings', () => { 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. - expect(plan.incremental.contextFileCount).toBeGreaterThan(0); + // 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', () => { @@ -386,7 +390,7 @@ describe('rescope — contract pins from review findings', () => { 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.replace('a0 = 2', 'a0 = 3')); + 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'); @@ -404,7 +408,7 @@ describe('rescope — contract pins from review findings', () => { ).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(600); + expect(big.fileLines).toBe(601); // the HEAD count, not the base or anchor expect(big.heavy).toBe(true); expect(big.addedRanges).toBeDefined(); }); @@ -440,3 +444,104 @@ describe('rescope — contract pins from review findings', () => { 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); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts index 343156166b5..c21a14d2f33 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -34,19 +34,16 @@ import type { CommandModule } from 'yargs'; import { mkdirSync, readFileSync } from 'node:fs'; -import { join, resolve } from 'node:path'; +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 { - LITERAL_PATHSPECS, - PINNED_DIFF_CONFIG, - PINNED_DIFF_FLAGS, -} from './lib/diff-flags.js'; +import { 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'; @@ -78,6 +75,7 @@ interface FetchedPlan { fetchedSha?: unknown; mergeBaseSha?: unknown; diffPath?: unknown; + diffPathAbsolute?: unknown; files?: unknown; incremental?: unknown; } @@ -176,6 +174,25 @@ function runRescope(args: RescopeArgs): void { ); 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 @@ -263,11 +280,6 @@ function runRescope(args: RescopeArgs): void { // 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 planFiles = planFilesRaw as Array<{ - path?: unknown; - kind?: unknown; - binary?: unknown; - }>; const candidates = planFiles .filter( (f): f is { path: string; kind: string; binary?: boolean } => @@ -296,31 +308,58 @@ function runRescope(args: RescopeArgs): void { packages, ); - // ONE capture, full-range, scoped to delta ∪ interaction: every hunk in - // the composite is a hunk of the PR's own diff, so downstream comment - // anchoring can never produce a line GitHub refuses. - let composite: Buffer; + // 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 { - composite = gitRaw( - '-C', - worktreePath, - LITERAL_PATHSPECS, - ...PINNED_DIFF_CONFIG, - 'diff', - ...PINNED_DIFF_FLAGS, - `${mergeBaseSha}..${fetchedSha}`, - '--', - ...deltaFiles, - ...interaction.keys(), - ); + if (origDiffPath === null) throw new Error('the plan names no diff file'); + origDiff = readFileSync(origDiffPath); } catch (err) { fail( RESCOPE_EXIT_FULL_RANGE, - `rescope: could not capture the scoped files' diff: ` + - `${(err as Error).message}. Continue with the full-range plan.`, + `rescope: cannot read the fetched diff (${(err as Error).message}). ` + + `Continue with the full-range plan.`, + ); + return; + } + const scoped = new Set([...delta, ...interaction.keys()]); + const sections = parseDiff(origDiff.toString('utf8')).files.filter((f) => + scoped.has(f.path), + ); + if (sections.length === 0) { + // Changed since the anchor, but present in no section of the PR's own + // diff — every delta file was restored to its merge-base state. There + // is nothing of the PR 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 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)); + const deltaReported = deltaFiles.filter((p) => sectionPaths.has(p)); let diffPlan; try { diffPlan = buildDiffPlan(composite.toString('utf8'), args.maxChunkLines); @@ -340,7 +379,7 @@ function runRescope(args: RescopeArgs): void { const diffRel = tmpFile(target, 'diff-incremental.txt'); const incremental: IncrementalScope = { anchor: anchorFull, - deltaFiles, + deltaFiles: deltaReported, interaction: [...interaction.entries()].map(([path, importsChanged]) => ({ path, importsChanged, @@ -365,14 +404,24 @@ function runRescope(args: RescopeArgs): void { incremental, }; - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); - atomicWriteFileSync(diffRel, composite); const out = args.out ?? args.plan; - atomicWriteFileSync(out, stringifyPlanReport(result)); + 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; + } writeStdoutLine(`Wrote incremental plan to ${out}`); writeStderrLine( `Incremental scope since ${anchorFull.slice(0, 12)}: ` + - `${deltaFiles.length} changed file(s), ${interaction.size} ` + + `${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).`, From c61499d67323da63fcf5de51c23645138073fc0e Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 15 Aug 2026 18:50:30 +0800 Subject: [PATCH 4/8] =?UTF-8?q?fix(review):=20round-3=20findings=20?= =?UTF-8?q?=E2=80=94=20follow=20the=20lineage,=20absolute=20full-diff=20pa?= =?UTF-8?q?th?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review findings on the rescope layer: - R3-1 (Critical): a file renamed BEFORE the anchor and deleted in the fix round carries two names — the post-image name in the interdiff, the left-side name on the PR diff's deletion section — so the section holding its unreviewed hunks matched no scoped name and silently vanished (or exited 3 as 'nothing new'). An unmatched delta file is now dropped only when a cheap per-file probe proves it a genuine RESTORATION (identical blobs on both sides of the PR range); any other lineage break refuses to the full range, and the check runs before the empty-sections exit so the refusal wins. - incremental.fullDiffPath is absolute: a cwd-relative path is meaningless to the later step the field exists for (R3-9), and the exit-3 contract in the header now names both of its causes (R3-8). - The roster's interaction-path reader applies the same validation the brief renderer does — anchor present, every entry carrying a surviving edge — and a malformed deltaFiles disables the narrowing entirely rather than just its delta-wins subtraction: with no trustworthy delta list there is no way to tell a seam-only file from a live one, and every malformation here must widen (R3-2, R3-10). - Tests: rename-then-delete refusal, restored-only exit 3, unwritable --out leaves the plan byte-identical, sliceDiffByLines gets a direct suite (parse → slice → parse round-trip, byte-exactness over invalid UTF-8 and lone CR, range ordering and clamping), and the resolver's literal-form candidate is pinned. --- .../review/lib/diff-plan.slice.test.ts | 66 +++++++++++++++++ .../commands/review/lib/import-graph.test.ts | 9 +++ .../cli/src/commands/review/lib/roster.ts | 40 +++++++---- .../cli/src/commands/review/rescope.test.ts | 70 +++++++++++++++++-- packages/cli/src/commands/review/rescope.ts | 61 ++++++++++++---- 5 files changed, 216 insertions(+), 30 deletions(-) create mode 100644 packages/cli/src/commands/review/lib/diff-plan.slice.test.ts 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/import-graph.test.ts b/packages/cli/src/commands/review/lib/import-graph.test.ts index a5583157d17..5d584ec7865 100644 --- a/packages/cli/src/commands/review/lib/import-graph.test.ts +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -183,6 +183,15 @@ describe('resolveSpecifier', () => { ).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), diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index 54238042313..eccaafa55b0 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -179,18 +179,34 @@ function incrementalInteractionPaths(plan: RosterPlan): Set { | null | undefined; const out = new Set(); - if (raw && Array.isArray(raw.interaction)) { - for (const e of raw.interaction) { - const path = (e as { path?: unknown } | null)?.path; - if (typeof path === 'string' && path.length > 0) out.add(path); - } - // A malformed block naming one path in BOTH lists must widen (the delta - // classification wins — its change is live), never narrow. - if (Array.isArray(raw.deltaFiles)) { - for (const p of raw.deltaFiles) { - if (typeof p === 'string') out.delete(p); - } - } + // 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)) 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; } diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts index faf482ee1ed..9f486be815f 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -164,8 +164,10 @@ describe('rescope', () => { { 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( - '.qwen/tmp/qwen-review-pr-7-diff.txt', + 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'), @@ -184,9 +186,9 @@ describe('rescope', () => { 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(join(repo, plan.incremental.fullDiffPath!), 'utf8'), - ).toContain('bystander'); + 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. @@ -545,3 +547,63 @@ describe('rescope — round-2 findings', () => { 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); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts index c21a14d2f33..84a7355cf9e 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -28,9 +28,11 @@ // 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 "the interdiff is empty" — the anchor state and the head state -// are identical trees, the same outcome as the same-SHA shortcut. Only exit 0 -// rewrites the plan, atomically. +// 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'; @@ -337,6 +339,45 @@ function runRescope(args: RescopeArgs): void { 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)); + const unmatched = deltaFiles.filter((p) => !sectionPaths.has(p)); + // A delta file with no section of the PR's own diff is one of two things, + // and only one of them is safe to drop. A RESTORATION (the fix round put + // the file back to its merge-base content) genuinely has nothing left to + // review. A LINEAGE MISMATCH does not: a file renamed before the anchor + // and deleted in the fix round is `new.ts` in the interdiff but `old.ts` + // in the PR diff (parseDiff labels a deletion with its left-side path), + // so the section carrying its unreviewed hunks is scoped out under a name + // nothing matched. Distinguishing them is one cheap probe per unmatched + // file: identical blobs on both sides of the PR range means restored. + const restored = (p: string): boolean => { + const at = (ref: string) => + gitOpt('-C', worktreePath, 'rev-parse', `${ref}:${p}`); + const b = at(mergeBaseSha); + const h = at(fetchedSha); + return b !== null && h !== null && b === h; + }; + const lineageLost = unmatched.filter((p) => !restored(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) { // Changed since the anchor, but present in no section of the PR's own // diff — every delta file was restored to its merge-base state. There @@ -349,16 +390,6 @@ function runRescope(args: RescopeArgs): void { ); return; } - 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)); const deltaReported = deltaFiles.filter((p) => sectionPaths.has(p)); let diffPlan; try { @@ -385,7 +416,9 @@ function runRescope(args: RescopeArgs): void { importsChanged, })), contextFileCount: candidates.filter((p) => !interaction.has(p)).length, - fullDiffPath: typeof plan.diffPath === 'string' ? plan.diffPath : null, + // 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 From 4aba5e911222bbe5aae7d32e7d53360e8e0738a9 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 03:13:34 +0800 Subject: [PATCH 5/8] =?UTF-8?q?fix(review):=20round-4=20findings=20?= =?UTF-8?q?=E2=80=94=20chunk-scoped=20role=20briefs,=20resolver=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review findings: - R4-1 (Critical): a chunk-scoped ROLE brief (the reverse auditors, the one role accepting a chunk) received per-file scope classes only from the globally capped list, so on a wide round its own files could be elided past entry 30 — the sole reviewer of that territory left without their class and with no way to recover the tail. Its own chunk's files are now listed in full, and the chunk brief's seam bullet drops the display cap for the same reason (R4-2); the cap stays where it belongs, on the whole-diff frame. - The resolver gains the `.jsx` emit row (a JSX source emits `.js` under the same convention as `.tsx`, R4-4) and normalises bare-package subpaths through the same POSIX rules relative specifiers already get, refusing escapes (R4-5). - A plan file that parses to JSON `null` now refuses instead of throwing a TypeError past the catch (R4-7), `fileLineCount` is `-C`-pinned like every other git call in the module (R4-9), and a `deltaFiles` array of non-string junk disables the roster narrowing exactly as a missing list does (R4-11). - Tests: chunk-scoped role brief listing, `.jsx` and subpath normalisation, junk-deltaFiles widening, JSON-null plan refusal. R4-8 declined with rationale, recorded in the code: a file absent at BOTH ends of the PR range is either a net-zero add-then-delete (safe to drop) or a rename-before-anchor whose deletion hunks sit under its pre-rename name (dropping loses them). This layer cannot tell them apart, and dropping re-opens the round-3 Critical, so the refusal stands. --- .../src/commands/review/agent-prompt.test.ts | 25 +++++++++ .../cli/src/commands/review/agent-prompt.ts | 56 ++++++++++++++++++- packages/cli/src/commands/review/lib/git.ts | 10 +++- .../commands/review/lib/import-graph.test.ts | 24 ++++++++ .../src/commands/review/lib/import-graph.ts | 11 +++- .../src/commands/review/lib/roster.test.ts | 14 +++++ .../cli/src/commands/review/lib/roster.ts | 10 +++- .../cli/src/commands/review/rescope.test.ts | 8 +++ packages/cli/src/commands/review/rescope.ts | 26 ++++++++- 9 files changed, 175 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 1ff48e243de..48cfad909e9 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -4719,6 +4719,31 @@ describe('incremental-scope briefs', () => { 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'); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 66ad7cc8570..3a2d04ac3c4 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -157,9 +157,44 @@ interface IncrementalScope { * 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 too — the entry cap alone still let one - * interaction row carry hundreds of imports into every brief. */ +/** 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); @@ -655,7 +690,8 @@ export function buildChunkAgentPrompt( ...seamHere.map( (e) => `- ${inertPath(e.path)} — **unchanged, cleared by the previous round**, back in ` + - `scope because it imports ${cappedEdges(e.importsChanged)}, which ` + + `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 ` + @@ -1016,6 +1052,20 @@ function diffReadingBlock( '', ] : []), + // 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/lib/git.ts b/packages/cli/src/commands/review/lib/git.ts index a7ee41cdd45..cb0f84393ba 100644 --- a/packages/cli/src/commands/review/lib/git.ts +++ b/packages/cli/src/commands/review/lib/git.ts @@ -209,9 +209,15 @@ export function gitRaw(...args: string[]): Buffer { * counters that disagreed would classify the same file heavy in one plan and * not the other. */ -export function fileLineCount(ref: string, path: string): number { +export function fileLineCount( + ref: string, + path: string, + repoRoot?: string, +): number { try { - const buf = gitRaw('show', `${ref}:${path}`); + 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++; diff --git a/packages/cli/src/commands/review/lib/import-graph.test.ts b/packages/cli/src/commands/review/lib/import-graph.test.ts index 5d584ec7865..d45c5d3405c 100644 --- a/packages/cli/src/commands/review/lib/import-graph.test.ts +++ b/packages/cli/src/commands/review/lib/import-graph.test.ts @@ -171,6 +171,30 @@ describe('resolveSpecifier', () => { ).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( diff --git a/packages/cli/src/commands/review/lib/import-graph.ts b/packages/cli/src/commands/review/lib/import-graph.ts index 701125d4af4..1f325878eb7 100644 --- a/packages/cli/src/commands/review/lib/import-graph.ts +++ b/packages/cli/src/commands/review/lib/import-graph.ts @@ -90,6 +90,9 @@ const EXT_MAP: ReadonlyArray<[RegExp, string]> = [ // 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'], @@ -157,7 +160,13 @@ export function resolveSpecifier( return null; } if (spec.startsWith(`${pkg.name}/`)) { - const sub = spec.slice(pkg.name.length + 1); + // 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 diff --git a/packages/cli/src/commands/review/lib/roster.test.ts b/packages/cli/src/commands/review/lib/roster.test.ts index 70ffc249387..8a64599ba6d 100644 --- a/packages/cli/src/commands/review/lib/roster.test.ts +++ b/packages/cli/src/commands/review/lib/roster.test.ts @@ -439,6 +439,20 @@ describe('requiredAgents — Step 3B', () => { 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 diff --git a/packages/cli/src/commands/review/lib/roster.ts b/packages/cli/src/commands/review/lib/roster.ts index eccaafa55b0..1fb76f3df60 100644 --- a/packages/cli/src/commands/review/lib/roster.ts +++ b/packages/cli/src/commands/review/lib/roster.ts @@ -194,7 +194,15 @@ function incrementalInteractionPaths(plan: RosterPlan): Set { // 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)) return out; + 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; diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts index 9f486be815f..388c5686034 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -445,6 +445,14 @@ describe('rescope — contract pins from review findings', () => { 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', () => { diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts index 84a7355cf9e..cc156b45c2e 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -123,7 +123,18 @@ function fail(code: number, message: string): void { function runRescope(args: RescopeArgs): void { let plan: FetchedPlan; try { - plan = JSON.parse(readFileSync(args.plan, 'utf8')) as FetchedPlan; + 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, @@ -364,6 +375,13 @@ function runRescope(args: RescopeArgs): void { gitOpt('-C', worktreePath, 'rev-parse', `${ref}:${p}`); const b = at(mergeBaseSha); const h = at(fetchedSha); + // Absent on BOTH sides is deliberately NOT droppable. Two shapes produce + // it and this layer cannot tell them apart: a file the PR added and this + // round deleted (net-zero — safe to drop), 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 lineageLost = unmatched.filter((p) => !restored(p)); @@ -433,7 +451,11 @@ function runRescope(args: RescopeArgs): void { ...(plan as Record), diffPath: diffRel, diffPathAbsolute: resolve(diffRel), - ...buildPlanReport(diffPlan, (path) => fileLineCount(fetchedSha, path)), + // `-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), + ), incremental, }; From 483976b160bfdbe6bd454bb4fc4d27fa3e78bf3f Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 16:18:00 +0800 Subject: [PATCH 6/8] =?UTF-8?q?fix(review):=20round-5/6=20Criticals=20?= =?UTF-8?q?=E2=80=94=20readers=20for=20restored=20files,=20honest=20exits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the posture announced last round, this lands Criticals only. - R6-10: a delta file the fix round RESTORED to its merge-base state fell between both reader classes — no PR-diff section, so no full review, and inside `delta`, so the widening skipped it as a candidate. Its imports of files that are still changing therefore had zero readers. The restoration probe now runs BEFORE the widening and splits the set: every changed file (restored included) still pulls its importers in, because a revert moves their seam too — round 1 cleared them against the pre-revert callee, and (importer@head x callee@base) is a pairing no round has seen — while the restored files themselves become candidates in a second pass keyed on the LIVE delta, since a restored file importing another restored file has no moving side to check. - R5-14: nothing past the plan write may throw. "Only exit 0 rewrites the plan" needs its contrapositive to hold, and a dead stdout (`qwen … | head`, a daemon redirect) made the courtesy reporting raise EPIPE — exit 1 over an already-rewritten plan, sending the caller down the "full-range plan untouched" branch against an incremental one. - R6-16: `fetchedSha`/`mergeBaseSha` were taken on type-check faith. Both ends of the PR range must be object ids: a clobbered plan naming a moving ref would resolve at call time, so the interdiff describes one tree and the worktree reads another while the exit-0 plan claims incremental scope. Each of the three tests was mutation-checked: reverting the fix it pins turns it red. --- .../cli/src/commands/review/rescope.test.ts | 60 +++++++- packages/cli/src/commands/review/rescope.ts | 137 +++++++++++++----- 2 files changed, 156 insertions(+), 41 deletions(-) diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts index c0cc20e3e30..8cb71df82e3 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -11,7 +11,7 @@ // 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 } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { execFileSync } from 'node:child_process'; import { mkdtempSync, @@ -618,3 +618,61 @@ describe('rescope — round-3 findings', () => { 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]); + }); +}); diff --git a/packages/cli/src/commands/review/rescope.ts b/packages/cli/src/commands/review/rescope.ts index bc7ae581fb2..37a11249a81 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -152,6 +152,24 @@ function runRescope(args: RescopeArgs): void { 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. @@ -289,7 +307,37 @@ function runRescope(args: RescopeArgs): void { ); 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 => { + const at = (ref: string) => + gitOpt('-C', worktreePath, 'rev-parse', `${ref}:${p}`); + 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 @@ -302,7 +350,9 @@ function runRescope(args: RescopeArgs): void { typeof f.path === 'string' && f.kind === 'source' && f.binary !== true && - !delta.has(f.path), + // 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 => { @@ -322,6 +372,20 @@ function runRescope(args: RescopeArgs): void { 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 @@ -348,7 +412,7 @@ function runRescope(args: RescopeArgs): void { ); return; } - const scoped = new Set([...delta, ...interaction.keys()]); + const scoped = new Set([...deltaLive, ...interaction.keys()]); const sections = parseDiff(origDiff.toString('utf8')).files.filter((f) => scoped.has(f.path), ); @@ -362,31 +426,13 @@ function runRescope(args: RescopeArgs): void { // 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)); - const unmatched = deltaFiles.filter((p) => !sectionPaths.has(p)); - // A delta file with no section of the PR's own diff is one of two things, - // and only one of them is safe to drop. A RESTORATION (the fix round put - // the file back to its merge-base content) genuinely has nothing left to - // review. A LINEAGE MISMATCH does not: a file renamed before the anchor - // and deleted in the fix round is `new.ts` in the interdiff but `old.ts` - // in the PR diff (parseDiff labels a deletion with its left-side path), - // so the section carrying its unreviewed hunks is scoped out under a name - // nothing matched. Distinguishing them is one cheap probe per unmatched - // file: identical blobs on both sides of the PR range means restored. - const restored = (p: string): boolean => { - const at = (ref: string) => - gitOpt('-C', worktreePath, 'rev-parse', `${ref}:${p}`); - const b = at(mergeBaseSha); - const h = at(fetchedSha); - // Absent on BOTH sides is deliberately NOT droppable. Two shapes produce - // it and this layer cannot tell them apart: a file the PR added and this - // round deleted (net-zero — safe to drop), 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 lineageLost = unmatched.filter((p) => !restored(p)); + // 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, @@ -399,9 +445,9 @@ function runRescope(args: RescopeArgs): void { return; } if (sections.length === 0) { - // Changed since the anchor, but present in no section of the PR's own - // diff — every delta file was restored to its merge-base state. There - // is nothing of the PR left to re-review. + // 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 ` + @@ -410,7 +456,7 @@ function runRescope(args: RescopeArgs): void { ); return; } - const deltaReported = deltaFiles.filter((p) => sectionPaths.has(p)); + const deltaReported = [...deltaLive].filter((p) => sectionPaths.has(p)); let diffPlan; try { diffPlan = buildDiffPlan(composite.toString('utf8'), args.maxChunkLines); @@ -480,15 +526,26 @@ function runRescope(args: RescopeArgs): void { ); return; } - 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); + // 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 = { From 83a1a6660e4371a0978e8082df5a6f652d46190d Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 16 Aug 2026 19:28:57 +0800 Subject: [PATCH 7/8] =?UTF-8?q?fix(review):=20round-6/7=20Criticals=20?= =?UTF-8?q?=E2=80=94=20whole=20tree=20entries,=20and=20an=20async-proof=20?= =?UTF-8?q?exit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Criticals only. - `restored()` compared blob oids (`rev-parse :` yields nothing else), so a fix round that reverts the content and KEEPS `chmod +x` — or swaps a file for a symlink with the same text — was misclassified as restored and dropped from scope. Its mode-only section is in the PR's own diff (parseDiff emits one, planChunks gives it a chunk), so the incremental path narrowed BELOW the full-range floor it is documented to hold and exited 3 "nothing new" over a change nobody reviewed. The probe now compares the whole tree entry, mode included, via a pathspec-pinned `ls-tree`. - The round-5 EPIPE guard caught only the synchronous throw. A dead stdout also surfaces as an ASYNC 'error' event on the stream, which no try/catch around the write can intercept and which terminates the process with exit 1 — over an already-rewritten plan, sending the orchestrator down the "full-range plan untouched" branch against an incremental one. A persistent no-op 'error' listener makes that shape inert; the test now pins both. Both tests were mutation-checked: restoring the blob-only probe, or removing the listeners, turns them red. --- .../cli/src/commands/review/rescope.test.ts | 50 +++++++++++++++++++ packages/cli/src/commands/review/rescope.ts | 39 +++++++++++++-- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/rescope.test.ts b/packages/cli/src/commands/review/rescope.test.ts index 8cb71df82e3..a84434f2a7a 100644 --- a/packages/cli/src/commands/review/rescope.test.ts +++ b/packages/cli/src/commands/review/rescope.test.ts @@ -676,3 +676,53 @@ describe('rescope — round-5/6 findings', () => { 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 index 37a11249a81..ede6ff085a8 100644 --- a/packages/cli/src/commands/review/rescope.ts +++ b/packages/cli/src/commands/review/rescope.ts @@ -43,7 +43,11 @@ 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 { PINNED_DIFF_CONFIG, PINNED_DIFF_FLAGS } from './lib/diff-flags.js'; +import { + LITERAL_PATHSPECS, + PINNED_DIFF_CONFIG, + PINNED_DIFF_FLAGS, +} from './lib/diff-flags.js'; import { buildDiffPlan, parseDiff, @@ -316,8 +320,28 @@ function runRescope(args: RescopeArgs): void { // 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 => { - const at = (ref: string) => - gitOpt('-C', worktreePath, 'rev-parse', `${ref}:${p}`); + // 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 @@ -526,6 +550,15 @@ function runRescope(args: RescopeArgs): void { ); 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 From 58ef6a36a665b80cbb717e2274727f13d1f2281c Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 17 Aug 2026 13:20:28 +0800 Subject: [PATCH 8/8] fix(review): drop the duplicate `incremental` field the merge left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main's #9100 declared `incremental?: unknown` on agent-prompt's local PlanReport, and this branch already had one for the rescoped plan; the merge kept both, which is TS2300 and failed the build for every PR in the stack. Kept the documented one. Missed locally because vitest transpiles through esbuild, which drops types without checking them — a duplicate interface member is invisible to the test run and only `tsc --build` sees it. --- packages/cli/src/commands/review/agent-prompt.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 181c8396347..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