diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 226010fd2a..5cba1ad64f 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -25,6 +25,7 @@ import { execFileSync } from 'node:child_process'; import { mkdirSync, mkdtempSync, + realpathSync, rmSync, utimesSync, writeFileSync, @@ -1479,7 +1480,7 @@ describe('--roster — every prompt the plan requires, in one call', () => { // is launched, which makes this the one place the pipeline can notice that // the tree those agents are about to read is not the commit they think it // is. A real git worktree, because `git status` is the oracle. - const dir = mkdtempSync(join(tmpdir(), 'ap-residue-')); + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'ap-residue-'))); // Ambient host git config (a global `commit.gpgsign` with no key, a // `core.hooksPath` that fails) makes the fixture commit throw and reddens // this test for reasons the branch never touched — the incident @@ -1504,15 +1505,18 @@ describe('--roster — every prompt the plan requires, in one call', () => { writeFileSync(join(wt, '__probe__.test.ts'), 'it("x", () => {});'); const plan = join(dir, 'plan.json'); - writeFileSync( - plan, - JSON.stringify({ - ...PLAN, - worktreePath: wt, - prNumber: '9207', - ownerRepo: 'QwenLM/qwen-code', - }), - ); + const writePlan = (fields: Record) => + writeFileSync( + plan, + JSON.stringify({ + ...PLAN, + worktreePath: wt, + prNumber: '9207', + ownerRepo: 'QwenLM/qwen-code', + ...fields, + }), + ); + writePlan({ fetchedSha: git('rev-parse', 'HEAD').trim() }); (agentPromptCommand.handler as (a: unknown) => void)({ plan, roster: true, @@ -1533,12 +1537,135 @@ describe('--roster — every prompt the plan requires, in one call', () => { expect(readFileSync(briefPath(plan, '1b'), 'utf8')).toContain( 'And right now it is not clean', ); + + // The handover is the wiring under test: drop it and the brief degrades + // in one of two ways, both refused — a WRONG sha (the forge's own) + // reaches the pin and is refused there, a MISSING one fails closed + // before the probe runs, because every worktree-mode fetch writes the + // field and its absence means the plan was tampered with. Either way + // the brief carries the unmeasured sentence, never a clean verdict. + const briefOf = (fields: Record) => { + writePlan(fields); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + roster: true, + }); + return readFileSync(briefPath(plan, '1a'), 'utf8'); + }; + const wrongSha = briefOf({ fetchedSha: `deadbeef${'0'.repeat(32)}` }); + expect(wrongSha).toContain('Whether it is clean could not be measured'); + expect(wrongSha).toContain('not the fetched PR head'); + // The framing names a reason, not a failed `git status` — the status + // never ran for these refusals, and a triager sent to debug the git + // environment would find nothing to fix. + expect(wrongSha).toContain('(reason: '); + expect(wrongSha).not.toContain('(`git status` failed'); + const noSha = briefOf({}); + expect(noSha).toContain('Whether it is clean could not be measured'); + expect(noSha).toContain('no usable record of the fetched head sha'); + // The stderr warning the handler prints for the same state carries the + // same neutral framing. + expect(writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('(reason: '), + ); } finally { rmSync(dir, { recursive: true, force: true }); gitIsolation.dispose(); } }); + // A SHA-256 repository is the shape the record validators must admit: + // fetch-pr writes `git rev-parse` verbatim, and in that repository class + // the answer is 64 hex. Git grew the format late, so probe for support and + // skip where it is absent rather than fail a host that cannot build the + // fixture. + const gitSha256Supported = (() => { + try { + const probe = mkdtempSync(join(tmpdir(), 'qwen-sha256-probe-')); + try { + execFileSync('git', ['init', '-q', '--object-format=sha256', probe], { + stdio: 'pipe', + }); + return true; + } finally { + rmSync(probe, { recursive: true, force: true }); + } + } catch { + return false; + } + })(); + + it.skipIf(!gitSha256Supported)( + 'pins a SHA-256 review worktree with the plan’s 64-hex record', + () => { + // A validator matching only 40-hex shas drops the record this + // repository class writes: every worktree-mode round then fails + // closed as though the plan were tampered with, and the verifier's + // scratch-tree command is built without `--fetched-sha`. The 64-hex + // record must reach BOTH the residue pin and the welded command. + const gitIsolation = isolateHostGitConfig(); + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'ap-sha256-'))); + try { + const git = (...args: string[]) => + execFileSync('git', args, { cwd: dir, encoding: 'utf8' }); + git('init', '-q', '-b', 'main', '--object-format=sha256'); + git('config', 'user.email', 't@t.t'); + git('config', 'user.name', 't'); + writeFileSync(join(dir, 'a.ts'), 'export const x = 1;\n'); + git('add', '-A'); + git('commit', '-qm', 'head'); + const sha64 = git('rev-parse', 'HEAD').trim(); + expect(sha64).toMatch(/^[0-9a-f]{64}$/); + const wt = join(dir, '.qwen', 'tmp', 'review-pr-sha256'); + git('worktree', 'add', '--detach', '-q', wt, 'HEAD'); + const plan = join(dir, 'plan.json'); + writeFileSync( + plan, + JSON.stringify({ + ...PLAN, + worktreePath: wt, + prNumber: '256', + ownerRepo: 'QwenLM/qwen-code', + fetchedSha: sha64, + }), + ); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + roster: true, + }); + + // The record reached the residue pin: the tree at the recorded sha + // measures clean instead of being refused for a missing record. + const brief = readFileSync(briefPath(plan, '1a'), 'utf8'); + expect(brief).not.toContain( + 'Whether it is clean could not be measured', + ); + expect(brief).not.toContain('no usable record of the fetched head'); + // And it reached the scratch-tree command welded into a verifier + // shard's brief — shards launch through the single-role path with + // their record key, exactly as the orchestrator runs them. + const findings = join(dir, 'findings.md'); + writeFileSync(findings, '- **[Critical]** probe'); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'verify', + findings, + }); + const recorded = readRecordedPrompts(plan); + const verifyKey = [...recorded.keys()].find((k) => + k.startsWith('verify--'), + ); + expect(verifyKey).toBeDefined(); + expect( + readFileSync(briefPath(plan, verifyKey ?? ''), 'utf8'), + ).toContain(`--fetched-sha ${sha64}`); + } finally { + rmSync(dir, { recursive: true, force: true }); + gitIsolation.dispose(); + } + }, + ); + it('builds and records the whole 3A roster', () => { const dir = mkdtempSync(join(tmpdir(), 'ap-roster-')); try { @@ -2839,6 +2966,42 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { expect( buildRoleBrief(PR_PLAN, 'verify', { key: 'verify; rm -rf /' }), ).toContain('--label verify__rm_-rf__'); + // The plan's fetched sha rides along when the plan carries a usable one: + // it is the shared-tree residue check's identity anchor, and without it + // the check would refuse every healthy run (#9742). Absent or malformed, + // nothing is welded — the record-less refusal is the fail-closed shape. + const sha = 'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef'; + // Pin the JOINED fragment, not the bare flag: without the continuation + // after `--label` the snippet is two statements — the command runs + // unpinned and the sha line dies as command-not-found — while + // `toContain('--fetched-sha …')` still passes. + expect( + buildRoleBrief({ ...PR_PLAN, fetchedSha: sha }, 'verify', { + key: 'verify--round-2--deadbeef1234', + }), + ).toContain( + `--label verify--round-2--deadbeef1234 \\ + --fetched-sha ${sha}`, + ); + // A SHA-256 repository records a 64-hex commit; the pipeline's own + // shape contract admits both full object-ID lengths, so that record + // welds in too — a validator that only matched 40 hex would leave + // every SHA-256 review's command unpinned. + const sha256 = 'ab'.repeat(32); + expect( + buildRoleBrief({ ...PR_PLAN, fetchedSha: sha256 }, 'verify', { + key: 'verify--round-2--deadbeef1234', + }), + ).toContain(`--fetched-sha ${sha256}`); + // And the sha-less brief must not carry a continuation after the label + // either — a dangling one would glue the closing fence onto the command. + expect(p).not.toMatch(/--label verify--round-2--deadbeef1234 \\/); + expect(p).not.toContain('--fetched-sha'); + expect( + buildRoleBrief({ ...PR_PLAN, fetchedSha: 'not-a-sha' }, 'verify', { + key: 'verify--round-2--deadbeef1234', + }), + ).not.toContain('--fetched-sha'); // No worktree, no scratch tree — a local or cross-repo review has no // pristine sibling to build, and HEAD is not what is under review there. expect(buildRoleBrief(PLAN, 'verify')).not.toContain('review scratch-tree'); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index a43402340d..03243b837a 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -99,7 +99,11 @@ import { SHA_RE } from './lib/ledger.js'; import { pathRulesFor } from './lib/path-rules.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { inertPath, scratchLabel } from './lib/paths.js'; -import { worktreeResidue, type WorktreeResidue } from './lib/worktree.js'; +import { + RESIDUE_PATH_CAP, + worktreeResidue, + type WorktreeResidue, +} from './lib/worktree.js'; import { isTerritoryFanOut, requiredAgents, @@ -151,6 +155,8 @@ interface PlanReport { prNumber?: unknown; ownerRepo?: unknown; worktreePath?: unknown; + /** The PR head sha fetch-pr recorded — the probe's identity anchor. */ + fetchedSha?: unknown; mergeBaseSha?: unknown; host?: unknown; repositoryContext?: unknown; @@ -1316,6 +1322,27 @@ function repositoryContextBlock(context: RepositoryContext): string[] { ]; } +/** + * The plan's fetched head sha when it carries a usable one. Absent or + * malformed answers nothing rather than a broken anchor: every worktree-mode + * fetch writes the field, so both call sites fail closed on that absence, + * each in its own way. + * + * A usable one is a FULL Git object ID: 40 hex for SHA-1 repositories and + * 64 for SHA-256 ones — fetch-pr records `git rev-parse` verbatim, and the + * pipeline's own shape contract admits both lengths (pr-context's + * COMMIT_SHA_RE carries its {40,64} breadth for exactly that class). A + * validator matching only the SHA-1 length would drop the record every + * SHA-256 review writes, failing closed as though the plan were tampered + * with and welding an unpinned scratch-tree command. + */ +function fetchedShaOf(report: PlanReport): string | undefined { + const sha = report.fetchedSha; + return typeof sha === 'string' && /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(sha) + ? sha + : undefined; +} + /** * The review worktree's residue, or nothing at all when there is no worktree to * have any. Resolved against the process cwd, like every other use of @@ -1325,7 +1352,28 @@ function repositoryContextBlock(context: RepositoryContext): string[] { function worktreeResidueOf(report: PlanReport): WorktreeResidue { const wt = report.worktreePath; if (typeof wt !== 'string' || !wt) return { paths: [], total: 0 }; - return worktreeResidue(resolve(wt)); + // Hand over the sha fetch-pr recorded: committing the contamination moves + // a forge's HEAD off it, so with it the probe refuses a forged admin entry + // (see worktreeResidue). The record raises the plant's cost; it does not + // make planting impossible — it is re-read from the plan file at every + // invocation, and a same-user writer can rewrite it along with the forge. + // Absent or malformed it fails CLOSED: every worktree-mode fetch writes + // the field, so a plan that names a worktree without it is tampered or + // corrupted, and measuring unpinned would certify whichever index the + // gitfile names. + const sha = fetchedShaOf(report); + if (sha === undefined) { + return { + paths: [], + total: 0, + unmeasured: + 'the plan carries no usable record of the fetched head sha — ' + + 'every worktree-mode fetch writes one, so its absence means ' + + 'tampering or corruption, and measuring without it would certify ' + + 'whichever index the .git gitfile names', + }; + } + return worktreeResidue(resolve(wt), RESIDUE_PATH_CAP, sha); } /** @@ -1380,7 +1428,7 @@ function worktreeEvidenceBlock( if (residue?.unmeasured) { parts.push( '', - `**Whether it is clean could not be measured** (\`git status\` failed: ` + + `**Whether it is clean could not be measured** (reason: ` + `${inertPath(residue.unmeasured)}). That is not the same as clean: treat ` + 'anything that surprises you in this tree as unverified until you have ' + 'checked it against `git show HEAD:`.', @@ -1649,6 +1697,11 @@ export function buildRoleBrief( // written into a shell command, and the one function that decides the // tree's name is also what keeps a metacharacter out of that command. const label = scratchLabel(opts.key ?? role); + // The identity anchor fetch-pr recorded, when the plan carries a usable + // one: with it the probe pins the shared tree and a healthy run measures + // clean — without it the no-record refusal fires on every run, and a + // tampering note that fires always is a note nobody reads. + const sha = fetchedShaOf(report); parts.push( '', '**Your scratch tree — where every probe, mutant and candidate fix goes.** ' + @@ -1667,7 +1720,8 @@ export function buildRoleBrief( // a bare interpolation, and the failure would be silent — every shard's // scratch tree unavailable, every probe demoted to a reading. `"\${QWEN_CODE_CLI:-qwen}" review scratch-tree --worktree ${shellQuotePath(resolve(wt))} \\`, - ` --label ${label}`, + ` --label ${label}${sha === undefined ? '' : ' \\'}`, + ...(sha === undefined ? [] : [` --fetched-sha ${sha}`]), '```', '', 'It reports `path` — work there, and leave what you leave: `cleanup` sweeps ' + @@ -3212,7 +3266,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { const residue = worktreeResidueOf(report); if (residue.unmeasured) { writeStderrLine( - `warning: could not measure whether the review worktree is clean (git status failed: ` + + `warning: could not measure whether the review worktree is clean (reason: ` + `${inertPath(residue.unmeasured)}). Every brief built by this call says so; an unmeasured tree is ` + 'not a clean one.', ); diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts index db899feba6..9f0c273363 100644 --- a/packages/cli/src/commands/review/lib/worktree.test.ts +++ b/packages/cli/src/commands/review/lib/worktree.test.ts @@ -15,18 +15,21 @@ import { execFileSync } from 'node:child_process'; import { appendFileSync, chmodSync, + copyFileSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, + realpathSync, + renameSync, rmSync, symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; -import { dirname, join } from 'node:path'; +import { basename, dirname, join } from 'node:path'; import { isolateHostGitConfig } from './test-utils.js'; import { discardWorktree, @@ -59,7 +62,7 @@ describe('worktreeResidue', () => { beforeEach(() => { gitIsolation = isolateHostGitConfig(); - repo = mkdtempSync(join(tmpdir(), 'qwen-residue-')); + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-residue-'))); gitRepo('init', '-q', '-b', 'main'); gitRepo('config', 'user.email', 't@t.t'); gitRepo('config', 'user.name', 't'); @@ -78,8 +81,44 @@ describe('worktreeResidue', () => { gitIsolation.dispose(); }); + // A completely genuine forge territory — `git init` with the + // contamination committed, plus a linked worktree of it — built outside the + // repo. Both redirect tests stand one up before planting their divergent + // links; one builder means an isolation fix lands once, not once per test. + const forgeTerritory = (outside: string, wtName: string) => { + const forgeRepo = join(outside, 'forge'); + mkdirSync(forgeRepo); + const fgit = (...args: string[]) => + execFileSync( + 'git', + [ + '-c', + 'user.email=t@t.t', + '-c', + 'user.name=t', + '-c', + 'commit.gpgsign=false', + ...args, + ], + { cwd: forgeRepo, encoding: 'utf8' }, + ); + fgit('init', '-q', '-b', 'main', '--template=', '.'); + writeFileSync(join(forgeRepo, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(forgeRepo, '__probe__.test.ts'), 'probe'); + fgit('add', '-A'); + fgit('commit', '-qm', 'the mutant, committed', '--no-verify'); + fgit('worktree', 'add', '--detach', '-q', join(outside, wtName), 'HEAD'); + return join(outside, wtName); + }; + it('is empty for the tree a review actually reads', () => { - expect(worktreeResidue(tree)).toEqual({ paths: [], total: 0 }); + const head = git('rev-parse', 'HEAD'); + expect(worktreeResidue(tree, 12, head)).toEqual({ paths: [], total: 0 }); + // Unpinned, the same empty measurement is refused, not certified: a + // forged pair answers clean too, and nothing local tells the two apart + // (#9557) — so a caller without the fetched sha gets unmeasured, never + // clean. + expect(worktreeResidue(tree).unmeasured).toContain('brought no record'); }); it('names a modified file and an untracked probe — the live #9207 shape', () => { @@ -97,7 +136,10 @@ describe('worktreeResidue', () => { mkdirSync(join(tree, 'node_modules', 'vitest'), { recursive: true }); mkdirSync(join(tree, 'dist'), { recursive: true }); writeFileSync(join(tree, 'dist', 'out.js'), 'built\n'); - expect(worktreeResidue(tree)).toEqual({ paths: [], total: 0 }); + expect(worktreeResidue(tree, 12, git('rev-parse', 'HEAD'))).toEqual({ + paths: [], + total: 0, + }); }); it('reports BOTH names of a rename — the restore needs the one that is gone', () => { @@ -182,7 +224,294 @@ describe('worktreeResidue', () => { } // A clean tree carries no reason — that is what makes the two states // distinguishable at the renderers. - expect(worktreeResidue(tree).unmeasured).toBeUndefined(); + expect( + worktreeResidue(tree, 12, git('rev-parse', 'HEAD')).unmeasured, + ).toBeUndefined(); + }); + + it('says UNMEASURED for a gitfile swapped at a repo that answers for this path', () => { + // The identity gate reads `--show-toplevel`, which prints the directory the + // `.git` FILE sits in — whatever that file points at. A repository whose + // `core.worktree` names this tree answers with this path, so the gate saw + // itself while every command after it would measure the plant's index. + // Measured in round 1: through discovery the swap certified a mutant + // clean. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + // Genuine first, so the fixture is known to be measurable at all. + expect(worktreeResidue(tree).paths.sort()).toEqual([ + '__probe__.test.ts', + 'a.ts', + ]); + + gitRepo('config', 'core.worktree', tree); + writeFileSync(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + // The shape with NO admin entry gets its own reason: a main checkout has + // no `gitdir` file to "not point back", and the triager hunting one is + // the confusion the distinct message exists to spare. + expect(got.unmeasured).toContain('no admin entry'); + }); + + it('says UNMEASURED for a forged admin entry when the caller pins the expected head', () => { + // The round trip proves only that the admin entry the gitfile names SAYS + // this tree is its worktree — and a same-user planter writes both halves + // of the pair: a repo carrying the contamination as committed content, + // and an admin entry whose `gitdir` file is hand-written to name this + // tree (four small writes). The gate then passes end-to-end and the pin + // measures the forge's index. Measured: without the caller's anchor the + // swap below answers clean with the mutant on disk. The anchor is the + // one thing the forge cannot reproduce — committing the contamination + // moves its HEAD off the fetched sha. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + const expected = git('rev-parse', 'HEAD'); + // A genuine tree with the right sha still measures: the anchor must not + // become a refusal of its own — in either case, the caller's guard + // admits an uppercase sha and the pin folds case on BOTH sides. + expect(worktreeResidue(tree, 12, expected).paths.sort()).toEqual([ + '__probe__.test.ts', + 'a.ts', + ]); + expect( + worktreeResidue(tree, 12, expected.toUpperCase()).paths.sort(), + ).toEqual(['__probe__.test.ts', 'a.ts']); + + // The forge: the contamination committed into the REAL repository — its + // HEAD moves off the fetched sha — and an admin entry hand-written + // beside the tree's own. Same common dir, so every shape check passes; + // only the pin can still tell the entry from the one `worktree add` + // wrote. + writeFileSync(join(repo, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(repo, '__probe__.test.ts'), 'probe'); + gitRepo('add', 'a.ts', '__probe__.test.ts'); + gitRepo('commit', '-qm', 'the mutant, as if it were the commit'); + const forgedHead = gitRepo('rev-parse', 'HEAD'); + const admin = join(repo, '.git', 'worktrees', 'evil'); + mkdirSync(admin, { recursive: true }); + writeFileSync(join(admin, 'gitdir'), `${join(tree, '.git')}\n`); + writeFileSync(join(admin, 'commondir'), '../..\n'); + writeFileSync(join(admin, 'HEAD'), `${forgedHead}\n`); + copyFileSync(join(repo, '.git', 'index'), join(admin, 'index')); + writeFileSync(join(tree, '.git'), `gitdir: ${admin}\n`); + + // Unpinned, the forge's index answers clean — and an unanchored clean + // verdict is exactly the one the probe refuses (#9557). + const unpinned = worktreeResidue(tree); + expect(unpinned.paths).toEqual([]); + expect(unpinned.unmeasured).toContain('brought no record'); + + // Pinned to the fetched sha: the forge's HEAD is the mutant's commit. + const pinned = worktreeResidue(tree, 12, expected); + expect(pinned.paths).toEqual([]); + expect(pinned.unmeasured).toContain('not the fetched PR head'); + + // And a pinned identity whose HEAD cannot be read gets its own reason — + // the gate passed, so "not a git worktree" would misname it. An unborn + // HEAD (a ref to a branch with no commit) keeps discovery alive while + // the pinned `rev-parse HEAD` fails — a garbage HEAD file would fail + // discovery itself and land in the outer catch instead. + writeFileSync(join(admin, 'HEAD'), 'ref: refs/heads/nope\n'); + expect(worktreeResidue(tree, 12, expected).unmeasured).toContain( + 'could not read its own HEAD', + ); + }); + + it('says UNMEASURED for a gitfile borrowing a SIBLING worktree’s admin entry', () => { + // The mismatch arm of the round trip: a real admin entry — a sibling's — + // whose `gitdir` file names the sibling's `.git`, not this tree's. + // `--show-toplevel` prints the directory the gitfile sits in, so the + // self-equality passes while the round trip catches the borrow. The arm + // needs its own witness: negating the comparison ships green without this + // test — measured, the gate then passes and certifies a tree measured + // against the sibling's index. + const sibling = join(repo, '.qwen', 'tmp', 'sibling-wt'); + gitRepo('worktree', 'add', '--detach', '-q', sibling, 'HEAD'); + const admin = readFileSync(join(sibling, '.git'), 'utf8') + .trim() + .replace(/^gitdir:\s*/, ''); + writeFileSync(join(tree, '.git'), `gitdir: ${admin}\n`); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('does not point back'); + }); + + it('says UNMEASURED — not "not a git worktree" — for a dangling backpointer', () => { + // The admin entry's `gitdir` file names a path that does not exist — a + // crash mid-`worktree add`, a cleanup gone wrong, a sloppy forge. `git + // rev-parse` still exits 0 in that state, so the path IS a worktree with + // an admin entry; an ENOENT out of the round-trip comparison must not + // land in the outer catch and be reported as the much vaguer "not a git + // worktree". Unresolvable is "does not point back" — same refusal. + const admin = readFileSync(join(tree, '.git'), 'utf8') + .trim() + .replace(/^gitdir:\s*/, ''); + writeFileSync(join(admin, 'gitdir'), `${join(repo, 'gone', '.git')}\n`); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('does not point back'); + }); + + it('accepts a backpointer spelled through a link that resolves at this tree', () => { + // The round trip's LEFT side is attacker-written, so its normalisation is + // load-bearing: a `gitdir` file spelled through a link that RESOLVES at + // this tree's `.git` does point back at this tree, and refusing it would + // fail closed on a shape that names the right tree. Measured: removing + // the realpathSync from the comparison flips this probe from clean to + // unmeasured — the witness that a spelling and a resolution are being + // compared, not two spellings. + const alias = join(repo, 'alias'); + symlinkSync(tree, alias); + const admin = readFileSync(join(tree, '.git'), 'utf8') + .trim() + .replace(/^gitdir:\s*/, ''); + writeFileSync(join(admin, 'gitdir'), `${join(alias, '.git')}\n`); + + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + const got = worktreeResidue(tree, 12, git('rev-parse', 'HEAD')); + + expect(got.unmeasured).toBeUndefined(); + expect(got.paths).toEqual(['__probe__.test.ts']); + }); + + it('says UNMEASURED when an ancestor of the tree is a symlink into forge territory', () => { + // A link planted at any ancestor below the checkout — here `.qwen/tmp`, + // the directory the pipeline itself names — redirects the chdir into + // territory holding a completely genuine `git init` + `worktree add` + // pair with the contamination COMMITTED: no forged admin entry, the + // round trip is real git state, and every check resolves THROUGH the + // link and agrees with itself. Measured: the redirect certified the + // mutant clean before the walk. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + expect(worktreeResidue(tree).paths.sort()).toEqual([ + '__probe__.test.ts', + 'a.ts', + ]); + + const outside = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-redirect-'))); + try { + forgeTerritory(outside, 'review-wt'); + + // The attack: the ancestor becomes a link into that territory. + rmSync(dirname(tree), { recursive: true, force: true }); + symlinkSync(outside, dirname(tree)); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + // The walk refuses it: the territory's common dir is no ancestor of + // the spelled path, so the walk lstats every component up to the + // root and finds the planted link on the way. + expect(got.unmeasured).toContain('resolves through a symlink'); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it('measures a healthy tree spelled through a symlink ABOVE the repository root', () => { + // The containment gate holds the caller's spelling against git's + // PHYSICAL common dir, so a checkout reached through a link above the + // repository — `/tmp` on every macOS box, a linked home — failed the + // literal test and was refused on every run: the note nobody reads, on + // a shape the walk deliberately does not look at (above the root is the + // user's own layout, not anything a probe can plant). The resolution is + // contained, so the healthy shape must measure. + const head = git('rev-parse', 'HEAD'); + const aliasHome = mkdtempSync(join(tmpdir(), 'qwen-spell-')); + const alias = join(aliasHome, 'alias'); + symlinkSync(dirname(repo), alias); + const spelled = join(alias, basename(repo), '.qwen', 'tmp', 'review-wt'); + try { + expect(worktreeResidue(spelled, 12, head)).toEqual({ + paths: [], + total: 0, + }); + // And the measurement is the tree's, not the spelling's: residue + // written at the physical path is named through the alias. + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + expect(worktreeResidue(spelled, 12, head).paths).toEqual([ + '__probe__.test.ts', + ]); + } finally { + rmSync(aliasHome, { recursive: true, force: true }); + } + }); + + it('says UNMEASURED when an INTERMEDIATE ancestor is a symlink the earlier gates cannot see', () => { + // The walk's own witness: the sibling redirect shape refuses at the + // walk itself (its common dir is no ancestor, so the walk climbs to + // the root and meets the link), the leaf-link shape refuses at the + // leaf lstat, and deleting the walk turns the redirect test red — + // before the containment refusal was removed it shipped green + // (measured). This shape passes every gate above it: the leaf is a + // real directory, the self-equality holds because both sides resolve + // through the same link, and the moved tree's gitfile still names the + // REAL repo's admin entry, so the common dir is the repo and the + // tree's literal path runs under it. Only the walk can refuse it. + expect(worktreeResidue(tree, 12, git('rev-parse', 'HEAD'))).toEqual({ + paths: [], + total: 0, + }); + + const outside = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-walk-'))); + try { + // Move the worktree out and plant a link at its parent pointing after + // it. The moved tree keeps naming its original admin entry — spelled + // absolutely, because its old relative spelling no longer resolves from + // outside the repo. + renameSync(tree, join(outside, 'review-wt')); + writeFileSync( + join(outside, 'review-wt', '.git'), + `gitdir: ${join(repo, '.git', 'worktrees', 'review-wt')}\n`, + ); + rmSync(dirname(tree), { recursive: true, force: true }); + symlinkSync(outside, dirname(tree)); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('resolves through a symlink'); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it('says UNMEASURED when the tree path itself is a symlink into forge territory', () => { + // The same concealment one hop closer: the LEAF replaced by a link. + // `spawnSync` chdirs through it, `--show-toplevel` answers the physical + // forge path, and both sides of the self-equality resolve through the + // same link, while the pin would freeze the forge's identity. Measured: + // the redirect certified the mutant clean before the leaf check. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + writeFileSync(join(tree, '__probe__.test.ts'), 'probe'); + expect(worktreeResidue(tree).paths.sort()).toEqual([ + '__probe__.test.ts', + 'a.ts', + ]); + + const outside = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-redirect-'))); + try { + const forgedTree = forgeTerritory(outside, 'leaf-wt'); + + // The attack: the leaf becomes a link into that territory. + rmSync(tree, { recursive: true, force: true }); + symlinkSync(forgedTree, tree); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('resolves through a symlink'); + } finally { + rmSync(outside, { recursive: true, force: true }); + } }); it('says UNMEASURED, not clean, when a repository is planted at the path', () => { @@ -270,7 +599,7 @@ describe('worktreeResidue', () => { const fresh = join(repo, 'nested', 'wt-sub'); gitRepo('worktree', 'add', '--detach', '-q', fresh, 'HEAD'); - const got = worktreeResidue(fresh); + const got = worktreeResidue(fresh, 12, gitRepo('rev-parse', 'HEAD')); expect(got.unmeasured).toBeUndefined(); expect(got).toEqual({ paths: [], total: 0 }); }); @@ -329,7 +658,7 @@ describe('worktreeResidue', () => { const nested = join(repo, 'nested', 'wt'); git('worktree', 'add', '--detach', '-q', nested, 'HEAD'); writeFileSync(join(nested, '__probe__.test.ts'), 'x'); - const healthy = worktreeResidue(nested); + const healthy = worktreeResidue(nested, 12, git('rev-parse', 'HEAD')); expect(healthy.unmeasured).toBeUndefined(); expect(healthy.paths).toEqual(['__probe__.test.ts']); }); @@ -367,7 +696,7 @@ describe('worktreeResidue', () => { // The blindness this closes: `status` exits 0 with zero bytes. expect(git('status', '--porcelain', '--untracked-files=all')).toBe(''); - const got = worktreeResidue(tree); + const got = worktreeResidue(tree, 12, git('rev-parse', 'HEAD')); expect(got.paths).toEqual(['__probe__.test.ts']); expect(got.total).toBe(1); expect(got.unmeasured).toBeUndefined(); @@ -402,7 +731,7 @@ describe('worktreeResidue', () => { // ...and one real leftover standing in the middle of all of it. writeFileSync(join(tree, '__probe__.test.ts'), 'x'); - const got = worktreeResidue(tree); + const got = worktreeResidue(tree, 12, git('rev-parse', 'HEAD')); expect(got.paths).toEqual(['__probe__.test.ts']); expect(got.total).toBe(1); @@ -458,7 +787,7 @@ describe('worktreeResidue', () => { // The blindness this closes: `status` exits 0 with zero bytes. expect(git('status', '--porcelain', '--untracked-files=all')).toBe(''); - const got = worktreeResidue(tree); + const got = worktreeResidue(tree, 12, git('rev-parse', 'HEAD')); expect(got.paths.sort()).toEqual([ 'probe_dir/.gitignore', @@ -502,6 +831,152 @@ describe('worktreeResidue', () => { expect(got.total).toBe(1); }, ); + + it('says UNMEASURED for a sha-less caller even when a dirty decoy is present', () => { + // The no-record refusal cannot be conditional on the measured list being + // empty: a forged pair can commit the contamination and leave an + // unrelated untracked decoy, and the decoy alone is what the + // measurement then reports — the committed contamination is by + // construction absent from any residue list. Dirty paths still point at + // the tree either way, so they are kept for diagnostics; the clean + // certificate is what the unanchored identity forfeits. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2; // MUTANT\n'); + git('add', 'a.ts'); + git('commit', '-qm', 'the mutant, committed'); + writeFileSync(join(tree, 'dirty-decoy.txt'), 'decoy\n'); + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual(['dirty-decoy.txt']); + expect(got.total).toBe(1); + expect(got.unmeasured).toContain('brought no record'); + }); + + // Windows filesystems refuse a `\n` inside a name, so the fixture the + // misparse needs cannot exist there — the same convention as the other + // POSIX-only shapes in this suite. + it.skipIf(process.platform === 'win32')( + 'measures a worktree below a directory whose name carries a newline', + () => { + // The discovery answers are three arbitrary filesystem paths, so a + // newline-delimited parse of one combined answer misreads any + // directory that carries one: extra records, misassigned + // gitdir/commondir, and a genuine worktree reported as not one. Each + // value gets its own query. + const home = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-nl-'))); + try { + const nlRepo = join(home, 'dir\nwith-newline', 'repo'); + mkdirSync(nlRepo, { recursive: true }); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: nlRepo }); + execFileSync('git', ['config', 'user.email', 't@t.t'], { + cwd: nlRepo, + }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: nlRepo }); + writeFileSync(join(nlRepo, 'a.ts'), 'x\n'); + execFileSync('git', ['add', '-A'], { cwd: nlRepo }); + execFileSync('git', ['commit', '-qm', 'head'], { cwd: nlRepo }); + const wt = join(nlRepo, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(wt), { recursive: true }); + execFileSync('git', ['worktree', 'add', '--detach', '-q', wt, 'HEAD'], { + cwd: nlRepo, + }); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: wt, + encoding: 'utf8', + }).trim(); + + expect(worktreeResidue(wt, 12, head)).toEqual({ + paths: [], + total: 0, + }); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }, + ); + + it('measures a review worktree under a checkout that is itself a linked worktree', () => { + // `fetch-pr` creates the review worktree from the process cwd with no + // main-checkout requirement, so the cwd may itself be a linked worktree: + // the review tree's common dir then belongs to the MAIN checkout, whose + // parent is a sibling of the tree's path, not an ancestor. The identity + // checks — round trip, sha pin, the symlink walk — all hold that shape, + // so the measurement must answer rather than refuse. + const home = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-sib-'))); + try { + const main = join(home, 'main'); + mkdirSync(main); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: main }); + execFileSync('git', ['config', 'user.email', 't@t.t'], { cwd: main }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: main }); + writeFileSync(join(main, 'a.ts'), 'x\n'); + execFileSync('git', ['add', '-A'], { cwd: main }); + execFileSync('git', ['commit', '-qm', 'head'], { cwd: main }); + const sib = join(home, 'sib'); + execFileSync('git', ['worktree', 'add', '--detach', '-q', sib, 'HEAD'], { + cwd: main, + }); + const wt = join(sib, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(wt), { recursive: true }); + execFileSync('git', ['worktree', 'add', '--detach', '-q', wt, 'HEAD'], { + cwd: sib, + }); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: wt, + encoding: 'utf8', + }).trim(); + + expect(worktreeResidue(wt, 12, head)).toEqual({ paths: [], total: 0 }); + // And the measurement is the tree's: residue written there is named. + writeFileSync(join(wt, '__probe__.test.ts'), 'probe'); + expect(worktreeResidue(wt, 12, head).paths).toEqual([ + '__probe__.test.ts', + ]); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + it('measures a review worktree of a --separate-git-dir checkout', () => { + // In the layout `git init --separate-git-dir` creates, the common dir + // intentionally lives outside the checkout, so its parent is no + // ancestor of the review tree's path — a supported repository shape the + // probe must measure, not refuse. + const home = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-sep-'))); + try { + const checkout = join(home, 'checkout'); + const gitDir = join(home, 'elsewhere', 'repo.git'); + mkdirSync(join(home, 'elsewhere')); + execFileSync('git', [ + 'init', + '-q', + '-b', + 'main', + `--separate-git-dir=${gitDir}`, + checkout, + ]); + execFileSync('git', ['config', 'user.email', 't@t.t'], { + cwd: checkout, + }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: checkout }); + writeFileSync(join(checkout, 'a.ts'), 'x\n'); + execFileSync('git', ['add', '-A'], { cwd: checkout }); + execFileSync('git', ['commit', '-qm', 'head'], { cwd: checkout }); + const wt = join(checkout, '.qwen', 'tmp', 'review-wt'); + mkdirSync(dirname(wt), { recursive: true }); + execFileSync('git', ['worktree', 'add', '--detach', '-q', wt, 'HEAD'], { + cwd: checkout, + }); + const head = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: wt, + encoding: 'utf8', + }).trim(); + + expect(worktreeResidue(wt, 12, head)).toEqual({ paths: [], total: 0 }); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); }); describe('worktreeResidue — the blind sets', () => { @@ -512,7 +987,7 @@ describe('worktreeResidue — the blind sets', () => { beforeEach(() => { gitIsolation = isolateHostGitConfig(); - repo = mkdtempSync(join(tmpdir(), 'qwen-blind-')); + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-blind-'))); git(repo, 'init', '-q', '-b', 'main'); git(repo, 'config', 'user.email', 't@t.t'); git(repo, 'config', 'user.name', 't'); @@ -626,7 +1101,7 @@ describe('worktreeResidue — index bits', () => { beforeEach(() => { gitIsolation = isolateHostGitConfig(); - repo = mkdtempSync(join(tmpdir(), 'qwen-bits-')); + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-bits-'))); git(repo, 'init', '-q', '-b', 'main'); git(repo, 'config', 'user.email', 't@t.t'); git(repo, 'config', 'user.name', 't'); diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts index d57edd0016..13a122da70 100644 --- a/packages/cli/src/commands/review/lib/worktree.ts +++ b/packages/cli/src/commands/review/lib/worktree.ts @@ -130,6 +130,8 @@ export function redirectedAncestor( // never matches and the walk climbs past the checkout into exactly the // system links this is not about. The symlink test above it stays lstat — // canonicalising THAT would resolve away the thing being looked for. + // Where the stop is no ancestor of the walk's path neither stop test + // fires and the walk lstats every component up to the filesystem root. const stop = resolve(stopAt); let stopReal = stop; try { @@ -474,10 +476,18 @@ interface IgnoreRule { function ignoreSourcesOf( cwd: string, paths: string[], + anchor: readonly string[] = [], ): Map | null { const r = spawnSync( 'git', - [...pipelineExcludeArgs(), 'check-ignore', '-z', '-v', '--stdin'], + [ + ...anchor, + ...pipelineExcludeArgs(), + 'check-ignore', + '-z', + '-v', + '--stdin', + ], { cwd, input: `${paths.join('\0')}\0`, @@ -539,13 +549,17 @@ function hidesEverything(pattern: string): boolean { * out of the worktree is not asked about at all — `ls-files` would reject the * pathspec, and unknown provenance is untrusted provenance. */ -function trackedIgnoreSources(cwd: string, sources: Set): Set { +function trackedIgnoreSources( + cwd: string, + sources: Set, + anchor: readonly string[] = [], +): Set { const inside = [...sources].filter( (s) => s.length > 0 && !isAbsolute(s) && !s.split('/').some((p) => p === '..'), ); if (inside.length === 0) return new Set(); - const r = spawnSync('git', ['ls-files', '-z', '--', ...inside], { + const r = spawnSync('git', [...anchor, 'ls-files', '-z', '--', ...inside], { cwd, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024, @@ -557,6 +571,13 @@ function trackedIgnoreSources(cwd: string, sources: Set): Set { return new Set(r.stdout.split('\0').filter((p) => p.length > 0)); } +/** + * The residue list's default cap, exported so a caller that must name it — + * the only way to reach the sha parameter after it — names THIS module's + * default instead of restating a literal that could silently drift from it. + */ +export const RESIDUE_PATH_CAP = 12; + /** * The paths a tree carries that its HEAD commit does not — probe residue, seen * from the reading side (#9207). @@ -608,7 +629,30 @@ function trackedIgnoreSources(cwd: string, sources: Set): Set { * contamination under a clean status — because no local check can tell a * planted repo from the tree it replaced: a genuine review worktree holds its * `.git` as a gitFILE naming its admin entry, and anything else is refused as - * unmeasured rather than certified clean. + * unmeasured rather than certified clean. Two further shapes fail closed on + * the same principle. A symlink at the tree path or any ancestor redirects + * every check into territory holding a completely genuine worktree pair + * with the contamination committed — all of it resolving through the link + * and agreeing with itself — so a path reached through a link is refused. + * The walk that finds one stops at the repository the common dir belongs to + * where that contains the tree; where it does not — a review worktree under + * a checkout that is itself a linked worktree, a `--separate-git-dir` + * clone — git puts no constraint on where a linked worktree lives, so the + * walk lstats every component up to the filesystem root instead, and the + * round trip and the sha pin below hold the layout. And a FORGED admin + * entry — hand-written to name this tree back, which the round trip + * cannot tell from the entry `worktree add` wrote — is refused when the + * caller supplies the commit the tree must hold: + * a forge carrying the contamination as committed content cannot also + * reproduce the fetched head sha, so a pinned `rev-parse HEAD` disagreeing + * with the caller's record is a refusal. The record arrives read from disk, + * so it raises the plant's cost rather than making planting impossible. + * Callers without any record still get residue NAMED — a forge answers + * clean, never dirty, so dirty readings still point at the tree — but + * never a verdict: without the record nothing separates the tree from a + * forged pair whose index already holds the contamination as committed + * content, so the measurement is refused as unmeasured, the paths + * retained for the reader to act on. * * One blind spot the identity checks cannot close: `git status` never looks * INSIDE a committed gitlink (mode 160000), and untracked content there does @@ -624,7 +668,11 @@ function trackedIgnoreSources(cwd: string, sources: Set): Set { * resolves on disk. No string form of it can — Node's fs API takes strings here * — so the name is disclosed as git rendered it rather than silently dropped. */ -export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { +export function worktreeResidue( + cwd: string, + cap = RESIDUE_PATH_CAP, + expectedHeadSha?: string, +): WorktreeResidue { // A genuine review worktree carries its `.git` as a FILE naming its admin // entry. A `.git` DIRECTORY at this path is a repository planted over the // contamination — `git init` + a commit answers a clean `git status` for a @@ -652,20 +700,209 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { // add`, a cleanup whose `rmSync` failed — `status` exits 0 against the // enclosing user checkout: the wrong tree's dirty state answered as this // one's. Fail closed the way a loud git failure below does. - const top = spawnSync('git', ['rev-parse', '--show-toplevel'], { - cwd, - encoding: 'utf8', - env: sanitizedGitEnv(), - }); + // One invocation per value: the answers are three arbitrary filesystem + // paths, and a POSIX name may carry a newline, so no combined + // newline-delimited answer can be split unambiguously — a healthy + // worktree below a directory whose name holds one parses to extra + // records, misassigns gitDir/commondir, and reports the checkout as not + // a worktree. Measured with exactly such a directory. + const discover = (flag: string): string | null => { + const r = spawnSync('git', ['rev-parse', '--path-format=absolute', flag], { + cwd, + encoding: 'utf8', + env: sanitizedGitEnv(), + }); + if (r.error || r.status !== 0 || typeof r.stdout !== 'string') { + return null; + } + // Remove only git's terminal record delimiter: every other byte + // belongs to the path, so neither a split nor a trim is a parse here. + return r.stdout.endsWith('\n') ? r.stdout.slice(0, -1) : r.stdout; + }; + const toplevel = discover('--show-toplevel'); + const gitDir = discover('--git-dir'); + const commonDir = discover('--git-common-dir'); let isWorktree = false; + let anchor: string[] = []; try { - isWorktree = - !top.error && - top.status === 0 && - typeof top.stdout === 'string' && - realpathSync(top.stdout.trim()) === realpathSync(cwd); + if ( + toplevel !== null && + gitDir !== null && + commonDir !== null && + realpathSync(toplevel) === realpathSync(cwd) + ) { + isWorktree = true; + // First, the leaf itself: a symlink AT the tree path redirects the + // chdir and every check after it. lstat, not realpath — canonicalising + // would resolve away the thing being looked for. + if (lstatSync(cwd).isSymbolicLink()) { + return { + paths: [], + total: 0, + unmeasured: + `the path resolves through a symlink (${resolve(cwd)}) — ` + + 'every identity check resolves through it and agrees with ' + + 'itself, while the commands below would measure wherever it ' + + 'points', + }; + } + // No component of the path may be a symlink: a link planted at any + // ancestor redirects the chdir into territory holding a completely + // genuine `git init` + `worktree add` pair with the contamination + // COMMITTED — no forged admin entry needed, the round trip below is + // real git state — and every check here resolves THROUGH the link + // and agrees with itself: `--show-toplevel` answers the physical + // forge path and the self-equality above holds. Measured: the shape + // certified a mutant clean before the walk. The walk's stop is the + // checkout the common dir belongs to — above that is the user's own + // layout, and `/var` is a symlink on every macOS box — and where the + // bound IS an ancestor of the tree path it lstats every component + // between them and stops there. Where it is NOT, the stop test never + // fires and the walk lstats every component up to the filesystem + // root instead: git puts no constraint on where a linked worktree + // lives — a review worktree under a checkout that is itself a linked + // worktree has the MAIN checkout's common dir, a sibling of its + // path, and a `--separate-git-dir` clone's lives wherever the user + // put it — so those layouts are held by the round trip and the sha + // pin below, not refused here. Refusing them on the bound alone was + // a false positive measured against both; the walk catches a + // redirect in either layout, and a steered bound buys a forge + // nothing the pin and the no-record refusal do not already cost it. + const spelled = resolve(cwd); + const bound = dirname(commonDir); + const redirected = redirectedAncestor(dirname(spelled), bound); + if (redirected !== null) { + return { + paths: [], + total: 0, + unmeasured: + `the path resolves through a symlink (${redirected}) — every ` + + 'identity check resolves through it and agrees with itself, ' + + 'while the commands below would measure wherever it points', + }; + } + // And the gitfile must name an admin entry that names this tree BACK. + // The pin below freezes THIS identity for the commands after it, so a + // gitfile swapped after this gate cannot redirect them; it cannot help + // when the swap is already in place when the probe starts, because then + // the gate resolves the plant too — a repository whose `core.worktree` + // points here answers `--show-toplevel` with this path. And it freezes + // NAMES, not what they point at: a writer active between the pin and + // the measurement can still rewrite the pinned admin entry's HEAD, + // index and commondir — or swap the tree path itself — and every + // "pinned" command measures the swap. The pin raises that attack's + // cost — the swap must now land inside one function's window — it does + // not close it; closing wants a snapshot measured at gate time or a + // sandbox boundary (#9556). `scratch-tree` gates its own reset on the + // round-trip + // for the same reason, and a planted standalone repo has no admin entry + // to round-trip at all. The two shapes get distinct reasons: the + // standalone repo has no `gitdir` file to "not point back", and whoever + // triages the refusal would otherwise hunt for one that does not exist. + // Its own try: a plant has no `gitdir` file to read, and letting that + // ENOENT fall into the outer catch reported it as "not a git worktree", + // which is a different and much vaguer thing than what was found. + let backpointer: string | null = null; + try { + backpointer = readFileSync(join(gitDir, 'gitdir'), 'utf8').trim(); + } catch { + // No admin entry at all — a standalone repository answering for this + // path, which is exactly the shape being refused. + } + if (backpointer === null) { + return { + paths: [], + total: 0, + unmeasured: + 'the .git gitfile names a repository with no admin entry for ' + + 'this tree — a standalone repository answering for this path, ' + + 'whose index the commands below would measure', + }; + } + let pointsBack = false; + try { + pointsBack = + realpathSync(dirname(resolve(gitDir, backpointer))) === + realpathSync(cwd); + } catch { + // A backpointer that does not resolve does not point back at this + // tree. Letting the ENOENT fall into the outer catch reported the + // shape as "not a git worktree" — a different and much vaguer thing + // than what was found. + } + if (!pointsBack) { + return { + paths: [], + total: 0, + unmeasured: + 'the .git gitfile names an admin entry that does not point back ' + + 'at this tree — the commands below would measure whichever ' + + 'repository it does name', + }; + } + // PIN the identity this gate just verified, for every spawn below. + // Without it the gate is one-shot: each later command re-discovers the + // repository through the same `.git` file the check read, and that file + // is writable by anything running as this user — so a gitfile swapped in + // afterwards, pointing at a repo whose index already holds the + // contamination, answers a clean `status` for a dirty tree. Measured: + // through discovery the swap certifies a mutant clean deterministically, + // with no race; pinned, the same tree still reports ` M a.ts` and the + // untracked probe file. Measured too, because a WRONG pin would be worse + // than none: across a standalone checkout, a linked worktree, a + // superproject with an initialised submodule and a worktree reached + // through a symlinked ancestor, all five commands below return + // byte-identical output pinned and unpinned. + anchor = [ + `--git-dir=${realpathSync(gitDir)}`, + `--work-tree=${realpathSync(toplevel)}`, + ]; + // And the pinned identity must hold the commit the caller fetched, when + // the caller brings that record. The round trip above proves only that + // the admin entry the gitfile names SAYS this tree is its worktree — a + // same-user planter writes both halves of the pair, so a forged entry + // beside a repo carrying the contamination as committed content passes + // every local check and measures clean. The one thing the forge cannot + // reproduce is the fetched head sha: committing the contamination moves + // its HEAD. The record is only as anchored as the caller's read of it + // — a same-user writer who rewrites it feeds the pin the forge's own + // sha — so this is cost, not closure. Measured: through the forge the + // pinned call answers unmeasured, where round 1's unpinned probe + // certified a mutant clean; the sha-less probe refuses its own clean + // verdict for the same reason (see the end of this function). + if (expectedHeadSha !== undefined) { + const head = spawnSync('git', [...anchor, 'rev-parse', 'HEAD'], { + cwd, + encoding: 'utf8', + env: sanitizedGitEnv(), + }); + const got = + head.error || head.status !== 0 || typeof head.stdout !== 'string' + ? null + : head.stdout.trim(); + if ( + got === null || + got.toLowerCase() !== expectedHeadSha.toLowerCase() + ) { + return { + paths: [], + total: 0, + unmeasured: + got === null + ? 'the pinned identity could not read its own HEAD — the ' + + 'commands below would have measured a repository git ' + + 'cannot read' + : `the pinned identity is checked out at ${got}, not the ` + + `fetched PR head ${expectedHeadSha} — the gitfile names ` + + 'a repository answering for this path, and its index is ' + + 'what the commands below would measure', + }; + } + } + } } catch { // A cwd that no longer resolves is not a tree this probe can measure. + isWorktree = false; } if (!isWorktree) { return { @@ -679,6 +916,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { const r = spawnSync( 'git', [ + ...anchor, ...pipelineExcludeArgs(), // The measurement must not itself become the execution: `core.fsmonitor` // runs a command on `status`, and this tree's config is writable by @@ -749,7 +987,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { // from the name. const others = spawnSync( 'git', - ['-c', 'core.fsmonitor=', 'ls-files', '--others', '-z'], + [...anchor, '-c', 'core.fsmonitor=', 'ls-files', '--others', '-z'], { cwd, encoding: 'utf8', @@ -780,7 +1018,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { extras.push(rec); } if (extras.length > 0) { - const hiddenBy = ignoreSourcesOf(cwd, extras); + const hiddenBy = ignoreSourcesOf(cwd, extras, anchor); if (hiddenBy === null) { return { paths: paths.slice(0, cap), @@ -793,6 +1031,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { const fromTheCommit = trackedIgnoreSources( cwd, new Set([...hiddenBy.values()].map((rule) => rule.source)), + anchor, ); for (const rec of extras) { const rule = hiddenBy.get(rec); @@ -826,7 +1065,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { // gitlink is certified clean. const stage = spawnSync( 'git', - ['-c', 'core.fsmonitor=', 'ls-files', '-s', '-z'], + [...anchor, '-c', 'core.fsmonitor=', 'ls-files', '-s', '-z'], { cwd, encoding: 'utf8', @@ -884,7 +1123,7 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { // read as "no bits found" and fell through to a clean verdict. const bits = spawnSync( 'git', - ['-c', 'core.fsmonitor=', 'ls-files', '-v', '-z'], + [...anchor, '-c', 'core.fsmonitor=', 'ls-files', '-v', '-z'], { cwd, encoding: 'utf8', @@ -907,6 +1146,28 @@ export function worktreeResidue(cwd: string, cap = 12): WorktreeResidue { 'status` cannot see edits to the tracked files they cover', }; } + // Without the caller's record nothing above distinguishes this tree from + // a forged pair whose index already holds the contamination as committed + // content, so a measurement no record anchored is refused rather than + // certified — clean or dirty. The named paths are kept either way: a + // forge answers CLEAN, never dirty, so dirty readings still point at the + // tree and the reader can act on them. And the refusal cannot wait for + // an EMPTY measurement: a forged pair commits the contamination and + // leaves one unrelated untracked decoy, and the decoy alone is what the + // residue list then carries — the committed mutant is absent from any + // such list by construction. + if (expectedHeadSha === undefined) { + return { + paths: paths.slice(0, cap), + total: paths.length, + unmeasured: + 'the caller brought no record of the commit this tree must hold, ' + + 'and a status measured through an unanchored identity would ' + + 'certify whichever index the .git gitfile names — a forged pair ' + + 'answers clean — so the measurement is refused rather than ' + + 'certified', + }; + } return { paths: paths.slice(0, cap), total: paths.length }; } diff --git a/packages/cli/src/commands/review/scratch-tree.test.ts b/packages/cli/src/commands/review/scratch-tree.test.ts index ee0efd3756..1678af80e6 100644 --- a/packages/cli/src/commands/review/scratch-tree.test.ts +++ b/packages/cli/src/commands/review/scratch-tree.test.ts @@ -28,13 +28,19 @@ import { mkdirSync, mkdtempSync, readFileSync, + realpathSync, rmSync, symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, join } from 'node:path'; -import { runScratchTree, scratchTreeCommand } from './scratch-tree.js'; +import yargs, { type Argv } from 'yargs'; +import { + runScratchTree, + scratchTreeCommand, + type ScratchTreeArgs, +} from './scratch-tree.js'; import { scratchWorktreePath } from './lib/paths.js'; import { isolateHostGitConfig } from './lib/test-utils.js'; @@ -54,7 +60,7 @@ describe('runScratchTree', () => { beforeEach(() => { gitIsolation = isolateHostGitConfig(); - repo = mkdtempSync(join(tmpdir(), 'qwen-scratch-tree-')); + repo = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-scratch-tree-'))); git(repo, 'init', '-q', '-b', 'main'); git(repo, 'config', 'user.email', 't@t.t'); git(repo, 'config', 'user.name', 't'); @@ -695,7 +701,11 @@ describe('runScratchTree', () => { writeFileSync(join(worktree, 'a.ts'), 'export const x = 2;\n'); writeFileSync(join(worktree, '__probe__.test.ts'), 'it("x", () => {});'); - const r = run(); + const r = runScratchTree({ + worktree, + label: 'verify--round-1--abc123', + fetchedSha: headSha, + }); expect(r.available).toBe(true); expect(r.sharedTreeResidue.sort()).toEqual(['__probe__.test.ts', 'a.ts']); expect(r.sharedTreeResidueTotal).toBe(2); @@ -710,7 +720,11 @@ describe('runScratchTree', () => { for (let i = 0; i < 13; i++) { writeFileSync(join(worktree, `f${i}.ts`), 'x\n'); } - const r = run(); + const r = runScratchTree({ + worktree, + label: 'verify--round-1--abc123', + fetchedSha: headSha, + }); expect(r.sharedTreeResidueTotal).toBe(13); expect(r.sharedTreeResidue).toHaveLength(12); expect(r.note).toContain('1 more paths not listed here'); @@ -728,7 +742,11 @@ describe('runScratchTree', () => { cwd: worktree, }); - const r = run(); + const r = runScratchTree({ + worktree, + label: 'verify--round-1--abc123', + fetchedSha: headSha, + }); expect(r.sharedTreeResidue.sort()).toEqual([ '.gitignore', 'a.ts', @@ -769,10 +787,99 @@ describe('runScratchTree', () => { }, ); - it('says nothing about residue when the shared worktree is clean', () => { + it('refuses a CLEAN shared worktree it measured without the fetched sha', () => { + // The probe's clean verdict is the dangerous one: a forged pair answers + // clean too, and this caller brings no record to pin the identity — so + // an empty measurement is unmeasured, never clean (#9557). The note is + // the fail-closed half: an unmeasured tree is not a clean one. const r = run(); expect(r.sharedTreeResidue).toEqual([]); expect(r.note).not.toContain('NOT clean'); + expect(r.sharedTreeUnmeasured).toContain('brought no record'); + expect(r.note).toContain('could not be measured'); + // ...and the note does not blame `git status`: the refusal fired AFTER a + // clean status, so a triager sent to debug the git environment finds + // nothing. The framing names a reason, not a failed command. + expect(r.note).toContain('(reason: '); + expect(r.note).not.toContain('git status failed'); + }); + + it('measures a CLEAN shared worktree when the caller brings the fetched sha', () => { + // The pipeline caller's shape: fetch-pr records the sha in the plan and + // agent-prompt welds it into this command, so a healthy run measures + // clean — an unmeasured note that fired on every run would be noise + // nobody reads, and the genuine refusals would drown in it (#9742). + const r = runScratchTree({ + worktree, + label: 'verify--round-1--pinned', + fetchedSha: headSha, + }); + expect(r.available).toBe(true); + expect(r.sharedTreeResidue).toEqual([]); + expect(r.sharedTreeUnmeasured).toBeUndefined(); + expect(r.note).not.toContain('could not be measured'); + }); + + it('refuses BEFORE any reset or creation when the worktree is not at the fetched sha it brought', () => { + // The pin-mismatch signal the anchor exists for: the shared tree at B + // while the plan records reviewed commit A used to proceed with + // reset/creation at B and report `available: true`, handing a verifier + // an available tree at code other than the reviewed head with the + // mismatch disclosed only inside a NOTE. The refusal must come first — + // no reset, no creation, no path. + const r = runScratchTree({ + worktree, + label: 'verify--round-1--wrong-sha', + fetchedSha: `deadbeef${'0'.repeat(32)}`, + }); + expect(r.available).toBe(false); + expect(r.path).toBeUndefined(); + expect(r.note).toContain('not the fetched PR head'); + expect( + existsSync(scratchWorktreePath(worktree, 'verify--round-1--wrong-sha')), + ).toBe(false); + }); + + it('refuses a fetched sha that is not a full Git object ID', () => { + // The record arrives over a CLI flag and is welded into commands a + // verifier copies; a shape the pin cannot compare is refused before it + // reaches anything — neither the 39-hex truncation nor a non-hex string + // is a commit. Full object IDs are 40 hex (SHA-1) or 64 hex (SHA-256). + for (const sha of ['not-a-sha', 'a'.repeat(39), 'g'.repeat(40)]) { + const r = runScratchTree({ + worktree, + label: 'verify--bad-sha', + fetchedSha: sha, + }); + expect(r.available).toBe(false); + expect(r.path).toBeUndefined(); + expect(r.note).toContain('not a full Git object ID'); + } + // A 64-hex value IS the shape — on this SHA-1 tree it reaches the + // mismatch refusal, proving the validator admitted it. + const sha256Shape = runScratchTree({ + worktree, + label: 'verify--sha256-shape', + fetchedSha: 'ab'.repeat(32), + }); + expect(sha256Shape.available).toBe(false); + expect(sha256Shape.note).toContain('not the fetched PR head'); + expect(sha256Shape.note).not.toContain('not a full Git object ID'); + }); + + it('folds case when comparing the fetched sha, like the residue pin', () => { + // The plan records `git rev-parse` verbatim and a caller may carry it + // uppercase; the pin folds case on both sides, so the scratch-tree + // validation must too — an uppercase record of the RIGHT commit is not + // a mismatch. + const r = runScratchTree({ + worktree, + label: 'verify--round-1--upper-sha', + fetchedSha: headSha.toUpperCase(), + }); + expect(r.available).toBe(true); + expect(r.sharedTreeResidue).toEqual([]); + expect(r.sharedTreeUnmeasured).toBeUndefined(); }); it('links the review worktree’s node_modules in, and says so', () => { @@ -826,7 +933,11 @@ describe('runScratchTree', () => { const parent = join(repo, '.qwen', 'tmp'); chmodSync(parent, 0o555); // `git worktree add` cannot create the directory try { - const r = runScratchTree({ worktree, label: 'verify--round-1--zzz' }); + const r = runScratchTree({ + worktree, + label: 'verify--round-1--zzz', + fetchedSha: headSha, + }); expect(r.available).toBe(false); expect(r.sharedTreeResidue).toEqual(['__probe__.test.ts']); // The total belongs to the same report: a list longer than its own total @@ -842,6 +953,55 @@ describe('runScratchTree', () => { }, ); + describe('the CLI option contract', () => { + // Every fetchedSha test above builds its args by hand, but the only + // production delivery of the sha is the `--fetched-sha` flag, read off + // yargs' camel-cased parse as `fetchedSha`. If the option key and the + // field ever drift, every real invocation arrives unpinned and the suite + // stays green — the bug class `--build-test` shipped into `test-plan`, + // pinned here the same way: parse through the real builder, and assert on + // what the run does with the parse rather than on the parse's shape. + it('parses --fetched-sha into the field runScratchTree actually reads', () => { + // .strict() matters: a lenient parser camel-cases unknown flags and + // passes them through, so dropping the --fetched-sha registration from + // the builder would keep this test green while the real command (whose + // root parser IS strict) rejects the flag. + const parse = (argv: string[]) => + (scratchTreeCommand.builder as (y: Argv) => Argv)( + yargs([]).strict(), + ).parseSync(argv) as unknown as ScratchTreeArgs; + + // Reachable only if the parsed field reached the residue anchor: the + // identical call without it answers unmeasured instead of clean. + const clean = runScratchTree( + parse([ + '--worktree', + worktree, + '--label', + 'verify--round-1--cli', + '--fetched-sha', + headSha, + ]), + ); + expect(clean.sharedTreeUnmeasured).toBeUndefined(); + expect(clean.sharedTreeResidue).toEqual([]); + + // And a wrong sha still reaches the pin through the same parse. + const forged = runScratchTree( + parse([ + '--worktree', + worktree, + '--label', + 'verify--round-1--cli-forged', + '--fetched-sha', + `deadbeef${'0'.repeat(32)}`, + ]), + ); + expect(forged.available).toBe(false); + expect(forged.note).toContain('not the fetched PR head'); + }); + }); + describe('the command handler', () => { beforeEach(() => { process.exitCode = undefined; diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts index 93a89362e3..ba9f53037a 100644 --- a/packages/cli/src/commands/review/scratch-tree.ts +++ b/packages/cli/src/commands/review/scratch-tree.ts @@ -56,6 +56,7 @@ import { } from './lib/paths.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { + RESIDUE_PATH_CAP, discardWorktree, exposeDependencies, redirectedAncestor, @@ -119,6 +120,15 @@ export interface ScratchTreeReport { export interface ScratchTreeArgs { worktree: string; label: string; + /** + * The commit the worktree must hold — fetch-pr's record from the plan, + * welded into the verifier's command. The residue probe's identity anchor: + * with it a healthy shared tree measures clean; without it the + * measurement is refused rather than certified. Malformed, or disagreeing + * with the worktree's own HEAD, the command refuses before creating or + * resetting anything: a scratch tree may only stand at the reviewed head. + */ + fetchedSha?: string; out?: string; } @@ -453,6 +463,34 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport { ); } + // The record the caller welded in, validated BEFORE any reset or creation: + // both paths check a commit out, and the tree a verifier probes must hold + // the reviewed head. A record that is not a full object ID cannot anchor + // the residue pin, and one the shared tree does not answer means the tree + // is at some other commit — either way a scratch tree created now would + // hold code other than the reviewed head, so none is created or reset. + // Matched, the record and the tree spell the same commit, and the + // checkout below proceeds at git's own canonical rendering of it — the + // comparison folds case on both sides, exactly as the residue pin does. + if (args.fetchedSha !== undefined) { + if (!/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(args.fetchedSha)) { + return unavailable( + `--fetched-sha ${inertPath(args.fetchedSha)} is not a full Git ` + + 'object ID (40 or 64 hex), and no scratch tree is safe to create ' + + 'or reset against a record the residue pin cannot anchor', + ); + } + if (args.fetchedSha.toLowerCase() !== headSha.toLowerCase()) { + return unavailable( + `the review worktree is checked out at ${headSha.slice(0, 9)}, not the ` + + `fetched PR head ${inertPath(args.fetchedSha)} — a scratch tree ` + + 'created now would hold code other than the reviewed head, so ' + + 'none is created or reset until the shared tree is back at its ' + + 'record', + ); + } + } + // BEFORE any checkout runs — the reuse path's reset and the rebuild path's // `worktree add` both execute configured content filters. const filters = localFilterCommands(worktree); @@ -470,12 +508,15 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport { } // Read BEFORE the tree is created, so it describes the shared tree as this - // call found it and can never be confused with anything this call did. - const residue = worktreeResidue(worktree); + // call found it and can never be confused with anything this call did. The + // fetched sha, when the caller brought it, is the probe's identity anchor: + // with it a healthy tree measures clean, and a forged pair is refused at + // the pin (see worktreeResidue). + const residue = worktreeResidue(worktree, RESIDUE_PATH_CAP, args.fetchedSha); const sharedTreeResidue = residue.paths; const residueNote = residue.unmeasured - ? ` NOTE: whether the shared review worktree is clean could not be measured (git status ` + - `failed: ${inertPath(residue.unmeasured)}). An unmeasured tree is not a clean one — if a later read ` + + ? ` NOTE: whether the shared review worktree is clean could not be measured ` + + `(reason: ${inertPath(residue.unmeasured)}). An unmeasured tree is not a clean one — if a later read ` + 'of it surprises you, check the path against `git show HEAD:` before believing it.' : sharedTreeResidue.length > 0 ? ` WARNING: the shared review worktree is NOT clean — ${sharedTreeResidue @@ -668,6 +709,16 @@ export const scratchTreeCommand: CommandModule = { 'block. Two agents sharing a label share a tree, which is the race ' + 'this command exists to remove.', }) + .option('fetched-sha', { + type: 'string', + describe: + 'The commit the worktree must hold, as fetch-pr recorded it in the ' + + 'plan: the shared-tree residue check pins the tree to it, so a ' + + 'healthy tree measures clean and a forged identity is refused. ' + + 'Without it an empty measurement is reported as unmeasured, never ' + + 'clean; malformed or disagreeing with the worktree HEAD, the ' + + 'command refuses before creating or resetting anything.', + }) .option('out', { type: 'string', describe: 'Write the JSON report here',