diff --git a/packages/cli/src/commands/review/capture-local.incremental.test.ts b/packages/cli/src/commands/review/capture-local.incremental.test.ts new file mode 100644 index 00000000000..c0eb499bece --- /dev/null +++ b/packages/cli/src/commands/review/capture-local.incremental.test.ts @@ -0,0 +1,1878 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The local review-fix loop, end to end against real git: round 1 captures +// full and writes a content-anchor candidate; the candidate promoted to a +// cache scopes round 2 to what changed since — same model, same HEAD — with +// one import hop of dependents; and every gate (model, HEAD, malformed cache) +// degrades to the FULL capture with the reason said out loud, never to a skip. + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + rmSync, + writeFileSync, + mkdirSync, + readFileSync, + realpathSync, + symlinkSync, + existsSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { stateIdOf } from './lib/local-anchor.js'; +import { captureLocalCommand } from './capture-local.js'; +import { buildChunkAgentPrompt } from './agent-prompt.js'; +import { isolateHostGitConfig } from './lib/test-utils.js'; +import type { IncrementalScope } from './lib/report.js'; + +// The refusal contract is "every reason is said out loud" and SKILL.md +// branches on specific stderr strings — so stderr is part of the interface +// under test, recorded here rather than left to flow to the real terminal. +const stderrLines: string[] = []; +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: vi.fn(), + writeStderrLine: vi.fn((line: string) => { + stderrLines.push(line); + }), + writeStderrLineSafe: vi.fn(), +})); + +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(() => { + stderrLines.length = 0; + repo = realpathSync(mkdtempSync(join(tmpdir(), 'review-loc-inc-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'commit.gpgsign', 'false'); + git('config', 'core.hooksPath', join(repo, '.no-such-hooks')); +}); + +afterEach(() => { + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +const CHANGED = 'src/changed.ts'; +const CALLER = 'src/caller.ts'; +const BYSTANDER = 'src/bystander.ts'; + +/** Commit a baseline, then dirty all three files — round 1's working state. */ +function seedDirtyTree(): void { + // Real repos gitignore `.qwen/` (Step 8 checks exactly that); the fixture + // must too, or the cache file and the plan output masquerade as untracked + // review scope. + write('.gitignore', '.qwen/\nplan.json\n'); + write(CHANGED, 'export const v = 0;\n'); + write(CALLER, "import { v } from './changed.js';\nexport const c = v;\n"); + write(BYSTANDER, 'export const b = 0;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + write(CHANGED, 'export const v = 1;\n'); + write(CALLER, "import { v } from './changed.js';\nexport const c = v + 1;\n"); + write(BYSTANDER, 'export const b = 1;\n'); +} + +type Plan = Record & { + chunks: Array<{ id: number }>; + files: Array<{ path: string }>; + incremental?: { scope?: IncrementalScope }; + cacheCandidatePath: string; + diffPath: string; +}; + +function capture(extra: Record = {}): Plan { + const out = join(repo, 'plan.json'); + // The identity is the RUNTIME's, not a flag: `capture-local` reads + // `QWEN_CODE_MODEL_IDENTITY` the way the child shell publishes it. Tests + // name a model the same way they always did; the harness puts it where the + // command actually looks. + const { model, ...argv } = extra as { model?: string }; + const prev = process.env['QWEN_CODE_MODEL_IDENTITY']; + if (model !== undefined) process.env['QWEN_CODE_MODEL_IDENTITY'] = model; + try { + (captureLocalCommand.handler as (argv: unknown) => void)({ + out, + target: 'local', + untracked: true, + ...argv, + }); + } finally { + if (prev === undefined) delete process.env['QWEN_CODE_MODEL_IDENTITY']; + else process.env['QWEN_CODE_MODEL_IDENTITY'] = prev; + } + return JSON.parse(readFileSync(out, 'utf8')) as Plan; +} + +/** What Step 8 does on a clean high-effort end: candidate + ledger → cache. */ +function promoteCandidate(plan: Plan, model: string): string { + const candidate = JSON.parse( + readFileSync(plan.cacheCandidatePath, 'utf8'), + ) as Record; + const cachePath = join(repo, '.qwen/review-cache/local.json'); + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + writeFileSync( + cachePath, + JSON.stringify({ ...candidate, lastModelId: model }), + ); + return cachePath; +} + +/** Append an open Critical to a promoted cache's ledger. */ +function recordOpenCritical(cachePath: string): void { + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as Record< + string, + unknown + >; + cache['findings'] = [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + file: CHANGED, + line: 1, + title: 'blocker', + }, + ]; + writeFileSync(cachePath, JSON.stringify(cache)); +} + +describe('capture-local — incremental local rounds', () => { + it('round 1 writes a candidate covering every captured file', () => { + seedDirtyTree(); + const plan = capture(); + expect(plan.incremental).toBeUndefined(); + // R17-4: the plan carries the written candidate's own stateId so Step 8 + // can tell this round's candidate from a concurrent run's overwrite — + // the path alone cannot (stable per target, no lease). + const published = JSON.parse( + readFileSync(plan.cacheCandidatePath, 'utf8'), + ) as { stateId: string }; + expect(plan['cacheCandidateStateId']).toBe(published.stateId); + const candidate = JSON.parse( + readFileSync(plan.cacheCandidatePath, 'utf8'), + ) as { files: Record; headSha: string | null }; + expect(Object.keys(candidate.files).sort()).toEqual([ + BYSTANDER, + CALLER, + CHANGED, + ]); + expect(candidate.headSha).toBe(git('rev-parse', 'HEAD')); + }); + + it('round 2 scopes to the changed file plus its importer; the bystander is out', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + + write(CHANGED, 'export const v = 2;\n'); // the fix + const plan = capture({ cache: cachePath, model: 'model-a' }); + + expect(plan.incremental).toBeDefined(); + expect(plan.incremental!.scope!.deltaFiles).toEqual([CHANGED]); + expect(plan.incremental!.scope!.interaction).toEqual([ + { path: CALLER, importsChanged: [CHANGED] }, + ]); + expect(plan.incremental!.scope!.contextFileCount).toBe(1); + expect(plan.files.map((f) => f.path).sort()).toEqual([CALLER, CHANGED]); + + const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); + expect(diff).toContain('+export const v = 2;'); + expect(diff).toContain('caller.ts'); + expect(diff).not.toContain('bystander'); + // The full capture is preserved beside the scoped one. + expect( + readFileSync(plan.incremental!.scope!.fullDiffPath!, 'utf8'), + ).toContain('bystander'); + }); + + it('an attribute flip re-reviews the file — including with NO worktree change', () => { + // What a round READS is the rendering. `binary` turns a file's section + // into "Binary files … differ", so a round can end clean having read no + // content of it; drop the attribute and the same bytes are text nobody + // has reviewed. Mode and blob cannot see that. + // + // The rendering attributes ride each file's IDENTITY now, asked of `git + // check-attr` rather than re-derived from the attribute sources — so a + // flip moves that one file and the round stays incremental, instead of + // refusing the whole anchor. The second half is why it cannot be derived + // by hand: `.git/info/attributes` is not in the worktree, so nothing + // about the tree changes at all. + seedDirtyTree(); + write('.gitattributes', `${CHANGED} binary\n`); + const cachePath = promoteCandidate(capture(), 'model-a'); + + // (a) a tracked attributes file changes. + write('.gitattributes', '\n'); + const viaWorktree = capture({ cache: cachePath, model: 'model-a' }); + expect(viaWorktree.incremental).toBeDefined(); + expect(viaWorktree.incremental!.scope!.deltaFiles).toContain(CHANGED); + + // (b) the same KIND of flip through `.git/info/attributes`, which is not + // in the worktree at all — no file identity derived from the tree could + // ever cover it, and nothing about the tree moves. This is why the + // attributes are asked of git rather than read from the sources. + write('.gitattributes', '\n'); + const cache2 = promoteCandidate(capture(), 'model-a'); + mkdirSync(join(repo, '.git', 'info'), { recursive: true }); + writeFileSync( + join(repo, '.git', 'info', 'attributes'), + `${CHANGED} binary\n`, + ); + const viaInfo = capture({ cache: cache2, model: 'model-a' }); + expect(viaInfo.incremental).toBeDefined(); + expect(viaInfo.incremental!.scope!.deltaFiles).toContain(CHANGED); + }); + + it('a cache from before the rendering attributes re-reviews everything', () => { + // Identities written by an older CLI carry no attribute component, so + // every one of them compares unequal and every file re-enters scope. The + // round still runs — it is a wider scope, not a refusal — and nothing is + // skipped on a comparison that could not be made. + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as { + files: Record; + headSha: string | null; + stateId: string; + }; + // Strip the attribute half back to the old `:` shape. + for (const [path, id] of Object.entries(cache.files)) { + const parts = id.split(':'); + cache.files[path] = parts.slice(0, 2).join(':'); + } + // …and re-stamp `stateId`, or the integrity gate refuses first and this + // test would pass for the wrong reason. + cache.stateId = stateIdOf(cache.headSha, cache.files); + writeFileSync(cachePath, JSON.stringify(cache)); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles.sort()).toEqual( + [BYSTANDER, CALLER, CHANGED].sort(), + ); + }); + + it('a pending DELETION lets the loop converge instead of re-arming for ever', () => { + // `UNHASHABLE` never equals itself, deliberately — state that could not + // be captured is re-reviewed rather than certified. But the "nothing + // changed, stop" decision used to key on that same list, which made the + // stop unreachable for any change set holding a deletion: round N+1 + // re-hashed the section to UNHASHABLE, announced "1 changed file(s)" over + // a byte-identical diff, and re-armed itself for N+2 until HEAD moved. + seedDirtyTree(); + rmSync(join(repo, CHANGED)); + const cachePath = promoteCandidate(capture(), 'model-a'); + + // Nothing moves between the rounds. + stderrLines.length = 0; + const plan = capture({ cache: cachePath, model: 'model-a' }); + // The round says what is true, and the two halves match: no CONTENT + // moved, and the unhashable path is still in scope. The bare + // "nothing to re-review" sentence must NOT appear — SKILL.md stops the + // orchestrator on exactly that string, so printing it beside a plan that + // carries chunks stops the round over live scope. + expect(stderrLines.join('\n')).toContain('No content changes since'); + expect(stderrLines.join('\n')).toContain('could not be hashed'); + expect(stderrLines.join('\n')).toContain('Their sections are in scope'); + expect(stderrLines.join('\n')).not.toContain('nothing to re-review'); + expect(stderrLines.join('\n')).not.toContain('changed file(s)'); + expect((plan.chunks as unknown[]).length).toBeGreaterThan(0); + // The deletion is still not CERTIFIED — the scope keeps the wider list on + // purpose, so an unreadable path is re-reviewed rather than skipped. Both + // facts are true at once, and separating them is the fix: the stop reads + // what MOVED, the scope reads what could not be ruled out. + expect(plan.incremental!.scope!.deltaFiles).toContain(CHANGED); + + // With something genuinely new beside it, the round runs and the count a + // human reads names the unreadable path apart from the real change. + write('src/other.ts', 'export const o = 1;\n'); + stderrLines.length = 0; + const next = capture({ cache: cachePath, model: 'model-a' }); + expect(next.incremental!.scope!.deltaFiles).toContain('src/other.ts'); + expect(stderrLines.join('\n')).toContain('unreadable path(s)'); + }); + + it('a config-side diff driver moves the identity, like the attribute does', () => { + // `check-attr` answers attribute VALUES, and `diff=` is only a + // NAME: the behaviour lives in git config. `diff..binary` flips a + // section between readable hunks and "Binary files … differ" while the + // attribute value, the mode and the blob all stand still — so the + // identity compared equal and the newly-readable section was sliced out, + // the loop certifying content the previous round had only seen as a + // marker. + seedDirtyTree(); + write('.gitattributes', `${CHANGED} diff=mydrv\n`); + git('config', 'diff.mydrv.binary', 'true'); + const cachePath = promoteCandidate(capture(), 'model-a'); + + // Only the CONFIG changes: no file in the tree moves, and `check-attr`'s + // answer is identical before and after. + git('config', 'diff.mydrv.binary', 'false'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toContain(CHANGED); + }); + + it('an identical state under the same model and HEAD yields 0 chunks and says so', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual([]); + expect(plan.chunks).toEqual([]); + }); + + it('a different model degrades to the full capture', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + write(CHANGED, 'export const v = 2;\n'); + const plan = capture({ cache: cachePath, model: 'model-b' }); + expect(plan.incremental).toBeUndefined(); + expect(plan.files.map((f) => f.path).sort()).toEqual([ + BYSTANDER, + CALLER, + CHANGED, + ]); + }); + + it('a moved HEAD degrades to the full capture', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'user committed the changes'); + write(CHANGED, 'export const v = 2;\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeUndefined(); + }); + + it('a malformed cache and a missing --model both degrade to the full capture', () => { + seedDirtyTree(); + const cachePath = join(repo, '.qwen/review-cache/local.json'); + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + writeFileSync(cachePath, 'not json'); + expect( + capture({ cache: cachePath, model: 'm' }).incremental, + ).toBeUndefined(); + + const good = promoteCandidate(capture(), 'model-a'); + expect(capture({ cache: good }).incremental).toBeUndefined(); + }); + + it('the block the REAL brief renderer reads — not merely the shape', () => { + // The finding this pins: the local flow wrote the block flat while both + // consumers (`incrementalScopeOf` here, `incrementalInteractionPaths` in + // the roster) key on `incremental.scope`. Every shape assertion above + // stayed green, the diff WAS sliced, and the round looked incremental + // everywhere — while no chunk brief carried the frame, so each widened + // file was re-reviewed from scratch. Only driving the real renderer sees + // it, so this test does. + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + write(CHANGED, 'export const v = 2;\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual([CHANGED]); + + const briefs = (plan.chunks as Array<{ id: number }>).map((c) => + buildChunkAgentPrompt(plan as never, c.id), + ); + expect(briefs.length).toBeGreaterThan(0); + expect(briefs.some((b) => b.includes('INCREMENTAL'))).toBe(true); + // …and the seam itself: the importer's brief must name what it imports + // that changed, or the agent has no reason to look at the interaction + // rather than re-read the file. + expect(briefs.some((b) => b.includes(CALLER))).toBe(true); + }); + + it('a brand-new untracked file since the last round is delta, not skipped', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + write('src/new-untracked.ts', 'export const n = 1;\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual([ + 'src/new-untracked.ts', + ]); + const diff = readFileSync(join(repo, plan.diffPath), 'utf8'); + expect(diff).toContain('new-untracked'); + expect(diff).not.toContain('bystander'); + }); +}); + +describe('capture-local — round-2 regressions from the stop work', () => { + it('does not call a tracked, unmodified FILE review a clean-tree stop', () => { + // An empty diff is not a decided round for a file target: SKILL.md's + // no-diff branch owes it a whole-file review. Marked decided, the round + // turned from "Review did not complete" — which it was before the stop + // existed — into a PASSING gate over a file nobody read. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + + const plan = capture({ file: CHANGED }); + expect(plan.chunks.length).toBe(0); + expect(plan['nothingToReview']).toBeUndefined(); + }); + + it('does not tell a FILE review of an unchanged file that the tree is clean', () => { + // The field gate excludes file reviews; the prose channel beside it did + // not, so stderr still said "the working tree is clean … do not run the + // review agents" over a capture that was pathspec-scoped — 0 chunks says + // nothing about the tree (the bystanders here are dirty), and an + // orchestrator that stops on prose left the user-named file unread. The + // no-diff branch owes this shape a whole-file review. + seedDirtyTree(); + git('add', CHANGED); + git('commit', '-q', '--no-verify', '-m', 'commit only the reviewed file'); + + stderrLines.length = 0; + const plan = capture({ file: CHANGED }); + expect(plan['nothingToReview']).toBeUndefined(); + const err = stderrLines.join('\n'); + expect(err).not.toContain('the working tree is clean'); + expect(err).toContain('whole-file review'); + }); + + it('stamps the stop sidecar with the run that asked for it', () => { + // The sidecar decides `completed`, while its NAME is the flattened + // target token — which is not injective, so a concurrent review whose + // path flattens alike writes the same file and would decide the other + // run's completion. The epoch fence separates EARLIER runs, not + // concurrent ones; only a nonce does. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + const prev = process.env['QWEN_REVIEW_RUN_ID']; + process.env['QWEN_REVIEW_RUN_ID'] = 'run-abc'; + try { + capture(); + } finally { + if (prev === undefined) delete process.env['QWEN_REVIEW_RUN_ID']; + else process.env['QWEN_REVIEW_RUN_ID'] = prev; + } + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['runId']).toBe('run-abc'); + }); +}); + +describe('capture-local — a narrower round cannot certify a wider one', () => { + it('refuses the anchor when this round excludes untracked files', () => { + // With `--no-untracked` the untracked block never runs and records no + // `skipped` entries, so the skipped-content gate sees zero — while a + // cached untracked path reads as VANISHED rather than out of scope. The + // slice keeps nothing and the round stops decided over bytes it never + // captured; the stop does not advance the cache, so every later narrow + // round repeats it. + seedDirtyTree(); + write('src/untracked.ts', 'export const u = 1;\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + + const narrow = capture({ + cache: cachePath, + model: 'model-a', + untracked: false, + }); + expect(narrow.incremental).toBeUndefined(); + expect(narrow['nothingToReview']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('excludes untracked files'); + }); +}); + +describe('capture-local — round-5 sibling gaps', () => { + it('does not stop a FILE review whose anchored change was discarded', () => { + // `scope-emptied` lacked the exclusion both sibling stops carry, so the + // same tree decided differently depending on whether a cache existed: + // with one it completed as a decided round, without one it routed to the + // whole-file review SKILL.md owes a file target. + seedDirtyTree(); + write('src/foo.ts', 'export const real = 1;\n'); + const first = capture({ file: 'src/foo.ts', model: 'model-a' }); + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + writeFileSync( + first['cachePath'] as string, + readFileSync(first.cacheCandidatePath, 'utf8'), + ); + // Discard the reviewed change entirely; HEAD does not move. The file was + // untracked, so discarding it removes it — the anchored path vanishes and + // the slice keeps nothing. + rmSync(join(repo, 'src/foo.ts')); + + const second = capture({ + file: 'src/foo.ts', + cache: join(repo, '.qwen/review-cache'), + model: 'model-a', + }); + expect(second['nothingToReview']).toBeUndefined(); + }); +}); + +describe('capture-local — the cache namespace discriminates the subject', () => { + it('gives a file review its own key, so colliding targets keep separate ledgers', () => { + // The anchor gate's `source` check is the second layer, not the first: it + // can only refuse a cache the round already opened, which leaves the + // LEDGER — read and written by the orchestrator, not the gate — sharing + // one file. `safeTarget` is not injective, so `src/foo.ts` and + // `src_foo.ts` flattened to one key and erased each other's findings. + seedDirtyTree(); + write('src_foo.ts', 'export const collide = 1;\n'); + write('src/foo.ts', 'export const real = 1;\n'); + + const a = capture({ file: 'src/foo.ts', model: 'model-a' }); + const b = capture({ file: 'src_foo.ts', model: 'model-a' }); + expect(a['target']).toBe(b['target']); // the token still collides… + expect(a['cachePath']).not.toBe(b['cachePath']); // …the cache key does not + }); + + it('keeps a root file named `local` out of the whole-tree cache', () => { + // The token space reserves nothing: `safeTarget('local') === 'local'`, so + // a root file by that name produced the whole-tree key byte for byte and + // the two rounds served each other their ledgers. + seedDirtyTree(); + write('local', 'not the whole tree\n'); + + const wholeTree = capture({ model: 'model-a' }); + const rootFile = capture({ file: 'local', model: 'model-a' }); + expect(rootFile['target']).toBe(wholeTree['target']); + expect(rootFile['cachePath']).not.toBe(wholeTree['cachePath']); + }); +}); + +describe('capture-local — the decided stops are machine-readable', () => { + it('marks the unchanged-since-last-round stop in the plan', () => { + // `compose-review` runs only in Step 6, and this stop fires in Step 1, so + // no composed verdict exists — and `qwen review run` polls for exactly + // that, reporting "Review did not complete" over a round whose own output + // was decided. The signal is a field the CLI wrote, not a sentence the + // model chose off stderr. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second.chunks.length).toBe(0); + expect(second['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + }); + + it('does NOT mark a capture that SKIPPED files — it read nothing, twice over', () => { + // The safety half. An empty diff beside a non-empty skip list is not a + // clean tree: that round could not read what it skipped, owes a "Not + // reviewed" section, and must never reach the parent as complete — + // exactly the failure this command exists to end, arriving through the + // front door. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + // A symlink to a DIRECTORY is skipped, not reviewed. + mkdirSync(join(repo, 'somedir'), { recursive: true }); + symlinkSync(join(repo, 'somedir'), join(repo, 'dirlink')); + + const plan = capture(); + expect((plan['skippedFiles'] as unknown[]).length).toBeGreaterThan(0); + expect(plan['nothingToReview']).toBeUndefined(); + }); + + it('marks the scope-emptied round, which neither other stop reaches', () => { + // The third decided shape. A cached path that VANISHED — the change + // discarded with `git checkout --` — is a change by design, so the + // unchanged-since stop cannot fire; and the clean-tree stop is gated on + // `!incremental`. The slice keeps nothing, so the plan carried + // `chunks: []` with an `incremental` block and no field at all: neither + // SKILL stop fired, `agent-prompt --roster` threw on the first + // diff-reading role, and the parent reported "Review did not complete". + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + // Discard every reviewed change; HEAD does not move. + git('checkout', '--', '.'); + + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.chunks.length).toBe(0); + expect(plan['incremental']).toBeDefined(); + expect(plan['nothingToReview']).toEqual({ reason: 'scope-emptied' }); + // R17-2: the split key rides the plan. A discarded change leaves the + // file PRESENT with the cited bytes gone, so presence cannot route the + // SUPERSEDED split — the capture names the paths itself. + const scope = ( + plan['incremental'] as { scope: { supersededPaths?: string[] } } + ).scope; + expect(scope.supersededPaths).toEqual( + expect.arrayContaining([CHANGED, CALLER, BYSTANDER]), + ); + }); + + it('names a DELETED untracked cached path in supersededPaths the same way', () => { + // The other half of the removed set: an UNTRACKED file the cached round + // reviewed, deleted since. (A deleted TRACKED file is different on + // purpose: its deletion is a live diff against HEAD, so it stays in the + // slice as a change — only content that leaves every diff lands here.) + // Same field, same split — the finding citing it is superseded whether + // the file left the tree or only the change did. + seedDirtyTree(); + write('src/untracked.ts', 'export const u = 1;\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + git('checkout', '--', '.'); + rmSync(join(repo, 'src/untracked.ts')); + + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan['nothingToReview']).toEqual({ reason: 'scope-emptied' }); + const scope = ( + plan['incremental'] as { scope: { supersededPaths?: string[] } } + ).scope; + expect(scope.supersededPaths).toEqual( + expect.arrayContaining(['src/untracked.ts', CHANGED]), + ); + }); + + it('refuses the anchor when a cached path is UNMEASURABLE, not gone', () => { + // R19-3: `vanishedStillOnDisk` folded every lstat failure into + // "genuinely gone", so a cached path under an unmeasurable ancestor + // read as a deletion and the round ended at a DECIDED stop over bytes + // no round captured. ENOTDIR stages it as root: the cached untracked + // file's parent directory is replaced by a regular FILE — the cached + // path can no longer be proven absent, and unmeasurable is + // uncertifiable: the anchor refuses at the cost of a full round. + seedDirtyTree(); + write('sub/u.ts', 'export const u = 1;\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + rmSync(join(repo, 'sub'), { recursive: true, force: true }); + write('sub', '// a regular file where the directory was\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['incremental']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('still on disk'); + }); + + it('reads core.fileMode as a bool — a legacy false spelling still folds', () => { + // R20-1: `config --get` echoes the STORED spelling, so `off`/`no`/`0` + // failed a `!== 'false'` test and the exec fold was silently disabled — + // an executable file whose edit is discarded in place then refused as + // "dropped out while still on disk", every round, for ever. + seedDirtyTree(); + git('config', 'core.fileMode', 'off'); + execFileSync('chmod', ['+x', join(repo, CHANGED)]); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + // The designed discarded-change shape, HEAD unmoved: the edit goes + // back, and the exec bit the cache recorded is re-applied — `git diff + // HEAD` stays empty under this knob (`git checkout --` resets the mode, + // so the chmod comes after), and the path drops out of the capture with + // the worktree reading 100755 against HEAD's 100644. + git('checkout', '--', CHANGED); + execFileSync('chmod', ['+x', join(repo, CHANGED)]); + + stderrLines.length = 0; + capture({ cache: cachePath, model: 'model-a' }); + expect(stderrLines.join('\n')).not.toContain('still on disk'); + }); + + it('re-reviews a materialized symlink under core.symlinks=false — disclosed', () => { + // R20-5: with the knob off git materializes a tracked symlink as a + // regular file, so the worktree side reads `100644::` + // against HEAD's `120000:` and the designed discarded-change shape + // cannot be certified. A mode fold does not close it (the spellings + // also differ in carrying attributes at all), and equalizing the + // attributes would drop the rendering dimension for that path — so the + // bounded over-review is the accepted answer, pinned here so a later + // change cannot turn it into a silent certification. + seedDirtyTree(); + symlinkSync('changed.ts', join(repo, 'src/link.ts')); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'add symlink'); + rmSync(join(repo, 'src/link.ts')); + symlinkSync('caller.ts', join(repo, 'src/link.ts')); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + git('config', 'core.symlinks', 'false'); + rmSync(join(repo, 'src/link.ts')); + writeFileSync(join(repo, 'src/link.ts'), 'changed.ts'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + // Refused, out loud, and NEVER decided: over-review, not certification. + expect(stderrLines.join('\n')).toContain('still on disk'); + expect(second['nothingToReview']).toBeUndefined(); + }); + + it('certifies a restored SUBMODULE pointer instead of refusing it for ever', () => { + // R20-3: a gitlink is unhashable on both sides by design (a directory in + // the worktree, type `commit` in the tree), so the both-UNHASHABLE + // refusal fired every round once round 1 had touched a submodule — the + // permanent wedge. But git measures submodules itself and the pinned + // flags keep them in the capture, so a gitlink's ABSENCE from the diff + // is git's own answer that the pointer did not move. + seedDirtyTree(); + const sub = realpathSync(mkdtempSync(join(tmpdir(), 'review-sub-'))); + execFileSync('git', ['init', '-q', '--template=', '.'], { cwd: sub }); + execFileSync('git', ['config', 'user.email', 'a@b'], { cwd: sub }); + execFileSync('git', ['config', 'user.name', 'a'], { cwd: sub }); + writeFileSync(join(sub, 'm.ts'), 'export const m = 0;\n'); + execFileSync('git', ['add', '-A'], { cwd: sub }); + execFileSync('git', ['commit', '-q', '--no-verify', '-m', 'm0'], { + cwd: sub, + }); + git( + '-c', + 'protocol.file.allow=always', + 'submodule', + 'add', + '-q', + sub, + 'mod', + ); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'add submodule'); + // Round 1 reviews a MOVED pointer, so the gitlink is in the population. + writeFileSync(join(sub, 'm.ts'), 'export const m = 1;\n'); + execFileSync('git', ['commit', '-q', '--no-verify', '-am', 'm1'], { + cwd: sub, + }); + execFileSync('git', ['-c', 'protocol.file.allow=always', 'pull', '-q'], { + cwd: join(repo, 'mod'), + }); + const first = capture({ model: 'model-a' }); + const cachePath = promoteCandidate(first, 'model-a'); + // R22-1 first: the DIRTY pointer, unchanged, CONVERGES — the gitlink + // records `160000:` instead of UNHASHABLE (which never equals + // itself and wedged the unchanged-since stop for the change set's + // lifetime). + const rerun = capture({ cache: cachePath, model: 'model-a' }); + expect(rerun['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + // …but the oid alone must never certify INTERNAL edits: dirty the + // submodule's content (pointer unmoved) and the identity flips to + // UNHASHABLE — no decided stop may fire over bytes `git diff` renders + // only as `-dirty`. + writeFileSync(join(repo, 'mod/m.ts'), 'export const m = 2;\n'); + const dirtyRun = capture({ cache: cachePath, model: 'model-a' }); + expect(dirtyRun['nothingToReview']).toBeUndefined(); + writeFileSync(join(repo, 'mod/m.ts'), 'export const m = 1;\n'); + // …and a visibility bit INSIDE the submodule must break cleanliness the + // same way: `status --porcelain` honours it, so without the interior + // oracle the identity held still over an edit no round can see (the + // fix-induced half of R22-1). + writeFileSync(join(repo, 'mod/m.ts'), 'export const m = 3;\n'); + execFileSync('git', ['update-index', '--assume-unchanged', 'm.ts'], { + cwd: join(repo, 'mod'), + }); + const hiddenRun = capture({ cache: cachePath, model: 'model-a' }); + expect(hiddenRun['nothingToReview']).toBeUndefined(); + execFileSync('git', ['update-index', '--no-assume-unchanged', 'm.ts'], { + cwd: join(repo, 'mod'), + }); + writeFileSync(join(repo, 'mod/m.ts'), 'export const m = 1;\n'); + // The user restores the pointer: `git diff HEAD` goes quiet for it. + git('submodule', 'update', '--recursive'); + git('checkout', '--', '.'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(stderrLines.join('\n')).not.toContain('still on disk'); + expect(second['incremental']).toBeDefined(); + + // …and the certification is a MEASUREMENT, not the diff's silence: with + // the submodule's gitdir pointer gone its HEAD cannot be read, `git + // diff` shows nothing either way, and an unmeasurable pointer must + // refuse — the R20-3 follow-up's odb-removal shape. + rmSync(join(repo, 'mod/.git'), { recursive: true, force: true }); + stderrLines.length = 0; + capture({ cache: cachePath, model: 'model-a' }); + expect(stderrLines.join('\n')).toContain('still on disk'); + rmSync(sub, { recursive: true, force: true }); + }); + + it('withholds the candidate when a cached path dropped out while on disk', () => { + // R23: the candidate write gated on treeHeldStill and the visibility + // bits, never on the dropped-out set — so a refused-anchor round wrote + // a candidate silently OMITTING the dropped path, Step 8 promoted the + // omission, and two rounds later a scope-emptied stop certified bytes + // no round read. The same uncertainty that refuses the anchor withholds + // the candidate. + seedDirtyTree(); + write('deploy.sh', 'echo v1\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + // Visibility narrows without any flag moving: ignore the file, edit it. + write('.git/info/exclude', 'deploy.sh\n'); + write('deploy.sh', 'echo v2\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['incremental']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('still on disk'); + expect(second['cacheCandidatePath']).toBeDefined(); + expect(existsSync(second['cacheCandidatePath'] as string)).toBe(false); + expect(stderrLines.join('\n')).toContain( + 'candidate would record their absence as reviewed state', + ); + }); + + it('keeps a DIRECTORY subject out of the anchor — it has no bytes', () => { + // R20-2: `qwen review ` is a supported entrance, and the subject + // was injected into the hashed population unconditionally — recorded as + // UNHASHABLE, which never equals itself, so `changedSince` reported the + // directory every round and the unchanged-since stop was unreachable + // for that target for ever. Its FILES carry the bytes; the directory + // carries none. + seedDirtyTree(); + const plan = capture({ file: 'src', model: 'model-a' }); + const cand = JSON.parse(readFileSync(plan.cacheCandidatePath, 'utf8')) as { + files: Record; + }; + expect(Object.keys(cand.files)).not.toContain('src'); + expect(Object.keys(cand.files)).toContain(CHANGED); + // …and the round after it converges: no UNHASHABLE entry keeps the + // symmetric difference non-empty. The cache goes to the path the plan + // published — a FILE review is namespaced, not `local.json`. + const cachePath = plan['cachePath'] as string; + mkdirSync(dirname(join(repo, cachePath)), { recursive: true }); + writeFileSync( + join(repo, cachePath), + JSON.stringify({ ...cand, lastModelId: 'model-a' }), + ); + stderrLines.length = 0; + const second = capture({ + file: 'src', + cache: join(repo, cachePath), + model: 'model-a', + }); + // A FILE target never stops decided at unchanged-since (R23 gave it the + // exclusion both sibling stops carry — SKILL owes the shape a + // whole-file review, cache or no cache), so convergence shows as the + // ABSENCE of the wedge instead: no phantom directory in the delta, no + // could-not-be-hashed misdiagnosis, an admitted anchor. + expect(second['nothingToReview']).toBeUndefined(); + expect(second['incremental']).toBeDefined(); + const scope2 = ( + second['incremental'] as { scope: { deltaFiles: string[] } } + ).scope; + expect(scope2.deltaFiles).not.toContain('src'); + expect(stderrLines.join('\n')).not.toContain('could not be hashed'); + }); + + it('publishes the stop at a name the PARENT can predict', () => { + // `--out` is the orchestrator's to choose — it must be, because the + // CLI-derived target token does not exist yet at Step 1 — so a parent + // polling the plan by name found nothing for every file review and + // reported "Review did not complete" over a decided round. The sidecar is + // named from the same target the parent derives. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + + // Deliberately NOT the name a parent could guess. + const out = join(repo, 'somewhere-else.json'); + (captureLocalCommand.handler as (argv: unknown) => void)({ + out, + target: 'local', + untracked: true, + }); + + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['reason']).toBe('clean-tree'); + }); + + it('marks a genuinely clean tree', () => { + // `seedDirtyTree` commits a base and then dirties it; committing that + // work leaves the tree clean, which is the shape this stop is about. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + + const plan = capture(); + expect(plan.chunks.length).toBe(0); + expect(plan['nothingToReview']).toEqual({ reason: 'clean-tree' }); + }); + + it('withholds the clean-tree stop under --no-untracked, out loud', () => { + // R15-2: the stop's claim is "nothing staged, nothing unstaged, nothing + // untracked", and with `--no-untracked` the third clause is checked by + // nobody — the untracked enumeration never runs and records no + // `skipped` entries, so a tracked-clean tree with pending untracked + // work passed every conjunct and `qwen review run` exited 0 decided + // over files no round enumerated. SKILL.md's own recovery from an + // oversized-untracked skip re-runs with exactly this flag. The anchor + // gate has carried the exclusion since the candidate recorded + // `untracked`; this pins the stop gate's copy, and the prose twin's. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + write('pending-work.ts', 'export const untrackedEdit = 1;\n'); + + const plan = capture({ untracked: false }); + expect(plan.chunks.length).toBe(0); + expect(plan['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + const err = stderrLines.join('\n'); + expect(err).toContain( + 'untracked files were not enumerated (--no-untracked)', + ); + expect(err).not.toContain('the working tree is clean'); + // The same flag on a genuinely clean tree is withheld too — the capture + // cannot tell the two apart, and fail-closed is the direction every + // sibling gate leans. + stderrLines.length = 0; + rmSync(join(repo, 'pending-work.ts')); + const second = capture({ untracked: false }); + expect(second['nothingToReview']).toBeUndefined(); + }); + + it('withholds BOTH incremental stops under --no-untracked, out loud', () => { + // R16-1: the unchanged-since-last-round and scope-emptied stops lacked + // the `--no-untracked` exclusion their sibling clean-tree stop carries. + // The anchor gate's untracked clause only refuses a NARROWER round than + // the cache, so two narrow rounds pass it and either stop decides + // "nothing to review" over untracked content neither round enumerated: + // round 1 promotes a candidate with `untracked: false`, new untracked + // work appears, the tracked content stays byte-identical (or the change + // is discarded), and the loop stops while the brand-new file is never + // seen. Both stops follow the sibling: withheld, out loud. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a', untracked: false }), + 'model-a', + ); + // Brand-new untracked work, invisible to BOTH rounds. + write('pending-work.ts', 'export const untrackedEdit = 1;\n'); + + // Arm 1: tracked content byte-identical — unchanged-since-last-round. + const unchanged = capture({ + cache: cachePath, + model: 'model-a', + untracked: false, + }); + expect(unchanged.incremental).toBeDefined(); + expect(unchanged.chunks.length).toBe(0); + expect(unchanged['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + const err = stderrLines.join('\n'); + expect(err).toContain( + 'untracked files were not enumerated (--no-untracked)', + ); + expect(err).toContain('NOT a decided nothing-to-review'); + + // Arm 2: discard the tracked changes — scope-emptied. + stderrLines.length = 0; + git('checkout', '--', '.'); + const emptied = capture({ + cache: cachePath, + model: 'model-a', + untracked: false, + }); + expect(emptied.incremental).toBeDefined(); + expect(emptied.chunks.length).toBe(0); + expect(emptied['nothingToReview']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain( + 'untracked files were not enumerated (--no-untracked)', + ); + }); +}); + +describe('capture-local — --cache takes the DIRECTORY', () => { + it('resolves the cache from the target IT derived, not one the caller guessed', () => { + // The name is `.json`, and `target` is derived inside this + // command — so a caller running BEFORE it has to predict, and predicting + // is wrong for any non-canonical spelling. Through a symlinked + // directory, the typed path flattens to `srclink_foo.ts` while the + // command canonicalises to `src_foo.ts`: the prediction misses, the + // cache is never passed, and the round silently loses both incremental + // scoping and the findings ledger. + seedDirtyTree(); + write('src/foo.ts', 'export const real = 1;\n'); + symlinkSync(join(repo, 'src'), join(repo, 'srclink')); + + // Round 1 through the SYMLINKED spelling. + const first = capture({ file: 'srclink/foo.ts', model: 'model-a' }); + expect(first['target']).toBe('src_foo.ts'); + const cacheDir = join(repo, '.qwen/review-cache'); + mkdirSync(cacheDir, { recursive: true }); + // Written where the CAPTURE says this target's cache lives — the same + // field the orchestrator reads. A file review's cache is namespaced by + // source path, so a hand-spelled `.json` is not it. + writeFileSync( + first['cachePath'] as string, + readFileSync(first.cacheCandidatePath, 'utf8'), + ); + + // Round 2 hands over the DIRECTORY and never names the file. + write('src/foo.ts', 'export const real = 2;\n'); + const second = capture({ + file: 'srclink/foo.ts', + cache: cacheDir, + model: 'model-a', + }); + expect(second.incremental?.scope?.deltaFiles).toEqual(['src/foo.ts']); + }); + + it('reads a directory holding no cache for this target as no anchor', () => { + seedDirtyTree(); + const cacheDir = join(repo, '.qwen/review-cache'); + mkdirSync(cacheDir, { recursive: true }); + expect(capture({ cache: cacheDir, model: 'model-a' }).incremental).toBe( + undefined, + ); + }); +}); + +describe('capture-local — the cache key is the SOURCE path, not the token', () => { + it('refuses a cache whose flattened token collides with another file', () => { + // `safeTarget` is not injective: `src/foo.ts` and `src_foo.ts` both + // flatten to `src_foo.ts`, and this PR keys the cache by that token. The + // token gate alone passed each file the other's cache — scoping against a + // state describing a different file, and erasing that file's anchor and + // open findings on promotion. + seedDirtyTree(); + write('src_foo.ts', 'export const collide = 1;\n'); + write('src/foo.ts', 'export const real = 1;\n'); + + const first = capture({ file: 'src/foo.ts', model: 'model-a' }); + expect(first['target']).toBe('src_foo.ts'); + const candidate = JSON.parse( + readFileSync(first.cacheCandidatePath, 'utf8'), + ) as Record; + expect(candidate['source']).toBe('src/foo.ts'); + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + const cachePath = join(repo, '.qwen/review-cache/src_foo.ts.json'); + writeFileSync(cachePath, JSON.stringify(candidate)); + + // The OTHER file, whose token is the same one. + const other = capture({ + file: 'src_foo.ts', + cache: cachePath, + model: 'model-a', + }); + expect(other['target']).toBe('src_foo.ts'); + expect(other.incremental).toBeUndefined(); + // …and SAID, like every sibling gate's reason: a refactor relocating the + // check into the reader (fail-quiet null) would surface "the cache is + // missing or unreadable" — a false diagnosis for exactly this collision. + expect(stderrLines.join('\n')).toContain('belongs to source path'); + + // …and the file the cache actually belongs to still scopes. + write('src/foo.ts', 'export const real = 2;\n'); + const same = capture({ + file: 'src/foo.ts', + cache: cachePath, + model: 'model-a', + }); + expect(same.incremental).toBeDefined(); + }); + + it('derives the source even when an explicit --target rides along on --file', () => { + // The pre-fix `--target` describe documented this combination, and a + // caller following it left `sourcePath` undefined: the cache fell out of + // the digest namespace, the candidate recorded no `source`, and the + // gate's source clause degraded to `undefined === undefined` and passed — + // so the TOKEN-colliding pair below (which the target gate cannot tell + // apart) shared one cache, and the second file erased the first's anchor + // on promotion. The derivation wins now for EVERY `--file` capture: the + // parent (`qwen review run`) pins its artifact names to it anyway. + seedDirtyTree(); + write('src/a.ts', 'export const a = 1;\n'); + write('src_a.ts', 'export const collide = 1;\n'); + + const first = capture({ file: 'src/a.ts', target: 't', model: 'model-a' }); + expect(first['target']).toBe('src_a.ts'); + const candidate = JSON.parse( + readFileSync(first.cacheCandidatePath, 'utf8'), + ) as Record; + expect(candidate['source']).toBe('src/a.ts'); + const cachePath = join(repo, first['cachePath'] as string); + expect(cachePath).toContain('file-src_a.ts-'); + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + writeFileSync(cachePath, JSON.stringify(candidate)); + + // The token-colliding OTHER file under the same explicit token must not + // inherit it: the derived tokens agree, so the source gate is the only + // layer that can tell the two subjects apart. + write('src_a.ts', 'export const collide = 2;\n'); + stderrLines.length = 0; + const second = capture({ + file: 'src_a.ts', + target: 't', + cache: cachePath, + model: 'model-a', + }); + expect(second.incremental).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('belongs to source path'); + }); + + it('a hostile source path reaches stderr escaped, never raw', () => { + // Mirrors the hostile-lastModelId pin: the refusal interpolates the + // cache's recorded source through `display()`, so a crafted value cannot + // forge warning lines or emit terminal escapes. + seedDirtyTree(); + write('src_foo.ts', 'export const collide = 1;\n'); + const cachePath = promoteCandidate( + capture({ file: 'src_foo.ts', model: 'model-a' }), + 'model-a', + ); + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as Record< + string, + unknown + >; + cache['source'] = 'evil\nWARNING: forged line \u001b[31m'; + writeFileSync(cachePath, JSON.stringify(cache)); + stderrLines.length = 0; + write('src/foo.ts', 'export const real = 1;\n'); + capture({ file: 'src/foo.ts', cache: cachePath, model: 'model-a' }); + const err = stderrLines.join('|'); + expect(err).not.toContain('\u001b'); // no raw ESC byte at the terminal + expect(err).toContain('\\n'); // the newline arrives as an escape, quoted + }); +}); + +describe('capture-local — the local same-model gate', () => { + const A = 'qwen3-max@aaaaaaaa'; + const B = 'qwen3-max@bbbbbbbb'; + + it('records the PROVIDER-QUALIFIED identity in the candidate itself', () => { + // Step 8 used to merge `lastModelId: "{{model}}"` in afterwards, and + // `{{model}}` interpolates the BARE model id. The capture records what + // the runtime published instead, so the token that gets compared is the + // one that distinguishes two providers exposing one model name. + seedDirtyTree(); + const plan = capture({ model: A }); + const candidate = JSON.parse( + readFileSync(plan.cacheCandidatePath, 'utf8'), + ) as { lastModelId?: string }; + expect(candidate.lastModelId).toBe(A); + }); + + it('refuses an anchor another PROVIDER certified under the same name', () => { + // The failure the bare comparison allowed: two provider configurations + // exposing `qwen3-max` compared equal, so provider B honoured provider + // A's anchor and scoped — and then certified — over code only A read. + seedDirtyTree(); + const round1 = capture({ model: A }); + const candidate = JSON.parse( + readFileSync(round1.cacheCandidatePath, 'utf8'), + ) as Record; + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + const cachePath = join(repo, '.qwen/review-cache/local.json'); + // Promoted verbatim — the candidate already carries who certified it. + writeFileSync(cachePath, JSON.stringify(candidate)); + + write(CHANGED, 'export const v = 2;\n'); + const other = capture({ cache: cachePath, model: B }); + expect(other.incremental).toBeUndefined(); + + // …and the same provider still scopes. + const same = capture({ cache: cachePath, model: A }); + expect(same.incremental?.scope?.deltaFiles).toEqual([CHANGED]); + }); + + it('names the fallback when the CACHED identity is empty too', () => { + // `roundModelIdFrom` records `''` when the runtime published nothing — + // reachable in normal operation, not an error state. The refusal must + // print the fallback on the cached side as well; a blank certifier name + // ("reviewed by , not …") reads as a recorded-but-different identity + // when both sides are unrecorded. + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), ''); + write(CHANGED, 'export const v = 2;\n'); + capture({ cache: cachePath, model: 'model-b' }); + expect(stderrLines.join('\n')).toContain( + 'reviewed by an unrecorded model, not model-b', + ); + }); + + it('treats a runtime that published NO identity as a mismatch', () => { + // An unverifiable contract is a failed one: empty never matches, so the + // round degrades to the full capture rather than honouring an anchor it + // cannot attribute. + seedDirtyTree(); + const cachePath = promoteCandidate(capture({ model: A }), A); + write(CHANGED, 'export const v = 2;\n'); + expect( + capture({ cache: cachePath, model: '' }).incremental, + ).toBeUndefined(); + }); +}); + +describe('capture-local — a staged move across rounds', () => { + it('keeps the rename section when only its deleted SOURCE is in scope', () => { + // The capture's pinned flags include `--find-renames`, so a staged move + // comes back as ONE section labelled with the NEW path — a comment here + // once claimed otherwise on the strength of a measurement that did not + // hold. `changedSince` reports the deleted SOURCE (its recorded identity + // is UNHASHABLE, which never equals itself), so on the round after the + // move the keep-set holds the source and no section is labelled with it. + // Matching the new side alone cut the whole section: a zero-byte slice, a + // plan with no chunks, `deltaFiles` naming a path no section carries, and + // the "their sections are in scope" line printed over it. The stop + // sentence cannot fire either, and the candidate re-records the same + // state — so the cycle repeats until HEAD moves. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'round 1 work'); + git('mv', CHANGED, 'src/moved.ts'); + + const round1 = capture(); + expect(round1.files.map((f) => f.path)).toContain('src/moved.ts'); + const cache = promoteCandidate(round1, 'model-a'); + + // Round 2: nothing moved since round 1. + const round2 = capture({ cache, model: 'model-a' }); + const scope = round2.incremental?.scope; + expect(scope).toBeDefined(); + // The source is what changed since the anchor… + expect(scope!.deltaFiles).toContain(CHANGED); + // …and the section it names is PUBLISHED, not sliced away. + const sliced = readFileSync(join(repo, round2.diffPath), 'utf8'); + expect(sliced).toContain(`rename from ${CHANGED}`); + expect(sliced).toContain('rename to src/moved.ts'); + expect(round2.chunks.length).toBeGreaterThan(0); + }); +}); + +describe('capture-local — identity soundness and refusal contract', () => { + it.skipIf(process.platform === 'win32')( + 'an exec-bit flip alone is a change — bytes equal, mode not', + () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + execFileSync('chmod', ['+x', join(repo, CHANGED)]); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual([CHANGED]); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'a retargeted symlink whose new target holds equal bytes is a change', + () => { + seedDirtyTree(); + write('src/t1.txt', 'same\n'); + write('src/t2.txt', 'same\n'); + execFileSync('ln', ['-s', 't1.txt', join(repo, 'src/link')]); + const cachePath = promoteCandidate(capture(), 'model-a'); + rmSync(join(repo, 'src/link')); + execFileSync('ln', ['-s', 't2.txt', join(repo, 'src/link')]); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toContain('src/link'); + }, + ); + + it('a file named __proto__ is tracked like any other', () => { + seedDirtyTree(); + write('__proto__', 'p1\n'); + const round1 = capture(); + const candidate = JSON.parse( + readFileSync(round1.cacheCandidatePath, 'utf8'), + ) as { files: Record }; + expect(Object.keys(candidate.files)).toContain('__proto__'); + const cachePath = promoteCandidate(round1, 'model-a'); + write('__proto__', 'p2\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual(['__proto__']); + }); + + it('an untracked file DELETED since the cached round re-opens its importer', () => { + seedDirtyTree(); + write('src/n.ts', 'export const n = 1;\n'); + write('src/c.ts', "import { n } from './n.js';\nexport const c2 = n;\n"); + const cachePath = promoteCandidate(capture(), 'model-a'); + rmSync(join(repo, 'src/n.ts')); + const plan = capture({ cache: cachePath, model: 'model-a' }); + // n.ts has no diff section left, but its disappearance is a change: the + // importer re-enters through the widening, and the round must NOT stop + // as "no changes". + expect(plan.incremental!.scope!.deltaFiles).toEqual([]); + expect(plan.incremental!.scope!.interaction.map((e) => e.path)).toContain( + 'src/c.ts', + ); + expect(stderrLines.join('\n')).not.toContain('No changes since the last'); + }); + + it('a skipped (oversized) file refuses the incremental path out loud', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + writeFileSync(join(repo, 'huge.bin'), Buffer.alloc(1_100_000, 7)); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeUndefined(); + expect(plan.files.map((f) => f.path).sort()).toEqual([ + BYSTANDER, + CALLER, + CHANGED, + ]); + const err = stderrLines.join('\n'); + expect(err).toContain('SKIPPED'); + expect(err).not.toContain('No changes since the last'); + }); + + it('target and stateId integrity gates refuse, full plan preserved, reason out loud', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as Record< + string, + unknown + >; + writeFileSync(cachePath, JSON.stringify({ ...cache, target: 'other.ts' })); + let plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('belongs to target'); + + stderrLines.length = 0; + const files = { ...(cache['files'] as Record) }; + const k = Object.keys(files)[0]; + files[k] = '100644:0000000000000000000000000000000000000000'; + writeFileSync(cachePath, JSON.stringify({ ...cache, files })); + plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeUndefined(); + expect(plan.files.length).toBe(3); + expect(stderrLines.join('\n')).toContain('stateId does not match'); + }); + + it('refusal reasons for model/HEAD/malformed gates reach stderr verbatim', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + capture({ cache: cachePath, model: 'model-b' }); + expect(stderrLines.join('\n')).toContain( + 'was reviewed by model-a, not model-b', + ); + + stderrLines.length = 0; + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'move head'); + write(CHANGED, 'export const v = 5;\n'); + capture({ cache: cachePath, model: 'model-a' }); + expect(stderrLines.join('\n')).toContain( + 'HEAD moved since the last local round', + ); + + stderrLines.length = 0; + writeFileSync(cachePath, 'not json'); + capture({ cache: cachePath, model: 'model-a' }); + expect(stderrLines.join('\n')).toContain( + 'the cache is missing or unreadable', + ); + }); + + it('the no-change stop and the clean-tree warning stay distinct on stderr', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + stderrLines.length = 0; + capture({ cache: cachePath, model: 'model-a' }); + const err = stderrLines.join('\n'); + expect(err).toContain('No changes since the last local review round'); + expect(err).not.toContain('the working tree is clean'); + }); + + it("a scoped round's candidate still covers EVERY captured file", () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + write(CHANGED, 'export const v = 2;\n'); + const round2 = capture({ cache: cachePath, model: 'model-a' }); + expect(round2.incremental).toBeDefined(); + // The candidate is built from the FULL capture before scoping: promote + // a narrowed one and every scoped-out file reads as changed forever. + const candidate = JSON.parse( + readFileSync(round2.cacheCandidatePath, 'utf8'), + ) as { files: Record }; + expect(Object.keys(candidate.files).sort()).toEqual([ + BYSTANDER, + CALLER, + CHANGED, + ]); + }); +}); + +describe('capture-local — round-2 findings', () => { + it.skipIf(process.platform === 'win32')( + 'a chmod off the USER class alone matches git: the identity moves with old/new mode', + () => { + seedDirtyTree(); + // 0755 cached; 0655 keeps group/other bits but drops the user bit — + // git prints old/new mode for exactly this, so the identity must move. + execFileSync('chmod', ['0755', join(repo, CHANGED)]); + const cachePath = promoteCandidate(capture(), 'model-a'); + execFileSync('chmod', ['0655', join(repo, CHANGED)]); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toEqual([CHANGED]); + }, + ); + + it('an unborn-HEAD cache validates and scopes — null headSha is a supported state', () => { + // A brand-new repo: no commits, everything untracked. + write('.gitignore', '.qwen/\nplan.json\n'); + write(CHANGED, 'export const v = 1;\n'); + const cachePath = promoteCandidate(capture(), 'model-a'); + expect( + (JSON.parse(readFileSync(cachePath, 'utf8')) as { headSha: unknown }) + .headSha, + ).toBeNull(); + write(CHANGED, 'export const v = 2;\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeDefined(); + expect(plan.incremental!.scope!.deltaFiles).toEqual([CHANGED]); + }); + + it('a hostile lastModelId reaches stderr escaped, never raw', () => { + seedDirtyTree(); + const cachePath = promoteCandidate(capture(), 'model-a'); + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as Record< + string, + unknown + >; + cache['lastModelId'] = 'evil\nWARNING: forged line \u001b[31m'; + writeFileSync(cachePath, JSON.stringify(cache)); + capture({ cache: cachePath, model: 'model-b' }); + const err = stderrLines.join('|'); + expect(err).not.toContain('\u001b'); // no raw ESC byte at the terminal + expect(err).toContain('\\n'); // the newline arrives as an escape, quoted + }); +}); + +describe('capture-local — round-3 findings', () => { + it("excludes the review's own plumbing even from a SUBDIRECTORY cwd", () => { + // ls-files returns repo-root-relative paths; the plumbing is written + // relative to the invocation cwd. A root-anchored filter matched nothing + // from a subdirectory, and the cache — rewritten every clean round — + // then changed the state every round by construction. + write('.gitignore', 'nothing-ignored\n'); // .qwen deliberately NOT ignored + write(CHANGED, 'export const v = 1;\n'); + write('sub/keep.ts', 'export const k = 1;\n'); + mkdirSync(join(repo, 'sub'), { recursive: true }); + // PLANT the plumbing at the SUBDIRECTORY path the review writes it to: + // without these the assertion below passes over an empty set and proves + // nothing (measured — it survived removing the cwd-aware prefixes). + write('sub/.qwen/tmp/qwen-review-parse-args.json', '{}\n'); + write('sub/.qwen/review-cache/local.json', '{}\n'); + write('sub/.qwen/reviews/2026-01-01-local.md', '# report\n'); + const prev = process.cwd(); + process.chdir(join(repo, 'sub')); + try { + const out = join(repo, 'sub/plan.json'); + (captureLocalCommand.handler as (argv: unknown) => void)({ + out, + target: 'local', + untracked: true, + }); + const plan = JSON.parse(readFileSync(out, 'utf8')) as Plan; + const paths = plan.files.map((f) => f.path); + expect(paths.some((p) => p.includes('.qwen/'))).toBe(false); + expect(paths).toContain('sub/keep.ts'); + } finally { + process.chdir(prev); + } + }); + + it('excludes .qwen/review-cache and .qwen/reviews, not just .qwen/tmp', () => { + write('.gitignore', 'nothing-ignored\n'); + write(CHANGED, 'export const v = 1;\n'); + write('.qwen/review-cache/local.json', '{}\n'); + write('.qwen/reviews/2026-01-01-local.md', '# report\n'); + const out = join(repo, 'plan.json'); + (captureLocalCommand.handler as (argv: unknown) => void)({ + out, + target: 'local', + untracked: true, + }); + const plan = JSON.parse(readFileSync(out, 'utf8')) as Plan; + expect( + plan.files.map((f) => f.path).some((p) => p.startsWith('.qwen/')), + ).toBe(false); + }); + + it.skipIf(process.platform === 'win32')( + 'a symlink retargeted to non-UTF-8 bytes is a change — the raw-bytes identity', + () => { + seedDirtyTree(); + // Two targets that differ only in invalid-UTF-8 bytes: a lossy decode + // collapses both to U+FFFD and the identity would hold still. + const linkPath = join(repo, 'src/link'); + // Buffer targets: `execFileSync`/`ln` re-encode a JS string as UTF-8 + // and never put the invalid bytes on disk — the shape this fix exists + // for would go untested. + symlinkSync(Buffer.from([0xff, 0x2e, 0x74]), linkPath); + const cachePath = promoteCandidate(capture(), 'model-a'); + rmSync(linkPath); + symlinkSync(Buffer.from([0xfe, 0x2e, 0x74]), linkPath); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental!.scope!.deltaFiles).toContain('src/link'); + }, + ); + + it('a null→string HEAD transition refuses like any other moved HEAD', () => { + // Unborn HEAD at round 1 (cache records null), first commit before + // round 2: the same worktree bytes now describe a different change. + write('.gitignore', '.qwen/\nplan.json\n'); + write(CHANGED, 'export const v = 1;\n'); + const cachePath = promoteCandidate(capture(), 'model-a'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'first commit'); + write(CHANGED, 'export const v = 2;\n'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan.incremental).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('HEAD moved'); + }); +}); + +describe('capture-local — an ignore rule between rounds is visibility, not deletion', () => { + it('refuses the anchor for a cached path still on disk but dropped from the capture', () => { + // An ignore rule added between rounds narrows the capture exactly like + // `--no-untracked` — `ls-files --others --exclude-standard` stops + // enumerating the path — while no flag changed, so the flag clause sees + // nothing. The cached path then reads as "vanished": the slice keeps zero + // sections and the scope-emptied stop fired over bytes no round + // captured, repeating every round because a stop never advances the + // cache. The only vanished-equals-change class is a path GONE from disk; + // one that still exists is invisible content, and the round must fall + // back to the full capture. + seedDirtyTree(); + write('deploy.sh', '#!/bin/sh\necho v1\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + + write('.git/info/exclude', 'deploy.sh\n'); + write('deploy.sh', '#!/bin/sh\necho v2, edited while invisible\n'); + + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second.incremental).toBeUndefined(); + expect(second['nothingToReview']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('still on disk'); + }); +}); + +describe('capture-local — round-10 anchor shapes', () => { + it('refuses the anchor when a vanished tracked path\u2019s bytes diverge from HEAD', () => { + // R10-1: `git update-index --assume-unchanged` hides the edited tracked + // file from `git diff HEAD` while `ls-tree HEAD` still names it, so the + // guard\u2019s old name-membership check certified a divergence no round + // ever read: the slice kept zero sections and the scope-emptied stop + // fired DECIDED over those bytes, repeating every round. Certify by + // BYTES instead \u2014 the path\u2019s worktree hash must equal its HEAD-tree + // identity \u2014 and a hidden divergence refuses the anchor. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + git('update-index', '--assume-unchanged', CHANGED); + write(CHANGED, 'export const v = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + + expect(second.incremental).toBeUndefined(); + expect(second['nothingToReview']).not.toEqual({ reason: 'scope-emptied' }); + expect(stderrLines.join('\n')).toContain('still on disk'); + + // Control: the designed discarded-change shape still certifies — bytes + // equal HEAD — and reaches the scope-emptied stop. + git('update-index', '--no-assume-unchanged', CHANGED); + git('checkout', '--', CHANGED, CALLER, BYSTANDER); + const third = capture({ cache: cachePath, model: 'model-a' }); + expect(third['nothingToReview']).toEqual({ reason: 'scope-emptied' }); + }); +}); + +describe('capture-local — round-12 stop shapes', () => { + it('a hidden divergence (--assume-unchanged edit) suppresses the clean-tree stop', () => { + // R11-5: the refusal proved a path diverges while invisible to the + // capture (`git diff HEAD` honours the bit), and `hash-object` reads + // through it — so the stop decided clean-tree while the date read the + // hidden edit as "the blocker moved", and `--fail-on` exited 0 over a + // standing Critical. A `vanishedStillOnDisk` refusal gets the same + // standing as `treeHeldStill: false` for the stop decision: no field, + // no sidecar, the not-clean warning shape. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + recordOpenCritical(cachePath); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'commit without the fix'); + git('update-index', '--assume-unchanged', CHANGED); + write(CHANGED, 'export const v = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + const err = stderrLines.join('\n'); + expect(err).toContain('still on disk'); + expect(err).not.toContain('the working tree is clean'); + expect(err).toContain('this is NOT a clean tree'); + }); +}); + +describe('capture-local — round-13 findings: visibility bits and empty anchors', () => { + it('a FILE review with no diff still anchors its subject (R13-2)', () => { + // The no-diff shape promoted an EMPTY files map, and an + // `--assume-unchanged` edit on the subject then hid from `git diff + // HEAD`: `changedSince` over the empty maps certified the + // "unchanged-since-last-round" stop over bytes no round ever read. + // `hash-object` reads through the bit, so the subject enters the anchor + // even without a diff section. + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/foo.ts', 'export const v = 0;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + + const first = capture({ file: 'src/foo.ts', model: 'model-a' }); + const candidate = JSON.parse( + readFileSync(first.cacheCandidatePath, 'utf8'), + ) as { files: Record }; + expect(Object.keys(candidate.files)).toEqual(['src/foo.ts']); + + mkdirSync(join(repo, '.qwen/review-cache'), { recursive: true }); + writeFileSync( + first['cachePath'] as string, + readFileSync(first.cacheCandidatePath, 'utf8'), + ); + + git('update-index', '--assume-unchanged', 'src/foo.ts'); + write('src/foo.ts', 'export const v = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ + file: 'src/foo.ts', + cache: join(repo, '.qwen/review-cache'), + model: 'model-a', + }); + // The hidden edit moved the subject's identity: re-reviewed, never + // stopped over. + expect(second['nothingToReview']).toBeUndefined(); + expect(second.incremental).toBeDefined(); + expect(second.incremental!.scope!.deltaFiles).toContain('src/foo.ts'); + expect(stderrLines.join('\n')).not.toContain('nothing to re-review'); + }); + + it('a cache with an empty files map refuses the anchor (R13-2)', () => { + // A no-diff whole-tree round promotes an empty files map, and + // `changedSince` over two empty maps answers "unchanged" under ANY tree + // state the capture cannot see — an anchor with no identities certifies + // nothing, so the gate refuses it out loud. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + const first = capture({ model: 'model-a' }); + expect(first['nothingToReview']).toEqual({ reason: 'clean-tree' }); + const candidate = JSON.parse( + readFileSync(first.cacheCandidatePath, 'utf8'), + ) as { files: Record }; + expect(Object.keys(candidate.files)).toEqual([]); + const cachePath = promoteCandidate(first, 'model-a'); + + git('update-index', '--assume-unchanged', BYSTANDER); + write(BYSTANDER, 'export const b = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second.incremental).toBeUndefined(); + expect(second['nothingToReview']).toBeUndefined(); + const err = stderrLines.join('\n'); + expect(err).toContain('recorded no file identities'); + // The visibility guard withholds the clean-tree stop on the fallback + // capture too, and names the bit. + expect(err).toContain('this is NOT a clean tree'); + expect(err).not.toContain('the working tree is clean'); + }); + + it('a hidden edit OUTSIDE the cached paths suppresses the unchanged stop (R13-6)', () => { + // The defence iterated `cache.files` keys only, so a hidden edit on a + // path the cached round did NOT review (clean at cache time) was + // enumerated by no gate: every comparison stood still and the round + // stopped decided over bytes no round ever read. + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/a.ts', 'export const a = 1;\n'); + write('src/b.ts', 'export const b = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + write('src/a.ts', 'export const a = 2;\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + + // The anchored file stands still (still dirty, byte-identical); b.ts + // gets edited behind the bit. + git('update-index', '--assume-unchanged', 'src/b.ts'); + write('src/b.ts', 'export const b = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + const err = stderrLines.join('\n'); + expect(err).toContain('carry an --assume-unchanged or'); + expect(err).not.toContain('nothing to re-review'); + + // Control: with the bit cleared (and the edit discarded) the stop fires. + git('update-index', '--no-assume-unchanged', 'src/b.ts'); + git('checkout', '--', 'src/b.ts'); + const third = capture({ cache: cachePath, model: 'model-a' }); + expect(third['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + }); + + it('a hidden edit suppresses the CLEAN-TREE stop — no cache involved (R13-6)', () => { + // The clean-tree claim needs no cache, so neither did this entrance: + // "nothing staged, nothing unstaged, nothing untracked" proves nothing + // while a marked path may carry an edit `git diff` cannot see. + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/a.ts', 'export const a = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + git('update-index', '--assume-unchanged', 'src/a.ts'); + write('src/a.ts', 'export const a = 999; // hidden edit\n'); + + stderrLines.length = 0; + const plan = capture(); + expect(plan['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + const err = stderrLines.join('\n'); + expect(err).not.toContain('the working tree is clean'); + expect(err).toContain('this is NOT a clean tree'); + expect(err).toContain('--assume-unchanged/--skip-worktree'); + + // Control: the genuinely clean tree still stops. + git('update-index', '--no-assume-unchanged', 'src/a.ts'); + git('checkout', '--', 'src/a.ts'); + const clean = capture(); + expect(clean['nothingToReview']).toEqual({ reason: 'clean-tree' }); + }); + + it('a hidden edit suppresses the scope-emptied stop (R13-6)', () => { + // The designed all-discarded shape, plus one path the cached round never + // reviewed hiding an edit: the empty slice proves only what `git diff` + // can see, so the stop is withheld and the refusal is said out loud. + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/a.ts', 'export const a = 1;\n'); + write('src/extra.ts', 'export const e = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + write('src/a.ts', 'export const a = 2;\n'); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + + git('checkout', '--', 'src/a.ts'); + git('update-index', '--assume-unchanged', 'src/extra.ts'); + write('src/extra.ts', 'export const e = 999; // hidden edit\n'); + + stderrLines.length = 0; + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['nothingToReview']).toBeUndefined(); + expect(stderrLines.join('\n')).toContain('carry an --assume-unchanged or'); + + // Control: without the bit the all-discarded shape still stops. + git('update-index', '--no-assume-unchanged', 'src/extra.ts'); + git('checkout', '--', 'src/extra.ts'); + const third = capture({ cache: cachePath, model: 'model-a' }); + expect(third['nothingToReview']).toEqual({ reason: 'scope-emptied' }); + }); +}); + +describe('capture-local — round-15 findings: the candidate under visibility bits', () => { + it('a visibility bit withholds the cache candidate (R14-1)', () => { + // The three decided stops are conditioned on the visibility bits, but + // the candidate write was not: `hash-object` reads the worktree bytes + // THROUGH a set bit while `git diff` cannot see them, so the candidate + // recorded the identity of bytes the round's diff never showed. + // Promoted, they became anchor state — and when the bit was cleared + // between rounds keeping the bytes, every comparison found no change, + // every visibility gate read clean, and the unchanged-since stop + // certified them: the loop decided "nothing to re-review" over bytes no + // round ever read. The same uncertainty that withholds a stop withholds + // the candidate. + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/foo.ts', 'export const v = 0;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + // A staged edit — visible to `git diff HEAD`. + write('src/foo.ts', 'export const v = 1;\n'); + git('add', 'src/foo.ts'); + const plain = capture({ model: 'model-a' }); + expect(existsSync(plain.cacheCandidatePath)).toBe(true); + + // A further edit hidden behind the bit on the SAME file: the diff still + // shows the staged hunk alone, while the candidate hashes read through. + git('update-index', '--assume-unchanged', 'src/foo.ts'); + write('src/foo.ts', 'export const v = 999; // hidden edit\n'); + stderrLines.length = 0; + const hidden = capture({ model: 'model-a' }); + // Withheld — including the unlink of the earlier round's candidate, + // whose name this plan publishes and Step 8 would otherwise promote. + expect(existsSync(hidden.cacheCandidatePath)).toBe(false); + const err = stderrLines.join('\n'); + expect(err).toContain('carry an --assume-unchanged or'); + expect(err).toContain('the cache candidate is withheld'); + // The round itself still proceeds on the first capture — only the + // anchor is withheld. + expect(hidden.chunks.length).toBeGreaterThan(0); + + // The hole the withholding closes, end to end: nothing was promoted, so + // clearing the bit between rounds keeping the bytes cannot produce an + // "unchanged since last round" stop over the hidden bytes — the next + // round captures full and the now-visible edit is in scope. + git('update-index', '--no-assume-unchanged', 'src/foo.ts'); + stderrLines.length = 0; + const next = capture({ + cache: join(repo, '.qwen/review-cache'), + model: 'model-a', + }); + expect(next['nothingToReview']).toBeUndefined(); + expect(next.incremental).toBeUndefined(); + expect(readFileSync(join(repo, next.diffPath), 'utf8')).toContain( + 'hidden edit', + ); + }); +}); diff --git a/packages/cli/src/commands/review/capture-local.test.ts b/packages/cli/src/commands/review/capture-local.test.ts index 0646bc33f44..8a1707c1496 100644 --- a/packages/cli/src/commands/review/capture-local.test.ts +++ b/packages/cli/src/commands/review/capture-local.test.ts @@ -19,6 +19,7 @@ import { DEADLINE_ENV } from './lib/deadline.js'; const captureMock = vi.hoisted(() => vi.fn()); const settingsMock = vi.hoisted(() => vi.fn(() => ({ merged: {} }))); +const visibilityMock = vi.hoisted(() => vi.fn((): string[] | null => [])); vi.mock('../../config/settings.js', async (orig) => ({ ...(await orig>()), loadSettings: settingsMock, @@ -27,6 +28,15 @@ vi.mock('./lib/local-diff.js', async (orig) => ({ ...(await orig>()), captureLocalDiff: captureMock, })); +// The git layer is tested elsewhere (the integration suites run a real +// repository); the scratch directory here is not one. The visibility-bit +// oracle answers "no tracked path carries a bit" — the shape a clean tree +// has — because without an answer the stops must fail closed, and the +// tests below pin the clean claim. +vi.mock('./lib/local-anchor.js', async (orig) => ({ + ...(await orig>()), + invisibleTrackedPaths: visibilityMock, +})); const { captureLocalCommand } = await import('./capture-local.js'); @@ -69,6 +79,8 @@ beforeEach(() => { cwd = process.cwd(); process.chdir(dir); errs = []; + visibilityMock.mockReset(); + visibilityMock.mockReturnValue([] as string[]); vi.spyOn(process.stderr, 'write').mockImplementation((chunk) => { errs.push(String(chunk)); return true; @@ -83,6 +95,40 @@ afterEach(() => { rmSync(dir, { recursive: true, force: true }); }); +describe('capture-local — the re-captures\u2019 skipped lists ride the guard', () => { + it('withholds the stop when only a RE-capture skipped content', () => { + // R21-2: the sampling loop kept only `.diff` from re-captures 1 and 2 — + // an unreviewable file entering the window lands in `skipped`, never in + // the diff BYTES, so the byte comparison read "held still" and the + // decided stops fired over content two of the three captures skipped. + // Skip-set movement is tree movement. + let call = 0; + captureMock.mockImplementation(() => { + call += 1; + return { + diff: Buffer.from('', 'utf8'), + untracked: [], + skipped: + call === 1 + ? [] + : [{ path: 'huge.bin', bytes: 1, reason: 'over the cap' }], + unbornHead: false, + repoRoot: dir, + }; + }); + run('plan.json'); + + const plan = JSON.parse(readFileSync(join(dir, 'plan.json'), 'utf8')); + expect(plan.nothingToReview).toBeUndefined(); + expect(existsSync(join(dir, '.qwen/tmp/qwen-review-local-stop.json'))).toBe( + false, + ); + expect(errs.join('')).toContain( + 'the working tree changed while the capture was being hashed', + ); + }); +}); + describe('capture-local (command boundary)', () => { it('writes the diff and a plan the review can read', () => { capture(); @@ -209,6 +255,32 @@ describe('capture-local (command boundary)', () => { expect(plan.effort).toBeUndefined(); }); + it('withholds the cache candidate when the visibility bits cannot be enumerated', () => { + // The candidate records the identity of the tree this round reviewed; + // an oracle the capture cannot run leaves that identity uncertified, so + // the write fails closed exactly like the decided stops do. + capture(); + visibilityMock.mockReturnValue(null); + run('plan.json'); + expect( + existsSync(join(dir, '.qwen/tmp/qwen-review-local-cache-candidate.json')), + ).toBe(false); + expect(errs.join('')).toContain('could not be enumerated'); + }); + + it('withholds the cache candidate while tracked paths carry a visibility bit', () => { + // `hash-object` reads through a set --assume-unchanged/--skip-worktree + // bit while `git diff` cannot see the edit it hides — the candidate + // would record the identity of bytes this round never reviewed. + capture(); + visibilityMock.mockReturnValue(['src/pay.ts']); + run('plan.json'); + expect( + existsSync(join(dir, '.qwen/tmp/qwen-review-local-cache-candidate.json')), + ).toBe(false); + expect(errs.join('')).toContain('the cache candidate is withheld'); + }); + it('escapes a filename carrying terminal control characters', () => { // A filename is workspace-controlled, and git permits an ESC or a newline in // one. Printed raw it can forge a second warning line or drive the user's diff --git a/packages/cli/src/commands/review/capture-local.toctou.test.ts b/packages/cli/src/commands/review/capture-local.toctou.test.ts new file mode 100644 index 00000000000..632a815597f --- /dev/null +++ b/packages/cli/src/commands/review/capture-local.toctou.test.ts @@ -0,0 +1,311 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The TOCTOU withhold branch, in isolation: when the re-capture after hashing +// returns different bytes, the candidate must NOT be written and the refusal +// must be said out loud — the one uncertainty in the anchor module that used +// to fail open. The capture layer is mocked with a stateful fake so the two +// captures can disagree deterministically; everything downstream is real. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + mkdtempSync, + rmSync, + writeFileSync, + readFileSync, + existsSync, + realpathSync, +} from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const stderrLines: string[] = []; +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: vi.fn(), + writeStderrLine: vi.fn((line: string) => { + stderrLines.push(line); + }), + writeStderrLineSafe: vi.fn(), +})); + +const captures: Array<{ diff: Buffer }> = []; +/** + * Successive `hashWorktreeFiles` answers, when a test needs the passes to + * disagree. Empty means "use the real one" — every other test in this file + * hashes for real. A list shorter than the number of passes REPEATS its last + * entry, which reads as "the tree stopped moving": a fixture says what it is + * about and the guard's extra samples see a settled tree. + */ +const hashPasses: Array> = []; +vi.mock('./lib/local-anchor.js', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + hashWorktreeFiles: (...args: Parameters) => + hashPasses.length > 0 + ? ((hashPasses.length > 1 + ? hashPasses.shift() + : hashPasses[0]) as Record) + : real.hashWorktreeFiles(...args), + }; +}); +vi.mock('./lib/local-diff.js', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + captureLocalDiff: vi.fn(() => { + const next = captures.length > 1 ? captures.shift() : captures[0]; + if (!next) throw new Error('fixture exhausted'); + return { + diff: next.diff, + untracked: [], + skipped: [], + unbornHead: false, + repoRoot: repo, + }; + }), + }; +}); + +import { captureLocalCommand } from './capture-local.js'; +import { isolateHostGitConfig } from './lib/test-utils.js'; + +let repo: string; +let cwd: string; +let gitIsolation: ReturnType; + +beforeEach(() => { + stderrLines.length = 0; + captures.length = 0; + hashPasses.length = 0; + repo = realpathSync(mkdtempSync(join(tmpdir(), 'review-toctou-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + const git = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }); + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'commit.gpgsign', 'false'); + writeFileSync(join(repo, 'a.ts'), 'export const a = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); +}); + +afterEach(() => { + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +const DIFF_A = Buffer.from( + 'diff --git a/a.ts b/a.ts\nindex 000..111 100644\n--- a/a.ts\n+++ b/a.ts\n@@ -1,1 +1,1 @@\n-export const a = 1;\n+export const a = 2;\n', + 'utf8', +); + +function run(extra: Record = {}): void { + (captureLocalCommand.handler as (argv: unknown) => void)({ + out: join(repo, 'plan.json'), + target: 'local', + untracked: true, + ...extra, + }); +} + +/** Read the plan report `run()` just wrote. */ +function report(): { incremental?: unknown; diffPath: string } { + return JSON.parse(readFileSync(join(repo, 'plan.json'), 'utf8')) as { + incremental?: unknown; + diffPath: string; + }; +} + +describe('capture-local — TOCTOU candidate withholding', () => { + it('a tree that moved between capture and hash withholds the candidate, out loud', () => { + captures.push( + { diff: DIFF_A }, + { diff: Buffer.from('changed mid-hash\n') }, + ); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + expect(stderrLines.join('\n')).toContain( + 'working tree changed while the capture was being hashed', + ); + }); + + it('a withhold REMOVES an earlier candidate left at the stable path', () => { + // The candidate path is stable per target: round A's candidate still + // sits there when round B withholds, and round B's plan still publishes + // `cacheCandidatePath` — so Step 8 read the stale file and promoted + // round A's anchor merged with round B's ledger. The withhold must + // leave the published path actually ABSENT. + captures.push({ diff: DIFF_A }, { diff: Buffer.from(DIFF_A) }); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(true); + + // Round B: the tree moves under the hash pass — the withhold path. + captures.push( + { diff: DIFF_A }, + { diff: Buffer.from('changed mid-hash\n') }, + ); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + }); + + it('a moved tree refuses THIS round\u2019s scoping too, not just the candidate', () => { + // Withholding only the candidate protects the NEXT round and leaves this + // one wrong: the scoping compares the very hashes the guard just proved + // may not describe the capture under review. A file edited during the + // hash pass and reverted before it is hashed reads as unchanged, + // `changedSince` reports nothing, and its diff section is sliced out — + // the round then says "nothing to re-review" over a capture no agent + // read. Promote a real candidate first, so the anchor is otherwise + // valid and the refusal can only come from the guard. + captures.push({ diff: DIFF_A }, { diff: Buffer.from(DIFF_A) }); + run({ model: 'model-a' }); + const cachePath = join(repo, 'cache.json'); + const promoted = JSON.parse( + readFileSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + 'utf8', + ), + ) as Record; + writeFileSync( + cachePath, + JSON.stringify({ ...promoted, lastModelId: 'model-a' }), + ); + + // Round 2: same anchor, but the tree moves under the hash pass. + stderrLines.length = 0; + captures.push( + { diff: DIFF_A }, + { diff: Buffer.from('changed mid-hash\n') }, + ); + run({ model: 'model-a', cache: cachePath }); + + expect(report().incremental).toBeUndefined(); + expect(stderrLines.join('\n')).toContain( + 'Incremental anchor not used — the working tree changed while the ' + + 'capture was being hashed', + ); + // The full capture is what the plan reviews. + expect(readFileSync(report().diffPath).equals(DIFF_A)).toBe(true); + }); + + it('does not call a MOVED tree clean, even with an empty capture', () => { + // The two stops contradicted each other. A review starting on an empty + // tree, with an autosave landing inside the capture window, withholds the + // candidate and refuses the anchor — and then wrote `clean-tree` anyway, + // because capture 0's diff is empty. stderr printed both lines back to + // back, the round stopped on the second, and the just-written change went + // unreviewed while the run was recorded as clean. + captures.push( + { diff: Buffer.from('') }, + { diff: DIFF_A }, + { diff: DIFF_A }, + ); + run(); + const plan = JSON.parse( + readFileSync(join(repo, 'plan.json'), 'utf8'), + ) as Record; + expect(plan['nothingToReview']).toBeUndefined(); + const err = stderrLines.join('\n'); + expect(err).toContain( + 'working tree changed while the capture was being hashed', + ); + // …and the PROSE must not contradict it either. The orchestrator branches + // on these sentences, and the round printed "the working tree is clean" + // right after the line above until this was gated too. + expect(err).not.toContain('the working tree is clean'); + expect(err).toContain('this is NOT a clean tree'); + }); + + it('catches a PHASE-ALIGNED write the pairwise guard let through', () => { + // Two samples of each kind, compared pairwise, never tied a capture to + // the hashes recorded beside it. Three timed writes defeat it: X→Y + // before the hash pass, Y→X before the re-capture, X→Y after it. The two + // captures agree (X, X) and the two hash passes agree (Y, Y), so + // `treeHeldStill` is true — and the candidate certifies Y's identity for + // a round that reviewed X. Promoted, the next round compares cache Y + // against tree Y, finds no delta and says "No changes" over bytes no + // round ever read. + // + // Interleaving a third sample of each kind means the write pattern has + // to keep alternating; this one stops, and the third hash pass reads + // what the captures did. + captures.push( + { diff: DIFF_A }, + { diff: Buffer.from(DIFF_A) }, + { diff: Buffer.from(DIFF_A) }, + ); + hashPasses.push( + { 'a.ts': '100644:oid-Y' }, + { 'a.ts': '100644:oid-Y' }, + { 'a.ts': '100644:oid-X' }, + ); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + expect(stderrLines.join('\n')).toContain( + 'working tree changed while the capture was being hashed', + ); + }); + + it('catches a same-bytes revert that STRADDLES the hash pass', () => { + // The hash pass sits BETWEEN the two diff snapshots, so a write that + // straddles it is invisible to the diffs alone: capture B0 → autosave + // writes B1 → the hashes read B1 → undo restores B0 → the re-capture + // reads B0. Both diffs agree and the candidate certifies B1's identity + // for a round that reviewed B0. + // + // The note here used to call that shape harmless and a + // different-bytes revert the uncatchable one — backwards: a + // different-bytes revert moves the endpoints and IS caught by the + // diffs. Re-hashing after the re-capture is what sees this one. + captures.push({ diff: DIFF_A }, { diff: Buffer.from(DIFF_A) }); + // The two diffs AGREE — that is the point. What disagrees is the pair of + // hash passes that bracket the re-capture: the first read B1, the second + // reads B0. + hashPasses.push({ 'a.ts': '100644:oid-B1' }, { 'a.ts': '100644:oid-B0' }); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + expect(stderrLines.join('\n')).toContain( + 'working tree changed while the capture was being hashed', + ); + }); + + it('a tree that held still writes the candidate and no warning', () => { + captures.push({ diff: DIFF_A }, { diff: Buffer.from(DIFF_A) }); + run(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(true); + expect(stderrLines.join('\n')).not.toContain('candidate is withheld'); + }); +}); diff --git a/packages/cli/src/commands/review/capture-local.ts b/packages/cli/src/commands/review/capture-local.ts index aa4f75ba3e6..bea2c817da1 100644 --- a/packages/cli/src/commands/review/capture-local.ts +++ b/packages/cli/src/commands/review/capture-local.ts @@ -17,15 +17,35 @@ // new file reported "no changes to review". import type { CommandModule } from 'yargs'; -import { mkdirSync, writeFileSync } from 'node:fs'; -import { dirname, resolve } from 'node:path'; +import { + existsSync, + lstatSync, + mkdirSync, + readFileSync, + statSync, + unlinkSync, + writeFileSync, +} from 'node:fs'; +import { createHash } from 'node:crypto'; +import { basename, dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; -import { REVIEW_TMP_DIR, tmpFile } from './lib/paths.js'; +import { + repoRelativeOf, + REVIEW_CACHE_DIR, + REVIEW_TMP_DIR, + tmpFile, +} from './lib/paths.js'; +import { safeTarget } from '../../utils/paths.js'; import { planEffortField } from './lib/effort.js'; import { EFFORT_OPTION, type ReviewEffort } from './parse-args.js'; import { captureLocalDiff, type SkippedFile } from './lib/local-diff.js'; -import { buildDiffPlan, READ_FILE_CHAR_CAP } from './lib/diff-plan.js'; import { + buildDiffPlan, + sliceDiffByLines, + READ_FILE_CHAR_CAP, +} from './lib/diff-plan.js'; +import { + type IncrementalBlock, buildPlanReport, warnOnReportSize, stringifyPlanReport, @@ -33,6 +53,23 @@ import { } from './lib/report.js'; import { operatorReviewSettings } from './lib/review-settings.js'; import { hasReviewDeadline } from './lib/deadline.js'; +import { gitOpt } from './lib/git.js'; +import { certifierMatchesRound, roundModelIdFrom } from './lib/round-model.js'; +import { + changedSince, + invisibleTrackedPaths, + movedSince, + hashWorktreeFiles, + readLocalCache, + revisionIdentities, + stateIdOf, + UNHASHABLE, + type LocalCacheCandidate, +} from './lib/local-anchor.js'; +import { + dependentsOfChanged, + discoverWorkspacePackages, +} from './lib/import-graph.js'; interface CaptureLocalArgs { out: string; @@ -40,9 +77,17 @@ interface CaptureLocalArgs { target: string; untracked: boolean; effort?: ReviewEffort; + cache?: string; } type CaptureLocalResult = PlanReport & { + /** + * The review's target token, as the CLI derived it — the stem every other + * artifact of this round must carry. Read it; do not recompute it. The + * plan's own `--out` is the one name the caller may choose freely, because + * the caller both writes and reads that one. + */ + target: string; /** The review's effort, recorded so the roster reads one value everywhere. */ effort?: ReviewEffort; diffPath: string; @@ -51,6 +96,27 @@ type CaptureLocalResult = PlanReport & { untrackedFiles: string[]; /** Untracked files that were NOT reviewed. Named, never silently dropped. */ skippedFiles: SkippedFile[]; + /** Present only when `--cache` scoped this capture incrementally. */ + incremental?: IncrementalBlock; + /** Where this round's content anchor landed — Step 8 promotes it on a clean run. */ + cacheCandidatePath: string; + /** + * The written candidate's own `stateId`, for Step 8 to CHECK before + * promoting. The candidate path is stable per target and local/file + * reviews take no lease, so a concurrent same-target run overwrites the + * file mid-round — indistinguishable by path, mtime or shape. A candidate + * whose stateId no longer matches this field is another run's: treat it + * exactly like a withheld candidate and say so (R17-4). Absent when the + * candidate was withheld. + */ + cacheCandidateStateId?: string; + /** + * Where this target's review cache lives — resolved here, not predicted. + * + * Every ledger read and the Step 8 write name `.json`, and + * `target` does not exist until this command derives it from `--file`. + */ + cachePath: string; }; /** @@ -69,14 +135,402 @@ function display(path: string): string { return CONTROL.test(path) ? JSON.stringify(path) : path; } +/** + * Cached paths that dropped out of THIS capture while still on disk — and + * that the base HEAD does not certify. + * + * A path the cached round hashed that this one no longer sees is normally a + * deletion, which the symmetric difference rightly treats as a change. But + * "still on disk and out of the capture" is not a deletion — it is a capture + * that stopped SEEING the path: an ignore rule added between rounds is the + * live case (`ls-files --others --exclude-standard` stops enumerating it), + * and the flag clause in `anchorRefusalReason` cannot see it because no flag + * changed. Such a path reads as "vanished", the slice keeps zero sections, + * and the scope-emptied stop fires over bytes no round captured — decided, + * and repeated every round, because a stop never advances the cache. + * + * The discriminator is the base HEAD, asked of BYTES, not of names: a + * TRACKED path left the diff because its bytes now equal the tree the diff + * is taken against, and only that equality certifies it — the designed + * discarded-change shape the scope-emptied stop decides. Naming the path in + * HEAD's tree is not the same fact: under + * `git update-index --assume-unchanged` (and `--skip-worktree`) git hides + * the edited tracked file from `git diff HEAD` while `ls-tree HEAD` still + * names it, so a name check certified a divergence no round ever read. A + * path whose worktree bytes do not byte-compare equal to its HEAD identity + * — or that either side cannot hash — is refused, at the cost of a full + * round. + */ +/** + * Whether a FILE review's subject is a directory — the one shape that must + * not enter the anchor's hashed population (see the call site). Only a + * confirmed directory answers true: an unmeasurable path keeps the + * pre-existing behaviour rather than silently dropping the subject's + * coverage, the opposite lean from the ENOENT-only rules elsewhere because + * the risk here is a poisoned anchor, not a false certification. + */ +function isDirectorySubject(repoRoot: string, rel: string): boolean { + try { + return lstatSync(join(repoRoot, rel)).isDirectory(); + } catch { + return false; + } +} + +function vanishedStillOnDisk( + repoRoot: string, + headSha: string | null, + cachedFiles: Record, + currentHashes: Record, +): string[] { + const onDisk: string[] = []; + for (const p of Object.keys(cachedFiles)) { + if (Object.hasOwn(currentHashes, p)) continue; + try { + lstatSync(join(repoRoot, p)); + } catch (err) { + // Only ENOENT proves deletion. Any other failure — EACCES under an + // unreadable ancestor, ELOOP, EIO — is a path that may well still + // hold the cached bytes, and folding it into "gone" let the round end + // at a DECIDED scope-emptied stop over bytes no round captured + // (R19-3). Unmeasurable is uncertifiable: it stays in the on-disk + // set, whose downstream is a refusal at the cost of a full round. + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + continue; // genuinely gone — the symmetric difference owns it + } + } + onDisk.push(p); + } + if (onDisk.length === 0) return []; + // Both identities in the one format this module compares + // (`revisionIdentities` mirrors the worktree hasher exactly). An unborn + // HEAD has no tree and answers nothing: no certification, so everything + // still on disk refuses — over-review is the affordable direction, same + // as a failed listing. + const worktree = hashWorktreeFiles(repoRoot, onDisk); + const head = revisionIdentities(repoRoot, headSha, onDisk); + // `core.fileMode=false` makes git itself ignore the EXEC bit — the stored + // tree keeps one mode while the worktree lstat reports another and + // `git diff HEAD` stays empty. Certifying by the FULL identity then + // refused every such path (and the stop suppression beside it withheld + // every stop) over a divergence git does not recognise: fold 100755 into + // 100644 and compare. + // + // The flag is read `--type=bool`, never raw: `--get` echoes the STORED + // spelling and git accepts `off`/`no`/`0`/`FALSE`, every one of which + // failed a `!== 'false'` test and silently disabled the fold (R20-1 — the + // same misreading R18-2 fixed for `core.sparseCheckout`). Only an + // EXPLICIT false folds: the knob defaults to true, and an unset one must + // not erase a real exec divergence from the comparison. + // + // DISCLOSED, not folded: `core.symlinks=false` erases the file↔symlink + // TYPE the same way — git materializes a tracked symlink as a regular + // file, so the worktree side reads `100644::` against HEAD's + // `120000:` and the designed discarded-change shape never certifies + // (R20-5). A mode fold does NOT close it: the two spellings also differ + // in whether they carry rendering attributes at all, so folding the mode + // leaves them unequal, and equalizing the attributes would mean dropping + // the rendering dimension for that path — trading a bounded over-review + // for a certification this layer cannot make. The cost is that such a + // path is re-reviewed every round on repos with the knob off; the + // direction is the affordable one, as everywhere else here. + const foldsExec = + gitOpt( + '-C', + repoRoot, + 'config', + '--type=bool', + '--get', + 'core.fileMode', + ) === 'false'; + const identity = (id: string | undefined): string | undefined => + foldsExec && id !== undefined && id.startsWith('100755:') + ? `100644:${id.slice('100755:'.length)}` + : id; + // A path unhashable on the WORKTREE side refuses — unmeasurable is + // uncertifiable. Submodule gitlinks are no longer that shape: both sides + // answer `160000:` for a readable, content-clean submodule (see + // `gitlinkIdentity` / `revisionIdentities`), so a restored pointer + // certifies through the ordinary equality below, and a dirty or + // unreadable one stays UNHASHABLE and refuses — the R20-3/R22-1 pair, + // closed by making the identity real instead of special-casing the + // placeholder. + return onDisk.filter( + (path) => + identity(worktree[path]) !== identity(head[path]) || + worktree[path] === UNHASHABLE, + ); +} + +/** + * Why a decided stop is withheld when tracked paths carry a visibility bit — + * or when the bits themselves could not be enumerated. + */ +function invisibleStopRefusal(invisible: string[] | null): string { + return invisible === null + ? 'The tracked-file visibility bits could not be enumerated ' + + '(`git ls-files -v` failed) — a stop is a DECIDED outcome, and a ' + + 'tree the capture cannot measure cannot be decided. Re-run the review.' + : `${invisible.length} tracked path(s) carry an --assume-unchanged or ` + + `--skip-worktree bit (e.g. ${display( + invisible[0].slice(0, 96), + )}) — \`git diff\` is blind to any edit on them, so no decided stop ` + + `can be made: the bytes it would certify were read by no round. Clear ` + + `the bit(s) (\`git update-index --no-assume-unchanged\` / ` + + `\`--no-skip-worktree\`) and re-run the review.`; +} + +/** + * Why a decided stop is withheld when the round ran with `--no-untracked` — + * shared by the two incremental stops; the clean-tree stop's exclusion has + * its own shape-specific sentence. The anchor gate admits no round NARROWER + * than the cache, so the cached round ran narrow too: neither ever + * enumerated untracked files, and a brand-new one is invisible to both. + */ +function untrackedStopRefusal(): string { + return ( + 'The incremental scope kept nothing to review, but untracked files ' + + 'were not enumerated (--no-untracked) — not by this round and not by ' + + 'the round the cache records — so a brand-new file is invisible to ' + + 'both, and this is NOT a decided nothing-to-review: report the ' + + 'untracked scope under "Not reviewed" and end the round without the ' + + 'stop.' + ); +} + +/** + * Why the cache candidate is withheld when tracked paths carry a visibility + * bit — or when the bits themselves could not be enumerated. + */ +function invisibleCandidateRefusal(invisible: string[] | null): string { + return invisible === null + ? 'The tracked-file visibility bits could not be enumerated ' + + '(`git ls-files -v` failed) — the cache candidate is withheld: an ' + + 'anchor recorded over a tree the capture cannot measure would let ' + + 'a later round certify bytes no round ever read. Re-run the review.' + : `${invisible.length} tracked path(s) carry an --assume-unchanged or ` + + `--skip-worktree bit (e.g. ${display( + invisible[0].slice(0, 96), + )}) — \`hash-object\` reads the worktree bytes through the bit ` + + `while \`git diff\` cannot see them, so the cache candidate is ` + + `withheld: promoted, it would anchor a later round on bytes this ` + + `round never reviewed. Clear the bit(s) ` + + `(\`git update-index --no-assume-unchanged\` / ` + + `\`--no-skip-worktree\`) and re-run the review.`; +} + +/** + * Why the previous round's anchor cannot scope this capture — or null when it + * can. Every reason is said out loud: an anchor silently ignored looks + * exactly like an anchor honoured over a full-size diff. + */ +function anchorRefusalReason( + cache: ReturnType, + /** The identity running THIS round, provider-qualified; empty means none. */ + model: string, + headSha: string | null, + target: string, + /** The path `target` was flattened from, when the review names one. */ + source: string | undefined, + skippedCount: number, + treeHeldStill: boolean, + /** Did THIS capture include untracked files? */ + untracked: boolean, + /** + * Cached paths still on disk but gone from this capture and not certified + * by the base HEAD — see `vanishedStillOnDisk`. + */ + vanishedStillPresent: readonly string[], +): string | null { + if (!treeHeldStill) { + // The hashes this scoping would compare against were computed over a tree + // that moved while they were being taken — the same uncertainty that + // withholds the cache candidate. Withholding only the candidate protects + // the NEXT round and leaves THIS one wrong: a file whose bytes changed + // during the hash pass hashes equal to the cached round, `changedSince` + // reports nothing, and its diff section is sliced out of scope — so the + // round says "nothing to re-review" over a capture no agent read. The + // guard's own promise is that no round certifies bytes it never + // reviewed; that promise is this one's too. + return 'the working tree changed while the capture was being hashed'; + } + if (skippedCount > 0) { + // Skipped content is in NO diff and NO hash: with it present, "zero + // delta" cannot mean "nothing changed", and an incremental round would + // certify the previous verdict over work the capture explicitly could + // not read. + return `the capture SKIPPED ${skippedCount} file(s) whose content cannot be certified`; + } + if (!cache) return 'the cache is missing or unreadable'; + if (!certifierMatchesRound(cache.lastModelId, model)) { + // `display()`: the model id is a string out of the model-written cache + // file — printed raw, a crafted value forges warning lines or emits + // terminal escapes. Capped for the same reason. + // The same-model contract cannot be verified when either side is + // missing, and an unverifiable contract is a failed one — which is what + // `certifierMatchesRound` answers for an empty running identity too. + return `the previous local round was reviewed by ${display( + (cache.lastModelId || 'an unrecorded model').slice(0, 64), + )}, not ${display(model || 'an unrecorded model')}`; + } + if (cache.target !== target) { + // A cache belonging to another target (a different file-path review) + // describes a different reviewed scope entirely. + return `the cache belongs to target ${display(cache.target.slice(0, 64))}, not ${display(target)}`; + } + if ((cache.source ?? undefined) !== source) { + // …and the TOKEN alone cannot answer that question, because + // `safeTarget` is not injective: `src/foo.ts` and `src_foo.ts` flatten to + // one token, as do `foo.ts`/`.foo.ts` and `foo..bar`/`foo/bar`. Under + // matching HEAD and identity the token gate passed each file the other's + // cache — scoping against a state describing a different file, and + // erasing that file's anchor and open findings on promotion. The capture + // records the path it flattened; compare that. + // + // A cache from before the field carries none, which reads as a mismatch + // against a file review and costs one full round — the safe direction. + return `the cache belongs to source path ${display( + (cache.source ?? 'an unrecorded path').slice(0, 96), + )}, not ${display(source ?? 'an unrecorded path')}`; + } + if (cache.untracked === true && !untracked) { + // Narrower than the round that wrote it: this capture cannot see the + // untracked files that one hashed, so their absence from it is scope, not + // change. Refusing costs a full round; honouring it certifies bytes + // nobody read. + return 'this round excludes untracked files the cached round reviewed'; + } + if (cache.stateId !== stateIdOf(cache.headSha, cache.files)) { + // Integrity: a shape-valid cache whose hashes were edited without + // recomputing stateId is not the state any clean round certified. + return 'the cache stateId does not match its own files (tampered or corrupted)'; + } + if (Object.keys(cache.files).length === 0) { + // A no-diff round promotes an empty files map, and an empty map + // certifies nothing: `changedSince` over two empty maps answers + // "unchanged" under ANY tree state the capture cannot see — an + // `--assume-unchanged` edit is the live case — so the unchanged stop + // would decide over bytes no round ever read. An anchor with no + // identities is not an anchor; the full capture costs one round. + return 'the cache recorded no file identities, so it cannot certify this round'; + } + if (cache.headSha !== headSha) { + // The captured diff is HEAD-vs-worktree: under a moved HEAD the same + // worktree bytes describe a different change under review. + return 'HEAD moved since the last local round'; + } + if (vanishedStillPresent.length > 0) { + // Visibility narrowed without any flag moving — an ignore rule added + // between rounds is the live case. The path's absence from this capture + // is scope, not a deletion, and honouring the anchor would stop decided + // over bytes no round captured. + return `${ + vanishedStillPresent.length + } cached path(s) dropped out of this capture while still on disk (e.g. ${display( + vanishedStillPresent[0].slice(0, 96), + )})`; + } + return null; +} + +/** + * The cache file `--cache` names: the path itself, or — when it names a + * directory — the same spelling `cachePathFor` writes (`/.json` + * for the whole tree, `/file--.json` for a file + * review). Null when a directory holds no cache for this target, which + * every caller already treats as "no anchor". + */ +function resolveCachePath( + given: string, + target: string, + source: string | undefined, +): string | null { + let isDir = false; + try { + isDir = statSync(given).isDirectory(); + } catch { + // Missing is not a directory; `readLocalCache` reports it as unreadable. + return given; + } + if (!isDir) return given; + // The SAME spelling `cachePathFor` writes. A resolver and a writer that + // disagree leave the round reporting "the cache is missing or unreadable" + // over a cache sitting right there — and for a file review they DID, since + // the namespace split moved the write and left this probe on the old name. + const candidate = join(given, basename(cachePathFor(target, source))); + return existsSync(candidate) ? candidate : null; +} + +/** + * Where a target's review cache lives. + * + * The whole-tree round keeps `local.json`. A FILE review gets its own + * namespace and a digest of the source path, because the flattened token + * alone does not discriminate the subject and the ledger layer has no other + * key: `safeTarget` is not injective (`src/foo.ts` and `src_foo.ts` flatten + * to one token, so two file reviews shared one cache and erased each other's + * findings), and the token space reserves nothing (a root file literally + * named `local` produced the whole-tree key byte for byte, and one named + * `pr-` produced PR 's). + * + * The anchor gate's `source` check is the second layer, not the first: it can + * only refuse a cache the round already opened, which leaves the LEDGER — + * read and written by the orchestrator, not by the gate — sharing the file. + * + * Safe to change the spelling of because nothing predicts it any more: the + * plan publishes this path and every reader takes it from there. A cache + * under the old name is simply not found, which costs one full round. + */ +function cachePathFor(target: string, source: string | undefined): string { + if (source === undefined) return join(REVIEW_CACHE_DIR, `${target}.json`); + const digest = createHash('sha256').update(source).digest('hex').slice(0, 8); + return join(REVIEW_CACHE_DIR, `file-${target}-${digest}.json`); +} + function runCaptureLocal(args: CaptureLocalArgs): void { - const { out, file, target } = args; + const { out, file } = args; + // DERIVED here when a file review does not name one, rather than recomputed + // by whoever calls this. `qwen review run` pins the artifact name it polls + // for from the same repo-relative path put through the same `safeTarget`, + // and the skill used to tell the orchestrator to apply that recipe BY HAND + // — character-class replacement, no canonicalisation. The two agreed only + // where the prose derivation happened to: `ln -s src srclink` then + // `qwen review run srclink/foo.ts` had the parent poll for + // `qwen-review-src_foo.ts-composed.json` while every child artifact was + // named `srclink_foo.ts`, so the poll never matched and a review that had + // already run — and with --comment, already posted — reported no verdict. + // + // One deriver, in code, and it runs for EVERY `--file` capture — including + // one an explicit `--target` rides along on. That combination used to skip + // the derivation: `sourcePath` stayed undefined, so the cache fell out of + // the digest namespace, the candidate recorded no `source`, and the gate's + // source clause degraded to `undefined === undefined` and passed — + // re-creating the cross-subject cache sharing both exist to close, while + // the explicit token named artifacts the parent's derived poll never + // matches anyway. An explicit `--target` names plain (non-file) rounds + // only; the cache-target gate below compares whatever was used. + const sourcePath = + file !== undefined + ? repoRelativeOf(gitOpt('rev-parse', '--show-toplevel') ?? '.', file).rel + : undefined; + const target = + sourcePath !== undefined ? safeTarget(sourcePath) : args.target; + // Visibility-bit sample 0 — BEFORE the first capture. The oracle rides the + // same both-endpoints discipline as the diffs and hashes below: sampled + // only after the loop, a bit set through every diff pass and cleared just + // before the one query read clean while the captured diffs were blind to + // the edit it hid — the candidate then certified bytes no pass ever + // showed. Bracketing narrows that to a set-AND-cleared toggle inside one + // inter-sample gap, the same honest-tightening the capture loop claims + // for itself. + const invisibleSamples: Array = [ + invisibleTrackedPaths(gitOpt('rev-parse', '--show-toplevel') ?? '.'), + ]; const capture = captureLocalDiff({ file, includeUntracked: args.untracked, }); - const diffText = capture.diff.toString('utf8'); // Two directories, and they are not the same one. The diff always lands in // `.qwen/tmp` (its path is ours to choose), but `--out` is the caller's — and @@ -84,13 +538,651 @@ function runCaptureLocal(args: CaptureLocalArgs): void { // dir turned into an ENOENT from `writeFileSync`. mkdirSync(REVIEW_TMP_DIR, { recursive: true }); mkdirSync(dirname(resolve(out)), { recursive: true }); + + const fullPlan = buildDiffPlan(capture.diff.toString('utf8')); + + // The content anchor for the NEXT round: hash every captured file's current + // bytes (`hash-object` without `-w` — computes, writes nothing) plus the + // HEAD this diff was based against. Written on every run, incremental or + // not, full-capture fallback or not: the candidate records what THIS round + // reviewed, and Step 8 promotes it to `.qwen/review-cache/` only on a clean + // high-effort end — the same division of labour as the PR cache. + const headSha = capture.unbornHead + ? null + : gitOpt('-C', capture.repoRoot, 'rev-parse', 'HEAD'); + // A rename section names only its NEW side, so the deleted SOURCE would go + // unhashed and two captures differing only in which head file git paired as + // the source would compare as "no changes". + // + // LIVE, not defensive. An earlier version of this comment claimed the + // opposite on the strength of a measurement that did not hold: the pinned + // flags include `--find-renames` (`lib/diff-flags.ts`), and the capture's + // own command over a staged `git mv` renders one rename section, not two — + // `similarity index 100%` for a pure move and `95%` for a move with a small + // edit. Local plans therefore DO carry `renameFrom`, which is what makes + // the slice filter below a live fix rather than a spare part. + const planPaths = [ + ...new Set([ + ...fullPlan.files.flatMap((f) => + f.renameFrom && f.renameFrom !== f.path + ? [f.path, f.renameFrom] + : [f.path], + ), + // A FILE review's subject enters the anchor even with NO diff section. + // Without it the no-diff shape promotes an empty files map, and an + // `--assume-unchanged` edit on the subject then hides from + // `git diff HEAD` while `hash-object` reads through the bit — hashing + // the subject keeps the next round's comparison honest instead of + // certifying "unchanged since last round" over bytes nobody read. + // + // …but a DIRECTORY subject is not hashable, and `qwen review ` + // is a supported entrance. Recorded, it lands in every candidate as + // UNHASHABLE — which never equals itself — so `changedSince` reports + // the directory every round, `stateChanged` is never empty, and the + // `unchanged-since-last-round` stop is unreachable for that target + // for ever (R20-2), while the round prints "could not be hashed on + // either side" about the user's own subject. Its FILES are already + // in `fullPlan.files`; the directory itself carries no bytes to + // certify. Only a confirmed directory is skipped: an lstat that + // fails leaves the subject in, which is the pre-existing coverage. + ...(sourcePath !== undefined && + !isDirectorySubject(capture.repoRoot, sourcePath) + ? [sourcePath] + : []), + ]), + ]; + const hashes = hashWorktreeFiles(capture.repoRoot, planPaths); + // TOCTOU guard: the diff was snapshotted before the hashes were computed, + // and an editor save landing in that window makes the candidate certify + // bytes THIS round never reviewed — the one uncertainty in this module + // that failed OPEN. Differing bytes withhold the candidate (the plan still + // reviews the FIRST capture) AND refuse this round's own incremental + // scoping, which reads the very same hashes — see `anchorRefusalReason`'s + // first clause. The cost is a full-range review now and no anchor next + // round. + // + // BOTH endpoints are re-read, the diff and the hashes, because the hash + // pass sits BETWEEN the two diff snapshots and a write that straddles it + // is invisible to the diffs alone. Capture B0 → autosave writes B1 → the + // hashes read B1 → undo restores B0 → the re-capture reads B0: the two + // diffs agree, and the candidate certifies B1's identity for a round that + // reviewed B0. The earlier note here had this backwards — it called a + // same-bytes revert harmless and a different-bytes revert the uncatchable + // one, when a different-bytes revert moves the endpoints and IS caught, + // and the same-bytes straddle is what poisons the hashes. "No editor does + // that by accident" is no answer to an autosave racing an undo. + // + // THREE consecutive states, not two, and the two kinds interleaved. + // + // Pairwise agreement never tied a capture to the hashes recorded beside + // it: sampling B0 H0 B1 H1 and asking only "B0 == B1" and "H0 == H1" + // passes for three phase-aligned writes — X→Y before the hash pass, Y→X + // before the re-capture, X→Y after it. Both checks hold, `treeHeldStill` + // is true, and the candidate certifies Y's identity for a round that + // reviewed X. Promoted, the next round compares cache Y against tree Y, + // finds no delta and says "No changes" over bytes no round ever read — + // the exact promise this guard exists to keep. + // + // Interleaved sampling does not make that impossible; nothing short of + // holding the tree still can, and this module cannot. It raises the price + // from three timed writes to five, and every write has to land in a + // window bounded by the neighbouring sample of the OTHER kind. The + // honest description is a tightened sample, not a proof — and every + // failure of it withholds the candidate, so the cost of being wrong is a + // full round, never a false certification. + // + // The extra pass is one more `git diff` and one more batched + // `hash-object` over paths already read twice. + const captures = [capture.diff]; + const skippedPasses = [capture.skipped]; + const hashPasses = [hashes]; + for (let i = 0; i < 2; i++) { + const re = captureLocalDiff({ file, includeUntracked: args.untracked }); + captures.push(re.diff); + // The re-captures' SKIPPED lists ride the comparison too: a file that + // enters the tree inside the window and lands in a skip class (over the + // cap, binary, an embedded repo, the budget) is in no capture's diff + // BYTES, so the byte comparison alone reads "held still" while two of + // the three captures explicitly skipped content — and every stop gate + // reads only capture 0's skipped list (R21-2). Skip-set movement is + // tree movement. + skippedPasses.push(re.skipped); + hashPasses.push(hashWorktreeFiles(capture.repoRoot, planPaths)); + // Visibility-bit samples 1 and 2 — interleaved with the re-captures for + // the same reason the hashes are: see sample 0 above. + invisibleSamples.push(invisibleTrackedPaths(capture.repoRoot)); + } + const skipSet = (list: readonly SkippedFile[]): string => + JSON.stringify(list.map((f) => f.path).sort()); + const treeHeldStill = + captures.every((d) => d.equals(captures[0])) && + skippedPasses.every((l) => skipSet(l) === skipSet(skippedPasses[0])) && + // `movedSince`, not `changedSince`: a path unhashable on both reads did + // not move between them, and treating it as a move would withhold the + // candidate on every round holding a pending deletion — the same + // conflation that made the convergence stop unreachable. + hashPasses.every((h) => movedSince(hashPasses[0], h).length === 0); + const candidate: LocalCacheCandidate = { + v: 1, + target, + headSha, + files: hashes, + stateId: stateIdOf(headSha, hashes), + // Recorded HERE, from the identity the runtime published, rather than + // merged in by Step 8 from `{{model}}`. `{{model}}` interpolates the BARE + // model id while `roundModelIdFrom` is provider-qualified + // (`@`), so two provider + // configurations exposing one model name wrote — and compared — equal, + // and each passed the other's gate. That is the identity-channel class + // the PR flow closed by moving the comparison into the command; a local + // round is the same contract ("an anchor is honoured only under the model + // whose clean verdict certified it") and needs the same treatment. An + // empty string means the runtime published nothing, which the gate reads + // as a mismatch rather than a pass. + lastModelId: roundModelIdFrom(process.env), + // What this round could SEE. A later round that sees less cannot certify + // this one's state: with `--no-untracked` (or a `.gitignore` entry added + // between rounds) the untracked block never runs and records no `skipped` + // entries, so a cached untracked path reads as VANISHED rather than + // out-of-scope — the slice keeps nothing and the round stops decided over + // bytes it never captured. The stop does not advance the cache, so every + // later narrow round repeats it. + untracked: args.untracked !== false, + // The path the target token was FLATTENED from, when there is one. + // + // `safeTarget` is not injective: `src/foo.ts` and `src_foo.ts` both + // flatten to `src_foo.ts`, as do `foo.ts`/`.foo.ts` and + // `foo..bar`/`foo/bar`. This PR newly keys the review cache by that + // token, so the gate below — comparing tokens alone — could not tell two + // different files apart: a review of `src_foo.ts` accepted `src/foo.ts`'s + // cache, scoped against a state describing another file, and promoting it + // erased the first file's anchor and its open findings. + ...(sourcePath !== undefined ? { source: sourcePath } : {}), + }; + // The visibility-bit oracle — every stop is a claim that nothing in the + // tree needs review, and the candidate records the identity of the tree + // this round reviewed; `git diff` is blind to the marked paths, so the + // three stops AND the candidate write are conditioned on the enumeration + // coming back clean AT EVERY SAMPLE POINT: the three bracketing samples + // taken alongside the captures above, plus a final one here after the + // loop. A bit visible at ANY of them withholds (the union is reported), + // and a single failed enumeration withholds everything — the same + // fail-closed lean as one unhashable path. + let invisibleBits: string[] | null | undefined; + const invisibleTracked = (): string[] | null => { + if (invisibleBits === undefined) { + invisibleBits = invisibleTrackedPaths(capture.repoRoot); + } + if (invisibleBits === null || invisibleSamples.some((v) => v === null)) { + return null; + } + return [ + ...new Set([ + ...invisibleSamples.flatMap((v) => v ?? []), + ...invisibleBits, + ]), + ]; + }; + const invisibleCertified = (): boolean => { + const inv = invisibleTracked(); + return inv !== null && inv.length === 0; + }; + const cacheCandidatePath = tmpFile(target, 'cache-candidate.json'); + // Read the cache BEFORE the candidate write: the dropped-out-while-on-disk + // set gates that write (below), and computing it after let a refused + // anchor's round write a candidate that silently OMITTED the dropped path + // — Step 8 promoted the omission, and two rounds later a scope-emptied + // stop certified bytes no round read (R23). Empty when no `--cache` + // scoped this round; the scoping branch below reuses these values. + const cachePathEarly = + args.cache !== undefined + ? resolveCachePath(args.cache, target, sourcePath) + : null; + const cacheEarly = + cachePathEarly === null ? null : readLocalCache(cachePathEarly); + const vanishedPresent: readonly string[] = + cacheEarly === null + ? [] + : vanishedStillOnDisk( + capture.repoRoot, + headSha, + cacheEarly.files, + hashes, + ); + // The same uncertainty that withholds a decided stop withholds the + // candidate: `hash-object` reads the worktree bytes THROUGH a set + // assume-unchanged/skip-worktree bit while `git diff` cannot see them, so + // the hashes above record the identity of bytes this round's diff never + // showed. Promoted, the unread bytes become anchor state — and when the + // bit is cleared between rounds keeping the bytes, every comparison finds + // no change, every visibility gate reads clean, and the unchanged-since + // stop certifies them: a loop deciding "nothing to re-review" over bytes + // no round ever read. A cached path dropped out while still on disk is + // the same uncertainty from the other side: this capture cannot SEE the + // path, so the candidate would record its absence as reviewed state. + const invisible = invisibleTracked(); + const candidateWritten = + treeHeldStill && + invisible !== null && + invisible.length === 0 && + vanishedPresent.length === 0; + if (candidateWritten) { + writeFileSync(cacheCandidatePath, JSON.stringify(candidate, null, 2)); + } else { + // The path is stable per target, so an earlier round's candidate still + // sits under the `cacheCandidatePath` this plan publishes, and Step 8 + // would promote that stale anchor merged with this round's ledger. + // Absent IS the withheld state — fail quiet. + try { + unlinkSync(cacheCandidatePath); + } catch { + // nothing to remove + } + if (!treeHeldStill) { + writeStderrLine( + 'The working tree changed while the capture was being hashed — the ' + + 'cache candidate is withheld, so the next round cannot anchor on ' + + 'bytes this round never reviewed. The review itself proceeds on ' + + 'the first capture.', + ); + } else if (invisible === null || invisible.length > 0) { + writeStderrLine(invisibleCandidateRefusal(invisible)); + } else { + writeStderrLine( + `The cache candidate is withheld: ${vanishedPresent.length} cached ` + + `path(s) dropped out of this capture while still on disk, so the ` + + `candidate would record their absence as reviewed state. The ` + + `review itself proceeds in full.`, + ); + } + } + + // Incremental scoping, when the caller brought the previous round's anchor. + let diffBytes = capture.diff; + let plan = fullPlan; + let incremental: IncrementalBlock | undefined; + /** + * Machine-readable: this round has nothing to review, and that is a + * DECIDED outcome rather than a failure. + * + * Both stops used to exist only as a stderr sentence the orchestrator + * matched on, so the parent (`qwen review run`) could not tell them from a + * round that fell over: it polls for a composed verdict, finds none, and + * exits 1 with "Review did not complete". A user who committed without + * fixing a blocker gets that on every later round, over a round whose own + * output rendered the blocker as still standing. + */ + let nothingToReview: { reason: string } | undefined; + // Cached paths this capture dropped while still on disk and diverging + // from HEAD — see `vanishedStillOnDisk`. Empty when no `--cache` scoped + // this round. Read by the anchor refusal AND the stop gates below. + if (args.cache !== undefined) { + // A DIRECTORY resolves to this command's own target, because the caller + // cannot name the file. + // + // The cache is `/.json`, and `target` exists only after the + // derivation above — the whole point of which is that a hand-applied + // recipe disagreed with it. Step 1 has to decide whether a cache exists + // BEFORE running this command, so it was left predicting the name: for + // `ln -s src srclink` then a review of `srclink/foo.ts`, the typed + // spelling flattens to `srclink_foo.ts` while this command canonicalises + // to `src_foo.ts`. The prediction misses, `--cache` is never passed, and + // the round silently loses both incremental scoping and the findings + // ledger — in exactly the spelling classes the canonicalisation exists + // to handle, with no refusal line printed anywhere. + // + // Passing the directory ends the guessing: one deriver, and a caller + // that knows only where caches live. A file path still works unchanged. + const cache = cacheEarly; + const refusal = anchorRefusalReason( + cache, + roundModelIdFrom(process.env), + headSha, + target, + sourcePath, + capture.skipped.length, + treeHeldStill, + args.untracked !== false, + vanishedPresent, + ); + if (refusal !== null) { + writeStderrLine( + `Incremental anchor not used — ${refusal}. Running the full local review.`, + ); + } else { + // SYMMETRIC difference, via the tested helper: a path the cached round + // hashed that no longer appears in this capture (an untracked file + // deleted between rounds) is a change — its importers must re-enter + // through the widening even though the path itself has no diff + // section left to review. + const stateChanged = changedSince(cache!.files, hashes); + // What actually MOVED, which is not the same list. A path that could + // not be hashed on either side is in `stateChanged` for ever — that is + // deliberate, so unreadable state is re-reviewed rather than certified + // — but keying the "nothing changed" stop on it made that stop + // unreachable for any change set holding a pending deletion, and told + // the user "1 changed file(s)" about a byte-identical diff every round. + // Scope keeps the wider list; the stop and the human-facing count take + // this one. + const stateMoved = movedSince(cache!.files, hashes); + const changedSet = new Set(stateChanged); + const changed = planPaths.filter((p) => changedSet.has(p)); + // One import hop over the still-clean SOURCE files, read from the LIVE + // working tree — the same tree the local review runs against. + const candidates = fullPlan.files + .filter( + (f) => f.kind === 'source' && !f.binary && !changedSet.has(f.path), + ) + .map((f) => f.path); + const readTree = (rel: string): string | null => { + try { + return readFileSync(join(capture.repoRoot, rel), 'utf8'); + } catch { + return null; + } + }; + const interaction = dependentsOfChanged( + changedSet, + candidates, + readTree, + discoverWorkspacePackages(planPaths, readTree), + ); + const keep = new Set([...changed, ...interaction.keys()]); + const fullDiffPath = tmpFile(target, 'diff-full.txt'); + writeFileSync(fullDiffPath, capture.diff); + diffBytes = sliceDiffByLines( + capture.diff, + fullPlan.files + // Either SIDE of a rename keeps the section. A rename section is + // labelled with its NEW path, while `changedSince` reports the + // deleted SOURCE — its recorded identity is UNHASHABLE, which never + // equals itself, so the source is in `keep` on every round and the + // target is in none. Matching `f.path` alone cut the whole section: + // a zero-byte slice, a plan with no chunks, `deltaFiles` naming a + // path no section carries, and the branch below still printing + // "Their sections are in scope". The stop sentence cannot fire + // either (`stateChanged` is non-empty), and the candidate re-records + // the same state, so every round repeats it — a review cycle spun + // over an empty diff with no convergence until HEAD moves. + .filter( + (f) => + keep.has(f.path) || + (f.renameFrom !== undefined && keep.has(f.renameFrom)), + ) + .map((f) => ({ startLine: f.diffStart, endLine: f.diffEnd })), + ); + plan = buildDiffPlan(diffBytes.toString('utf8')); + // Under `scope`, exactly as the PR flow writes it — see + // `IncrementalBlock`. Written flat here once, which rendered no + // incremental frame on any local round while the diff was sliced + // regardless, so nothing looked wrong. + const removedFromSlice = stateChanged.filter( + (p) => !planPaths.includes(p), + ); + incremental = { + scope: { + anchor: cache!.stateId, + deltaFiles: changed, + interaction: [...interaction.entries()].map( + ([path, importsChanged]) => ({ path, importsChanged }), + ), + contextFileCount: candidates.filter((p) => !interaction.has(p)) + .length, + fullDiffPath, + // The scope-emptied split key — see IncrementalScope. Computed + // here, not re-derived by the orchestrator: file PRESENCE cannot + // answer it (a discarded change leaves the file present with the + // cited bytes gone), and no other published channel names these + // paths at all. + ...(removedFromSlice.length > 0 + ? { supersededPaths: removedFromSlice } + : {}), + }, + }; + // Paths that vanished since the cached round have no diff section and + // no deltaFiles entry — say they existed, or a deletion-only round + // reads as if nothing drove its scope. + const removedCount = removedFromSlice.length; + // The stop condition is the SYMMETRIC set: a deleted-since-cache + // path with no diff section left is still a change, and "no + // changes" must not be claimed over it. + if ( + stateMoved.length === 0 && + stateChanged.length === 0 && + // …and NOT a file review, the exclusion BOTH sibling stops carry: a + // cached round-2 file review of a tracked-unmodified subject passed + // every anchor clause and stopped decided here, while the identical + // tree without a cache routes to the whole-file review SKILL.md + // owes a file target — the cache-vs-no-cache disagreement the + // scope-emptied exclusion was added to kill, one stop over (R23). + // The file shape gets its own sentence below instead of falling + // into the unhashable-paths diagnosis beside it. + args.file === undefined + ) { + const invisible = invisibleTracked(); + if (invisible !== null && invisible.length === 0) { + if (args.untracked !== false) { + nothingToReview = { reason: 'unchanged-since-last-round' }; + writeStderrLine( + `No changes since the last local review round (same model, ` + + `same HEAD, same content) — nothing to re-review.`, + ); + } else { + // …but "same content" was only proven over tracked paths: the + // gate admits no narrower round than the cache, so the cached + // round ran `--no-untracked` too — neither enumerated untracked + // files, and a brand-new one is invisible to both. The same + // exclusion the clean-tree stop carries, out loud. + writeStderrLine(untrackedStopRefusal()); + } + } else { + // …but "unchanged" was only proven over what `git diff` can see: + // a marked path's edit leaves every comparison above standing + // still. The same uncertainty that refuses an anchor withholds a + // stop — the round decides nothing and says why. + writeStderrLine(invisibleStopRefusal(invisible)); + } + } else if ( + stateMoved.length === 0 && + stateChanged.length === 0 && + args.file !== undefined + ) { + // The excluded file shape, said honestly: nothing changed since the + // cached round, and a file target takes the whole-file review + // instead of a decided stop — cache and no-cache agree. + writeStderrLine( + `No content changes since the last local review round for this ` + + `file target — a file review never stops decided here; the ` + + `whole-file review reads the current state.`, + ); + } else if (stateMoved.length === 0) { + // Nothing MOVED, but the scope is not empty: a path unhashable + // on both sides stays in it, because "could not capture it + // twice" is not "unchanged". Saying "nothing to re-review" + // here would be false twice over — the plan carries chunks, + // and SKILL.md stops the orchestrator on that exact sentence, + // so it would stop over live scope. + writeStderrLine( + `No content changes since the last local review round, but ` + + `${stateChanged.length} path(s) could not be hashed on either ` + + `side (a pending deletion, or a name this layer cannot read) ` + + `— they are re-reviewed every round and never certified. ` + + `Their sections are in scope.`, + ); + } else { + writeStderrLine( + `Incremental scope since state ${display( + cache!.stateId.slice(0, 12), + )}: ` + + `${changed.length} changed file(s), ${interaction.size} ` + + `interaction file(s) (one import hop), ` + + (stateChanged.length > stateMoved.length + ? `${stateChanged.length - stateMoved.length} unreadable ` + + `path(s) re-reviewed every round (never certified), ` + : '') + + (removedCount > 0 + ? `${removedCount} cached path(s) whose recorded change is ` + + `gone from this capture (deleted, or discarded — named in ` + + `the plan's supersededPaths), ` + : '') + + `${incremental.scope!.contextFileCount} clean file(s) left out of ` + + `scope.`, + ); + } + } + } + + // Decided BEFORE the report is written, because the branch that prints the + // clean-tree warning runs after it. Only the genuinely clean shape counts: + // a capture that SKIPPED files reviewed nothing AND could not read what it + // skipped, so that round owes a "Not reviewed" section and must never read + // as complete. + if ( + plan.diffLines === 0 && + !incremental && + capture.skipped.length === 0 && + // …and NOT a file review. A tracked, unmodified file has an empty diff + // and is not a decided round: SKILL.md's no-diff branch owes it a + // whole-file review ("read the file and review its current state"). Left + // in, this turned that case from "Review did not complete" — which it was + // before the stop existed — into a PASSING gate over a file nobody read. + args.file === undefined && + // …and the guard has to agree. `treeHeldStill` false means a write landed + // inside the capture window — the exact race the three-pass sampling + // exists to catch — so capture 0's empty diff describes a tree that no + // longer exists. Without this the round printed both "the working tree + // changed while the capture was being hashed" AND "the working tree is + // clean", stopped on the second, and recorded the just-written change as + // reviewed-and-clean. Same discipline as the skipped-content gate beside + // it: a stop is a DECIDED outcome, and neither unread nor moved content + // can be decided. + treeHeldStill && + // …and the anchor refusal did not just prove a path diverges while + // INVISIBLE to the capture — an `--assume-unchanged` edit is the live + // case: `git diff HEAD` honours the bit, so an empty diff proves + // nothing about that path, and the blocker date below reads hidden + // edits through it. The same uncertainty that refused the anchor + // withholds the stop, or the round decides clean over bytes no round + // ever read. + vanishedPresent.length === 0 && + // …and nothing tracked is INVISIBLE to the diff the cleanness claim is + // about: a path carrying an assume-unchanged/skip-worktree bit hides + // any edit from the empty diff this stop certifies, so "nothing staged, + // nothing unstaged, nothing untracked" proves nothing while one is + // set. The bits are enumerated, not the edits — one ANYWHERE withholds + // the stop, because the capture cannot tell which marked path diverges. + invisibleCertified() + ) { + if (args.untracked !== false) { + nothingToReview = { reason: 'clean-tree' }; + } else { + // The stop's claim is "nothing staged, nothing unstaged, nothing + // untracked", and under `--no-untracked` the third clause was never + // checked: the untracked enumeration does not run and records no + // `skipped` entries, so a tracked-clean tree with pending untracked + // work passed every conjunct above. SKILL.md's own recovery from an + // oversized-untracked skip is "re-run with `--no-untracked`", which + // lands exactly here — deciding clean over the very content the first + // run could not read. The ANCHOR gate has carried this exclusion + // since the candidate recorded `untracked`; the stop follows it, out + // loud like every other withheld stop. + writeStderrLine( + 'The tracked tree is clean, but untracked files were not ' + + 'enumerated (--no-untracked), so this is NOT a decided clean ' + + 'tree: report the untracked scope under "Not reviewed" and end ' + + 'the round without a clean verdict.', + ); + } + } + + // …and the third decided shape, which the two above miss because both are + // gated on `!incremental`. A cached path that VANISHED (deleted, or the + // change discarded with `git checkout --`) is a change by design, so + // `stateChanged` is non-empty and the unchanged-since stop cannot fire — + // but it carries no section, the widening pulls nothing in, and the slice + // keeps zero. The plan then had `chunks: []` with an `incremental` block + // and no field at all: neither SKILL stop fired, `agent-prompt --roster` + // threw "the plan has no `chunks[]`" on the first diff-reading role, and + // the parent reported "Review did not complete" over a decided round. + if ( + // …and only when no MORE SPECIFIC stop already fired. The + // unchanged-since-last-round round also keeps zero sections, and it has a + // reason of its own that SKILL.md branches on separately; overwriting it + // here sent that round down the wrong branch. + nothingToReview === undefined && + incremental !== undefined && + plan.chunks.length === 0 && + capture.skipped.length === 0 && + treeHeldStill && + // …and NOT a file review, the same exclusion both sibling stops carry. + // A file review whose anchored change was discarded has nothing in the + // slice, but SKILL.md owes it a whole-file review exactly as it does for + // the no-cache case — and without this the two disagreed on identical + // trees: with a cache the round completed decided, without one it routed + // to the whole-file review. + args.file === undefined + ) { + // …and the visibility bits are clean, the same gate both sibling stops + // carry: the empty slice proves only what `git diff` can see. Withheld, + // the refusal is said out loud like the unchanged stop's own. + const invisible = invisibleTracked(); + if (invisible !== null && invisible.length === 0) { + if (args.untracked !== false) { + nothingToReview = { reason: 'scope-emptied' }; + } else { + // The emptied scope proves only the TRACKED content returned to the + // cached state — neither round enumerated untracked files (the gate + // admits no narrower round than the cache). The same exclusion both + // sibling stops carry, out loud. + writeStderrLine(untrackedStopRefusal()); + } + } else { + writeStderrLine(invisibleStopRefusal(invisible)); + } + } + + // Published at a name the PARENT can predict, beside the plan rather than + // inside it. `qwen review run` has to find this without knowing `--out`: + // that path is the orchestrator's to choose (SKILL.md says so, and it must, + // because the CLI-derived `` token does not exist yet at Step 1), + // so a parent polling `qwen-review--plan.json` found nothing for + // every file review and reported "Review did not complete" over a decided + // round. This name is derived from the same `target` the parent derives. + if (nothingToReview) { + writeFileSync( + tmpFile(target, 'stop.json'), + `${JSON.stringify( + { + ...nothingToReview, + // The parent's stamp, echoed back. This file decides `completed`, + // while its NAME is the flattened target token — not injective, so + // a concurrent review whose path flattens alike writes the same + // path and would decide the other run's completion. Absent when the + // capture was not launched by `qwen review run`, which is exactly + // when no parent is reading. + ...(process.env['QWEN_REVIEW_RUN_ID'] + ? { runId: process.env['QWEN_REVIEW_RUN_ID'] } + : {}), + }, + null, + 2, + )}\n`, + 'utf8', + ); + } + const diffPath = tmpFile(target, 'diff.txt'); // Write the bytes, not the string: a re-encode would rewrite the content of // every hunk touching a file git handed us in a non-UTF-8 encoding. - writeFileSync(diffPath, capture.diff); + writeFileSync(diffPath, diffBytes); - const plan = buildDiffPlan(diffText); const result: CaptureLocalResult = { + // The token the CLI derived, so nothing downstream has to re-derive it. + // `qwen review run` pins the artifact name it waits for from the same + // canonicalisation; an orchestrator that recomputes the stem by hand gets + // a different answer wherever a symlink sits below the repo root, and + // every artifact it names then misses the poll. + target, diffPath, diffPathAbsolute: resolve(diffPath), // No ref to `git show` a pre-change file out of, so per-file line counts and @@ -102,6 +1194,19 @@ function runCaptureLocal(args: CaptureLocalArgs): void { }), untrackedFiles: capture.untracked, skippedFiles: capture.skipped, + ...(incremental ? { incremental } : {}), + ...(nothingToReview ? { nothingToReview } : {}), + // Where this target's cache lives, resolved by the deriver rather than + // predicted by the reader. Every ledger read and the Step 8 write name + // `.json`, and `target` does not exist until this command derives + // it — `safeTarget` is not hand-reproducible in the cases that matter + // (past 64 characters it suffixes a digest, and symlink canonicalisation + // diverges from any hand recipe). A round-2 medium review of + // `srclink/foo.ts` predicted `srclink_foo.ts.json`, found nothing, and + // ruled on zero ledger entries over a Critical that still stood. + cachePath: cachePathFor(target, sourcePath), + cacheCandidatePath, + ...(candidateWritten ? { cacheCandidateStateId: candidate.stateId } : {}), ...planEffortField(args.effort), }; @@ -130,21 +1235,89 @@ function runCaptureLocal(args: CaptureLocalArgs): void { `${display(s.reason)}. List it under "Not reviewed" in the review output.`, ); } - if (plan.diffLines === 0) { + if (plan.diffLines === 0 && !incremental) { // "Nothing to review" and "nothing was reviewable" are different sentences, // and only one of them is a clean tree. An oversized blob or an embedded repo // as the *only* change lands here with an empty diff and a non-empty skip // list, and calling that clean would hand the review a green verdict over // work it explicitly could not read — the whole failure this command exists // to end, arriving through the front door. + // + // The incremental no-changes case is deliberately NOT this branch: its 0 + // chunks mean "identical to the state the last round reviewed", which the + // scoping block already said in its own words — "the working tree is + // clean" would be false, and false in the direction that certifies. writeStderrLine( capture.skipped.length > 0 ? `WARNING: 0 chunks — nothing reviewable was captured, but ` + `${capture.skipped.length} untracked file(s) were SKIPPED (above). ` + `This is not a clean tree: report them under "Not reviewed" and do ` + `not certify the working tree as reviewed.` - : 'WARNING: the working tree is clean — 0 chunks. There is nothing to ' + - 'review; do not run the review agents.', + : file !== undefined + ? // The same exclusion the field gate above applies: an empty diff + // is not a decided round for a FILE target. The capture was + // pathspec-scoped, so 0 chunks says nothing about the tree — and + // SKILL's no-diff branch owes a whole-file review for exactly + // this shape. The field channel got the exclusion; this prose + // channel — which the orchestrator also reads — did not, and a + // round that stopped on it left the user-named file unread. + '0 chunks — no diff was captured for the file the review named. ' + + 'This is NOT a decided stop: the no-diff branch owes it a ' + + 'whole-file review — read the file and review its current ' + + 'state; do not report nothing-to-review.' + : treeHeldStill && + vanishedPresent.length === 0 && + invisibleCertified() + ? // The prose channel carries the field gate's untracked + // exclusion too — the orchestrator reads BOTH, and a prose + // "clean" beside a withheld field re-opens the contradiction + // the moved-tree branch below closed. + args.untracked !== false + ? 'WARNING: the working tree is clean — 0 chunks. There is nothing ' + + 'to review; do not run the review agents.' + : '0 chunks — the tracked tree is clean, but untracked files ' + + 'were not enumerated (--no-untracked). This is NOT a decided ' + + 'clean tree: report the untracked scope under "Not ' + + 'reviewed" and end the round without a clean verdict.' + : vanishedPresent.length > 0 + ? // …and NOT when the anchor refusal just proved a path + // diverges while invisible to `git diff` — the field gate + // above withheld the stop, so the prose must not claim clean + // either. Same discipline as the moved-tree branch beside it. + 'WARNING: 0 chunks, but a cached path dropped out of this ' + + 'capture while still on disk and diverges from HEAD (above): ' + + 'this is NOT a clean tree — the divergence is invisible to ' + + '`git diff`. Re-run the review rather than reporting ' + + 'nothing to review.' + : !treeHeldStill + ? // …and NOT when the guard just proved the tree moved. The + // machine-readable stop is gated on `treeHeldStill`; this + // sentence was not, so the round printed "the working tree + // changed while the capture was being hashed" and "the working + // tree is clean" back to back and the orchestrator — which reads + // prose here — stopped on the second. The same contradiction the + // field-level gate closed, one layer up. + 'WARNING: 0 chunks, but the working tree changed while the ' + + 'capture was being hashed (above): this is NOT a clean tree. ' + + 'Re-run the review rather than reporting nothing to review.' + : // The shape that remains: the tree held still and no cached + // path diverges, but tracked paths carry a visibility bit + // (or the bits could not be enumerated), and the field gate + // above withheld the stop over it. + (() => { + const inv = invisibleTracked(); + return inv === null + ? 'WARNING: 0 chunks, but the tracked-file visibility ' + + 'bits could not be enumerated (`git ls-files -v` ' + + 'failed): this is NOT a clean tree. Re-run the ' + + 'review rather than reporting nothing to review.' + : `WARNING: 0 chunks, but ${inv.length} tracked ` + + 'path(s) carry an --assume-unchanged/--skip-worktree ' + + `bit (e.g. ${display(inv[0].slice(0, 96))}): ` + + '`git diff` is blind to any edit on them, so this ' + + 'is NOT a clean tree. Clear the bit(s) and re-run ' + + 'the review rather than reporting nothing to review.'; + })(), ); } writeStderrLine( @@ -175,7 +1348,7 @@ export const captureLocalCommand: CommandModule = { type: 'string', default: 'local', describe: - 'Target suffix for the diff file name (`local`, or a filename for a file-path review)', + 'Target suffix for the artifact names. Defaults to `local`; a `--file` review derives it from the file path and ignores this.', }) .option('untracked', { type: 'boolean', @@ -183,7 +1356,22 @@ export const captureLocalCommand: CommandModule = { describe: 'Include untracked, non-ignored files. On by default: `git diff` cannot see them, so without this a brand-new file goes unreviewed.', }) - .option('effort', EFFORT_OPTION), + .option('effort', EFFORT_OPTION) + .option('cache', { + type: 'string', + describe: + "The previous local round's review cache — the file, or the " + + 'DIRECTORY holding it (`.qwen/review-cache`), in which case this ' + + "command resolves this target's cache file — the same spelling " + + 'the plan publishes as `cachePath` — from the target IT derives. ' + + 'Prefer the directory for a file review: the target is ' + + "this command's to compute, and a caller that predicts the name " + + 'gets it wrong for any non-canonical spelling. When the anchor ' + + 'validates — same identity, same HEAD — the capture is scoped to ' + + 'files whose content changed since that round, widened by one ' + + 'import hop; on any refusal it degrades to the full capture and ' + + 'says why.', + }), handler: (argv) => { runCaptureLocal(argv as unknown as CaptureLocalArgs); }, diff --git a/packages/cli/src/commands/review/capture-local.visibility.test.ts b/packages/cli/src/commands/review/capture-local.visibility.test.ts new file mode 100644 index 00000000000..c518e8af456 --- /dev/null +++ b/packages/cli/src/commands/review/capture-local.visibility.test.ts @@ -0,0 +1,160 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + existsSync, + mkdtempSync, + mkdirSync, + readFileSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { isolateHostGitConfig } from './lib/test-utils.js'; + +const stderrLines: string[] = []; +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: vi.fn(), + writeStderrLine: vi.fn((line: string) => { + stderrLines.push(line); + }), + writeStderrLineSafe: vi.fn(), +})); + +// The oracle is mocked PER SAMPLE — the real one answers what `ls-files -v` +// says at call time, and a bit set-then-cleared inside the capture window +// cannot be scripted against real git from outside the handler. Everything +// else in the module stays real. +const invisibleScript: Array = []; +let invisibleCalls = 0; +vi.mock('./lib/local-anchor.js', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + invisibleTrackedPaths: (repoRoot: string): string[] | null => { + const scripted = invisibleScript[invisibleCalls]; + invisibleCalls += 1; + return scripted !== undefined + ? scripted + : real.invisibleTrackedPaths(repoRoot); + }, + }; +}); + +import { captureLocalCommand } from './capture-local.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); +} + +function capture(): Record { + const out = join(repo, 'plan.json'); + (captureLocalCommand.handler as (argv: unknown) => void)({ + out, + target: 'local', + untracked: true, + }); + return JSON.parse(readFileSync(out, 'utf8')) as Record; +} + +beforeEach(() => { + stderrLines.length = 0; + invisibleScript.length = 0; + invisibleCalls = 0; + repo = realpathSync(mkdtempSync(join(tmpdir(), 'review-loc-vis-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'commit.gpgsign', 'false'); + git('config', 'core.hooksPath', join(repo, '.no-such-hooks')); + write('.gitignore', '.qwen/\nplan.json\n'); + write('src/a.ts', 'export const v = 0;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); +}); + +afterEach(() => { + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +describe('capture-local — visibility oracle rides the sampling discipline', () => { + it('withholds stop and candidate when only an EARLY sample saw the bit', () => { + // R15-1: sampled once, after the capture loop, the oracle read clean + // when a bit set through every diff pass was cleared just before the + // one query — the diffs were blind to the edit the bit hid, the hashes + // held still, and the candidate certified bytes no pass ever showed. + // The oracle now brackets the loop like the diffs and hashes do: a bit + // visible at ANY sample point withholds. Scripted: sample 0 (before the + // first capture) sees the bit; every later sample is clean — exactly + // the shape the single post-loop query certified. + invisibleScript.push(['src/a.ts'], [], [], []); + + const plan = capture(); + // Four samples, exactly — 0 before the first capture, 1-2 in the loop, + // 3 after. The scripted mock consumes by CALL ORDER, so without this + // pin a deleted sample 0 merely shifts the script and the dirty sample + // still lands somewhere: the sandboxed verifier's mutation matrix + // proved the guard unpinned (M6 survived) and this line red under it. + expect(invisibleCalls).toBe(4); + expect(plan['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + expect(stderrLines.join('\n')).toContain( + '--assume-unchanged/--skip-worktree', + ); + }); + + it('withholds when any single sample fails to enumerate', () => { + // A failed enumeration is indistinguishable from a hidden bit — the + // same fail-closed lean as one unhashable path, at every sample point. + invisibleScript.push([], null, [], []); + + const plan = capture(); + expect(plan['nothingToReview']).toBeUndefined(); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(false); + }); + + it('control: all samples clean still decides the clean tree', () => { + invisibleScript.push([], [], [], []); + + const plan = capture(); + expect(plan['nothingToReview']).toEqual({ reason: 'clean-tree' }); + expect( + existsSync( + join(repo, '.qwen/tmp/qwen-review-local-cache-candidate.json'), + ), + ).toBe(true); + }); +}); diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 1f531396004..0579c351cda 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -199,6 +199,61 @@ describe('runCleanup', () => { mocks.rmSync.mockReset(); }); + it('spares THIS run\u2019s stop sidecar, sweeps a foreign one', () => { + // The PR stop path writes the sidecar and runs cleanup in the same + // breath, and the parent's first poll is up to 250 ms away — swept + // here, no reader could ever observe the decision and a decided round + // exited 1 (human review on #9659). Kept only under a matching runId; + // foreign or unstamped residue sweeps as before. + const prev = process.env['QWEN_REVIEW_RUN_ID']; + process.env['QWEN_REVIEW_RUN_ID'] = 'run-A'; + try { + mocks.existsSync.mockReturnValue(true); + mocks.readdirSync.mockImplementation((p: string): string[] => + String(p).endsWith('tmp') ? ['qwen-review-pr-9-stop.json'] : [], + ); + mocks.readFileSync.mockImplementation((p: string) => { + if (String(p).endsWith('qwen-review-pr-9-stop.json')) { + return JSON.stringify({ reason: 'up-to-date', runId: 'run-A' }); + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + runCleanup('pr-9'); + expect(mocks.rmSync).not.toHaveBeenCalledWith( + expect.stringContaining('qwen-review-pr-9-stop.json'), + expect.anything(), + ); + + mocks.rmSync.mockClear(); + process.env['QWEN_REVIEW_RUN_ID'] = 'run-B'; + runCleanup('pr-9'); + expect(mocks.rmSync).toHaveBeenCalledWith( + expect.stringContaining('qwen-review-pr-9-stop.json'), + expect.anything(), + ); + } finally { + if (prev === undefined) delete process.env['QWEN_REVIEW_RUN_ID']; + else process.env['QWEN_REVIEW_RUN_ID'] = prev; + } + }); + + it('refuses the bare `pr` target — its prefix engulfs every PR family', () => { + // R20-4 follow-up: `tmpPrefix('pr')` is `qwen-review-pr-`, a strict + // prefix of EVERY PR round's family, and the lease guard lives inside + // the `pr-` branch a bare `pr` never enters — one `cleanup pr` swept + // every PR's artifacts at once, unguarded. A repo-root file literally + // named `pr` derives exactly this token. + runCleanup('pr'); + expect(process.exitCode).toBe(1); + process.exitCode = 0; + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('Refusing to clean target "pr"'), + ); + expect(mocks.rmSync).not.toHaveBeenCalled(); + // The numbered form is untouched. + expect(() => runCleanup('pr-123')).not.toThrow(); + }); + it('keeps the lease when branch deletion fails', () => { mocks.execFileSync.mockImplementation(() => { throw new Error('branch is locked'); diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index 8898fa84b97..a72c7ee75bf 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -50,6 +50,7 @@ import { tmpFile, tmpPrefix, } from './lib/paths.js'; +import { safeTarget } from '../../utils/paths.js'; interface CleanupArgs { target: string; @@ -702,6 +703,22 @@ function pruneWorktrees(): void { } export function runCleanup(target: string): void { + // A bare `pr` target's sweep prefix (`qwen-review-pr-`) is a strict prefix + // of EVERY PR family, and the lease guard lives inside the `pr-` branch + // below — which a bare `pr` never enters — so one `cleanup pr` deleted + // every PR round's artifacts at once, unguarded (R20-4 follow-up: a + // repo-root file literally named `pr` derives exactly this token). Refused + // outright: no target legitimately owns that family prefix. + if (safeTarget(target) === 'pr') { + writeStderrLine( + `Refusing to clean target "pr": its sweep prefix would match every ` + + `PR review's artifacts (qwen-review-pr--*), and PR leases are ` + + `checked per-number. For a file review of a path named "pr", remove ` + + `only the plan you wrote and its -prompts directory.`, + ); + process.exitCode = 1; + return; + } // Before anything is deleted: the whole temp dir hangs off one path, and a // symlink anywhere above it redirects EVERY sweep below — the scratch family, // the base-tree lock, the side files. The scratch sweep alone used to answer @@ -985,6 +1002,35 @@ export function runCleanup(target: string): void { continue; } if (!file.startsWith(prefix)) continue; + // THIS run's stop sidecar outlives its own cleanup: the PR stop path + // writes the sidecar and runs cleanup in the same breath, and the + // parent's first poll is up to 250 ms away — swept here, neither the + // in-run snapshot nor the post-close fallback could ever observe the + // decision, and an already-decided up-to-date/empty-diff round exited 1 + // "Review did not complete" (human review on #9659). The sidecar is + // kept only when its runId matches the environment the parent stamped — + // a foreign or unstamped one is residue and sweeps as before; the NEXT + // run's cleanup (different nonce) collects this one. + if (file === `${prefix}stop.json`) { + const envRunId = process.env['QWEN_REVIEW_RUN_ID']; + if (envRunId) { + try { + const sidecar = JSON.parse( + readFileSync(join(REVIEW_TMP_DIR, file), 'utf8'), + ) as { runId?: unknown }; + if (sidecar.runId === envRunId) { + writeStdoutLine( + `Kept ${join(REVIEW_TMP_DIR, file)}: this run's stop verdict — ` + + `the parent reads it after the child exits; the next run's ` + + `cleanup collects it.`, + ); + continue; + } + } catch { + // Unreadable or malformed: residue, swept below. + } + } + } const full = join(REVIEW_TMP_DIR, file); if (preserved.has(file)) { writeStdoutLine( diff --git a/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts b/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts new file mode 100644 index 00000000000..25240921f23 --- /dev/null +++ b/packages/cli/src/commands/review/lib/diff-plan.slice.test.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `sliceDiffByLines` carries two contracts nothing else in the review can +// re-establish: the bytes it keeps are byte-identical to the input's (a +// decode/re-encode rewrites every hunk of a non-UTF-8 file), and the ranges +// it is given are `parseDiff`'s own per-file ranges — so a round-trip through +// parse → slice → parse must reproduce the selected sections exactly. + +import { describe, it, expect } from 'vitest'; +import { parseDiff, sliceDiffByLines } from './diff-plan.js'; + +const DIFF = [ + 'diff --git a/a.ts b/a.ts', + 'index 111..222 100644', + '--- a/a.ts', + '+++ b/a.ts', + '@@ -1,1 +1,1 @@', + '-const a = 1;', + '+const a = 2;', + 'diff --git a/b.ts b/b.ts', + 'index 333..444 100644', + '--- a/b.ts', + '+++ b/b.ts', + '@@ -1,1 +1,1 @@', + '-const b = 1;', + '+const b = 2;', + '', +].join('\n'); + +describe('sliceDiffByLines', () => { + it('keeps exactly the selected file sections, parseable on their own', () => { + const parsed = parseDiff(DIFF); + const keep = parsed.files + .filter((f) => f.path === 'b.ts') + .map((f) => ({ startLine: f.diffStart, endLine: f.diffEnd })); + const out = sliceDiffByLines(Buffer.from(DIFF, 'utf8'), keep); + const text = out.toString('utf8'); + expect(text).toContain('b/b.ts'); + expect(text).not.toContain('a.ts'); + expect(parseDiff(text).files.map((f) => f.path)).toEqual(['b.ts']); + }); + + it('is byte-exact: invalid UTF-8 and lone CR survive verbatim', () => { + const raw = Buffer.concat([ + Buffer.from('keep '), + Buffer.from([0x80, 0x0d, 0x81]), + Buffer.from('\ndrop\n'), + ]); + const out = sliceDiffByLines(raw, [{ startLine: 1, endLine: 1 }]); + expect([...out]).toEqual([...raw.subarray(0, raw.indexOf(0x0a) + 1)]); + }); + + it('re-orders ranges by line and clamps past the last line', () => { + const buf = Buffer.from('l1\nl2\nl3\n', 'utf8'); + expect( + sliceDiffByLines(buf, [ + { startLine: 3, endLine: 9 }, + { startLine: 1, endLine: 1 }, + ]).toString('utf8'), + ).toBe('l1\nl3\n'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/diff-plan.ts b/packages/cli/src/commands/review/lib/diff-plan.ts index f01466bcc99..9548b4690a8 100644 --- a/packages/cli/src/commands/review/lib/diff-plan.ts +++ b/packages/cli/src/commands/review/lib/diff-plan.ts @@ -803,3 +803,33 @@ export function chunksCoverDiff( } return expected === diffLines + 1; } + +/** + * Cut a captured diff down to the file sections named by `keep`, by BYTES. + * + * The capture contract is bytes end to end: a decode/re-encode rewrites the + * content of every hunk touching a file git handed over in a non-UTF-8 + * encoding. The caller passes the 1-based inclusive LINE ranges `parseDiff` + * reported per file (`diffStart`/`diffEnd`), and this maps them to byte + * ranges over the same newline structure `parseDiff` walked. Slicing an + * existing diff — rather than re-capturing with pathspecs — is also what + * keeps RENAME sections intact: a pathspec-scoped `git diff` cannot see the + * rename source, un-pairs the rename, and renders the file as a whole-file + * add whose hunks exist nowhere in the original diff. + */ +export function sliceDiffByLines( + diff: Buffer, + keep: ReadonlyArray<{ startLine: number; endLine: number }>, +): Buffer { + // Byte offset of the start of each 1-based line; sentinel = buffer length. + const starts: number[] = [0]; + for (let i = 0; i < diff.length; i++) { + if (diff[i] === 0x0a) starts.push(i + 1); + } + const offsetOf = (line1: number): number => + line1 - 1 < starts.length ? starts[line1 - 1] : diff.length; + const parts = [...keep] + .sort((a, b) => a.startLine - b.startLine) + .map((r) => diff.subarray(offsetOf(r.startLine), offsetOf(r.endLine + 1))); + return Buffer.concat(parts); +} diff --git a/packages/cli/src/commands/review/lib/git.ts b/packages/cli/src/commands/review/lib/git.ts index 2f5a2067b45..07f4a344425 100644 --- a/packages/cli/src/commands/review/lib/git.ts +++ b/packages/cli/src/commands/review/lib/git.ts @@ -60,9 +60,39 @@ export function git(...args: string[]): string { * every other subcommand would try to open the name as an ordinary file. */ export function gitWithInput(input: Buffer, args: string[]): string { - return execFileSync('git', args, { ...gitOpts(), encoding: 'utf8', input }) - .replace(/\r\n/g, '\n') - .trim(); + return gitWithInputRaw(input, args).replace(/\r\n/g, '\n').trim(); +} + +/** + * `gitWithInput` with the output UNTOUCHED — no CRLF rewrite, no trim. + * + * For a NUL-delimited protocol the convenience form is a corruption. `git + * check-attr --stdin -z` echoes each path back as a record key, and a path + * may legally begin with whitespace or contain `\r\n`: the trim eats the + * leading byte of the first record so its key no longer matches the path that + * was asked about, and the CRLF rewrite can collide one record's key with a + * sibling's. The caller then reads a MALFORMED identity rather than an honest + * `UNHASHABLE` — and in one concrete direction it fails OPEN, because the + * eaten record is the `diff` attribute, so a `diff=` path never folds + * its driver's `binary` setting in and the config-side binary↔text flip the + * identity exists to track goes invisible. + */ +export function gitWithInputRaw(input: Buffer, args: string[]): string { + return execFileSync('git', args, { + ...gitOpts(), + encoding: 'utf8', + input, + // The same raised ceiling `gitRaw` takes, for the same reason. The one + // caller is `check-attr --stdin -z`, which emits roughly three records + // per path: this repository's ~6,270 hashable source files produce about + // 1.16 MB, over `execFileSync`'s 1 MB default. Past it the call throws + // ENOBUFS, `renderingAttributes`' blanket catch answers an empty map, and + // every identity becomes UNHASHABLE — which never equals itself, so every + // path reads as changed on every round while the stateId stays stable and + // no refusal ever prints. The whole target is silently re-reviewed for + // ever and the unchanged-since stop becomes unreachable. + maxBuffer: 512 * 1024 * 1024, + }); } /** diff --git a/packages/cli/src/commands/review/lib/local-anchor.batch.test.ts b/packages/cli/src/commands/review/lib/local-anchor.batch.test.ts new file mode 100644 index 00000000000..ea22c7d1699 --- /dev/null +++ b/packages/cli/src/commands/review/lib/local-anchor.batch.test.ts @@ -0,0 +1,118 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The batch mechanics of `hashWorktreeFiles`, against a mocked git layer: +// the E2E suites only ever hash a handful of files, so the 200-file window +// arithmetic and the failed-batch fallback had zero coverage — both were +// measured surviving mutations (an empty batch window, a bypassed fallback). + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync, realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const gitOpt = vi.fn<(...args: string[]) => string | null>(); +vi.mock('./git.js', () => ({ + gitOpt: (...args: string[]) => gitOpt(...args), + // Two consumers, told apart by the command: `hash-object --stdin` for a + // symlink's link text, `check-attr` for the rendering attributes. Without + // the second these tests would exercise the unknown-attributes fallback + // rather than the batching they are about. + gitWithInput: vi.fn((input: Buffer, args: string[]) => + args.includes('check-attr') ? checkAttr(input) : 'link-oid', + ), + // `check-attr --stdin -z` goes through the RAW variant: the convenience + // form's `.trim()` steals the first record's key from a path that begins + // with whitespace. + gitWithInputRaw: vi.fn((input: Buffer, args: string[]) => + args.includes('check-attr') ? checkAttr(input) : 'link-oid', + ), +})); + +/** `git check-attr --stdin -z`'s echo, for the one attribute these ask for. */ +function checkAttr(input: Buffer): string { + return String(input) + .split('\0') + .filter((p) => p !== '') + .map((p) => `${p}\0diff\0unspecified\0`) + .join(''); +} + +import { hashWorktreeFiles, UNHASHABLE } from './local-anchor.js'; + +let dir: string; +let paths: string[]; + +beforeEach(() => { + gitOpt.mockReset(); + dir = realpathSync(mkdtempSync(join(tmpdir(), 'anchor-batch-'))); + paths = []; + for (let i = 0; i < 201; i++) { + const p = `f${String(i).padStart(3, '0')}.txt`; + writeFileSync(join(dir, p), String(i)); + paths.push(p); + } +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +/** A git stub answering each batch with one fake oid per pathspec. */ +function answerBatches(): void { + gitOpt.mockImplementation((...args: string[]) => { + // Faithful to real git: no `diff..binary` is configured here, so + // the config probes the driver fold runs answer null (the fixture paths + // all answer `diff=unspecified`, which is a legal driver NAME too, and is + // probed like any other candidate). + if (args.includes('config')) return null; + const files = args.slice(args.indexOf('--') + 1); + return files.map((f) => `oid-${f}`).join('\n'); + }); +} + +describe('hashWorktreeFiles — batching', () => { + it('windows at 200 pathspecs per ls call and maps every file to its own oid', () => { + answerBatches(); + const out = hashWorktreeFiles(dir, paths); + expect(Object.keys(out)).toHaveLength(201); + for (const p of paths) + expect(out[p]).toBe(`100644:oid-${p}:diff=unspecified`); + const batchSizes = gitOpt.mock.calls + .filter((c) => !c.includes('config')) // the driver probes are not ls calls + .map((c) => c.slice(c.indexOf('--') + 1).length); + expect(batchSizes).toEqual([200, 1]); + }); + + it('a failed batch falls back to per-file hashing — one bad file costs itself', () => { + let batchCalls = 0; + gitOpt.mockImplementation((...args: string[]) => { + if (args.includes('config')) return null; // no driver configured + const files = args.slice(args.indexOf('--') + 1); + if (files.length > 1) { + batchCalls++; + return null; // the whole batch refused, as one unreadable file does + } + return files[0] === 'f007.txt' ? null : `oid-${files[0]}`; + }); + const out = hashWorktreeFiles(dir, paths.slice(0, 10)); + expect(batchCalls).toBe(1); + expect(out['f007.txt']).toBe(UNHASHABLE); + expect(out['f003.txt']).toBe('100644:oid-f003.txt:diff=unspecified'); + }); + + it('a mismatched batch reply (wrong line count) also takes the fallback', () => { + gitOpt.mockImplementation((...args: string[]) => { + if (args.includes('config')) return null; // no driver configured + const files = args.slice(args.indexOf('--') + 1); + if (files.length > 1) return 'just-one-line'; + return `oid-${files[0]}`; + }); + const out = hashWorktreeFiles(dir, paths.slice(0, 3)); + for (const p of paths.slice(0, 3)) + expect(out[p]).toBe(`100644:oid-${p}:diff=unspecified`); + }); +}); diff --git a/packages/cli/src/commands/review/lib/local-anchor.integration.test.ts b/packages/cli/src/commands/review/lib/local-anchor.integration.test.ts new file mode 100644 index 00000000000..0ac7ac96f14 --- /dev/null +++ b/packages/cli/src/commands/review/lib/local-anchor.integration.test.ts @@ -0,0 +1,508 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real `git check-attr`, over paths that only real git renders faithfully. +// The property under test is byte-fidelity of a NUL-delimited protocol, and a +// mocked wrapper cannot break it the way the real one did. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + existsSync, + readFileSync, + mkdtempSync, + mkdirSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { + changedSince, + hashWorktreeFiles, + revisionIdentities, + UNHASHABLE, + invisibleTrackedPaths, +} from './local-anchor.js'; +import { isolateHostGitConfig } from './test-utils.js'; + +let repo: string; +let cwd: string; +let gitIsolation: ReturnType; + +const git = (...args: string[]) => + execFileSync('git', args, { cwd: repo, encoding: 'utf8' }); + +beforeEach(() => { + repo = realpathSync(mkdtempSync(join(tmpdir(), 'anchor-attr-'))); + cwd = process.cwd(); + process.chdir(repo); + gitIsolation = isolateHostGitConfig(); + git('init', '-q', '--template=', '.'); + git('config', 'user.email', 'a@b'); + git('config', 'user.name', 'a'); + git('config', 'core.autocrlf', 'false'); +}); + +afterEach(() => { + process.chdir(cwd); + rmSync(repo, { recursive: true, force: true }); + gitIsolation.dispose(); +}); + +describe('hashWorktreeFiles — the attributes probe is not buffer-bound', () => { + it('answers for a path count whose check-attr output passes 1 MB', () => { + // `check-attr --stdin -z` emits roughly three NUL records per path, so a + // few thousand files pass `execFileSync`'s 1 MB default and the call + // throws ENOBUFS. The blanket catch then answers an empty attribute map + // and every identity becomes UNHASHABLE — which never equals itself, so + // every path reads as changed on every round. Nothing surfaces: the + // stateId stays stable, so the anchor still validates and no refusal ever + // prints, while the whole target is silently re-reviewed for ever and the + // unchanged-since stop is unreachable. + const paths: string[] = []; + for (let i = 0; i < 4000; i++) { + const rel = `f${i}-${'p'.repeat(60)}.ts`; + writeFileSync(join(repo, rel), 'export const a = 1;\n'); + paths.push(rel); + } + + const out = hashWorktreeFiles(repo, paths); + + expect(Object.keys(out)).toHaveLength(paths.length); + // Attributes reached the identity — an ENOBUFS would have left every one + // of these UNHASHABLE instead. + expect(out[paths[0]]).toContain('diff='); + expect(Object.values(out).some((v) => v === 'unhashable')).toBe(false); + }); +}); + +describe('hashWorktreeFiles — the attributes probe is byte-faithful', () => { + it('keeps the record of a path that begins with whitespace', () => { + // A leading space is legal in a path on Linux and macOS, and + // `check-attr --stdin -z` echoes the path back as each record's key. Read + // through a wrapper that trims, the first record's key loses that byte: + // it no longer matches the path that was asked about, every record shifts + // onto a phantom key, and the path gets a MALFORMED identity rather than + // an honest UNHASHABLE. + // + // That fails OPEN in one direction. The stolen record is the `diff` + // attribute, so a `diff=` path never folds its driver's `binary` + // setting in — and the config-side binary↔text flip the identity exists + // to track becomes invisible between rounds. + const leading = ' leading.ts'; + writeFileSync(join(repo, leading), 'export const a = 1;\n'); + writeFileSync(join(repo, 'plain.ts'), 'export const b = 1;\n'); + writeFileSync(join(repo, '.gitattributes'), '" leading.ts" diff=custom\n'); + + const out = hashWorktreeFiles(repo, [leading, 'plain.ts']); + + // The whitespace path is answered for, under its own name… + expect(Object.keys(out)).toContain(leading); + // …and its record is the one git gave for IT: the driver name survives, + // which is what the config-side `binary` lookup keys on. + expect(out[leading]).toContain('diff=custom'); + // The sibling is unaffected either way — it is the control that shows the + // probe ran at all rather than falling back wholesale. + expect(out['plain.ts']).toContain('diff=unspecified'); + }); + + it("folds a driver's binary flag only into the paths naming THAT driver", () => { + // The fold used to match the record string by substring, so a driver + // whose name is a PREFIX of another (`md` / `mdbook`) folded its config + // into the other's paths: toggling `diff.md.binary` then re-reviewed + // every `mdbook` file each round although its bytes, mode and own driver + // never moved — the wasted re-review the attribute component exists to + // prevent. + writeFileSync(join(repo, 'a.md'), '# a\n'); + writeFileSync(join(repo, 'book.md'), '# book\n'); + writeFileSync( + join(repo, '.gitattributes'), + 'a.md diff=md\nbook.md diff=mdbook\n', + ); + git('config', 'diff.md.binary', 'true'); + + const out = hashWorktreeFiles(repo, ['a.md', 'book.md']); + + // The fold lands on the path naming the driver… + expect(out['a.md']).toContain('diff=md'); + expect(out['a.md']).toContain('md.binary=true'); + // …and the PREFIXED driver's path keeps its identity clean of it. + expect(out['book.md']).toContain('diff=mdbook'); + expect(out['book.md']).not.toContain('md.binary'); + }); + + it('folds a driver whose NAME CONTAINS A COMMA — matched as a value, never re-parsed', () => { + // `*.bin diff=a,b` is a legal gitattributes line, and the fold used to + // re-parse the comma-joined attribute serialization — `split(',')` can + // never match a value containing a comma. The flag was silently dropped + // from the identity, so flipping it changed how `git diff` rendered the + // same bytes while the identity stood still: the next round's gate + // compared equal and sliced the file out of scope, carrying the previous + // verdict forward against a different rendering. `.gitattributes` is + // worktree content of the reviewed PR, so the driver name is plantable. + writeFileSync(join(repo, 'data.bin'), 'x\n'); + writeFileSync(join(repo, '.gitattributes'), 'data.bin diff=a,b\n'); + git('config', 'diff.a,b.binary', 'true'); + + const on = hashWorktreeFiles(repo, ['data.bin']); + expect(on['data.bin']).toContain('a,b.binary=true'); + + git('config', 'diff.a,b.binary', 'false'); + const off = hashWorktreeFiles(repo, ['data.bin']); + expect(off['data.bin']).toContain('a,b.binary=false'); + expect(off['data.bin']).not.toBe(on['data.bin']); + }); + + it('a driver literally named `set` is uncertifiable — the answer is ambiguous', () => { + // `data.bin diff=set` is a legal gitattributes line naming a DRIVER + // `set`, and `check-attr` answers it byte-identically to the `set` + // attribute STATE. Folding the spelling into the identity — even with + // the `diff.set.binary` config folded beside it — collapsed two + // DIFFERENT rendering states into one identity: plain `diff` renders + // readable hunks under `diff.set.binary=true` while `diff=set` renders + // "Binary files differ" (probed, git 2.39.5), yet both answer `set` and + // both fold the same config, so the pair can never be told apart on + // this stream. The fold existed to track the config-side flip; the + // state-vs-name conflation is one entrance it cannot close, and the + // module's standard for a rendering it cannot capture is UNHASHABLE — + // re-reviewed every round rather than certified across the flip. + // `.gitattributes` is worktree content of the reviewed PR, so the driver + // name is plantable; `unset` is plantable the same way (and needs no + // config at all to diverge — see the sibling suite below). + writeFileSync(join(repo, 'data.bin'), 'x\n'); + writeFileSync(join(repo, '.gitattributes'), 'data.bin diff=set\n'); + git('config', 'diff.set.binary', 'true'); + + const out = hashWorktreeFiles(repo, ['data.bin']); + expect(out['data.bin']).toBe(UNHASHABLE); + }); + + it('a plain `diff` attribute is uncertifiable too — the sibling ambiguity', () => { + // The state half of the pair above: a path whose `diff` attribute is + // merely SET (no driver) is answered `set`, byte-identically to a + // driver literally named `set`. Neither half can be certified without + // the other, so both take UNHASHABLE. + writeFileSync(join(repo, 'data.bin'), 'x\n'); + writeFileSync(join(repo, '.gitattributes'), 'data.bin diff\n'); + + const out = hashWorktreeFiles(repo, ['data.bin']); + expect(out['data.bin']).toBe(UNHASHABLE); + }); +}); +describe('hashWorktreeFiles — a decoded path is not a name', () => { + it('refuses to hash a path carrying U+FFFD', () => { + // The capture pins `core.quotePath=false` and decodes with `toString`, + // so every invalid byte folds to U+FFFD. Beside a file LITERALLY named + // with one, two plan paths fold to a single key: `lstat` succeeds on the + // real file, the invalid-byte sibling inherits its identity, is never + // hashed, and its changes compare unchanged for ever. The `lstat` guard + // cannot see it, because the stat succeeds. + writeFileSync(join(repo, '\ufffd.ts'), 'export const a = 1;\n'); + const out = hashWorktreeFiles(repo, ['\ufffd.ts', 'plain.ts']); + expect(out['\ufffd.ts']).toBe('unhashable'); + }); +}); +describe('hashWorktreeFiles — every diff-driver spelling reaches the fold', () => { + it('folds the EMPTY driver name, which git accepts as `diff..binary`', () => { + // `*.dat diff=` is a legal attributes line, `check-attr --stdin -z` + // answers it with an empty value, and `git config diff..binary true` + // flips that section between readable hunks and "Binary files differ" + // with the mode and the blob standing still (verified against git + // 2.47.3). Excluding the empty spelling was the last entrance of the + // family whose `set`/`unset`/`unspecified` siblings were already closed. + writeFileSync(join(repo, 'a.dat'), 'hello\n'); + writeFileSync(join(repo, '.gitattributes'), '*.dat diff=\n'); + const before = hashWorktreeFiles(repo, ['a.dat'])['a.dat']; + + execFileSync('git', ['config', 'diff..binary', 'true'], { cwd: repo }); + const after = hashWorktreeFiles(repo, ['a.dat'])['a.dat']; + + // The bytes and the mode did not move; the RENDERING did, so the + // identity has to. + expect(after).not.toBe(before); + expect(after).toContain('.binary=true'); + }); +}); + +describe('hashWorktreeFiles — an undecodable driver name unhashes the WHOLE identity', () => { + it('marks the identity UNHASHABLE, not a composite ending in the slot', () => { + // The record stream is utf8-decoded, so an invalid byte in a driver NAME + // folds to U+FFFD and the config probe could never match the raw-byte key + // git itself matches — `renderingAttributes` answers UNHASHABLE for the + // path (verified against git 2.39.5: `check-attr --stdin -z` echoes the + // raw byte back). Composing that answer onto the mode-and-blob prefix + // produced `100644::unhashable` — a string that compares equal to + // itself across rounds, so flipping `diff..binary` changed + // the rendering while the identity stood still and the section was + // sliced out of scope. The module's own standard applies to the WHOLE + // identity: what cannot be named faithfully cannot be certified. + writeFileSync( + join(repo, '.gitattributes'), + Buffer.concat([ + Buffer.from('data.bin diff='), + Buffer.from([0xff]), + Buffer.from('drv\n'), + ]), + ); + writeFileSync(join(repo, 'data.bin'), 'x\n'); + writeFileSync(join(repo, 'plain.ts'), 'export const a = 1;\n'); + + const out = hashWorktreeFiles(repo, ['data.bin', 'plain.ts']); + + expect(out['data.bin']).toBe('unhashable'); + // The sibling is the control arm: the probe ran and answered normally, + // so the UNHASHABLE above is the undecodable-name discipline, not a + // wholesale fallback (an ENOBUFS-style empty answer map would unhash + // BOTH). + expect(out['plain.ts']).toContain('diff=unspecified'); + }); + + it('re-enters scope on the next round instead of comparing unchanged', async () => { + // The consequence the identity exists for: UNHASHABLE never equals + // itself, so a round that hashed the path re-reviews it rather than + // comparing it unchanged and slicing it out of scope with the previous + // verdict riding on a rendering no round saw. The composite identity + // this replaces compared equal to itself, which is exactly what made + // the rendering flip invisible. + const { changedSince } = await import('./local-anchor.js'); + writeFileSync( + join(repo, '.gitattributes'), + Buffer.concat([ + Buffer.from('data.bin diff='), + Buffer.from([0xff]), + Buffer.from('drv\n'), + ]), + ); + writeFileSync(join(repo, 'data.bin'), 'x\n'); + + const before = hashWorktreeFiles(repo, ['data.bin']); + const after = hashWorktreeFiles(repo, ['data.bin']); + + expect(changedSince(before, after)).toContain('data.bin'); + }); +}); + +describe('hashWorktreeFiles — a path listed twice is hashed once', () => { + it('dedups repeated input paths at the module boundary', () => { + // Two open Criticals citing the SAME file is an ordinary ledger shape, + // and the blocker date passes its file list straight through. + // `check-attr` answers once per input OCCURRENCE, so a duplicate used to + // append the rendering suffix once per listing — an identity that never + // matches the cache's single-suffix one, read as "moved" under every + // possible tree state, for ever. + writeFileSync(join(repo, 'f.ts'), 'export const a = 1;\n'); + + const single = hashWorktreeFiles(repo, ['f.ts']); + const dup = hashWorktreeFiles(repo, ['f.ts', 'f.ts']); + + expect(dup['f.ts']).toBe(single['f.ts']); + expect(Object.keys(dup)).toEqual(['f.ts']); + }); +}); + +describe('revisionIdentities — ledger paths are read literally', () => { + it('survives a path beginning with pathspec magic', () => { + // The paths come from the model-written ledger, an untrusted-input + // boundary: a name beginning `:(` is pathspec magic, and known-but- + // unsupported magic fatals `ls-tree` with exit 128 — the WHOLE batch. + // The catch then answered `{}`, reading every datable sibling in the + // call undatable too, so one hostile or malformed ledger path cleared + // the entire `--fail-on` gate. + writeFileSync(join(repo, 'sib.md'), 'sib\n'); + git('config', 'commit.gpgsign', 'false'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + const head = git('rev-parse', 'HEAD').trim(); + + const ids = revisionIdentities(repo, head, [':(glob)notes.md', 'sib.md']); + + // The sibling is dated normally; the magic name is simply absent from + // the tree — undatable, not fatal. + expect(ids['sib.md']).toMatch(/^100644:[0-9a-f]{40}:/); + expect(ids[':(glob)notes.md']).toBeUndefined(); + }); +}); + +describe('hashWorktreeFiles — a state name spelled as a value is uncertifiable', () => { + // `check-attr` answers an attribute STATE and a VALUE assignment that + // spells a state name byte-identically, while `git diff` renders them + // differently: `*.dat -diff` renders "Binary files … differ" and + // `*.dat diff=unset` renders readable hunks, both answered `diff: unset` + // (probed, git 2.39.5 — no config involved). The identity cannot carry a + // distinction the probe stream cannot name, so such a path takes + // UNHASHABLE — re-reviewed every round rather than certified across a + // rendering flip. + it('folds the whole identity to UNHASHABLE when `diff` answers `unset` or `set`', () => { + writeFileSync(join(repo, 'data.dat'), 'line one\nline two\n'); + + writeFileSync(join(repo, '.gitattributes'), '*.dat -diff\n'); + const stateUnset = hashWorktreeFiles(repo, ['data.dat']); + expect(stateUnset['data.dat']).toBe(UNHASHABLE); + + writeFileSync(join(repo, '.gitattributes'), '*.dat diff=unset\n'); + const namedUnset = hashWorktreeFiles(repo, ['data.dat']); + expect(namedUnset['data.dat']).toBe(UNHASHABLE); + + writeFileSync(join(repo, '.gitattributes'), '*.dat diff\n'); + const stateSet = hashWorktreeFiles(repo, ['data.dat']); + expect(stateSet['data.dat']).toBe(UNHASHABLE); + }); + + it('a flip between the two ambiguous spellings re-enters scope', () => { + // The pre-fix identity was byte-identical across the flip, so + // `changedSince` read the file as unchanged and the newly-readable + // section was sliced out of scope carrying the previous verdict. + writeFileSync(join(repo, 'data.dat'), 'line one\nline two\n'); + writeFileSync(join(repo, '.gitattributes'), '*.dat -diff\n'); + const round1 = hashWorktreeFiles(repo, ['data.dat']); + writeFileSync(join(repo, '.gitattributes'), '*.dat diff=unset\n'); + const round2 = hashWorktreeFiles(repo, ['data.dat']); + expect(changedSince(round1, round2)).toEqual(['data.dat']); + }); + + it('keeps unambiguous answers folding — the closure narrows, not widens', () => { + writeFileSync(join(repo, 'data.dat'), 'line one\nline two\n'); + writeFileSync(join(repo, 'plain.ts'), 'export const a = 1;\n'); + writeFileSync(join(repo, '.gitattributes'), '*.dat diff=mydrv\n'); + + const ids = hashWorktreeFiles(repo, ['data.dat', 'plain.ts']); + // A real driver name still folds into the identity… + expect(ids['data.dat']).toContain('diff=mydrv'); + expect(ids['data.dat']).not.toBe(UNHASHABLE); + // …and `unspecified` — the answer every unattributed path gets — stays + // foldable, or the anchor would re-review the entire tree every round. + expect(ids['plain.ts']).toContain('diff=unspecified'); + expect(ids['plain.ts']).not.toBe(UNHASHABLE); + }); +}); + +describe('invisibleTrackedPaths — sparse-checkout owns its S bits', () => { + it('exempts out-of-cone S paths, keeps manual bits and non-sparse absences', () => { + // R17-5: every out-of-cone tracked path in a sparse checkout is + // S-tagged by design and absent from the worktree — counting them made + // the oracle non-empty on every sample, withholding the candidate and + // all three decided stops on a completely clean materialized tree, for + // ever. An absent path holds no file to hide an edit in, so under + // sparse-checkout an absent S path is exempt; an in-cone manually + // skip-worktree'd file still flags, and outside sparse-checkout an + // absent S path stays flagged — there the bit is a user's hand and the + // absence is a deletion it hides. + writeFileSync(join(repo, 'in.ts'), 'export const a = 1;\n'); + git('add', 'in.ts'); + mkdirSync(join(repo, 'sub'), { recursive: true }); + writeFileSync(join(repo, 'sub/out.ts'), 'export const b = 1;\n'); + git('add', 'sub/out.ts'); + git('commit', '-q', '--no-verify', '-m', 'base'); + + git('sparse-checkout', 'set', '--cone', '.'); + // Cone mode with only the root: sub/ leaves the worktree, S-tagged. + expect(existsSync(join(repo, 'sub/out.ts'))).toBe(false); + expect(invisibleTrackedPaths(repo)).toEqual([]); + + // The assume-unchanged family is never exempted, sparse or not: git + // does not manage those bits for any feature, so a lowercase tag is + // always a user's hand. (A manual skip-worktree on an in-cone file is + // unconstructible here — cone-mode git silently re-clears it, measured + // on git 2.47.) + git('update-index', '--assume-unchanged', 'in.ts'); + expect(invisibleTrackedPaths(repo)).toEqual(['in.ts']); + git('update-index', '--no-assume-unchanged', 'in.ts'); + + // Outside sparse-checkout, an absent S path is a hidden deletion and + // stays flagged. + git('sparse-checkout', 'disable'); + git('update-index', '--skip-worktree', 'sub/out.ts'); + rmSync(join(repo, 'sub/out.ts')); + expect(invisibleTrackedPaths(repo)).toEqual(['sub/out.ts']); + }); + + it('exempts the combined-bit lowercase `s` out-of-cone spelling too', () => { + // R18-4: an entry carrying BOTH skip-worktree and assume-unchanged + // renders lowercase `s`, so an S-only match missed it and the wedge the + // exemption closes re-opened for any de-coned path that ever carried + // assume-unchanged. Membership comes from `check-rules`, not the tag. + writeFileSync(join(repo, 'in.ts'), 'export const a = 1;\n'); + mkdirSync(join(repo, 'sub'), { recursive: true }); + writeFileSync(join(repo, 'sub/out.ts'), 'export const b = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + git('update-index', '--assume-unchanged', 'sub/out.ts'); + git('sparse-checkout', 'set', '--cone', '.'); + expect(existsSync(join(repo, 'sub/out.ts'))).toBe(false); + expect(invisibleTrackedPaths(repo)).toEqual([]); + }); + + it('an inherited GLOBAL core.sparseCheckout never turns the exemption on', () => { + // R18-2 axis (b): the flag read keeps HOME, so a global `true` used to + // apply the exemption on a repo that is not sparse — and an absent + // manually-bitted path was swallowed. The read is `--worktree` now: + // repository state only. + writeFileSync(join(repo, 'in.ts'), 'export const a = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + git('config', '--global', 'core.sparseCheckout', 'true'); + git('update-index', '--skip-worktree', 'in.ts'); + rmSync(join(repo, 'in.ts')); + expect(invisibleTrackedPaths(repo)).toEqual(['in.ts']); + }); + + it('reads the flag as a canonicalized bool — legacy spellings count', () => { + // R18-2 axis (a): `--get` alone echoes the stored spelling, so the + // legacy sparse recipe (`core.sparseCheckout on` + info/sparse-checkout + // + read-tree) failed a === 'true' comparison and re-wedged. Cone mode + // spelled `yes` here exercises the canonicalization end to end. + writeFileSync(join(repo, 'in.ts'), 'export const a = 1;\n'); + mkdirSync(join(repo, 'sub'), { recursive: true }); + writeFileSync(join(repo, 'sub/out.ts'), 'export const b = 1;\n'); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'base'); + git('sparse-checkout', 'set', '--cone', '.'); + // Written into .git/config by hand: modern `git config` canonicalizes a + // bool-valued core variable on WRITE, so only the raw file can hold the + // legacy spelling the manual recipe left behind. + // `sparse-checkout set` enables extensions.worktreeConfig and writes + // the flag into config.worktree — which is also why the production read + // is `--worktree`. + const cfg = join(repo, '.git/config.worktree'); + writeFileSync( + cfg, + readFileSync(cfg, 'utf8').replace( + /sparseCheckout = true/i, + 'sparseCheckout = yes', + ), + ); + expect(readFileSync(cfg, 'utf8')).toMatch(/sparseCheckout = yes/i); + // What `--get` echoes for the stored `yes` is version-dependent (2.47 + // canonicalizes even on read; 2.43 — this repo's CI git, where R18-2 + // was probed — echoes the raw spelling), which is exactly why the + // production read pins `--type=bool`: on every git it answers `true`. + expect(existsSync(join(repo, 'sub/out.ts'))).toBe(false); + expect(invisibleTrackedPaths(repo)).toEqual([]); + }); +}); + +describe('hashWorktreeFiles — a configured `unspecified` driver is uncertifiable', () => { + it('takes UNHASHABLE under diff.unspecified.binary, folds nothing without it', () => { + // R17-3: `check-attr` answers `diff=unspecified` byte-identically for + // the no-rule state and an explicit `diff=unspecified` value, and the + // two render differently exactly when `diff.unspecified.binary` is + // configured — the fold cannot split what the stream spells alike, so + // the config's presence makes the dimension ambiguous for every path + // that answered it. Without the config nothing changes. + writeFileSync(join(repo, 'data.dat'), 'plain bytes\n'); + git('add', 'data.dat'); + + const before = hashWorktreeFiles(repo, ['data.dat'])['data.dat']; + expect(before).not.toBe(UNHASHABLE); + expect(before).toContain('diff=unspecified'); + + git('config', 'diff.unspecified.binary', 'true'); + const after = hashWorktreeFiles(repo, ['data.dat'])['data.dat']; + expect(after).toBe(UNHASHABLE); + }); +}); diff --git a/packages/cli/src/commands/review/lib/local-anchor.oracle.test.ts b/packages/cli/src/commands/review/lib/local-anchor.oracle.test.ts new file mode 100644 index 00000000000..3ea19b139c9 --- /dev/null +++ b/packages/cli/src/commands/review/lib/local-anchor.oracle.test.ts @@ -0,0 +1,129 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +// The git layer is scripted, not real: the shape these tests pin — a manual +// skip-worktree bit surviving on an IN-CONE path whose file was deleted — is +// version-dependent (git 2.43 retains the bit, 2.47 re-clears it and +// restores the file during `sparse-checkout set`), so real git on a modern +// machine cannot construct it end to end. The integration suite drives the +// constructible arms against real git; this file drives the 2.43 arms +// against the same production code with git's answers replayed. +const script = { + lsFiles: '' as string, + sparseBool: null as string | null, + checkRules: '' as string, + checkRulesThrows: false, +}; +vi.mock('./git.js', async (importOriginal) => { + const real = await importOriginal(); + return { + ...real, + gitRaw: (...args: string[]): Buffer => { + if (args.includes('ls-files')) return Buffer.from(script.lsFiles); + return real.gitRaw(...args); + }, + gitOpt: (...args: string[]): string | null => { + if (args.includes('core.sparseCheckout')) return script.sparseBool; + return real.gitOpt(...args); + }, + gitWithInputRaw: (input: Buffer, args: string[]): string => { + if (args.includes('check-rules')) { + if (script.checkRulesThrows) throw new Error('no such subcommand'); + return script.checkRules; + } + return real.gitWithInputRaw(input, args); + }, + }; +}); + +import { invisibleTrackedPaths } from './local-anchor.js'; + +let repo: string; + +beforeEach(() => { + repo = realpathSync(mkdtempSync(join(tmpdir(), 'oracle-'))); + script.lsFiles = ''; + script.sparseBool = null; + script.checkRules = ''; + script.checkRulesThrows = false; +}); + +afterEach(() => { + rmSync(repo, { recursive: true, force: true }); +}); + +describe('invisibleTrackedPaths — the check-rules arms real git cannot stage', () => { + it('flags an absent IN-RULES path under sparse — the 2.43 hidden deletion', () => { + // R18-3: git 2.43 retains a manual --skip-worktree on an in-cone path; + // delete the file and every diff goes quiet while the bit hides the + // deletion. Absence alone must not exempt — membership does: an + // in-rules absent path is a deletion no round read. + script.lsFiles = 'S in.ts\0S sub/out.ts\0'; + script.sparseBool = 'true'; + script.checkRules = 'in.ts\0'; // in.ts IS in the rules; sub/out.ts is not + // Neither file exists on disk; only the out-of-rules one is exempt. + expect(invisibleTrackedPaths(repo)).toEqual(['in.ts']); + }); + + it('exempts nothing when check-rules is unavailable — fail closed', () => { + // An older git without the subcommand answers nothing: the exemption + // stands down and the pre-exemption behaviour (flag every bit) returns — + // a wedge on that git, never a certification. + script.lsFiles = 'S sub/out.ts\0'; + script.sparseBool = 'true'; + script.checkRulesThrows = true; + expect(invisibleTrackedPaths(repo)).toEqual(['sub/out.ts']); + }); + + it('an UNMEASURABLE out-of-rules path keeps flagging — only ENOENT is absence', () => { + // R19-2: every lstat failure used to fold into "absent", so a PRESENT + // flagged path under an unreadable ancestor was exempted and the oracle + // read clean while `git diff` was blind to its bytes. ENOTDIR stages + // the unmeasurable shape deterministically (EACCES needs a non-root + // runner): the path runs through a regular FILE, so it cannot be + // proven absent — and unmeasurable is uncertifiable. + script.lsFiles = 'S f/x.ts\0'; + script.sparseBool = 'true'; + script.checkRules = ''; // out of the rules — exempt IF it were absent + writeFileSync(join(repo, 'f'), 'a regular file, not a directory\n'); + expect(invisibleTrackedPaths(repo)).toEqual(['f/x.ts']); + }); + + it('an undecodable name is never exempted — U+FFFD cannot be measured', () => { + // R19-1: the utf8 decode folds invalid bytes to U+FFFD; lstat on the + // mangled spelling misses the PRESENT file and check-rules is fed a + // name git never knew — both halves of the exemption run on a name + // that is not the path's. The discipline hashWorktreeFiles and + // revisionIdentities already apply lands here too: flagged, always. + script.lsFiles = 'h b\uFFFD.dat\0'; + script.sparseBool = 'true'; + script.checkRules = ''; + expect(invisibleTrackedPaths(repo)).toEqual(['b\uFFFD.dat']); + }); + + it('a PRESENT out-of-rules path keeps flagging — a file can hide an edit', () => { + // Out-of-rules but materialized: whatever put the file there, its bytes + // are invisible to `git diff` while the bit stands — the exemption is + // for paths with NOTHING on disk, only. + script.lsFiles = 'S sub/out.ts\0'; + script.sparseBool = 'true'; + script.checkRules = ''; + mkdirSync(join(repo, 'sub'), { recursive: true }); + writeFileSync(join(repo, 'sub/out.ts'), 'export const b = 1;\n'); + expect(invisibleTrackedPaths(repo)).toEqual(['sub/out.ts']); + }); +}); diff --git a/packages/cli/src/commands/review/lib/local-anchor.test.ts b/packages/cli/src/commands/review/lib/local-anchor.test.ts new file mode 100644 index 00000000000..a902dd8ef53 --- /dev/null +++ b/packages/cli/src/commands/review/lib/local-anchor.test.ts @@ -0,0 +1,197 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The pure halves of the local anchor. The cache is model-written prose-gated +// JSON, so `readLocalCache` is an untrusted-input boundary (malformed → null, +// never a throw and never a skip), and the byte slicer is pinned against the +// re-encode hazard: it must reproduce the exact bytes of the sections it keeps. + +import { describe, it, expect } from 'vitest'; +import { + UNHASHABLE, + changedSince, + readLocalCache, + stateIdOf, +} from './local-anchor.js'; +import { sliceDiffByLines } from './diff-plan.js'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +describe('stateIdOf', () => { + it('is order-independent over files and sensitive to every field', () => { + const a = stateIdOf('head1', { 'a.ts': 'h1', 'b.ts': 'h2' }); + expect(stateIdOf('head1', { 'b.ts': 'h2', 'a.ts': 'h1' })).toBe(a); + expect(stateIdOf('head2', { 'a.ts': 'h1', 'b.ts': 'h2' })).not.toBe(a); + expect(stateIdOf('head1', { 'a.ts': 'hX', 'b.ts': 'h2' })).not.toBe(a); + expect(stateIdOf(null, { 'a.ts': 'h1', 'b.ts': 'h2' })).not.toBe(a); + }); + + it('field separators prevent adjacency collisions', () => { + expect(stateIdOf('h', { ab: 'c' })).not.toBe(stateIdOf('h', { a: 'bc' })); + }); +}); + +describe('changedSince', () => { + it('reports modified, added, and removed paths — nothing else', () => { + const changed = changedSince( + { 'same.ts': 'h1', 'mod.ts': 'h2', 'gone.ts': 'h3' }, + { 'same.ts': 'h1', 'mod.ts': 'hX', 'new.ts': 'h4' }, + ); + expect(changed.sort()).toEqual(['gone.ts', 'mod.ts', 'new.ts']); + }); + + it('UNHASHABLE never compares equal — not even to itself', () => { + // "Could not capture it twice" is not "unchanged": a submodule pointer, + // a mangled filename, an unreadable file all re-enter scope every round. + expect(changedSince({ sub: UNHASHABLE }, { sub: UNHASHABLE })).toEqual([ + 'sub', + ]); + }); + + it('a legacy string identity still compares as an ordinary stable value', () => { + // Older caches may carry values no current producer emits ('absent', + // bare oids); they compare as opaque strings — equal is equal. + expect( + changedSince({ 'del.ts': 'absent' }, { 'del.ts': 'absent' }), + ).toEqual([]); + }); + + it('prototype-member names behave as ordinary keys in the comparison', () => { + expect(changedSince({ toString: 'h1' }, { toString: 'h1' })).toEqual([]); + expect(changedSince({ toString: 'h1' }, {})).toEqual(['toString']); + }); +}); + +describe('readLocalCache', () => { + const write = (content: string): string => { + const dir = mkdtempSync(join(tmpdir(), 'local-anchor-')); + const p = join(dir, 'cache.json'); + writeFileSync(p, content); + return p; + }; + + it('round-trips a valid cache, optional lastModelId included', () => { + const cache = { + v: 1, + target: 'local', + headSha: 'abc', + files: { 'a.ts': 'h1' }, + stateId: 's1', + lastModelId: 'm1', + round: 2, // model-written extras must not fail validation + }; + const p = write(JSON.stringify(cache)); + const parsed = readLocalCache(p)!; + expect(parsed.files).toEqual({ 'a.ts': 'h1' }); + expect(parsed.lastModelId).toBe('m1'); + expect(parsed.headSha).toBe('abc'); + rmSync(join(p, '..'), { recursive: true, force: true }); + }); + + it('null on every malformation — absent file, bad JSON, wrong shapes', () => { + expect(readLocalCache('/no/such/file.json')).toBeNull(); + for (const bad of [ + 'null', // JSON.parse succeeds; the object guard must still refuse + JSON.stringify({ + v: 1, + target: 't', + headSha: null, + files: {}, + stateId: 5, + }), + JSON.stringify({ + v: 1, + target: 't', + headSha: null, + files: 'x', + stateId: 's', + }), + 'not json', + JSON.stringify({ + v: 2, + target: 't', + headSha: null, + files: {}, + stateId: 's', + }), + JSON.stringify({ v: 1, headSha: null, files: {}, stateId: 's' }), + JSON.stringify({ + v: 1, + target: 't', + headSha: 5, + files: {}, + stateId: 's', + }), + JSON.stringify({ + v: 1, + target: 't', + headSha: null, + files: { a: 5 }, + stateId: 's', + }), + JSON.stringify({ + v: 1, + target: 't', + headSha: null, + files: null, + stateId: 's', + }), + // typeof [] === 'object': an array-shaped map must refuse, not pass + // with index-string keys. + JSON.stringify({ + v: 1, + target: 't', + headSha: null, + files: ['blob-a'], + stateId: 's', + }), + ]) { + const p = write(bad); + expect(readLocalCache(p)).toBeNull(); + rmSync(join(p, '..'), { recursive: true, force: true }); + } + }); +}); + +describe('sliceDiffByLines', () => { + it('keeps exact bytes of the kept ranges, in line order', () => { + const diff = Buffer.from('l1\nl2\nl3\nl4\nl5\n', 'utf8'); + const out = sliceDiffByLines(diff, [ + { startLine: 4, endLine: 5 }, + { startLine: 1, endLine: 2 }, + ]); + expect(out.toString('utf8')).toBe('l1\nl2\nl4\nl5\n'); + }); + + it('does not re-encode: non-UTF-8 bytes survive verbatim', () => { + // 0x80 alone is invalid UTF-8; a decode/re-encode would replace it. + const diff = Buffer.concat([ + Buffer.from('keep '), + Buffer.from([0x80, 0x81]), + Buffer.from('\ndrop\n'), + ]); + const out = sliceDiffByLines(diff, [{ startLine: 1, endLine: 1 }]); + expect([...out.subarray(5, 7)]).toEqual([0x80, 0x81]); + expect(out.toString('latin1')).not.toContain('drop'); + }); + + it('preserves lone \\r bytes — the CRLF-normalising idiom must never touch this path', () => { + // All three text wrappers in lib/git.ts apply `.replace(/\\r\\n/g, '\\n')`; + // a regression routing the slice through one of them rewrites every hunk + // touching a CRLF file. + const diff = Buffer.from('a\r\nb\nkeep\r\n', 'utf8'); + const out = sliceDiffByLines(diff, [{ startLine: 1, endLine: 3 }]); + expect([...out]).toEqual([...diff]); + }); + + it('a range past the last line clamps to the buffer end', () => { + const diff = Buffer.from('a\nb', 'utf8'); // no trailing newline + expect( + sliceDiffByLines(diff, [{ startLine: 2, endLine: 9 }]).toString('utf8'), + ).toBe('b'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/local-anchor.ts b/packages/cli/src/commands/review/lib/local-anchor.ts new file mode 100644 index 00000000000..5c00ecf8653 --- /dev/null +++ b/packages/cli/src/commands/review/lib/local-anchor.ts @@ -0,0 +1,818 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The local review-fix loop's anchor: content-addressed per-file state. +// +// A PR round anchors on a commit sha. A local round has none — the reviewed +// state is a dirty working tree, and the local capture path is FORBIDDEN from +// writing to the index, the worktree, or any ref (`local-diff.ts` spells out +// why). So the anchor is content: `git hash-object` — no `-w`, computes and +// writes nothing — over every file the plan covered, plus the HEAD the diff +// was based against. The next round re-hashes the same paths and compares: +// under the same HEAD and the same model, a file whose bytes are identical to +// what the previous clean round reviewed is skipped, one import hop of +// dependents re-enters (same widening, same reasons as `fetch-pr --since`), and the +// rest is the delta. +// +// HEAD equality is a hard gate, not a convenience. The captured diff is +// HEAD-vs-worktree: if HEAD moved between rounds, the same worktree bytes +// describe a DIFFERENT change under review (a reset exposes commits the last +// round never saw), so content equality alone certifies nothing. A moved HEAD +// degrades to the full capture, with the reason said out loud. + +import { createHash } from 'node:crypto'; +import { lstatSync, readFileSync, readlinkSync } from 'node:fs'; +import { join } from 'node:path'; +import { gitOpt, gitRaw, gitWithInput, gitWithInputRaw } from './git.js'; +import { LITERAL_PATHSPECS } from './diff-flags.js'; + +/** + * Per-file identity for a path whose state CANNOT be captured: a directory + * (an embedded repo / submodule gitlink above all), a FIFO, an unreadable + * file, a plan path git C-quoted into something no stat can find. Never + * compares equal — not even to itself — so such a path re-enters the scope + * every round. Over-review is the affordable direction; the previous cut + * mapped all of these to `absent`, where a submodule pointer change compared + * "unchanged" forever and silently left incremental scope. + */ +export const UNHASHABLE = 'unhashable'; + +export interface LocalCacheCandidate { + v: 1; + target: string; + /** null on an unborn HEAD (repo with no commits). */ + headSha: string | null; + /** path → `:` identity, for every file the plan covered. */ + files: Record; + /** Content-addressed id of the whole reviewed state, for display and logs. */ + stateId: string; + /** + * The repo-relative path `target` was flattened from, on a file review. + * + * `safeTarget` is not injective — `src/foo.ts` and `src_foo.ts` flatten to + * one token — and the cache is keyed by the token, so the token alone + * cannot tell two files apart. Absent on a plain `local` round, which has + * no single source. + */ + source?: string; + /** + * The identity reviewing this round, provider-qualified, as the runtime + * published it — written by the CAPTURE, not merged in afterwards. + * + * Step 8 used to add it from `{{model}}`, which interpolates the BARE model + * id: two provider configurations exposing one model name recorded the same + * token and passed each other's same-model gate, which is the contract's + * whole point. Empty when the runtime published no identity, and the gate + * reads empty as a mismatch — an unverifiable contract is a failed one. + */ + lastModelId: string; + /** + * Did the capture that wrote this include untracked files? + * + * A later round that sees LESS cannot certify this state: absent untracked + * paths would read as vanished rather than out of scope. + */ + untracked?: boolean; +} + +/** + * The cache Step 8 writes from a candidate — the candidate's fields plus the + * model-written ledger (`round`, `findings`, …). Only the fields the scoping + * decision reads are typed; the rest ride as data. `lastModelId` is inherited + * from the candidate and optional here only because a cache written before it + * moved into the capture may not carry one — which the gate treats as a + * mismatch, so such a cache costs a full round and never a wrong scope. + */ +export interface LocalReviewCache + extends Omit { + lastModelId?: string; +} + +/** + * The per-file identity of `paths`' current worktree state, batched. + * + * An identity is `:` — `git hash-object` computes the blob id git + * WOULD store (content-addressed, indifferent to mtime and the index), and + * the mode prefix carries what content alone cannot: an exec-bit flip or a + * file↔symlink typechange is its own diff lines, so identical bytes under a + * different mode are NOT an identical change. Symlinks hash their link text + * at 120000, exactly what `git diff` renders. Anything that cannot be + * captured faithfully is `UNHASHABLE`, which never compares equal. + */ +export function hashWorktreeFiles( + repoRoot: string, + paths: readonly string[], +): Record { + // Dedup at the module boundary: `check-attr` answers once per input + // OCCURRENCE, so a caller that lists a path twice — two open Criticals + // cite the same file in an ordinary ledger — appends the rendering suffix + // once per listing and forges an identity that never matches the cache's + // single-suffix one. The blocker date then reads that file as "moved" + // under every possible tree state, for ever. + paths = [...new Set(paths)]; + // Null prototype: a file legally named `__proto__` must store and read as + // an ordinary own key. On a plain object the assignment hits the inherited + // setter (a silent no-op) and the read returns Object.prototype — the file + // could never enter a delta in any round. + const out: Record = Object.create(null) as Record< + string, + string + >; + const hashable: string[] = []; + const modes: Record = Object.create(null) as Record< + string, + string + >; + for (const p of paths) { + // `lstat`, not `stat`: a symlink's identity is its LINK TEXT at mode + // 120000 — exactly what `git diff` renders — not the target's bytes. + // Following the link let a retargeted symlink whose new target happened + // to hold equal content compare "unchanged". + // A path carrying U+FFFD is a decode, not a name. The capture pins + // `core.quotePath=false` and decodes with `toString('utf8')`, so every + // invalid byte folds to the replacement character — and when a file + // LITERALLY named with U+FFFD exists beside such a path, the two fold to + // one key: `lstat` succeeds on the real one, the invalid-byte sibling + // inherits its identity, is never hashed, and its changes compare + // unchanged for ever. That is the fail-open this identity exists to + // close, and the `lstat` guard below cannot see it because the stat + // SUCCEEDS. + // + // Over-review is the affordable direction, and a filename holding a real + // U+FFFD is rare enough that paying for it every round costs nothing + // measurable. + if (p.includes('\ufffd')) { + out[p] = UNHASHABLE; + continue; + } + let st; + try { + st = lstatSync(join(repoRoot, p)); + } catch { + // No stat-able file. A genuine deletion lands here — but so does a + // plan path git C-quoted out of an invalid-UTF-8 filename, whose REAL + // file exists and changes. The two are indistinguishable at this + // layer, and treating both as a stable "absent" identity let the + // second kind compare unchanged forever. UNHASHABLE re-reviews both + // every round; a deletion's diff section is small, and over-review is + // the affordable direction. + out[p] = UNHASHABLE; + continue; + } + if (st.isSymbolicLink()) { + try { + // RAW BYTES: git identities and diffs use the link text's bytes, and + // a default-encoding readlink round-trips through a JS string where + // invalid UTF-8 collapses to U+FFFD — two distinct targets could + // then share one identity and a retarget compare "unchanged". + const target = readlinkSync(join(repoRoot, p), { encoding: 'buffer' }); + const oid = gitWithInput(target, [ + '-C', + repoRoot, + 'hash-object', + '--stdin', + ]); + out[p] = `120000:${oid}`; + } catch { + out[p] = UNHASHABLE; + } + } else if (st.isFile()) { + // The mode is part of the identity: `git diff` reports an exec-bit + // flip as its own lines, so identical bytes under a flipped bit are + // NOT an identical change. USER bit only (S_IXUSR) — git canonicalizes + // regular-file modes on that bit alone, and masking all three classes + // held the identity still across a chmod git visibly reports (0755 → + // 0655 prints old/new mode lines while g+other bits kept 0o111 truthy). + modes[p] = (st.mode & 0o100) !== 0 ? '100755' : '100644'; + hashable.push(p); + } else if (st.isDirectory()) { + // A directory in the population is normally a submodule GITLINK (the + // pinned diff flags keep them visible; a plain directory subject is + // excluded upstream). Recorded UNHASHABLE it never equals itself, so + // a dirty pointer wedged the unchanged-since stop for the change + // set's whole lifetime (R22-1) — yet git measures this identity for + // itself. A gitlink whose HEAD is readable AND whose content is clean + // records `160000:`, the exact identity `revisionIdentities` + // reads out of `ls-tree`; a content-dirty or unreadable submodule + // stays UNHASHABLE — the pointer oid says nothing about internal + // edits, and unmeasurable is uncertifiable. + out[p] = gitlinkIdentity(repoRoot, p); + } else { + // FIFOs, sockets, and any other shape: not capturable. + out[p] = UNHASHABLE; + } + } + const BATCH = 200; + for (let i = 0; i < hashable.length; i += BATCH) { + const batch = hashable.slice(i, i + BATCH); + const res = gitOpt('-C', repoRoot, 'hash-object', '--', ...batch); + const lines = res === null ? null : res.split('\n'); + if (lines !== null && lines.length === batch.length) { + batch.forEach((p, j) => (out[p] = `${modes[p]}:${lines[j]}`)); + continue; + } + // The batch failed as a unit (one unreadable file fails them all) — + // re-try one by one so a single pathological file costs itself, not + // its 199 neighbours. + for (const p of batch) { + const oid = gitOpt('-C', repoRoot, 'hash-object', '--', p); + out[p] = oid === null ? UNHASHABLE : `${modes[p]}:${oid}`; + } + } + // …and how each one RENDERS, which mode and blob cannot say. `binary`, + // `-diff` and `text` turn a section from readable hunks into "Binary files + // … differ" while every byte and mode stands still, so a round that read + // only the marker and a round where the attribute is gone compared equal + // and the newly-readable section was sliced out of scope. + // + // Per file rather than as one digest beside the map, and asked of git + // rather than re-derived. A digest over the attribute SOURCES diverged from + // git's own resolution in every corner anyone looked at — a relative + // `core.attributesFile` resolves against the repo root, not the process + // cwd; a linked worktree honours the COMMONDIR's `info/attributes`; + // `diff..binary` flips the rendering from config, which no + // attributes file mentions — and each divergence left the digest equal + // while the rendering moved. It also could not survive a changing path set: + // one new file changed the digest and refused the anchor. Folded in here, + // the existing per-path comparison handles all of it. + const attrs = renderingAttributes(repoRoot, hashable); + for (const p of hashable) { + if (out[p] === UNHASHABLE) continue; + const a = attrs[p]; + // A path git could not answer for takes UNHASHABLE, not a placeholder + // component: a placeholder equals itself, so two rounds that both failed + // to read the attributes would compare "unchanged" and certify a + // rendering neither had seen — the same fail-open this whole field + // exists to close. UNHASHABLE re-reviews it instead. + // + // …and when the answer ITSELF is UNHASHABLE — a driver name that did not + // survive the decode — the WHOLE identity takes it: appending the slot + // composed `100644::unhashable`, which equals itself across + // rounds, so a rendering flip moved nothing and the section was sliced + // out of scope carrying the previous verdict. What cannot be named + // faithfully cannot be certified — the module's own standard, applied + // to the identity and not just the slot. + out[p] = + a === undefined || a === UNHASHABLE ? UNHASHABLE : `${out[p]}:${a}`; + } + return out; +} + +/** + * The effective rendering attributes of each path, as GIT reports them. + * + * `git check-attr` answers under every source git honours, in git's own + * precedence, with git's own path resolution — `.gitattributes` at any level, + * `.git/info/attributes`, the COMMONDIR's copy in a linked worktree, + * `core.attributesFile` resolved the way git resolves it, and the config-side + * diff drivers a hand-derivation cannot see at all. + * + * A path git could not answer for gets `'unknown'` from the caller, which + * never equals a real answer — an unavailable probe must not certify the + * state it could not read. + */ +function renderingAttributes( + repoRoot: string, + paths: readonly string[], +): Record { + const out: Record = Object.create(null) as Record< + string, + string + >; + if (paths.length === 0) return out; + // `diff` alone would miss the two that set it indirectly: `binary` implies + // `-diff -text`, and `text` decides eol normalisation, which changes the + // bytes a hunk shows. + const ATTRS = ['diff', 'binary', 'text']; + let raw: string; + try { + // `-z` on both sides: NUL-delimited input and output, so a path holding a + // newline or a colon cannot forge a record — the same reason every + // listing in this file is byte-faithful. + // + // …and RAW, because the convenience wrapper is not. Its `.trim()` eats a + // leading whitespace byte — legal in a path on Linux and macOS — so the + // first record's echoed key stops matching the path that was asked + // about, and every record shifts onto a phantom key: the path gets a + // MALFORMED identity instead of an honest `UNHASHABLE`. That fails OPEN + // in one direction, because the stolen record is the `diff` attribute, so + // a `diff=` path never folds its driver's `binary` setting in and + // the config-side binary↔text flip this whole function exists to track + // goes invisible. The `\r\n` → `\n` rewrite can collide one record's key + // with a sibling's the same way. + raw = gitWithInputRaw(Buffer.from(`${paths.join('\0')}\0`), [ + '-C', + repoRoot === '' ? '.' : repoRoot, + 'check-attr', + '--stdin', + '-z', + ...ATTRS, + ]); + } catch { + return out; // every path falls back to `'unknown'` + } + // Records are ` NUL NUL NUL`, repeated. + const drivers = new Set(); + // Structured path → driver, recorded while the records are parsed, because + // the comma-joined serialization cannot be re-parsed on the way back: a + // driver NAME may contain a comma (`*.bin diff=a,b` is a legal gitattributes + // line), and a `split(',')` match can never equal such a value — the fold + // below would silently drop its `binary` flag from the identity, leaving + // the identity still across a flip that changes the rendering. + const diffDriverByPath = Object.create(null) as Record; + /** Paths whose driver name did not survive the decode — see below. */ + const undecodableDriver = new Set(); + /** Paths whose `diff` answer cannot name a rendering state — see below. */ + const ambiguousDiff = new Set(); + const f = raw.split('\0'); + for (let i = 0; i + 2 < f.length; i += 3) { + const [path, attr, value] = [f[i], f[i + 1], f[i + 2]]; + if (path === undefined || attr === undefined || value === undefined) break; + if (attr === 'diff') { + // EVERY answer is a driver candidate: `set`, `unset` and `unspecified` + // are legal driver NAMES too (`data.bin diff=set`), answered by + // `check-attr` byte-identically to the like-spelled attribute states — + // **and so is the EMPTY one.** `*.dat diff=` is a legal attributes + // line, `check-attr --stdin -z` answers it with an empty value, and + // `git config diff..binary true` flips that section between readable + // hunks and "Binary files differ" with the mode and the blob standing + // still (verified against git 2.47.3). Excluding it was the same + // family's last entrance, left open by the fix that closed the others + // while its own comment claimed every answer was covered. + // Excluding those spellings left such a driver's `diff..binary` + // out of the fold — `git diff` flips the section between readable + // hunks and "Binary files differ" while the identity stands still. + // The fold below still asks the CONFIG first, so a plain state answer + // with no driver so named costs one probe and folds nothing. + if (value.includes('\ufffd')) { + // A driver NAME is bytes, and this stream was decoded: an invalid + // byte folded to U+FFFD, so the config probe would ask for + // `diff..binary` (re-encoded as EF BF BD) and never match the + // raw-byte key git itself matches. Nothing would fold, and flipping + // that config would change the rendering with every identity + // component standing still. The same discipline this module applies + // to a decoded PATH: what cannot be named faithfully cannot be + // certified. + // Recorded, not written here: this loop appends one record at a + // time, so writing UNHASHABLE now would have the path's later + // `binary`/`text` records append onto it (`unhashable,binary=…`). + undecodableDriver.add(path); + // …and it is NOT recorded as a driver: probing + // `diff..binary` spawns a `git config` that cannot match, + // and leaving the path in the map would let the fold below append + // onto the UNHASHABLE this earns it. + continue; + } + if (value === 'set' || value === 'unset') { + // `check-attr` answers an attribute STATE and a VALUE assignment that + // spells a state name byte-identically, and `git diff` renders them + // differently: `*.dat -diff` and `*.dat diff=unset` both answer + // `diff: unset`, but the state renders "Binary files … differ" while + // the driver named `unset` renders readable hunks (probed, git + // 2.39.5 — no config involved at all). The `set` pair is the same + // conflation one config away: plain `diff` and `diff=set` both answer + // `set`, and a `diff.set.binary=true` flips ONLY the driver's + // rendering — the config fold below appends the setting to BOTH + // identities (each answers `set`, each probes the same config key), + // so it cannot split the pair. What this stream cannot name + // faithfully cannot be certified — the whole identity takes + // UNHASHABLE, the module's standard for a rendering it cannot + // capture, and the path re-reviews every round instead. + // `unspecified` stays foldable on purpose: it is the answer for every + // path no attributes rule mentions, and an UNHASHABLE there would + // re-review every unattributed file in the tree every round — the + // anchor's whole payoff, spent on a driver that could only be named + // `unspecified` on purpose. + // Not recorded as a driver either, the undecodable case's reason: + // the fold must not append onto the UNHASHABLE this earns it. + ambiguousDiff.add(path); + continue; + } + drivers.add(value); + diffDriverByPath[path] = value; + } + out[path] = + out[path] === undefined + ? `${attr}=${value}` + : `${out[path]},${attr}=${value}`; + } + // Applied after the stream, so no later record for the same path can + // append onto it. + for (const path of undecodableDriver) out[path] = UNHASHABLE; + for (const path of ambiguousDiff) out[path] = UNHASHABLE; + // `diff=` names a driver whose behaviour lives in git CONFIG, not + // in any attributes file — and `diff..binary` flips a section + // between readable hunks and "Binary files … differ" with the attribute + // value, the mode and the blob all standing still. `check-attr` reports the + // NAME; the config is a second question, and only for the paths that name + // one. (`textconv` is the driver's other rendering knob and is neutralised + // by the pinned `--no-textconv`; unpinning that flag means adding it here.) + for (const driver of drivers) { + const binary = gitOpt( + '-C', + repoRoot === '' ? '.' : repoRoot, + 'config', + '--get', + `diff.${driver}.binary`, + ); + if (binary === null) continue; + if (driver === 'unspecified') { + // `check-attr` answers `diff=unspecified` byte-identically for the + // no-rule state AND an explicit `diff=unspecified` value, and the two + // render differently exactly when THIS config key exists (the driver + // named `unspecified` goes binary; the no-rule state stays readable — + // probed, git 2.47.3). The fold cannot split what the stream spells + // alike, so with the config present the whole dimension is ambiguous + // for every path that answered it: UNHASHABLE, the module's standard + // for a rendering it cannot certify. Without the config — every + // ordinary repo — nothing changes and `unspecified` stays foldable, + // for the reason the state-vs-value gate above records. + for (const path of Object.keys(out)) { + if (diffDriverByPath[path] === driver) out[path] = UNHASHABLE; + } + continue; + } + for (const [path, attrs] of Object.entries(out)) { + if (diffDriverByPath[path] === driver) { + out[path] = `${attrs},${driver}.binary=${binary}`; + } + } + } + return out; +} + +/** + * Per-file identities at a REVISION, in the exact format `hashWorktreeFiles` + * computes for the worktree — so a file a cached round reviewed WITHOUT + * hashing it can still be dated. The live shape is the no-diff whole-file + * review: the capture hashed no plan paths and Step 8 promoted an empty + * files map, but a no-diff capture means the bytes the round read WERE the + * cached HEAD's own bytes for the file. + * + * A path the tree does not name comes back absent, never an identity: the + * caller's `movedSince` comparison reads absent-on-one-side as a move. + * Gitlinks, trees, and names that did not survive the decode take + * UNHASHABLE exactly as the worktree hasher does, and the same rendering + * suffix joins the same way — byte equality under an attribute flip is NOT + * an identical change, so the two formats must agree on it too. An + * unreadable revision dates nothing. + */ +export function revisionIdentities( + repoRoot: string, + headSha: string | null, + paths: readonly string[], +): Record { + // Null prototype: the `__proto__`-as-a-filename discipline of + // `hashWorktreeFiles` — a revision can name one too. + const out: Record = Object.create(null) as Record< + string, + string + >; + if (headSha === null || paths.length === 0) return out; + let raw: Buffer; + try { + // LITERAL_PATHSPECS like every sibling pathspec-taking call: the paths + // come from the model-written ledger, an untrusted-input boundary, and a + // name beginning `:(` is pathspec magic — one such path fatals the WHOLE + // batch, and the catch would read every sibling in it as undatable too. + raw = gitRaw( + '-C', + repoRoot, + LITERAL_PATHSPECS, + 'ls-tree', + '-z', + headSha, + '--', + ...paths, + ); + } catch { + return out; + } + const regular: string[] = []; + for (const record of raw.toString('utf8').split('\0')) { + if (record === '') continue; + // ` SP SP TAB `: the path is everything after + // the FIRST tab, so a name holding further tabs survives; `-z` disables + // C-quoting, so the NUL-separated records carry raw bytes. + const tab = record.indexOf('\t'); + if (tab < 0) continue; + const path = record.slice(tab + 1); + if (path === '') continue; + if (path.includes('\ufffd')) { + out[path] = UNHASHABLE; + continue; + } + const meta = record.slice(0, tab).split(' ') as Array; + const [mode, type, oid] = meta; + if (mode === undefined || type === undefined || oid === undefined) { + out[path] = UNHASHABLE; + continue; + } + if (type === 'blob' && (mode === '100644' || mode === '100755')) { + out[path] = `${mode}:${oid}`; + regular.push(path); + } else if (type === 'blob' && mode === '120000') { + // A symlink's identity is its stored blob — the link text's bytes — + // with no rendering suffix: the worktree hasher's exact shape. + out[path] = `120000:${oid}`; + } else if (type === 'commit') { + // A gitlink's identity at a revision is the recorded pointer — the + // same `160000:` shape the worktree hasher answers for a clean, + // readable submodule, so the two sides compare instead of holding an + // UNHASHABLE that never equals itself (R22-1). + out[path] = `160000:${oid}`; + } else { + // Trees and any other shape: not capturable. + out[path] = UNHASHABLE; + } + } + const attrs = renderingAttributes(repoRoot, regular); + for (const p of regular) { + if (out[p] === UNHASHABLE) continue; + const a = attrs[p]; + out[p] = + a === undefined || a === UNHASHABLE ? UNHASHABLE : `${out[p]}:${a}`; + } + return out; +} + +/** + * A submodule gitlink's worktree identity: `160000:` when the pointer + * is measurable and the submodule's content is CLEAN, UNHASHABLE otherwise. + * + * The oid alone would compare equal across an internal edit (`git diff` + * renders that as `-dirty` — a change this identity must not hold + * still through), so cleanliness is part of measurability: a dirty + * submodule re-reviews every round, the affordable direction, exactly as + * an unreadable one does. + */ +function gitlinkIdentity(repoRoot: string, path: string): string { + const sub = join(repoRoot, path); + const oid = gitOpt('-C', sub, 'rev-parse', 'HEAD'); + if (oid === null || oid === '') return UNHASHABLE; + try { + const status = gitRaw('-C', sub, 'status', '--porcelain'); + if (status.length !== 0) return UNHASHABLE; + } catch { + return UNHASHABLE; + } + // `status --porcelain` HONOURS visibility bits set inside the submodule — + // an assume-unchanged internal edit reads clean (probed, git 2.43) — and + // the top-level oracle enumerates only the superproject's index, so + // cleanliness must ask the submodule's own bits too or the identity holds + // still over bytes no round can see (the fix-induced half of R22-1). The + // same oracle, one level down; a nested submodule's own interior is that + // submodule's dirt in THIS status once its pointer moves, and its bits + // one level deeper repeat this check when its gitlink is measured. + const bits = invisibleTrackedPaths(sub); + return bits !== null && bits.length === 0 ? `160000:${oid}` : UNHASHABLE; +} + +/** + * The tracked paths `git diff` is BLIND to — the ones carrying an + * `--assume-unchanged` bit (lowercase tags) or `--skip-worktree` (`S`) in + * `git ls-files -v` — or null when the enumeration itself failed. + * + * A decided stop is a claim that nothing in the tree needs review, and + * `git diff HEAD` honours those bits: an edit on a marked path shows no + * section, no hash moves, and every comparison a stop keys on stands still + * while the bytes were read by no round. The defence therefore asks for the + * BITS themselves rather than any edit — whether the hidden bytes diverge + * is exactly what the capture cannot tell, and over-review is the + * affordable direction. The same oracle, fail-closed the same way, guards + * the PR flow's clean-tree claim (`worktree.ts`). + */ +export function invisibleTrackedPaths(repoRoot: string): string[] | null { + let raw: Buffer; + try { + raw = gitRaw('-C', repoRoot, 'ls-files', '-v', '-z'); + } catch { + // Unmeasured is uncertifiable, exactly like a listed bit. + return null; + } + const tagged: string[] = []; + for (const rec of raw.toString('utf8').split('\0')) { + // ` ` records: lowercase tags are the assume-unchanged + // family (and the COMBINED bits render lowercase `s`), `S` is + // skip-worktree alone; every other tag leaves the path visible. + if (/^[a-zS]/.test(rec)) tagged.push(rec.slice(2)); + } + if (tagged.length === 0) return tagged; + // Sparse-checkout manages visibility bits itself: every out-of-cone + // tracked path is S-tagged BY DESIGN and absent from the worktree (and an + // out-of-cone path that ALSO carries assume-unchanged renders `s`), so + // counting them wedged every sparse repo for ever — candidate withheld, + // all three decided stops failing their conjuncts on a clean materialized + // tree. An absent out-of-cone path holds no file to hide an edit in. + // + // The exemption asks GIT which paths its rules cover, at every step where + // a hand re-derivation went wrong in a review round: + // - the flag reads `--worktree --type=bool`, because `--get` alone echoes + // the stored spelling (`yes`/`on`/`1` all failed a `=== 'true'`) and a + // GLOBAL `core.sparseCheckout = true` inherited through HOME must not + // turn the exemption on for a repo that is not sparse (R18-2); + // - membership comes from `git sparse-checkout check-rules`, not from the + // bit or the tag's case: git versions differ on whether a MANUAL + // `--skip-worktree` survives inside a cone (2.43 keeps it, 2.47 + // re-clears it), so "absent + S" is not "out of cone" (R18-3), and the + // combined-bit `s` spelling is (R18-4); + // - only an ABSENT path can be exempt — an in-rules path absent with a + // bit set is a deletion the bit hides, and a PRESENT out-of-rules path + // can hold a hidden edit, so both keep flagging. + // A failed check-rules (an older git without the subcommand) exempts + // nothing: fail closed, at the cost of the pre-exemption wedge on that + // git, never a certification. + const sparse = + gitOpt( + '-C', + repoRoot, + 'config', + '--worktree', + '--type=bool', + '--get', + 'core.sparseCheckout', + ) === 'true'; + if (!sparse) return tagged; + const absent = new Set( + tagged.filter((p) => { + // A name that did not survive the decode cannot be measured OR fed to + // check-rules faithfully (the re-encoded U+FFFD spelling matches + // nothing git knows) — the same discipline `hashWorktreeFiles` and + // `revisionIdentities` apply to undecodable paths. Never exempt it: + // it stays flagged (R19-1). + if (p.includes('\ufffd')) return false; + try { + lstatSync(join(repoRoot, p)); + return false; + } catch (err) { + // Only ENOENT proves absence: an EACCES/ENOTDIR/ELOOP failure on a + // PRESENT flagged path folded into "absent" and the exemption then + // certified bytes `git diff` was blind to (R19-2). Unmeasurable is + // uncertifiable — it stays flagged. + return (err as NodeJS.ErrnoException).code === 'ENOENT'; + } + }), + ); + if (absent.size === 0) return tagged; + let inRules: Set; + try { + const out = gitWithInputRaw( + Buffer.from([...absent].map((p) => `${p}\0`).join(''), 'utf8'), + ['-C', repoRoot, 'sparse-checkout', 'check-rules', '-z'], + ); + inRules = new Set(out.split('\0').filter((p) => p !== '')); + } catch { + return tagged; + } + return tagged.filter((p) => !(absent.has(p) && !inRules.has(p))); +} + +/** One id for the whole state: order-independent, HEAD included. */ +export function stateIdOf( + headSha: string | null, + files: Record, +): string { + const h = createHash('sha256'); + h.update(headSha ?? 'unborn'); + for (const path of Object.keys(files).sort()) { + // NUL-separated fields: no path or blob id contains one, so adjacent + // entries cannot collide by concatenation. + h.update(`\0${path}\0${files[path]}`); + } + return h.digest('hex'); +} + +/** + * Parse a local review cache, fail-quiet. The file is model-written under + * Step 8's prose rules, so every field is re-validated: a malformed cache + * degrades to "no anchor" — a full capture — never to a throw and never to a + * skip. + */ +export function readLocalCache(path: string): LocalReviewCache | null { + let raw: unknown; + try { + raw = JSON.parse(readFileSync(path, 'utf8')); + } catch { + return null; + } + const c = raw as { + v?: unknown; + target?: unknown; + source?: unknown; + untracked?: unknown; + headSha?: unknown; + files?: unknown; + stateId?: unknown; + lastModelId?: unknown; + }; + if ( + !c || + c.v !== 1 || + typeof c.target !== 'string' || + (c.headSha !== null && typeof c.headSha !== 'string') || + typeof c.stateId !== 'string' || + typeof c.files !== 'object' || + c.files === null || + // `typeof [] === 'object'`: an array-shaped files map would pass with + // index-string keys and silently mark every real path changed instead + // of taking the loud refusal this validator promises. + Array.isArray(c.files) + ) { + return null; + } + // Null prototype for the same `__proto__`-as-a-filename reason as + // `hashWorktreeFiles` — JSON.parse already made the keys own properties; + // keep them own properties here too. + const files: Record = Object.create(null) as Record< + string, + string + >; + for (const [k, v] of Object.entries(c.files as Record)) { + if (typeof v !== 'string') return null; + files[k] = v; + } + return { + v: 1, + target: c.target, + headSha: c.headSha as string | null, + files, + stateId: c.stateId, + ...(typeof c.lastModelId === 'string' + ? { lastModelId: c.lastModelId } + : {}), + // Carried through, because the target token it sits beside is not + // injective and the gate compares this instead. Absent stays absent: a + // cache from before the field reads as a mismatch against a file review, + // which costs one full round. + ...(typeof c.source === 'string' ? { source: c.source } : {}), + ...(typeof c.untracked === 'boolean' ? { untracked: c.untracked } : {}), + }; +} + +/** + * The paths whose content differs between the cached state and now — added, + * removed, and modified alike. Symmetric difference over the two key sets + * with value comparison on the intersection. + * + * Callers that need to know whether anything genuinely MOVED must use + * `movedSince` instead: a path this returns may be here only because it could + * not be hashed, which is a reason to review it every round and not a reason + * to believe the tree changed. + */ +export function changedSince( + cached: Record, + current: Record, +): string[] { + // `Object.hasOwn`, never `in` or bare reads: both maps can be JSON-parsed + // or model-written, and a path named after any Object.prototype member + // (`toString`, `constructor`) must behave as an ordinary key. + const eq = (a: string | undefined, b: string | undefined): boolean => + a !== undefined && + a === b && + // UNHASHABLE never equals — not even itself. It marks state that could + // not be captured, and "could not capture it twice" is not "unchanged". + a !== UNHASHABLE; + const out: string[] = []; + for (const path of Object.keys(current)) { + const cachedId = Object.hasOwn(cached, path) ? cached[path] : undefined; + if (!eq(cachedId, current[path])) out.push(path); + } + for (const path of Object.keys(cached)) { + if (!Object.hasOwn(current, path)) out.push(path); + } + return out; +} + +/** + * The paths that genuinely MOVED — `changedSince` minus the ones that are in + * it only because neither side could be hashed. + * + * `UNHASHABLE` never equals itself, deliberately: state that could not be + * captured must be re-reviewed every round rather than certified. But that + * makes it permanently present in `changedSince`, and a round that keyed its + * "nothing changed, stop" decision on that list could never reach it. Any + * pending deletion of a tracked file hashes this way, so the local + * review-fix loop could not converge for a change set containing one: round + * N+1 re-hashed the same section to `UNHASHABLE`, announced "1 changed + * file(s)" over a byte-identical diff, re-sliced it into scope, and re-armed + * itself for round N+2 — until HEAD moved. + * + * Both facts are needed and they are not the same fact. The scope keeps using + * `changedSince` (over-reviewing an unhashable path is the safe direction); + * the stop, and anything a human reads as "what changed", uses this. + */ +export function movedSince( + cached: Record, + current: Record, +): string[] { + return changedSince(cached, current).filter((path) => { + const before = Object.hasOwn(cached, path) ? cached[path] : undefined; + const after = Object.hasOwn(current, path) ? current[path] : undefined; + // Unhashable on BOTH sides is "still unreadable", not "changed". Either + // side merely absent IS a move — the path entered or left the capture. + return !(before === UNHASHABLE && after === UNHASHABLE); + }); +} diff --git a/packages/cli/src/commands/review/lib/local-diff.integration.test.ts b/packages/cli/src/commands/review/lib/local-diff.integration.test.ts index 465ee3bc656..7378aaec8c7 100644 --- a/packages/cli/src/commands/review/lib/local-diff.integration.test.ts +++ b/packages/cli/src/commands/review/lib/local-diff.integration.test.ts @@ -256,6 +256,153 @@ describe('captureLocalDiff — untracked files', () => { expect(res.text).not.toContain('.qwen/tmp'); }); + it('excludes plumbing a round in ANOTHER directory wrote', () => { + // The three `paths.ts` constants are cwd-relative for EVERY invocation, + // so a round run from `sub/` writes `sub/.qwen/…`. A filter built from + // THIS invocation's cwd matches none of it, and a repo that does not + // ignore `.qwen` then hands the next root-invoked round the previous + // round's cache, reports and args record as the user's untracked work — + // and the cache changes every round by construction, so an incremental + // round could never again report "no changes". + write('sub/.qwen/review-cache/local.json', '{"lastCommitSha":"abc"}\n'); + write('sub/.qwen/reviews/2026-01-01-local.md', '# round\n'); + write('sub/.qwen/tmp/qwen-review-local-cache-candidate.json', '{}\n'); + write('sub/real.ts', 'export const r = 1;\n'); + + const res = capture(); + expect(res.untracked).toEqual(['sub/real.ts']); + expect(res.text).not.toContain('.qwen'); + }); + + it('keeps a directory that merely LOOKS like plumbing', () => { + // Segment-exact: `.qwen-notes` and `tmp` are the user's. + write('.qwen-notes/tmp/a.md', 'mine\n'); + write('tmp/reviews/b.md', 'also mine\n'); + + const res = capture(); + expect(res.untracked.sort()).toEqual([ + '.qwen-notes/tmp/a.md', + 'tmp/reviews/b.md', + ]); + }); + + it('reviews a plumbing path the user NAMED, instead of dropping it mutely', () => { + // `/review .qwen/reviews/round-notes.md` is a deliberate request. Filtered + // out, the round reported "the working tree is clean — 0 chunks" over the + // one file it was asked about, with no `Not reviewed` record: mute, which + // the SkippedFile contract forbids. + write('.qwen/reviews/round-notes.md', '# notes\n'); + write('.qwen/reviews/other.md', '# not asked for\n'); + + const res = capture({ file: '.qwen/reviews/round-notes.md' }); + expect(res.untracked).toEqual(['.qwen/reviews/round-notes.md']); + expect(res.text).toContain('round-notes.md'); + // …and only the named one: its siblings are still plumbing. + expect(res.text).not.toContain('other.md'); + }); + + it('reviews the children of a NAMED plumbing directory target', () => { + // A directory is a legal file target — a tab-completed `src/` classifies + // as one — and `.qwen/reviews/` is the shape the named-path exemption was + // designed for. But the exemption matched `p === pathspec`, and a child + // path never equals its parent directory: every child was filtered out + // with no skipped record, and the round reviewed none of the content the + // user named — the same mute drop the file-shaped exemption closed. + write('.qwen/reviews/one.md', '# one\n'); + write('.qwen/reviews/two.md', '# two\n'); + + const res = capture({ file: '.qwen/reviews/' }); + expect(res.untracked.sort()).toEqual([ + '.qwen/reviews/one.md', + '.qwen/reviews/two.md', + ]); + expect(res.skipped).toEqual([]); + }); + + it('reviews NAMED plumbing even when ignore rules cover it', () => { + // The common configuration is `.qwen/` in `.gitignore` — this repo's own + // shape — and `ls-files --others --exclude-standard` applies exclusion to + // an explicitly pathspec-NAMED file too, so the named plumbing never + // reached the filter at all: empty candidates, empty diff, no skipped + // record, and the round reported clean over the one file it was asked + // about. A deliberately named path wins over ignore rules. + write('.gitignore', '.qwen/*\n'); + git('add', '.gitignore'); + git('commit', '-q', '-m', 'ignore'); + write('.qwen/reviews/round-notes.md', '# notes\n'); + write('.qwen/reviews/other.md', '# sibling\n'); + + const res = capture({ file: '.qwen/reviews/round-notes.md' }); + expect(res.untracked).toEqual(['.qwen/reviews/round-notes.md']); + expect(res.text).toContain('round-notes.md'); + // …and only the named one: its siblings are still plumbing. + expect(res.text).not.toContain('other.md'); + + // The directory shape under the same ignore rules. + const dir = capture({ file: '.qwen/reviews/' }); + expect(dir.untracked.sort()).toEqual([ + '.qwen/reviews/other.md', + '.qwen/reviews/round-notes.md', + ]); + }); + + it('drops a TRACKED plumbing section, which ignore rules never reach', () => { + // Ignore rules do not apply to tracked files, so a repo that once + // committed its `.qwen` plumbing gets the cache back in `git diff HEAD` + // every round — and Step 8 rewrites that cache after every clean round, + // so the section is there by construction. The round reviews its own + // ledger JSON and `changedSince` never empties. + write('.qwen/review-cache/local.json', '{"lastCommitSha":"old"}\n'); + git('add', '-f', '.qwen/review-cache/local.json'); + git('commit', '-q', '--no-verify', '-m', 'committed plumbing'); + write('.qwen/review-cache/local.json', '{"lastCommitSha":"new"}\n'); + write('tracked.ts', 'export const a = 2;\n'); + + const res = capture(); + expect(res.text).not.toContain('.qwen/review-cache'); + // …while the real tracked change survives. + expect(res.text).toContain('tracked.ts'); + }); + + it('drops TRACKED plumbing under a NAMED DIRECTORY target, keeping its other content', () => { + // "Whatever came back is what the user asked for" is only true for + // FILE-shaped pathspecs. A directory target kept every tracked plumbing + // section beneath it while the untracked half dropped plumbing + // descendants — the two halves of one capture disagreed, and a repo that + // committed its own `.qwen` plumbing reviewed its ledger JSON as user + // content, whose every-round churn kept `changedSince` from ever + // emptying. + write('sub/.qwen/review-cache/local.json', '{"round":1}\n'); + write('sub/code.ts', 'export const c = 1;\n'); + git('add', '-f', 'sub/.qwen/review-cache/local.json'); + git('add', 'sub/code.ts'); + git('commit', '-q', '--no-verify', '-m', 'committed plumbing'); + write('sub/.qwen/review-cache/local.json', '{"round":2}\n'); + write('sub/code.ts', 'export const c = 2;\n'); + write('sub/note.md', 'user content\n'); + write('sub/.qwen/tmp/qwen-review-local-plan.json', '{}\n'); + + const res = capture({ file: 'sub/' }); + expect(res.text).toContain('sub/code.ts'); + expect(res.text).not.toContain('review-cache'); + // The untracked half agrees: plumbing descendants drop, user content stays. + expect(res.untracked).toEqual(['sub/note.md']); + }); + + it('keeps TRACKED plumbing sections the user NAMED', () => { + // The named-path exemption is deliberate: `/review + // .qwen/reviews/saved.md` on a committed report file asks for exactly + // the section the plumbing drop exists to remove. + write('.qwen/reviews/saved.md', '# old\n'); + git('add', '-f', '.qwen/reviews/saved.md'); + git('commit', '-q', '--no-verify', '-m', 'committed plumbing'); + write('.qwen/reviews/saved.md', '# new\n'); + + const res = capture({ file: '.qwen/reviews/saved.md' }); + expect(res.text).toContain('saved.md'); + expect(res.text).toContain('-# old'); + }); + it('reports an oversized tracked diff instead of inlining it', () => { // The aggregate budget covered only untracked files; a tracked diff could // grow to the 512 MiB gitRaw buffer. Stage one big tracked file past the @@ -270,6 +417,29 @@ describe('captureLocalDiff — untracked files', () => { expect(res.text).not.toContain('huge.ts'); }); + it('rejects an oversized tracked diff WHOLE — the cap runs before any parse', () => { + // The plumbing drop used to decode and section-parse the tracked diff + // BEFORE the cap gate, making the pathological case the gate exists for + // pay the gate's avoided cost — and near `gitRaw`'s 512 MiB ceiling an + // all-ASCII diff decodes past Node's maximum string length, so the + // decode threw instead of producing the skip record. An over-cap diff is + // never inlined and never parsed: plant one whose plumbing sections + // would have shrunk it UNDER the cap, and confirm it is still rejected + // whole rather than rescued by the drop. + const big = '{"pad":"' + 'x'.repeat(MAX_UNTRACKED_TOTAL_BYTES) + '"}\n'; + write('.qwen/review-cache/local.json', big); + git('add', '-f', '.qwen/review-cache/local.json'); + git('commit', '-q', '--no-verify', '-m', 'committed plumbing'); + write('.qwen/review-cache/local.json', big.replace('{"pad"', '{"PAD"')); + write('tracked.ts', 'export const a = 2;\n'); + + const res = capture(); + expect(res.skipped.some((f) => f.path === 'tracked changes')).toBe(true); + // Nothing was rescued out of it — neither the plumbing nor the real edit. + expect(res.text).not.toContain('review-cache'); + expect(res.text).not.toContain('tracked.ts'); + }); + it('reviews a dangling symlink instead of skipping it', () => { // Git renders a symlink as its **link text** at mode 120000, and the link // text does not depend on the target existing. A symlink pointing nowhere is diff --git a/packages/cli/src/commands/review/lib/local-diff.ts b/packages/cli/src/commands/review/lib/local-diff.ts index d62cc17b1b8..29f7ba407f9 100644 --- a/packages/cli/src/commands/review/lib/local-diff.ts +++ b/packages/cli/src/commands/review/lib/local-diff.ts @@ -24,8 +24,10 @@ // diff is a concatenation of per-file sections; the result parses exactly like // any other. -import { lstatSync, statSync, type Stats, realpathSync } from 'node:fs'; -import { join, relative, resolve, isAbsolute, sep } from 'node:path'; +import { lstatSync, statSync, type Stats } from 'node:fs'; +import { join, sep } from 'node:path'; +import { repoRelativeOf } from './paths.js'; +import { parseDiff, sliceDiffByLines } from './diff-plan.js'; import { LITERAL_PATHSPECS, NULL_DEVICE, @@ -91,6 +93,8 @@ export interface LocalDiffCapture { skipped: SkippedFile[]; /** True when HEAD does not exist yet (a repo with no commits). */ unbornHead: boolean; + /** The repo root every path in the capture is relative to. */ + repoRoot: string; } /** @@ -190,21 +194,11 @@ function toRepoPathspec(repoRoot: string, file: string): string { // throws on a path that does not exist yet, which a `--file` may legitimately // be (a brand-new untracked file is exactly this feature's subject), so fall // back to the non-canonical form rather than failing the review. - let abs = resolve(process.cwd(), file); - try { - abs = realpathSync(abs); - } catch { - // Not on disk yet — resolve() is the best we have, and the check below still - // holds for it. - } - const rel = relative(repoRoot, abs); - // `rel.startsWith('..')` is not the containment check it looks like: a file - // called `..foo.ts` at the repository root relativises to `..foo.ts`, and the - // scoped review would refuse to look at a perfectly ordinary file on the - // grounds that it had escaped. What escapes is `..` itself, or a path whose - // FIRST SEGMENT is `..`. - const escapes = - rel === '' || rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel); + // Canonicalisation and the segment-aware escape check live in + // `repoRelativeOf` because `qwen review run` pins the artifact name it polls + // for from the same answer; a second derivation here is how the two spellings + // drifted before. + const { rel, abs, escapes } = repoRelativeOf(repoRoot, file); if (escapes) { throw new Error( `--file ${file} resolves to ${abs}, which is outside the repository ` + @@ -242,14 +236,23 @@ export function isBinarySection(section: Buffer): boolean { } /** Repo-root-relative paths of untracked, non-ignored files. */ -function listUntracked(repoRoot: string, pathspec?: string): string[] { +function listUntracked( + repoRoot: string, + pathspec?: string, + excludeStandard = true, +): string[] { const args = [ '-C', repoRoot, LITERAL_PATHSPECS, 'ls-files', '--others', - '--exclude-standard', + // Deliberately named plumbing wins over ignore rules — repos ordinarily + // ignore `.qwen/` (this repo's own `.gitignore` does), and exclusion + // applies to an explicitly pathspec-NAMED file too, so with the flag the + // named file never reached the filter at all and the round reported + // clean over the one file it was asked about. + ...(excludeStandard ? ['--exclude-standard'] : []), '--full-name', '-z', ]; @@ -293,6 +296,73 @@ function diffUntracked(repoRoot: string, path: string): Buffer { * `file` scopes the capture to a single path (a `/review ` target). * Nothing here writes to the index, the worktree, or any ref. */ +/** + * Is this repo-relative path the review's own plumbing? + * + * Segment-exact at ANY depth, not anchored to the cwd. The three constants in + * `paths.ts` are cwd-relative for every invocation, so a round started from + * `sub/` writes `sub/.qwen/…` — which a filter built from THIS invocation's + * cwd does not match. A repo that does not ignore `.qwen` then lets the next + * root-invoked round capture the previous round's cache, reports, and args + * record as the user's untracked work, and the cache changes every round by + * construction, so an incremental round could never again report "no + * changes". Matching the segment wherever it sits closes both directions with + * no dependence on where either round ran. + * + * Segment-exact matters for the same reason `toRepoPathspec` records: a + * directory named `.qwen-notes` or `tmpfiles` is the user's, not ours. + */ +export function isReviewPlumbing(repoRelPath: string): boolean { + return /(?:^|\/)\.qwen\/(?:tmp|review-cache|reviews)(?:\/|$)/.test( + repoRelPath, + ); +} + +/** + * The tracked diff with the review's own plumbing sections removed. + * + * Ignore rules never apply to TRACKED files, so a repo that once committed + * its `.qwen` plumbing gets the cache back in `git diff HEAD` every round — + * and Step 8 rewrites that cache after every clean round, so the section is + * there by construction. The round then reviews its own ledger JSON, and + * `changedSince` can never empty: the incremental loop can never report "no + * changes". Exactly the pathology the untracked filter beside this exists to + * prevent, on the half it could not reach. + * + * The exemption is a NAMED plumbing pathspec, file- or directory-shaped: + * `/review .qwen/reviews/saved.md` is a deliberate request for exactly the + * section this drop exists to remove, so whatever came back stays. A + * NON-plumbing pathspec still drops: a directory target carries every + * tracked section beneath it, and "whatever came back is what the user + * asked for" is only true of file-shaped names — a repo that committed its + * own plumbing would otherwise review its ledger JSON as user content under + * a `sub/` target, and its every-round churn would keep `changedSince` from + * ever emptying. This matches the untracked filter beside this, which drops + * plumbing descendants of a directory target the same way. + */ +function dropPlumbingSections(diff: Buffer, pathspec?: string): Buffer { + if (diff.length === 0) return diff; + if (pathspec !== undefined && isReviewPlumbing(pathspec)) return diff; + // The cap gate runs BEFORE any decode: an over-cap tracked diff is + // rejected by the caller whole, never inlined, so parsing it first made + // the pathological case this gate exists for pay the cost it avoids — + // and near `gitRaw`'s 512 MiB ceiling an all-ASCII diff decodes past + // Node's maximum string length, so the decode throws instead of + // producing the caller's graceful skip record. The one semantic delta, + // accepted: a diff whose plumbing DROP would have shrunk it under the + // cap is rejected whole rather than rescued — an 11 MB diff is the cap's + // pathology whatever it is made of, and the skip record says so. + if (diff.length > MAX_UNTRACKED_TOTAL_BYTES) return diff; + const files = parseDiff(diff.toString('utf8')).files; + if (!files.some((f) => isReviewPlumbing(f.path))) return diff; + return sliceDiffByLines( + diff, + files + .filter((f) => !isReviewPlumbing(f.path)) + .map((f) => ({ startLine: f.diffStart, endLine: f.diffEnd })), + ); +} + export function captureLocalDiff(opts: { file?: string; includeUntracked?: boolean; @@ -315,6 +385,12 @@ export function captureLocalDiff(opts: { // The user typed `--file` relative to *their* directory; every git call here // runs with `-C `. Re-base it, and strip it of pathspec magic. const pathspec = file ? toRepoPathspec(repoRoot, file) : undefined; + // The user NAMED a plumbing path — a file or a directory under + // `.qwen/tmp|review-cache|reviews` — when reviewing it is the point. Both + // halves of the capture key on this one predicate so they cannot disagree: + // the tracked drop keeps its sections, and the untracked filter lists its + // files even when ignore rules cover them. + const namedPlumbing = pathspec !== undefined && isReviewPlumbing(pathspec); // `git diff HEAD` is what covers the whole tracked scope: a bare `git diff` // omits staged changes. @@ -328,7 +404,7 @@ export function captureLocalDiff(opts: { base, ]; if (pathspec) trackedArgs.push('--', pathspec); - const trackedDiff = gitRaw(...trackedArgs); + const trackedDiff = dropPlumbingSections(gitRaw(...trackedArgs), pathspec); const untracked: string[] = []; const skipped: SkippedFile[] = []; @@ -353,13 +429,28 @@ export function captureLocalDiff(opts: { } if (includeUntracked) { - // The review writes its own scratch files under `.qwen/tmp` — the args - // record, the parsed-args verdict, the diff, the plan — *before* this - // capture runs. In a repo that does not ignore `.qwen`, `ls-files --others` - // lists them as the user's untracked work, and the review would report on - // its own plumbing. They are never the change under review; drop them. - const candidates = listUntracked(repoRoot, pathspec).filter( - (p) => !p.startsWith('.qwen/tmp/') && p !== '.qwen/tmp', + // The review writes its own plumbing before and after this capture runs: + // scratch files under `.qwen/tmp` (the args record, the diff, the plan), + // the persistent incremental cache under `.qwen/review-cache` (rewritten + // by Step 8 after every clean round), and per-round artifacts under + // `.qwen/reviews`. In a repo that does not ignore `.qwen`, `ls-files + // --others` lists all of them as the user's untracked work — and the + // cache is worse than noise: hashed into its own next-round candidate it + // changes every round by construction, so an incremental round could + // never again report "no changes". None of it is ever the change under + // review; drop it. + // + // …unless the user NAMED plumbing. An explicit `--file` under + // `.qwen/reviews` is a deliberate request to review it — round notes, a + // saved report — and dropping it here left the round claiming "the + // working tree is clean, 0 chunks" over the one file it was asked about, + // with no `Not reviewed` record: mute, which this module's `SkippedFile` + // contract forbids. A NAMED plumbing path exempts itself and — for a + // directory-shaped target — its children; everything else under the + // plumbing directories is still dropped, exactly as the tracked half + // drops plumbing sections under a non-plumbing directory target. + const candidates = listUntracked(repoRoot, pathspec, !namedPlumbing).filter( + (p) => namedPlumbing || !isReviewPlumbing(p), ); if (candidates.length > MAX_UNTRACKED_FILES) { @@ -506,5 +597,11 @@ export function captureLocalDiff(opts: { } } - return { diff: Buffer.concat(parts), untracked, skipped, unbornHead }; + return { + diff: Buffer.concat(parts), + untracked, + skipped, + unbornHead, + repoRoot, + }; } diff --git a/packages/cli/src/commands/review/lib/paths.test.ts b/packages/cli/src/commands/review/lib/paths.test.ts index 804a27bf945..a18269594de 100644 --- a/packages/cli/src/commands/review/lib/paths.test.ts +++ b/packages/cli/src/commands/review/lib/paths.test.ts @@ -6,7 +6,10 @@ import { describe, it, expect } from 'vitest'; import { basename, dirname, join, resolve } from 'node:path'; +import { mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { + repoRelativeOf, inertPath, lastReviewEffortPath, tmpFile, @@ -197,3 +200,46 @@ describe('inertPath', () => { expect(inertPath('my probe.ts')).toBe('my probe.ts'); }); }); +describe('repoRelativeOf — the repository root is inside the repository', () => { + it('does not classify the root itself as an escape', () => { + // `classifyRunTarget` accepts a directory target, so the root is a + // reachable one — and calling it an escape split the two sides that must + // agree: the parent pinned the typed spelling while the child derived + // `safeTarget('') === 'target'`, so the poll never matched and a review + // that HAD run reported no verdict. `--file ` also threw + // "resolves to , which is outside the repository at ". + const root = realpathSync(mkdtempSync(join(tmpdir(), 'repo-rel-'))); + try { + const out = repoRelativeOf(root, root, root); + expect(out.escapes).toBe(false); + expect(out.rel).toBe(''); + // …and a genuine escape still is one. + expect(repoRelativeOf(root, '..', root).escapes).toBe(true); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); + +describe('canonicalise walk-up — backslash is a POSIX filename byte', () => { + it.skipIf(process.platform === 'win32')( + 'keeps a leading backslash through the ancestor walk', + () => { + // R23: the walk-up strip treated `\` as a separator on POSIX, so a + // dangling symlink literally named `\link` (realpath THROWS on a + // dangling link, which is what reaches the walk at all) came back as + // `link` — the capture then diffed a different name and the real + // entry dropped mutely; this PR's own `notes\` fixtures insist the + // byte is ordinary. Only the platform's separators are stripped now. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'repo-rel-'))); + try { + symlinkSync('no-such-target', join(root, '\\link')); + const out = repoRelativeOf(root, '\\link', root); + expect(out.escapes).toBe(false); + expect(out.rel).toBe('\\link'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 023614d501a..96f40efab74 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -9,8 +9,8 @@ // preferences resolve under Storage's project directory. Use `path.join` // rather than string concatenation so Windows backslashes are produced. -import { existsSync, statSync } from 'node:fs'; -import { join, resolve } from 'node:path'; +import { existsSync, realpathSync, statSync } from 'node:fs'; +import { isAbsolute, join, relative, resolve, sep } from 'node:path'; import { Storage } from '@qwen-code/qwen-code-core'; import { safeTarget } from '../../../utils/paths.js'; @@ -252,3 +252,91 @@ export function inertPath(p: string): string { // authoritative. return p.replace(/[\p{Cc}\p{Cf}\p{Zl}\p{Zp}\u2500`]+/gu, ' '); } + +/** + * `realpathSync(p)`, or the same answer for a path whose leaf does not exist + * yet: resolve the deepest ancestor that does, then re-append what was walked + * past. A path with no existing ancestor at all (an unreachable mount, a + * hostile chain) comes back untouched — the caller's containment check still + * rules on it, and refusing to name it at all would fail reviews that work. + */ +function canonicalise(abs: string): string { + const walked: string[] = []; + let cur = abs; + for (;;) { + try { + const real = realpathSync(cur); + return walked.length === 0 ? real : join(real, ...walked.reverse()); + } catch { + const parent = resolve(cur, '..'); + // `resolve('/', '..')` is `/`: the root is its own parent, so this is + // the termination condition, not a step. + if (parent === cur) return abs; + // Strip only THIS platform's separators: `\` is a legal POSIX + // filename byte (this PR's own `notes\` fixtures insist on it), and + // the two-class strip corrupted a leaf like `\link` into `link` on + // the walk-up — the capture then diffed a different name and the real + // file dropped mutely (R23: dangling `\link` at the repo root is the + // end-to-end shape, since only a realpath FAILURE reaches this walk). + walked.push( + cur.slice(parent.length).replace(sep === '/' ? /^\/+/ : /^[\\/]+/, ''), + ); + cur = parent; + } + } +} + +/** + * Where a user-supplied path sits relative to the repository root — the ONE + * answer both the parent's artifact pin and the child's capture must read. + * + * `qwen review run ` pins the artifact name it polls for from this, and + * `capture-local --file` scopes the diff from it. When the two spell the same + * file differently the parent polls a name no child ever writes: the review + * runs, posts, and then reports "no composed verdict was produced". They drifted + * once already, and the two corners below are why one call site cannot be + * trusted to re-derive it. + * + * `resolve` does not follow symlinks while `rev-parse --show-toplevel` returns + * the CANONICAL root — on macOS `/tmp` is a symlink to `/private/tmp`, so a + * path typed under `/tmp` relativises against a root sharing no prefix with it + * and comes back as a `..` walk out of a repository it is plainly inside. + * `realpathSync` throws on a path not on disk yet, which a reviewed file may + * legitimately be (a brand-new untracked file is exactly that feature's + * subject) — so the canonicalisation resolves the nearest ancestor that DOES + * exist and re-appends the rest, which is what makes the symlinked prefix and + * the not-yet-created file hold at the same time rather than one at a time. + * + * And `rel.startsWith('..')` is not the containment check it looks like: a + * file called `..foo.ts` at the repository root relativises to `..foo.ts` and + * would be read as having escaped. What escapes is `..` itself, a path whose + * FIRST SEGMENT is `..`, or an absolute one (no relative spelling exists). + */ +export function repoRelativeOf( + repoRoot: string, + file: string, + from: string = process.cwd(), +): { rel: string; abs: string; escapes: boolean } { + const abs = canonicalise(resolve(from, file)); + const rel = relative(repoRoot, abs); + // `rel === ''` is the repository ROOT, which is inside the repository — the + // one place the old test called an escape. `classifyRunTarget` accepts a + // directory target explicitly ("a tab-completed `src/` classifies as a file + // target"), so the root is a reachable one, and treating it as an escape + // split the two sides that must agree: the parent fell back to pinning the + // typed spelling while the child derived `safeTarget('') === 'target'`, so + // the poll never matched and a review that had run — and with `--comment` + // already posted — reported no verdict. `--file ` also threw the + // self-contradictory "resolves to , which is outside the repository + // at ". As a pathspec `.` scopes the diff to the whole tree, which is + // what naming the root asks for. + const escapes = rel === '..' || rel.startsWith('..' + sep) || isAbsolute(rel); + // Git spells every path with forward slashes on every platform, and `rel` + // flows VERBATIM into git pathspecs, the candidate's recorded `source`, + // and `cachePathFor`'s digest — where node's win32 `relative()` answers + // backslashes, one file got two different cache filenames across + // platforms and the win32 lane failed every assertion spelling the posix + // form (R21-1). Normalized HERE, after the escape check computed against + // the platform separator, so both consumers of `rel` see one spelling. + return { rel: sep === '/' ? rel : rel.split(sep).join('/'), abs, escapes }; +} diff --git a/packages/cli/src/commands/review/lib/report.ts b/packages/cli/src/commands/review/lib/report.ts index 1ffe98f876d..4572037e59d 100644 --- a/packages/cli/src/commands/review/lib/report.ts +++ b/packages/cli/src/commands/review/lib/report.ts @@ -260,3 +260,69 @@ export function stringifyPlanReport(report: unknown): string { ) + '\n' ); } + +/** + * The plan's `incremental` field, as both producers write it and every + * consumer reads it. + * + * NESTED, deliberately. The PR flow's block answers two questions — MAY this + * anchor scope the round (`since`/`effective`/`reason`, which only that flow + * has) and WHICH files it scoped to — and the second is what the brief + * renderer and the roster read. The local flow has no ruling to report, only + * a scope, so it writes the same `scope` key and nothing else. Flattening it + * on one side is not a shorter spelling of the same thing: the consumers key + * on `incremental.scope`, so a flat local block renders no incremental frame + * at all and every widened file is re-reviewed from scratch — the exact token + * burn this feature exists to prevent, and invisible, because the diff IS + * sliced and the round looks incremental everywhere else. + */ +export interface IncrementalBlock { + scope?: IncrementalScope; +} + +/** + * WHICH files an incrementally-scoped round reviews, and why. It lives HERE, + * beside the other plan-report shapes, and not in a module of its own: a + * types-only module is erased by esbuild at every import site, and the + * bundle-staleness digest guard rightly refuses a review-source file the + * bundle can never contain. + */ +export interface IncrementalScope { + /** + * What the scope is measured FROM: a commit sha on the PR flow, a + * content-addressed state id on the local flow. Display-only downstream — + * briefs render its first 12 characters. + */ + anchor: string; + /** Files changed since the anchor — reviewed on their hunks, in full. */ + deltaFiles: string[]; + /** + * Still-clean files pulled back in by the one-hop widening, each with the + * changed files it imports — the seam its brief directs the agent at. + */ + interaction: Array<{ path: string; importsChanged: string[] }>; + /** + * How many still-clean files this scope leaves out. A count, not a list: + * nothing downstream reads the names, and on a large plan 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 all of it. + * The local flow writes it; the PR flow has no retained full-range diff to + * point at yet and omits the field. + */ + fullDiffPath?: string | null; + /** + * Cached paths whose RECORDED change is gone from this capture — the file + * deleted, or the change discarded back to the diff base — while no diff + * section survives for them. The scope-emptied stop's split key: a cache + * finding citing one of these is SUPERSEDED (the bytes it cited no longer + * exist), and one citing any other path sits byte-identical to the round + * that recorded it. Published as a LIST because the split is per cited + * path and file presence cannot answer it — a discarded change leaves the + * file present with the cited bytes gone. Bounded by the cache's file + * count; absent when empty. + */ + supersededPaths?: string[]; +} diff --git a/packages/cli/src/commands/review/run-classify.integration.test.ts b/packages/cli/src/commands/review/run-classify.integration.test.ts new file mode 100644 index 00000000000..85e017524de --- /dev/null +++ b/packages/cli/src/commands/review/run-classify.integration.test.ts @@ -0,0 +1,138 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Real git, real cwd. `classifyRunTarget` pins the artifact names the parent +// polls for, and the child derives ITS names by canonicalising the path +// against the repo root — so the property that matters is that every +// spelling of one file produces one pin. `run.test.ts` cannot cover it: it +// mocks `child_process`, so the git-backed canonicalisation there falls back +// to the token as typed. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + mkdtempSync, + mkdirSync, + rmSync, + realpathSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { classifyRunTarget } from './run.js'; +import { repoRelativeOf } from './lib/paths.js'; +import { safeTarget } from '../../utils/paths.js'; +import { isolateHostGitConfig } from './lib/test-utils.js'; + +let repo: string; +let cwd: string; +let iso: ReturnType; + +beforeEach(() => { + repo = realpathSync(mkdtempSync(join(tmpdir(), 'run-classify-'))); + cwd = process.cwd(); + process.chdir(repo); + iso = isolateHostGitConfig(); + execFileSync('git', ['init', '-q', '--template=', '.'], { cwd: repo }); + mkdirSync(join(repo, 'src'), { recursive: true }); + mkdirSync(join(repo, 'pkg/deep'), { recursive: true }); +}); + +afterEach(() => { + process.chdir(cwd); + iso.dispose(); + rmSync(repo, { recursive: true, force: true }); +}); + +describe('classifyRunTarget — canonical file pins', () => { + it('every spelling of one file yields one pin', () => { + const canonical = classifyRunTarget('src/foo.ts'); + expect(canonical).toEqual({ kind: 'file', base: 'src_foo.ts' }); + for (const spelling of [ + './src/foo.ts', + 'src/../src/foo.ts', + join(repo, 'src/foo.ts'), + `src//foo.ts`, + ]) { + expect(classifyRunTarget(spelling)).toEqual(canonical); + } + }); + + it('a path typed from a SUBDIRECTORY pins the same name as from the root', () => { + const fromRoot = classifyRunTarget('pkg/deep/x.ts'); + process.chdir(join(repo, 'pkg')); + expect(classifyRunTarget('deep/x.ts')).toEqual(fromRoot); + expect(classifyRunTarget('./deep/x.ts')).toEqual(fromRoot); + }); + + it('a symlinked prefix pins the same name — on disk and not yet on disk', () => { + // macOS's `/tmp` is a symlink to `/private/tmp`, so a path typed under it + // relativises against a `--show-toplevel` root that shares no prefix with + // it: the pin used to fall back to the whole typed path flattened while + // the child, which canonicalises, wrote `src_foo.ts`. The poll then never + // matched and the run reported "no composed verdict was produced" over a + // review that had already run. + const link = join( + realpathSync(mkdtempSync(join(tmpdir(), 'rc-link-'))), + 'l', + ); + symlinkSync(repo, link); + try { + writeFileSync(join(repo, 'src/foo.ts'), 'export const a = 1;\n'); + expect(classifyRunTarget(join(link, 'src/foo.ts'))).toEqual({ + kind: 'file', + base: 'src_foo.ts', + }); + // And a file the review is about to CREATE — `realpathSync` throws on + // the leaf, so the canonicalisation has to resolve the ancestor that + // exists. Reviewing a brand-new untracked file is a supported target. + expect(classifyRunTarget(join(link, 'src/not-yet.ts'))).toEqual({ + kind: 'file', + base: 'src_not-yet.ts', + }); + } finally { + rmSync(link, { force: true }); + } + }); + + it('a root-level ..foo.ts is inside the repo, not an escape', () => { + // `rel.startsWith('..')` reads a perfectly ordinary filename as a walk out + // of the repository. What escapes is `..` itself or a FIRST SEGMENT of + // `..`. + writeFileSync(join(repo, '..foo.ts'), 'export const a = 1;\n'); + expect(classifyRunTarget(join(repo, '..foo.ts'))).toEqual({ + kind: 'file', + base: 'foo.ts', + }); + }); + + it('a path outside the repo keeps its typed spelling rather than a .. walk', () => { + const outside = classifyRunTarget(join(tmpdir(), 'elsewhere.ts')); + expect(outside.kind).toBe('file'); + expect((outside as { base: string }).base).not.toContain('..'); + }); +}); + +describe('classifyRunTarget — parent pin and child derivation agree for a backslash name', () => { + it.skipIf(process.platform === 'win32')( + 'a file literally named `notes\\` yields one pin on both sides', + () => { + // On POSIX a backslash is an ordinary filename character. The child + // (`capture-local --file`) derives its artifact stem through + // `repoRelativeOf` → `safeTarget` and never strips trailing + // backslashes; the parent pin must spell the file the same way or the + // poll never matches. (On Windows a backslash IS a separator and + // `resolve` normalizes it away — this shape is POSIX-only.) + writeFileSync(join(repo, 'notes\\'), 'export const a = 1;\n'); + const classified = classifyRunTarget('notes\\'); + expect(classified).toEqual({ + kind: 'file', + base: safeTarget(repoRelativeOf(repo, 'notes\\').rel), + }); + }, + ); +}); diff --git a/packages/cli/src/commands/review/run-skill-parity.test.ts b/packages/cli/src/commands/review/run-skill-parity.test.ts index 7be1766e99e..dd77a1f8e59 100644 --- a/packages/cli/src/commands/review/run-skill-parity.test.ts +++ b/packages/cli/src/commands/review/run-skill-parity.test.ts @@ -39,6 +39,19 @@ const SKILL_DIR = join(repoRoot, 'packages/core/src/skills/bundled/review'); const TARGETS = { pr: { cls: { kind: 'pr', number: '9014' } as const, token: 'pr-9014' }, file: { cls: { kind: 'file', base: 'foo.ts' } as const, token: 'foo.ts' }, + // A nested target: the token is the flattened repo-relative path, and this + // is the shape where the pre-PR `` stem and the pin disagreed. + fileNested: { + cls: { kind: 'file', base: 'src_foo.ts' } as const, + token: 'src_foo.ts', + }, + // A markdown target: the pin deliberately does NOT double the `.md`, and + // the Step 8 template carries the matching no-doubling rule — the shape + // where every file review of a `.md` path used to lose its `Report:` line. + fileMd: { + cls: { kind: 'file', base: 'docs_guide.md' } as const, + token: 'docs_guide.md', + }, local: { cls: { kind: 'local' } as const, token: 'local' }, }; @@ -103,30 +116,45 @@ describe('run pins match the bundled skill templates', () => { // A miss here means Step 8 no longer lists those stems: update // reportPatternFor and this oracle together to the new template. expect(stems).toEqual( - expect.arrayContaining(['local', 'pr-', '']), + expect.arrayContaining(['local', 'pr-', '']), ); - const render = (stem: string): string => + const render = (stem: string, token: string): string => `2026-08-13-101010-${stem}.md` .replace('pr-', 'pr-9014') - .replace('', 'foo.ts'); + .replace('', token); - expect(reportPatternFor(TARGETS.pr.cls).test(render('pr-'))).toBe( - true, - ); - expect(reportPatternFor(TARGETS.file.cls).test(render(''))).toBe( - true, + expect( + reportPatternFor(TARGETS.pr.cls).test(render('pr-', '')), + ).toBe(true); + // The file stem renders from the capture's token — for a root file and + // for a nested one alike, since the pin builds from the same derivation. + for (const { cls, token } of [TARGETS.file, TARGETS.fileNested]) { + expect(reportPatternFor(cls).test(render('', token))).toBe(true); + } + // A token that already ends in `.md`: the template ends the name at the + // token — the prose rule beside it, pinned here so a template edit that + // drops the rule fails next to the pin it must agree with. + expect(step8Corpus as string).toContain('do not double the extension'); + const mdName = `2026-08-13-101010-${TARGETS.fileMd.token}`; + expect(reportPatternFor(TARGETS.fileMd.cls).test(mdName)).toBe(true); + // …and the DOUBLED rendering — what a template without the rule writes — + // must not match, or the run's `Report:` line is silently lost again. + expect(reportPatternFor(TARGETS.fileMd.cls).test(`${mdName}.md`)).toBe( + false, ); - expect(reportPatternFor(TARGETS.local.cls).test(render('local'))).toBe( + expect(reportPatternFor(TARGETS.local.cls).test(render('local', ''))).toBe( true, ); // And each class refuses the neighbouring classes' rendered stems — the // cross-capture this pinning exists to prevent. - expect(reportPatternFor(TARGETS.pr.cls).test(render('local'))).toBe(false); + expect(reportPatternFor(TARGETS.pr.cls).test(render('local', ''))).toBe( + false, + ); expect( - reportPatternFor(TARGETS.local.cls).test(render('pr-')), + reportPatternFor(TARGETS.local.cls).test(render('pr-', '')), ).toBe(false); - expect(reportPatternFor(TARGETS.file.cls).test(render('local'))).toBe( + expect(reportPatternFor(TARGETS.file.cls).test(render('local', ''))).toBe( false, ); }); diff --git a/packages/cli/src/commands/review/run.test.ts b/packages/cli/src/commands/review/run.test.ts index 082ed147b9e..b8cd87d91ad 100644 --- a/packages/cli/src/commands/review/run.test.ts +++ b/packages/cli/src/commands/review/run.test.ts @@ -228,12 +228,23 @@ describe('target-pinned artifact patterns', () => { number: '9014', }); // A path that merely contains /pull/ is a FILE target to the parser, - // and the file identity is the basename (the skill's `{target}` token): + // and the file identity is the skill's `{target}` token: the + // repo-relative path put through the CLI's own `safeTarget` + // normalization, NOT the basename. (This suite mocks child_process, so + // the git-backed canonicalisation falls back to the token as typed — + // the canonical-spelling equivalence is pinned in + // `run-classify.integration.test.ts`, which uses real git.) expect(classifyRunTarget('docs/pull/42')).toEqual({ kind: 'file', - base: '42', + base: 'docs_pull_42', }); expect(classifyRunTarget('src/foo.ts')).toEqual({ + kind: 'file', + base: 'src_foo.ts', + }); + // Root-level targets are unchanged — which is why the drift went + // unnoticed: every fixture used one. + expect(classifyRunTarget('foo.ts')).toEqual({ kind: 'file', base: 'foo.ts', }); @@ -286,10 +297,18 @@ describe('target-pinned artifact patterns', () => { expect(local.test('review.md')).toBe(true); expect(local.test('2026-08-13-1622-pr-9045.md')).toBe(false); - // File reports carry the filename in the report stem's trailing slot. + // File reports carry the target token in the report stem's trailing + // slot — Step 8 names them from the capture's `target` field now, the + // same derivation this pattern builds from. const file = reportPatternFor({ kind: 'file', base: 'foo.ts' }); expect(file.test('2026-08-13-101010-foo.ts.md')).toBe(true); expect(file.test('2026-08-13-101010-bar.ts.md')).toBe(false); + // A subdirectory target: the pin and the instructed name must agree PAST + // the repo root — exactly where the pre-PR basename convention lost the + // Report: line for every file review of a nested path. + const nested = reportPatternFor({ kind: 'file', base: 'src_foo.ts' }); + expect(nested.test('2026-08-22-120000-src_foo.ts.md')).toBe(true); + expect(nested.test('2026-08-22-120000-foo.ts.md')).toBe(false); // A file literally named `pr-1234.md` claims its OWN report — the shape // the local branch's PR exclusion would self-reject (`.md` not doubled). const prNamed = reportPatternFor({ kind: 'file', base: 'pr-1234.md' }); @@ -297,6 +316,15 @@ describe('target-pinned artifact patterns', () => { }); }); +describe('a decided stop counts as completed', () => { + it('exits 0 when the capture said there was nothing to review', () => { + // `compose-review` runs only in Step 6; both stops fire in Step 1, so no + // composed verdict exists. Polling for the verdict alone reported + // "Review did not complete" over a round whose own output was decided. + expect(exitCodeFor(true, null, 'none')).toBe(0); + }); +}); + describe('exitCodeFor', () => { it('splits completed / no-verdict / blocking into 0 / 1 / 3', () => { expect(exitCodeFor(true, 'APPROVE', 'none')).toBe(0); @@ -308,6 +336,17 @@ describe('exitCodeFor', () => { // read as "the review blocked". expect(exitCodeFor(false, 'REQUEST_CHANGES', 'request-changes')).toBe(1); }); + + it('exits 0 on a decided stop round, whatever the ledger still holds', () => { + // A stop completes with `event: null` and no synthesised verdict: the + // ledger a stop renders is rewritten only by a cache-writing round, so a + // blocker fixed and committed stays `open` there — an exit code keyed on + // it was a failure no action cleared. The rendered list still names the + // entries; the gate waits for a composed verdict. + expect(exitCodeFor(true, null, 'request-changes')).toBe(0); + expect(exitCodeFor(true, null, 'none')).toBe(0); + expect(exitCodeFor(false, null, 'request-changes')).toBe(1); + }); }); describe('killProcessGroup', () => { @@ -1131,4 +1170,188 @@ describe('review run (handler)', () => { expect(result.timedOut).toBe(true); expect(process.exitCode).toBe(1); }); + + it('keeps a stop verdict a concurrent run overwrote before the child exited', async () => { + // The stop sidecar is the shared per-target name, written with plain + // `writeFileSync` — no per-run component, no O_EXCL. A concurrent run of + // the same target truncated-overwrites it with its own runId stamp while + // this run's child still lives, and the single post-close read then saw + // the foreign stamp: the runId fence correctly refused it as THIS run's + // verdict but turned a round the capture decided into "Review did not + // complete" (exit 1). The window spans the whole child session, so no + // micro-timing is needed. The capture poll snapshots the sidecar in-run, + // the same protection the composed verdict gets. + vi.useFakeTimers(); + let child!: FakeChild; + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + // Step 1: the capture decides nothing to review and writes the stop + // sidecar, stamped by THIS run. + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'clean-tree', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child = new FakeChild(); + return child; + }, + ); + + const done = runHandler(); + // The capture poll snapshots the sidecar while the child still runs... + await vi.advanceTimersByTimeAsync(1_000); + // ...then a concurrent run of the same target stamps the shared sidecar + // its own before the child exits. + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'scope-emptied', + runId: 'another-run', + }), + 'utf8', + ); + child.emit('close', 0); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(process.exitCode).toBe(0); + }); + + it('keeps a stop verdict a same-stem cleanup swept before the child exited', async () => { + // The sibling shape of the overwrite race: the sidecar sits under the + // same `qwen-review--` prefix the Step 9 cleanup sweep unlinks, + // and a same-stem full round can sweep it while the stop round's child + // is still alive. The in-run snapshot holds the verdict either way. + vi.useFakeTimers(); + let child!: FakeChild; + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'clean-tree', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child = new FakeChild(); + return child; + }, + ); + + const done = runHandler(); + await vi.advanceTimersByTimeAsync(1_000); + rmSync(join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json')); + child.emit('close', 0); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(process.exitCode).toBe(0); + }); + + it('reads a stop written and closed BEFORE the first poll tick', async () => { + // The cleanup-before-first-poll window (human review on #9659): the PR + // stop path writes the sidecar and runs cleanup in the same breath, and + // the parent's first in-run poll is up to 250 ms away. cleanup now + // SPARES the current run's sidecar (pinned in cleanup.test), so the + // post-close fallback must be able to read the decision with ZERO timer + // advance — the existing race arms all advance 1000 ms first, which is + // exactly how this window went unpinned. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'clean-tree', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + // Close synchronously on the next microtask — before ANY timer. + queueMicrotask(() => child.emit('close', 0)); + return child; + }, + ); + + await runHandler(); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(process.exitCode).toBe(0); + }); + + it('exits 0 under --fail-on for a decided stop round', async () => { + // A stop composes no verdict and synthesises none: the ledger it renders + // is rewritten only by a cache-writing round, so a blocker fixed and + // committed stays `open` there — gating on it was a failure no action + // could clear. The gate fires only on a composed REQUEST_CHANGES. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'clean-tree', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ 'fail-on': 'request-changes' }); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(0); + }); +}); + +describe('classifyRunTarget — a trailing backslash is a POSIX filename character', () => { + it('keeps the backslash: the child derivation never strips it', () => { + // The trim used to strip trailing backslashes too, but the child's + // `sourcePath` derivation (`repoRelativeOf` → `safeTarget`) never does, + // and on POSIX a backslash is an ordinary filename character: for a file + // literally named `notes\` the parent pinned `qwen-review-notes-…` while + // every child artifact carried `notes_` — the review ran (and with + // --comment posted) while the parent reported no verdict, every run, for + // that target. Only forward slashes are separators both sides strip. + expect(classifyRunTarget('notes\\')).toEqual({ + kind: 'file', + base: 'notes_', + }); + // A tab-completed trailing separator still classifies: + expect(classifyRunTarget('src/')).toEqual({ kind: 'file', base: 'src' }); + }); }); diff --git a/packages/cli/src/commands/review/run.ts b/packages/cli/src/commands/review/run.ts index 30ec7099614..ba5469fd4b5 100644 --- a/packages/cli/src/commands/review/run.ts +++ b/packages/cli/src/commands/review/run.ts @@ -31,13 +31,16 @@ import { isUnusableScriptEntry, } from '@qwen-code/qwen-code-core'; import { spawn, execFileSync } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; import { readdirSync, readFileSync, realpathSync, statSync } from 'node:fs'; -import { dirname, join, resolve } from 'node:path'; +import { dirname, join, normalize, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLineSafe, } from '../../utils/stdioHelpers.js'; -import { REVIEW_TMP_DIR, REVIEWS_DIR } from './lib/paths.js'; +import { REVIEW_TMP_DIR, REVIEWS_DIR, repoRelativeOf } from './lib/paths.js'; +import { safeTarget } from '../../utils/paths.js'; +import { gitOpt } from './lib/git.js'; import { EFFORT_LEVELS, parseReviewArgs } from './parse-args.js'; export interface RunReviewArgs { @@ -107,6 +110,28 @@ export type RunTargetClass = | { kind: 'file'; base: string } | { kind: 'local' }; +/** + * The repo-relative, normalised spelling of a user-typed path — the same + * identity `capture-local --file` derives before the child names anything. + * + * Falls back to a plain normalisation when the repo root cannot be resolved + * (no git, a detached invocation): the pin is then whatever the token + * spells, which is the pre-canonicalisation behaviour and no worse than it. + */ +function repoRelative(target: string): string { + const normalised = normalize(target).replace(/^\.\//, ''); + const root = gitOpt('rev-parse', '--show-toplevel'); + if (root === null) return normalised; + // Shared with `capture-local`'s own pathspec derivation (`repoRelativeOf` + // in lib/paths.ts) so the pin and the artifact it waits for cannot spell + // one file two ways — see that function for the two corners, a symlinked + // root prefix and a root-level `..foo.ts`, that a re-derivation here got + // wrong. A path genuinely outside the repo has no repo-relative spelling; + // leave it as the user typed it rather than pinning on a `..` walk. + const { rel, escapes } = repoRelativeOf(root, normalised); + return escapes ? normalised : rel; +} + export function classifyRunTarget(target?: string): RunTargetClass { if (!target) return { kind: 'local' }; const { target: t } = parseReviewArgs(target); @@ -114,14 +139,37 @@ export function classifyRunTarget(target?: string): RunTargetClass { return { kind: 'pr', number: String(t.number) }; } if (t.type === 'file') { - // The skill's `{target}` token for a file review is the file's basename - // (`--target ` in the capture step), so that is the identity - // the child's artifact names carry. Trailing separators are stripped + // The skill's `{target}` token for a file review is the file's + // repo-relative path put through `safeTarget` — the same normalization + // the CLI applies when it derives filenames — so that is the identity + // the child's artifact names carry. It used to be the BASENAME, and the + // two diverge for every file in a subdirectory: the child would write + // `qwen-review-src_index.ts-composed.json` while the parent polled + // `qwen-review-index.ts-composed.json`, never matched, and reported "no + // composed verdict was produced" over a review that had already run (and + // with `--comment`, already posted). Trailing slashes are stripped // first: a tab-completed `src/` classifies as a file target and reviews - // the directory, and a bare `.pop()` would return `''` — a pin - // (`qwen-review--composed.json`) no child artifact can ever carry. - const trimmed = t.path.replace(/[\\/]+$/, ''); - return { kind: 'file', base: trimmed.split(/[\\/]/).pop() || trimmed }; + // the directory, and the empty remainder would pin a name no child + // artifact can ever carry. + // The token is CANONICALISED before flattening, because the child + // canonicalises too: `capture-local --file` resolves the path against + // the caller's directory and re-bases it on the repo root, and SKILL.md + // names the artifacts from THAT. Flattening the raw token agreed only + // when the user typed the canonical repo-relative spelling — an absolute + // path, a `src/../src/foo.ts`, or a path typed from a subdirectory each + // produced a pin the child never writes: the same never-matching poll + // this pin was just fixed to avoid, for a new input class. + // + // Trailing FORWARD slashes only: the child's derivation never strips, + // and on POSIX a backslash is an ordinary filename character — stripping + // it spelled one file two ways (a file literally named `notes\` pinned + // `notes` while every child artifact carried `notes_`), so the poll + // never matched and a review that ran — and with --comment posted — + // reported no verdict, every run, for that target. On Windows + // `resolve` normalizes a trailing backslash away, so nothing needs it + // stripped here. + const trimmed = t.path.replace(/\/+$/, '') || t.path; + return { kind: 'file', base: safeTarget(repoRelative(trimmed)) }; } return { kind: 'local' }; } @@ -156,6 +204,82 @@ const escapeRe = (s: string): string => * Only a per-run nonce in the child's artifact names could key these * apart, and the bundled skill, not this command, would have to mint it. */ +/** The stop sidecar's exact filename for a target class. */ +function stopNameFor(cls: RunTargetClass): string { + // The capture's sidecar, not the plan: `--out` is the orchestrator's to + // choose, so the plan has no name the parent can predict. This one is + // derived from the same target the parent derives. + return `qwen-review-${planStemFor(cls)}-stop.json`; +} + +/** The stop sidecar's verdict-bearing shape. */ +interface StopVerdict { + reason: string; +} + +/** + * The sidecar's verdict, fenced by the run that asks. + * + * Stamped by THIS run, or it is not this run's verdict. The name is a + * flattened target token and that token is not injective, so a concurrent + * review whose path flattens alike writes the same file — and its verdict + * would decide this run's exit code. Absent stamp, foreign stamp, + * unreadable or not JSON: no claim either way. + */ +function readStopSidecar(path: string, runId: string): StopVerdict | null { + try { + const stop = JSON.parse(readFileSync(path, 'utf8')) as { + reason?: unknown; + runId?: unknown; + }; + if (stop.runId !== runId) return null; + if (typeof stop.reason !== 'string' || stop.reason === '') return null; + return { reason: stop.reason }; + } catch { + return null; + } +} + +/** + * The capture's own "nothing to review" verdict for this target, if it wrote + * one this run. + * + * Read off a sidecar the CLI writes beside the plan, and fenced by the run + * epoch the same way every other artifact here is: a stop left by an earlier + * run must not make this one look decided. This POST-CLOSE read is only the + * fallback — the sidecar is snapshotted in-run first, because a concurrent + * run of the same target can truncate-overwrite the shared name (and a + * same-stem cleanup sweep can unlink it) any time before this read: the + * fence correctly refuses a foreign stamp, but that refusal turns a round + * the capture decided into "Review did not complete". + */ +function nothingToReviewFrom( + cls: RunTargetClass, + cutoffMs: number, + runId: string, +): StopVerdict | null { + const found = newestArtifactSince( + REVIEW_TMP_DIR, + new RegExp(`^${escapeRe(stopNameFor(cls))}$`), + cutoffMs, + ); + if (!found) return null; + return readStopSidecar(found.path, runId); +} + +/** The `` slot in the plan's filename, per target class. */ +function planStemFor(cls: RunTargetClass): string { + switch (cls.kind) { + case 'pr': + return `pr-${cls.number}`; + case 'file': + return cls.base; + case 'local': + default: + return 'local'; + } +} + export function composedNameFor(cls: RunTargetClass): string { switch (cls.kind) { case 'pr': @@ -175,7 +299,7 @@ export function composedPatternFor(cls: RunTargetClass): RegExp { /** * The saved report under `.qwen/reviews/`, pinned as far as its naming * allows. PR reports reliably end `-pr-.md`, and file reports carry the - * filename in the same slot (`-