diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 366d3676041..92e63b0de19 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -3024,7 +3024,14 @@ jobs: "${GIT_SAFE[@]}" branch -D "$review_ref" || echo "::warning::could not remove review branch: $review_ref" done || true - rm -f .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true + # Both locations: leases moved to `.qwen/review-leases` (out of the + # directory the review sandbox mounts read-write), and a runner whose + # workspace persists can still be holding one written by an older + # build in the old place. `-r`, because the old path is inside that + # mounted directory and a reviewed PR can leave a DIRECTORY at the + # lease's name, which plain `rm -f` cannot remove. + rm -rf .qwen/review-leases/qwen-review-lease-pr-*.json 2>/dev/null || true + rm -rf .qwen/tmp/qwen-review-lease-pr-*.json 2>/dev/null || true echo "review worktrees cleaned" # A review job that dies abnormally — runner crash, host loss, or the diff --git a/packages/cli/src/commands/review/base-tree.test.ts b/packages/cli/src/commands/review/base-tree.test.ts index 4fc2d6211db..9625b06b2ff 100644 --- a/packages/cli/src/commands/review/base-tree.test.ts +++ b/packages/cli/src/commands/review/base-tree.test.ts @@ -21,6 +21,7 @@ import { utimesSync, mkdtempSync, mkdirSync, + readFileSync, rmSync, writeFileSync, existsSync, @@ -29,6 +30,9 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { runBaseTree, type BaseTreeReport } from './base-tree.js'; import { baseWorktreePath } from './lib/paths.js'; +import { baseTreeTrustPath, runNonce } from './lib/base-tree-trust.js'; +import { adminEntryOf, plantAdminEntry } from './lib/test-utils.js'; +import { runEpochMs } from './lib/prompt-record.js'; import type { BuildTestReport } from './build-test.js'; const okBuild = { @@ -43,6 +47,13 @@ const failedBuild = { build: [{ command: 'npm run build', exitCode: 2 }], } as unknown as BuildTestReport; +// Skipped on win32 for the same reason as the sibling suites: `mountRootFor` +// refuses every absolute Windows path (a drive letter is a colon), so +// containment is unavailable there by design and this gate never speaks. The +// assertion would fail for that reason and nothing else — first inside the +// merge queue, where that lane actually runs. +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + describe('runBaseTree', () => { let repo: string; let worktree: string; @@ -61,13 +72,19 @@ describe('runBaseTree', () => { return p; }; + // Captured ONCE per test, the way `fetch-pr` captures it once per run: the + // reuse marker is fenced on the plan's epoch, so a helper that re-captured on + // every call would simulate a new run each time and the fast path — the + // concurrent-shard guard the reuse test below pins — could never speak. + let planPath = ''; const run = ( over: { plan?: Record; worktree?: string } = {}, build: (w: string) => BuildTestReport = () => okBuild, ): BaseTreeReport => { const { plan: planOver, ...rest } = over; + if (planOver !== undefined || !planPath) planPath = writePlan(planOver); return runBaseTree({ - plan: writePlan(planOver), + plan: planPath, worktree, timeout: 60, install: false, @@ -77,6 +94,7 @@ describe('runBaseTree', () => { }; beforeEach(() => { + planPath = ''; repo = mkdtempSync(join(tmpdir(), 'qwen-base-tree-')); git(repo, 'init', '-q', '-b', 'main'); git(repo, 'config', 'user.email', 't@t.t'); @@ -96,6 +114,203 @@ describe('runBaseTree', () => { afterEach(() => rmSync(repo, { recursive: true, force: true })); + itWhereContainmentExists( + 'reports BUSY and leaves the tree standing when a tree THIS RUN built fails a reuse check (tracked dirt)', + () => { + // `rev-parse HEAD` does not move when working files change, and this tree + // is a direct child of the directory the sandbox mounts read-write — but + // tracked dirt on a tree THIS run stamped is not necessarily a rewrite by + // the reviewed code: a build can modify tracked files (codegen, lockfile + // rewrites), and a concurrent shard's A/B writes one (a snapshot + // `--update`). Discarding on that signal sweeps a live tree another shard + // may be mid-A/B in — the concurrent-shard clobber the fast path exists + // to prevent — so the fence declines, the way the build lock's EEXIST arm + // does, and the dirtied tree stands for the shard that is using it. + const tree = baseWorktreePath(worktree); + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + return okBuild; + }; + expect(run({}, build).available).toBe(true); + // One tracked file rewritten, the plan untouched: same run, genuine stamp. + writeFileSync(join(tree, 'a.txt'), 'after\n'); + + const second = run({}, build); + expect(second.available).toBe(false); + expect(second.note).toContain('no longer passes a reuse check'); + expect(second.note).not.toContain('reusing it'); + expect(builds).toEqual([tree]); // declined — no sweep, no rebuild + // The dirtied file is still on disk: discarding it is what was refused. + expect(readFileSync(join(tree, 'a.txt'), 'utf8')).toBe('after\n'); + }, + ); + + itWhereContainmentExists( + "does not REUSE when an untracked path appears that THIS RUN's build did not leave", + () => { + // The epoch fence excludes only trees a DIFFERENT run built: reviewed + // code holding the read-write mount can drop an untracked executable + // into a tree this run stamped, after the stamp — and + // `--untracked-files=no` cannot see it, while refusing all untracked + // files would disable every legitimate reuse (the pipeline's own build + // leaves `node_modules/` and `dist/` here). So the set the build left is + // recorded host-side at marker write, and anything beyond it is a + // plant: content, not concurrency, so the rebuild's discard sweeps it. + const tree = baseWorktreePath(worktree); + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + return okBuild; + }; + expect(run({}, build).available).toBe(true); + // Same run: the plan — and with it the run's identity — is untouched. + mkdirSync(join(tree, 'dist'), { recursive: true }); + writeFileSync( + join(tree, 'dist', 'cli.js'), + 'planted by the reviewed build', + ); + + const second = run({}, build); + expect(second.note).not.toContain('reusing it'); + expect(builds).toEqual([tree, tree]); // the rebuild fired again + // The plant went with the tree it was standing in. + expect(existsSync(join(tree, 'dist', 'cli.js'))).toBe(false); + expect(second.available).toBe(true); + }, + ); + + itWhereContainmentExists( + 'refuses a stamp forged from the mount-visible epoch formula (production geometry)', + () => { + // The plan lives at `.qwen/tmp/qwen-review-pr--fetch.json`, INSIDE + // the directory the sandbox mounts read-write, so reviewed code can + // stat it and compute the epoch the stamp used to carry — then rewrite + // the marker and overwrite an executable beside the build's own + // residue, where the untracked-set fence cannot tell it from what the + // build left. The stamp now carries a run secret that never enters the + // mount: the forge below produces exactly what the old fence compared + // against, and it matches nothing. + const tree = baseWorktreePath(worktree); + planPath = join(repo, '.qwen', 'tmp', 'qwen-review-pr-1-fetch.json'); + writeFileSync( + planPath, + JSON.stringify({ mergeBaseSha: baseSha, files: [] }), + ); + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + if (builds.length === 1) { + // The residue a real build leaves, recorded host-side as legitimate. + mkdirSync(join(w, 'dist'), { recursive: true }); + writeFileSync(join(w, 'dist', 'cli.js'), 'built by round 1'); + } + return okBuild; + }; + expect(run({}, build).available).toBe(true); + + // The reviewed code's move: plant over the executable the A/B will run, + // and refresh the stamp to the epoch formula the mount exposes. + writeFileSync( + join(tree, 'dist', 'cli.js'), + 'planted by the reviewed code', + ); + writeFileSync( + join(tree, '.qwen-review-base-ok'), + `${baseSha}\n${runEpochMs(planPath)}\n`, + ); + + const second = run({}, build); + expect(second.note).not.toContain('reusing it'); + expect(builds).toEqual([tree, tree]); // rebuilt, not reused + // The plant went with the tree it was standing in. + expect(existsSync(join(tree, 'dist', 'cli.js'))).toBe(false); + }, + ); + + it('does not settle on a planted FAILED marker that carries no run secret', () => { + // The failed-marker fast path settles the question with NO build at all — + // "infrastructure, never a finding against the PR" — and the file lives + // inside the mount, so a sha-only marker is one planted line away from + // suppressing the A/B lane for the whole round, reading as infrastructure + // rather than as an attack. Only a marker stamped with this run's secret + // settles it. + const tree = baseWorktreePath(worktree); + git(repo, 'worktree', 'add', '--detach', '-q', tree, baseSha); + writeFileSync(join(tree, '.qwen-review-base-failed'), `${baseSha}\n`); + + const builds: string[] = []; + const r = run({}, (w) => { + builds.push(w); + return okBuild; + }); + expect(r.note).not.toContain('already failed'); + expect(builds).toEqual([tree]); // the rebuild was attempted + expect(r.available).toBe(true); + }); + + itWhereContainmentExists( + 'does not REUSE a base tree an EARLIER RUN built, whose untracked plants the dirt check cannot see', + () => { + // `cleanStale` releases the review worktree and its branch but never + // `-base`, so this tree stands into the next round with a whole + // containerized build/test phase in between — and inside the mount the + // reviewed code writes where it likes. What it can drop there is + // untracked executable content, `dist/cli.js` and `node_modules/.bin/` + // being exactly what a host-side A/B measurement runs, and + // `--untracked-files=no` cannot see it: a blanket untracked refusal + // would disable every correctly-built tree's reuse and bring back the + // concurrent-shard clobber the fast path exists to prevent. So the + // marker carries the run that built it, and a stamp from another run is + // not a tree this run may certify. + const tree = baseWorktreePath(worktree); + const builds: string[] = []; + const build = (w: string) => { + builds.push(w); + return okBuild; + }; + expect(run({}, build).available).toBe(true); + mkdirSync(join(tree, 'dist'), { recursive: true }); + writeFileSync( + join(tree, 'dist', 'cli.js'), + 'planted by the reviewed build', + ); + + // The next run captures its own plan, and the plan's mtime IS the epoch. + const later = new Date(Date.now() + 60_000); + utimesSync(planPath, later, later); + + const second = run({}, build); + expect(second.note).not.toContain('reusing it'); + expect(builds).toEqual([tree, tree]); + // The plant went with the tree it was standing in. + expect(existsSync(join(tree, 'dist', 'cli.js'))).toBe(false); + }, + ); + + itWhereContainmentExists( + 'refuses to build through a rewritten review-worktree gitfile', + () => { + // `worktree add` resolves the repository through the REVIEW worktree's own + // gitfile, which lives in the directory the sandbox mounts read-write and + // which the build/test phase already ran the PR's code against. It checks + // files out, so it runs whatever that pointer leads to, on the host. + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + adminEntryOf(worktree), + worktree, + join(repo, '.git'), + ); + + const r = run(); + expect(r.available).toBe(false); + expect(JSON.stringify(r)).toContain('review temp dir'); + // The tree was never created, which is what says the spawn never ran — + // the note alone reads the same whichever side of it the gate fires on. + expect(existsSync(baseWorktreePath(worktree))).toBe(false); + }, + ); + it('creates a sibling worktree holding the BASE commit, not the head', () => { const r = run(); expect(r.available).toBe(true); @@ -140,8 +355,13 @@ describe('runBaseTree', () => { expect(second.path).toBe(first.path); expect(second.note).toContain('reusing'); expect(builds).toHaveLength(1); // one install+build, not two - // A marker for a DIFFERENT sha (rebase between runs) does not shortcut. - writeFileSync(join(first.path!, '.qwen-review-base-ok'), 'f'.repeat(40)); + // A marker for a DIFFERENT sha (rebase between runs) does not shortcut — + // stamped with this run's secret, so the sha arm is what answers and not + // the run fence standing in front of it. + writeFileSync( + join(first.path!, '.qwen-review-base-ok'), + `${'f'.repeat(40)}\n${runNonce(baseTreeTrustPath(worktree, planPath))}\n`, + ); expect(run({}, build).note).not.toContain('reusing'); }); diff --git a/packages/cli/src/commands/review/base-tree.ts b/packages/cli/src/commands/review/base-tree.ts index dd42529e33c..fa0f1bd020b 100644 --- a/packages/cli/src/commands/review/base-tree.ts +++ b/packages/cli/src/commands/review/base-tree.ts @@ -55,12 +55,19 @@ import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { baseWorktreePath } from './lib/paths.js'; import { + untrustedGitfile, discardWorktree, sanitizedGitEnv, worktreeCreateFailureDetail, type SweepResult, } from './lib/worktree.js'; import { runBuildTest, type BuildTestReport } from './build-test.js'; +import { + baseTreeTrustPath, + builtTreeRecord, + recordBuiltTree, + runNonce, +} from './lib/base-tree-trust.js'; export interface BaseTreeReport { /** @@ -119,6 +126,19 @@ function git(cwd: string, ...args: string[]): void { } } +// The tree's untracked AND ignored path set, as `git status` collapses it (a +// directory holding nothing tracked reports as one `path/` entry). Ignored is +// included because that is exactly where a plant hides in a real repository — +// `dist/` and `node_modules/` are gitignored here, so an untracked-only +// listing is blind to the executable a host-side A/B would run. +function untrackedPaths(tree: string): string[] { + return gitOut(tree, 'status', '--porcelain', '-z', '--ignored') + .split('\0') + .filter((e) => e.startsWith('?? ') || e.startsWith('!! ')) + .map((e) => e.slice(3)) + .sort(); +} + export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { const unavailable = (note: string): BaseTreeReport => ({ available: false, @@ -175,20 +195,106 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // rebase between runs — falls through to the rebuild below.) const marker = () => join(tree, '.qwen-review-base-ok'); const failedMarker = () => join(tree, '.qwen-review-base-failed'); + // The run's stamp secret, persisted OUTSIDE the mount beside the worktree + // leases (see lib/base-tree-trust.ts). The fence used to key on the plan's + // mtime, and the plan lives inside the directory the sandbox mounts + // read-write — reviewed code could stat it, compute the epoch, and write + // it into a forged stamp, after which every reuse check passed on an + // attacker-shaped tree. A random per-run secret the mount can neither + // read nor write is what a stamp now vouches with. Without it no marker + // can be trusted or written, so its absence fails the command, not the + // fence. + const trustPath = baseTreeTrustPath(worktree, args.plan); + let nonce: string; try { - if ( - existsSync(tree) && - readFileSync(marker(), 'utf8').trim() === baseSha && - gitOut(tree, 'rev-parse', 'HEAD') === baseSha - ) { - return { - available: true, - path: tree, - baseSha, - build: null, - note: `base tree already built at ${baseSha.slice(0, 9)} in ${tree} (reusing it — a concurrent or earlier probe built it)`, - }; + nonce = runNonce(trustPath); + } catch (err) { + return unavailable( + `could not establish the run's trust artifact ${trustPath}: ` + + `${(err as Error).message}`, + ); + } + try { + const stamp = readFileSync(marker(), 'utf8').trim().split('\n'); + if (existsSync(tree) && stamp[0] === baseSha && stamp[1] === nonce) { + // ...and THIS run built it — genuinely: the secret never enters the + // mount, so a mount-local writer cannot have refreshed or forged the + // stamp. `cleanStale` never releases `-base`, so an earlier round's + // tree stands into this one with a whole containerized build/test + // phase in between — long enough for the reviewed code to drop + // untracked executables in here, and `dist/cli.js` is what a host-side + // A/B runs. + if ( + // The reuse path RETURNS, so the gate below the rebuild never runs + // for it — and both facts these arms establish come from inside the + // mount: `rev-parse HEAD` resolves through that tree's own `.git`, + // and its working files are a direct child of the directory the + // sandbox mounts read-write. A planted repository answers the right + // sha for a working tree holding anything at all; a plain copy + // overwrites the tracked sources while HEAD never moves — and the + // A/B's BASE side is then the reviewed code's own, so a test the PR + // breaks fails identically on both sides and reads as pre-existing. + // + // `--untracked-files=no`: the pipeline's own build leaves + // `node_modules/` and `dist/` here, so an untracked-inclusive check + // would call every correctly-built tree dirty and disable reuse + // outright. (The untracked surface is fenced separately, below.) + untrustedGitfile(tree) !== null || + gitOut(tree, 'rev-parse', 'HEAD') !== baseSha || + gitOut(tree, 'status', '--porcelain', '--untracked-files=no') !== '' + ) { + // A state arm failed on a tree THIS RUN built. Falling through would + // discard it — but this run's own build can legitimately leave + // tracked dirt (codegen, a lockfile rewrite), and a concurrent + // shard's A/B can write one tracked file mid-measurement (a snapshot + // `--update`). Sweeping that tree reintroduces the concurrent-shard + // clobber this fast path exists to prevent: shard B would rmSync the + // tree shard A is mid-A/B in, and A's base side reads as empty + // output — a fabricated difference with a deterministic source tag. + // So decline, the way the build lock's EEXIST arm does. (A stamp + // that does NOT match this run still falls through to the rebuild: + // an earlier run's leftover or a forge is nobody's live tree, and + // the discard is what removes the plant.) + return unavailable( + `the base tree at ${baseSha.slice(0, 9)} was built by this run ` + + 'but no longer passes a reuse check (a concurrent probe may be ' + + 'writing it mid-A/B); declining to reuse or discard it — retry ' + + 'when the probe finishes, or settle the claim by reading', + ); + } + // The untracked surface. The state arms above are blind to it by + // design, and the window to abuse it is guaranteed: the sandboxed + // build/test phase overlaps the verifier shards, so reviewed code can + // drop `dist/cli.js` or `node_modules/.bin/` in here AFTER the + // stamp, and a host-side A/B would then execute the plant as the BASE + // side. The marker itself cannot vouch for the set the build left — + // it is a file inside the mount — so the set was recorded host-side + // when the marker was written, and anything beyond it is a plant: + // content, not concurrency, so this one DOES fall through to the + // rebuild whose discard sweeps it. + const recorded = builtTreeRecord(trustPath, tree); + const current = untrackedPaths(tree); + if ( + recorded !== null && + recorded.baseSha === baseSha && + current.every((p) => recorded.untracked.includes(p)) + ) { + return { + available: true, + path: tree, + baseSha, + build: null, + note: `base tree already built at ${baseSha.slice(0, 9)} in ${tree} (reusing it — a concurrent or earlier probe built it)`, + }; + } } + // No marker, or a stamp that does not carry this run's secret — an + // earlier run's leftover, or a forge computed from the mount-visible + // inputs. Not a refusal: an unusable leftover is what the rebuild exists + // for. Falling through discards the tree (removing a plant with it) and + // creates a fresh one through the review worktree's pointer, which the + // gate before `worktree add` checks. Same shape as `scratch-tree`'s + // reuse path, for the same reason. } catch { // No marker, unreadable marker, or a tree git cannot answer for: rebuild. } @@ -197,9 +303,16 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // the same "unavailable" — and the sweep destroys the evidence tree the // failure deliberately leaves standing. try { + const failed = readFileSync(failedMarker(), 'utf8').trim().split('\n'); if ( existsSync(tree) && - readFileSync(failedMarker(), 'utf8').trim() === baseSha + failed[0] === baseSha && + // The same fence the ok marker carries, on the path that needs it most: + // this branch settles the question with NO build at all, and the file + // lives inside the mount — a sha-only marker is one planted line away + // from suppressing the A/B lane for the whole round, reading as + // infrastructure rather than as an attack. + failed[1] === nonce ) { return { available: false, @@ -265,6 +378,16 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // Clear a stale base tree left by a crashed run — it would fail `add`. Its // stderr is kept, because it is usually what explains that failure. sweep = discardWorktree(worktree, tree); + // The same question the probe phase asks before its own `worktree add`: + // this resolves the repository through the REVIEW worktree's gitfile, + // which lives inside the directory the sandbox mounts read-write and + // which the build/test phase already gave the reviewed code a chance to + // rewrite. `worktree add` checks files out, so it runs whatever that + // pointer leads to, on the host. See `untrustedGitfile`. + const untrusted = untrustedGitfile(worktree); + if (untrusted !== null) { + throw new Error(`refusing to create a base tree: ${untrusted}`); + } git(worktree, 'worktree', 'add', '--detach', tree, baseSha); } catch (e) { return unavailable( @@ -316,9 +439,11 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { // Leave the tree standing. A base that does not build is a fact worth // looking at by hand, and deleting the evidence to save a directory is a // bad trade — `cleanup` sweeps it at the end of the review either way. - // The marker makes the failure a SETTLED answer for every later shard. + // The marker makes the failure a SETTLED answer for every later shard, + // and it carries the run secret for the same reason the ok marker does: + // a planted sha-only marker must settle nothing (the fence above). try { - writeFileSync(failedMarker(), `${baseSha}\n`); + writeFileSync(failedMarker(), `${baseSha}\n${nonce}\n`); } catch { // The tree may be too broken to hold a marker; the next shard repays. } @@ -335,9 +460,14 @@ export function runBaseTree(args: BaseTreeArgs): BaseTreeReport { } // The marker is what the fast path above trusts, so it is written only after - // a build that succeeded, and it records the SHA it vouches for. + // a build that succeeded, and it records the SHA and the run it vouches for + // — the run secret, not the mount-visible epoch it replaced. With it goes + // the host-side record of the untracked set this build legitimately left, + // so the reuse fence can tell that residue from a plant dropped later + // (marker first, so the marker itself is part of the recorded set). try { - writeFileSync(marker(), `${baseSha}\n`); + writeFileSync(marker(), `${baseSha}\n${nonce}\n`); + recordBuiltTree(trustPath, tree, baseSha, untrackedPaths(tree)); } catch { // The tree may be too broken to hold a marker; the next shard rebuilds. } diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 0579c351cda..0ef15e40ffe 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -3,6 +3,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +/** `gitProbe`'s answer: the split cleanup's two refusal tests steer. */ +type ProbeAnswer = { + out: string | null; + status: number | null; + refusal: string | null; +}; const mocks = vi.hoisted(() => ({ execFileSync: vi.fn(), @@ -30,9 +38,25 @@ const mocks = vi.hoisted(() => ({ writeStdoutLine: vi.fn(), writeStderrLine: vi.fn(), clearReviewWorktreeLease: vi.fn(), - readReviewWorktreeLease: vi.fn((): unknown => null), + readReviewWorktreeLease: vi.fn( + (_repositoryRoot: string, _target: string): unknown => null, + ), reviewLeaseHeldByAnotherSession: vi.fn((_lease: unknown): boolean => false), - refExists: vi.fn(() => true), + // cleanup's two remaining git spawns go through `lib/git`'s gated wrappers: + // both resolve their repository from `process.cwd()`, and a launch directory + // inside a review temp dir is one the reviewed code can point elsewhere. + git: vi.fn((..._args: string[]): string => ''), + // The probe, not `gitOpt`/`refExists`: a launch-dir refusal has to stay + // distinguishable from git's own "no such branch" and "nothing to prune", + // which is what the two refusal tests below steer. `status: 0` is the + // `refExists` default this replaces — the branch leg deletes. + gitProbe: vi.fn( + (..._args: string[]): ProbeAnswer => ({ + out: '', + status: 0, + refusal: null, + }), + ), // The parameter is declared so `mock.calls` is typed `[string][]` rather than // `[][]` — the paths it was asked to free are the assertion in the sweep test. releaseWorktree: vi.fn((_path: string) => ({ @@ -101,16 +125,28 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ vi.mock('../../services/review-worktree-lease.js', () => ({ clearReviewWorktreeLease: mocks.clearReviewWorktreeLease, readReviewWorktreeLease: mocks.readReviewWorktreeLease, + // The found-at variant the holder-skip message uses: delegate to the same + // mock so `mockReturnValueOnce` steering and call assertions reach both. + readReviewWorktreeLeaseAt: (repositoryRoot: string, target: string) => { + const lease = mocks.readReviewWorktreeLease(repositoryRoot, target); + return lease + ? { + lease, + path: `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`, + } + : null; + }, reviewLeaseHeldByAnotherSession: mocks.reviewLeaseHeldByAnotherSession, reviewLeasePath: (repositoryRoot: string, target: string) => - `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`, + `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`, isReviewLeaseFile: (fileName: string) => /^qwen-review-lease-pr-\d+\.json$/.test(fileName), })); vi.mock('./lib/git.js', () => ({ - refExists: mocks.refExists, releaseWorktree: mocks.releaseWorktree, + git: mocks.git, + gitProbe: mocks.gitProbe, })); vi.mock('./lib/gh.js', () => ({ @@ -160,6 +196,10 @@ import { type RawReview, } from './cleanup.js'; +// The deleted-cwd witness creates and removes a REAL directory, but node:fs +// is mocked module-wide above — reach the real module through importActual. +const realFs = await vi.importActual('node:fs'); + describe('runCleanup', () => { beforeEach(() => { vi.clearAllMocks(); @@ -188,7 +228,9 @@ describe('runCleanup', () => { // path-dependent implementations, and a later test reading the declared // `[]` default would otherwise inherit them. mocks.readdirSync.mockImplementation((_path: string): string[] => []); - mocks.refExists.mockReturnValue(true); + // Implementations survive clearAllMocks, and both refusal tests below + // install one: restore the branch-exists / no-prune-failure default. + mocks.gitProbe.mockReturnValue({ out: '', status: 0, refusal: null }); mocks.releaseWorktree.mockReturnValue({ existed: false, freed: false, @@ -254,20 +296,54 @@ describe('runCleanup', () => { expect(() => runCleanup('pr-123')).not.toThrow(); }); + it('degrades instead of throwing when the process cwd is deleted out from under it', () => { + // R19-4 (cleanup half): `redirectedAncestor`'s default stop reads + // process.cwd() in the CALLER's frame, outside the walk's own try, and + // REVIEW_TMP_DIR is a relative spelling — a launch directory deleted out + // from under the process (an operator `rm -rf` mid-review, the nested + // geometry) threw uv_cwd out of runCleanup before any degradation could + // run. With no live cwd the relative root cannot be resolved at all, so + // the sweep refuses with an explanation instead. + const anchor = process.cwd(); + const gone = realFs.mkdtempSync(join(tmpdir(), 'cleanup-deleted-cwd-')); + process.chdir(gone); + try { + realFs.rmSync(gone, { recursive: true, force: true }); + // The precondition, asserted rather than assumed: this is the throw the + // default stopAt used to let escape (the same shape the gitProbe + // witness pins in lib/git.integration.test.ts). + expect(process.cwd).toThrow(/uv_cwd/); + + expect(() => runCleanup('pr-123')).not.toThrow(); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('working directory no longer exists'), + ); + expect(process.exitCode).toBe(1); + // Nothing was swept from inside a root that cannot be resolved. + expect(mocks.rmSync).not.toHaveBeenCalled(); + expect(mocks.releaseWorktree).not.toHaveBeenCalled(); + } finally { + process.chdir(anchor); + process.exitCode = 0; + } + }); + it('keeps the lease when branch deletion fails', () => { - mocks.execFileSync.mockImplementation(() => { + // Once, because this suite's mocks keep their implementations across tests + // and each one sets what it needs: a standing throw here would fail every + // later branch delete and hold the lease for the wrong reason. + mocks.git.mockImplementationOnce(() => { throw new Error('branch is locked'); }); runCleanup('pr-123'); - expect(mocks.execFileSync).toHaveBeenCalledWith( - 'git', - ['branch', '-D', 'qwen-review/pr-123'], - // The env is sanitized: the check that gates this delete resolves the - // real repository, so the delete must not follow an exported `GIT_DIR` - // into another one. - expect.objectContaining({ stdio: 'pipe', env: expect.any(Object) }), + // The throwing wrapper, which carries the sanitized env and the launch-dir + // gate the direct spawn had neither of. + expect(mocks.git).toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-123', ); expect(mocks.writeStderrLine).toHaveBeenCalledWith( expect.stringContaining('Failed to delete branch qwen-review/pr-123'), @@ -275,6 +351,203 @@ describe('runCleanup', () => { expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); }); + it('keeps the lease when the branch leg is REFUSED, not merely absent', () => { + // `refExists` answered a launch-dir refusal as "no such branch", so this + // leg was skipped before `git` was ever reached: nothing on stderr, no + // `failedDestruction`, the lease released and "Nothing to clean" printed + // over a branch and a registration that both survived — the next + // `worktree add` at the fixed path then fails "missing but already + // registered" with nobody told why. The probe keeps the two apart. + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'rev-parse' + ? { out: null, status: null, refusal: 'POISONED LAUNCH DIR' } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + expect(mocks.git).not.toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-123', + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'Failed to delete branch qwen-review/pr-123: git could not run from this directory', + ), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('POISONED LAUNCH DIR'), + ); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + }); + + it('keeps the lease when the branch probe answers a fatal, not absence (exit 128)', () => { + // R19-1: with `--verify --quiet`, exit 1 is the ONLY genuine "no such + // branch". A 128 fatal (a corrupt .git) is a non-answer, and the leg used + // to read every non-refusal failure as absence: the delete was silently + // skipped, `failedDestruction` never set, the lease released, and + // "Nothing to clean" printed over a surviving branch. + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'rev-parse' + ? { out: null, status: 128, refusal: null } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + expect(mocks.git).not.toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-123', + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'Failed to delete branch qwen-review/pr-123: git could not answer whether it exists (exit 128)', + ), + ); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + // The success report over a destruction that never ran. + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + expect.stringContaining('Nothing to clean'), + ); + }); + + it('keeps the lease when the branch probe could not run at all (status null)', () => { + // R19-1's third shape: spawn ENOENT or the 120s timeout kill answers + // `{out: null, status: null, refusal: null}` — "the command could not be + // run at all" — which the leg read as absence exactly like the 128. + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'rev-parse' + ? { out: null, status: null, refusal: null } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + expect(mocks.git).not.toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-123', + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining( + 'Failed to delete branch qwen-review/pr-123: git could not answer whether it exists (exit null)', + ), + ); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + expect(mocks.writeStdoutLine).not.toHaveBeenCalledWith( + expect.stringContaining('Nothing to clean'), + ); + }); + + it('treats exit 1 from the branch probe as genuine absence — silently', () => { + // The idempotency contract at the top of cleanup.ts: missing branches are + // silent OK. Only OTHER non-zero/null statuses are non-answers. + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'rev-parse' + ? { out: null, status: 1, refusal: null } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + expect(mocks.git).not.toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-123', + ); + expect(mocks.writeStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('branch'), + ); + expect(mocks.clearReviewWorktreeLease).toHaveBeenCalledWith( + process.cwd(), + 'pr-123', + ); + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + 'Nothing to clean for target "pr-123".', + ); + }); + + it('keeps the lease when the symlink arm cannot prune', () => { + // The same collapse one call earlier: `pruneWorktrees()` answered null for + // "refused" and for "nothing to prune" alike, so the arm announced + // `Removed … link` and released the lease while the registration the prune + // was there to clear survived. A genuine prune failure stays swallowed — + // it must not mask the error that got us here — but a refusal is the one + // cause a user can act on, so it is reported and holds the lease. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.lstatSync.mockImplementation(((p: string) => ({ + isSymbolicLink: () => String(p).includes('review-pr-123'), + isDirectory: () => !String(p).includes('review-pr-123'), + })) as unknown as () => { + isSymbolicLink: () => boolean; + isDirectory: () => boolean; + }); + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'worktree' + ? { out: null, status: null, refusal: 'POISONED LAUNCH DIR' } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + // The link IS gone, so the announcement stays true — the stderr line is + // about the registration behind it, not about the unlink. + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('Removed worktree link'), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('Failed to prune after removing worktree link'), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('POISONED LAUNCH DIR'), + ); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + }); + + it('keeps the lease when the symlink arm cannot even ASK git to prune (R23-8)', () => { + // The probe's third shape — `{out: null, status: null, refusal: null}`, + // "the command could not be run at all" — used to read as a successful + // prune: the arm announced `Removed … link`, wrote no stderr line, and + // released the lease over a registration git never swept, so the next + // `worktree add` met "missing but already registered" with nobody told + // why. Only a genuine non-zero exit stays swallowed. Removing the + // `status === null` arm in pruneWorktrees turns this red. + mocks.execFileSync.mockReturnValue(Buffer.from('')); + mocks.lstatSync.mockImplementation(((p: string) => ({ + isSymbolicLink: () => String(p).includes('review-pr-123'), + isDirectory: () => !String(p).includes('review-pr-123'), + })) as unknown as () => { + isSymbolicLink: () => boolean; + isDirectory: () => boolean; + }); + mocks.gitProbe.mockImplementation( + (...args: string[]): ProbeAnswer => + args[0] === 'worktree' + ? { out: null, status: null, refusal: null } + : { out: '', status: 0, refusal: null }, + ); + + runCleanup('pr-123'); + + expect(mocks.writeStdoutLine).toHaveBeenCalledWith( + expect.stringContaining('Removed worktree link'), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('Failed to prune after removing worktree link'), + ); + expect(mocks.writeStderrLine).toHaveBeenCalledWith( + expect.stringContaining('could not be run at all'), + ); + expect(mocks.clearReviewWorktreeLease).not.toHaveBeenCalled(); + }); + it('clears the lease when cleanup succeeds', () => { mocks.execFileSync.mockReturnValue(Buffer.from('')); @@ -394,14 +667,13 @@ describe('runCleanup', () => { worktreePath: '/repo/.qwen/tmp/review-pr-123', branch: 'qwen-review/pr-123', }; - // First read (the gate): no lease yet. Second read (post-audit): session B - // has acquired one. + // First read (the gate): no lease yet — the gate short-circuits on the + // absent holder without asking the held question about nothing. Second + // read (post-audit): session B has acquired one. mocks.readReviewWorktreeLease .mockReturnValueOnce(null) .mockReturnValueOnce(lease); - mocks.reviewLeaseHeldByAnotherSession - .mockReturnValueOnce(false) - .mockReturnValueOnce(true); + mocks.reviewLeaseHeldByAnotherSession.mockReturnValueOnce(true); runCleanup('pr-123'); @@ -564,11 +836,7 @@ describe('runCleanup', () => { // prune lives — so without one here the family paths were reported swept // while their admin entries stayed behind and wedged the next // `worktree add` with `already exists`. - expect(mocks.execFileSync).toHaveBeenCalledWith( - 'git', - ['worktree', 'prune'], - expect.anything(), - ); + expect(mocks.gitProbe).toHaveBeenCalledWith('worktree', 'prune'); }); it('does not announce a clean sweep when it could not list the family', () => { diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index a72c7ee75bf..61388f3d418 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -13,7 +13,6 @@ // The command is idempotent — missing files / branches are silent OK. import type { CommandModule } from 'yargs'; -import { execFileSync } from 'node:child_process'; import { existsSync, lstatSync, @@ -28,15 +27,15 @@ import { clearReviewWorktreeLease, isReviewLeaseFile, readReviewWorktreeLease, + readReviewWorktreeLeaseAt, reviewLeaseHeldByAnotherSession, - reviewLeasePath, } from '../../services/review-worktree-lease.js'; -import { redirectedAncestor, sanitizedGitEnv } from './lib/worktree.js'; +import { redirectedAncestor } from './lib/worktree.js'; import { currentUser, getGhHost, ghApiAll, setGhHost } from './lib/gh.js'; import { parseReceiptCommentIds, parseReceiptIds } from './lib/receipt.js'; import { detectPlatformKind } from './lib/platform/registry.js'; import { a1Json, aoneWhoamiAccount } from './lib/platform/aone-client.js'; -import { refExists, releaseWorktree } from './lib/git.js'; +import { git, gitProbe, releaseWorktree } from './lib/git.js'; import { readBudgetStopUnfenced } from './lib/deadline.js'; import { promptRecordDir, runEpochMs } from './lib/prompt-record.js'; import { @@ -684,6 +683,7 @@ function scratchWorktreesOf(worktree: string): { /** * Clear registrations whose worktree directory is gone. A no-op when none are. + * Returns why git could not be run at all, or null when it ran. * * `releaseWorktree` runs this after its own unlink and says why: a * registration whose tree once stood at a path wedges the next @@ -691,15 +691,30 @@ function scratchWorktreesOf(worktree: string): { * out against `branch -D`. Best-effort like every other step on the cleanup * path — a prune that fails must not mask the error that got us here. */ -function pruneWorktrees(): void { - try { - execFileSync('git', ['worktree', 'prune'], { - stdio: 'pipe', - env: sanitizedGitEnv(), - }); - } catch { - // Reported by the next `worktree add` if it mattered. +function pruneWorktrees(): string | null { + // Through `lib/git`'s wrapper, not a direct spawn: `worktree prune` finds its + // repository from `process.cwd()`, and a launch directory inside a review + // temp dir is one the reviewed code can point elsewhere — a `core.hooksPath` + // in the plant's config then runs on the host. The sweep this exists to + // provide is not what the gate costs: measured from a poisoned launch + // directory, an ungated prune clears the PLANT's registrations and leaves the + // real stale one standing, so it was never sweeping the repository the + // registration belongs to. `rmSync` — the half that does clear the path — is + // not a git call and still runs. A genuine prune failure stays swallowed — + // it must not mask the error that got us here — but a refusal is the one + // cause a user can act on, and `gitOpt` answered null for both. + const probe = gitProbe('worktree', 'prune'); + if (probe.refusal !== null) return probe.refusal; + // `{out: null, status: null, refusal: null}` is the probe's third shape: + // git could not be run AT ALL (a spawn failure, the timeout kill, a deleted + // cwd), so the prune did not happen and the registration outlives the link + // — while the caller above announced "Removed ... link" and released the + // lease, and the next `worktree add` met "missing but already registered" + // with nobody told why. Only a genuine non-zero exit stays swallowed. + if (probe.status === null) { + return 'git could not be run at all (a spawn failure or the timeout kill)'; } + return null; } export function runCleanup(target: string): void { @@ -719,11 +734,31 @@ export function runCleanup(target: string): void { process.exitCode = 1; return; } + // Capture the root once, at entry, and hand it to the walks below as an + // explicit stopAt: `redirectedAncestor`'s default stop reads process.cwd() + // in the CALLER's frame — outside the walk's own try — and REVIEW_TMP_DIR + // is a relative spelling, so a launch directory deleted out from under the + // process (an operator `rm -rf` mid-review, the nested geometry) threw + // uv_cwd out of this best-effort sweep before any degradation could run. + // With no live cwd the relative root cannot be resolved at all, so degrade + // with an explanation rather than sweeping. + let repositoryRoot: string; + try { + repositoryRoot = process.cwd(); + } catch (err) { + writeStderrLine( + `Refusing to clean: the working directory no longer exists ` + + `(${(err as Error).message}), and ${REVIEW_TMP_DIR} is resolved ` + + `against it. Re-run from a live 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 // this, which announced the hazard and then kept deleting under it. - const redirected = redirectedAncestor(REVIEW_TMP_DIR); + const redirected = redirectedAncestor(REVIEW_TMP_DIR, repositoryRoot); if (redirected !== null) { writeStderrLine( `Refusing to clean: ${redirected} is a symlink, so every delete under ` + @@ -759,12 +794,12 @@ export function runCleanup(target: string): void { // receipts it never wrote. Skip the whole target: worktree, siblings, // branch, side files, audit, and the lease itself all belong to the // holder until its own cleanup releases them. - const holder = readReviewWorktreeLease(process.cwd(), target); - if (reviewLeaseHeldByAnotherSession(holder)) { + const holder = readReviewWorktreeLeaseAt(repositoryRoot, target); + if (holder && reviewLeaseHeldByAnotherSession(holder.lease)) { writeStdoutLine( `note: skipped cleanup for "${target}" — another review session ` + - `(session ${holder.sessionId}) still holds the worktree lease at ` + - `${reviewLeasePath(process.cwd(), target)}. Its own cleanup ` + + `(session ${holder.lease.sessionId}) still holds the worktree lease at ` + + `${holder.path}. Its own cleanup ` + `releases the lease when it finishes; if that session is gone, ` + `delete the lease file and re-run to force cleanup.`, ); @@ -779,7 +814,10 @@ export function runCleanup(target: string): void { // of this function ran BEFORE it. A link that appears at any component of // the temp path during that window redirects every delete below it, so the // same refusal is re-taken here rather than assumed to still hold. - const redirectedAfterAudit = redirectedAncestor(REVIEW_TMP_DIR); + const redirectedAfterAudit = redirectedAncestor( + REVIEW_TMP_DIR, + repositoryRoot, + ); if (redirectedAfterAudit !== null) { writeStderrLine( `Refusing to clean: ${redirectedAfterAudit} became a symlink during ` + @@ -793,7 +831,7 @@ export function runCleanup(target: string): void { // A lease can appear during the same window (a review that started after // the gate above read none). Re-check before destroying anything and take // the same skip path (#9205). - const holderAfterAudit = readReviewWorktreeLease(process.cwd(), target); + const holderAfterAudit = readReviewWorktreeLease(repositoryRoot, target); if (reviewLeaseHeldByAnotherSession(holderAfterAudit)) { writeStdoutLine( `note: skipped cleanup for "${target}" — a review session ` + @@ -835,9 +873,20 @@ export function runCleanup(target: string): void { // reaching it, so the family paths were unlinked and reported swept // while their admin entries stayed behind. It is the only prune in // this function, and a no-op when nothing is stale. - pruneWorktrees(); + const refusal = pruneWorktrees(); writeStdoutLine(`Removed ${label} link: ${path}`); removedAny = true; + if (refusal !== null) { + // The link IS gone, so the announcement above is true — but the + // registration it left behind is not, and a prune git never ran is + // not the swallowed best-effort case this function documents. + writeStderrLine( + `Failed to prune after removing ${label} link ${path}: git ` + + `could not run from this directory — ${refusal}`, + ); + failedAny = true; + failedDestruction = true; + } } catch (err) { // `force` suppresses ENOENT, not EACCES/EBUSY — and a link left at a // family path still wedges the next review's `worktree add`, which is @@ -914,15 +963,27 @@ export function runCleanup(target: string): void { } const branch = reviewBranch(prNumber); - if (refExists(branch)) { + // The probe, not `refExists`: a launch-dir refusal answers "no such + // branch" there, so this leg was skipped silently, the lease was released + // over a surviving branch, and the run still printed "Nothing to clean" — + // a success report over a destruction that never ran. + const branchProbe = gitProbe('rev-parse', '--verify', '--quiet', branch); + if (branchProbe.refusal !== null) { + writeStderrLine( + `Failed to delete branch ${branch}: git could not run from this ` + + `directory — ${branchProbe.refusal}`, + ); + failedAny = true; + failedDestruction = true; + } else if (branchProbe.status === 0) { try { - execFileSync('git', ['branch', '-D', branch], { - stdio: 'pipe', - // The CHECK that gates this delete resolves the real repository - // (`refExists` goes through the sanitized helpers); an exported - // `GIT_DIR` here would verify one repo and delete in another. - env: sanitizedGitEnv(), - }); + // The throwing wrapper, not a direct spawn: `branch -D` is a + // reference-transaction hook channel and finds its repository from + // `process.cwd()`, so an ungated delete from a poisoned launch + // directory executes the plant's hooks. A pointer rewritten between + // the probe and this call lands in the catch below as a failed + // destruction — the branch survives and the lease stays held. + git('branch', '-D', branch); writeStdoutLine(`Deleted ref: ${branch}`); removedAny = true; } catch (err) { @@ -932,6 +993,21 @@ export function runCleanup(target: string): void { failedAny = true; failedDestruction = true; } + } else if (branchProbe.status !== 1) { + // With `--verify --quiet`, exit 1 is the ONLY genuine absence — and it + // stays silent, per the idempotency contract at the top of this file. + // Every other non-answer — the probe's null status ("the command could + // not be run at all": spawn ENOENT, the timeout kill), a 128 fatal + // from a corrupt .git — used to fall through this chain exactly like + // the exit-1 case: the delete was silently skipped, the lease released, + // and "Nothing to clean" printed over a surviving branch. Name the + // non-answer and hold the lease, the same as a refused probe. + writeStderrLine( + `Failed to delete branch ${branch}: git could not answer whether ` + + `it exists (exit ${branchProbe.status})`, + ); + failedAny = true; + failedDestruction = true; } } @@ -1055,7 +1131,7 @@ export function runCleanup(target: string): void { } if (!failedDestruction) { - clearReviewWorktreeLease(process.cwd(), target); + clearReviewWorktreeLease(repositoryRoot, target); } // "Nothing to clean" is a claim about the tree, not about this run's luck. It diff --git a/packages/cli/src/commands/review/comment-status.aone.test.ts b/packages/cli/src/commands/review/comment-status.aone.test.ts index 4c540cd6c1d..ca376483cb7 100644 --- a/packages/cli/src/commands/review/comment-status.aone.test.ts +++ b/packages/cli/src/commands/review/comment-status.aone.test.ts @@ -43,7 +43,13 @@ vi.mock('./lib/git.js', () => ({ gitOpt: mocks.gitOpt, })); -vi.mock('./lib/paths.js', () => ({ +// PARTIAL, not a replacement: only `worktreePath` is being steered, and a +// total mock silently deletes every other export — which broke this suite the +// moment the handler's import graph reached `REVIEW_TMP_DIR` through the +// worktree gate. `importOriginal` keeps the module's real surface behind the +// one value the fixtures choose. +vi.mock('./lib/paths.js', async (importOriginal) => ({ + ...(await importOriginal()), worktreePath: (n: string | number) => `/repo/.qwen/tmp/review-pr-${n}`, })); diff --git a/packages/cli/src/commands/review/comment-status.handler.test.ts b/packages/cli/src/commands/review/comment-status.handler.test.ts index 11b805a9249..dd45fc7ab64 100644 --- a/packages/cli/src/commands/review/comment-status.handler.test.ts +++ b/packages/cli/src/commands/review/comment-status.handler.test.ts @@ -20,6 +20,7 @@ const mocks = vi.hoisted(() => ({ ensureAuthenticated: vi.fn(), setGhHost: vi.fn(), gitOpt: vi.fn((..._a: string[]): string | null => null), + untrustedGitfile: vi.fn((_t: string): string | null => null), writeFileSync: vi.fn(), mkdirSync: vi.fn(), writeStdoutLine: vi.fn(), @@ -37,7 +38,22 @@ vi.mock('./lib/git.js', () => ({ gitOpt: mocks.gitOpt, })); -vi.mock('./lib/paths.js', () => ({ +// PARTIAL for the same reason as paths.js below: only `untrustedGitfile` is +// steered — the trust-gate divergence cases need it answering differently on +// two successive calls — and a total mock would silently delete the module's +// other exports from the handler's import graph. +vi.mock('./lib/worktree.js', async (importOriginal) => ({ + ...(await importOriginal()), + untrustedGitfile: mocks.untrustedGitfile, +})); + +// PARTIAL, not a replacement: only `worktreePath` is being steered, and a +// total mock silently deletes every other export — which broke this suite the +// moment the handler's import graph reached `REVIEW_TMP_DIR` through the +// worktree gate. `importOriginal` keeps the module's real surface behind the +// one value the fixtures choose. +vi.mock('./lib/paths.js', async (importOriginal) => ({ + ...(await importOriginal()), worktreePath: (n: string | number) => `/repo/.qwen/tmp/review-pr-${n}`, })); @@ -310,4 +326,85 @@ describe('comment-status handler', () => { expect(report.liveHeadSha).toBe('headA'); expect(report.headMovedDuringFetch).toBe(false); }); + + // The probe a self-gated report would produce real facts through: the repo + // answers inside-work-tree, ancestry yes, one touching commit. + function probeAnswering() { + mocks.gitOpt.mockImplementation((...args: string[]) => { + if (args.includes('--is-inside-work-tree')) return 'true'; + if (args.includes('merge-base')) return ''; + if (args.includes('log')) return 'deadbeef'; + if (args.includes('rev-parse')) return 'headA'; + return null; + }); + } + + it('never pairs an untrusted verdict with measured thread facts — the gate is asked ONCE', async () => { + // The trust gate was evaluated twice per run — report level and inside + // the probe — and the two answers could diverge (a timeout on one side, + // a pointer swapped between them). First call refusing, second clean, + // the old pairing certified `worktreeUntrusted` beside REAL code facts: + // the report's own `threads[]` contradicting its trust state. + mocks.ghApiAll.mockReturnValue([ + { + id: 1, + user: { login: 'r' }, + path: 'a.ts', + line: 1, + original_commit_id: 's', + }, + ]); + probeAnswering(); + let n = 0; + mocks.untrustedGitfile.mockImplementation(() => + ++n === 1 + ? 'resolves to an admin entry inside the review temp dir' + : null, + ); + try { + queueHeads('headA', 'headA'); + await run(); + const report = reportWritten(); + expect(mocks.untrustedGitfile).toHaveBeenCalledTimes(1); + expect(report.worktreeUntrusted).toContain('review temp dir'); + expect(report.worktreeHeadSha).toBeNull(); + expect(report.threads[0].code.changedSinceComment).toBe('unknown'); + } finally { + mocks.untrustedGitfile.mockImplementation(() => null); + } + }); + + it('never pairs a clean verdict and a live HEAD with all-unknown facts — the same single answer', async () => { + // The mirror divergence: first call clean, second refusing. The old + // pairing wrote `worktreeUntrusted: null` and a populated HEAD beside + // thread facts the probe had degraded to `unknown` — a report that + // claims trust while its own payload measured nothing. + mocks.ghApiAll.mockReturnValue([ + { + id: 1, + user: { login: 'r' }, + path: 'a.ts', + line: 1, + original_commit_id: 's', + }, + ]); + probeAnswering(); + let n = 0; + mocks.untrustedGitfile.mockImplementation(() => + ++n === 1 + ? null + : 'resolves to an admin entry inside the review temp dir', + ); + try { + queueHeads('headA', 'headA'); + await run(); + const report = reportWritten(); + expect(mocks.untrustedGitfile).toHaveBeenCalledTimes(1); + expect(report.worktreeUntrusted).toBeNull(); + expect(report.worktreeHeadSha).toBe('headA'); + expect(report.threads[0].code.changedSinceComment).toBe(true); + } finally { + mocks.untrustedGitfile.mockImplementation(() => null); + } + }); }); diff --git a/packages/cli/src/commands/review/comment-status.ts b/packages/cli/src/commands/review/comment-status.ts index 501ee6c3e1f..7a306fa246c 100644 --- a/packages/cli/src/commands/review/comment-status.ts +++ b/packages/cli/src/commands/review/comment-status.ts @@ -34,6 +34,7 @@ import { } from './lib/gh.js'; import { gitOpt } from './lib/git.js'; import { worktreePath } from './lib/paths.js'; +import { untrustedGitfile } from './lib/worktree.js'; import { anyRootCarriesCriticalMarker, isBlockerBody, @@ -271,8 +272,30 @@ export function summarizeThreads(threads: ThreadStatus[]): ThreadSummary { * worktree. Memoized per (path, sinceSha): several threads routinely anchor * to the same file at the same commit. */ -export function makeGitProbe(worktree: string): CodeChangeProbe { +export function makeGitProbe( + worktree: string, + // The trust verdict, asked ONCE by the report and handed down so the probe + // and the report can never disagree: evaluated separately, the two calls + // straddle a rewrite (or merely a timeout on one side) and the report + // certifies a trust state its own `threads[]` payload contradicts — a + // `worktreeUntrusted` string beside measured code facts, or a clean verdict + // beside all-`unknown` ones. Defaulted rather than required so standalone + // callers (the integration suite, the canary) keep self-gating. + untrusted: string | null = untrustedGitfile(worktree), +): CodeChangeProbe { + // The pointer this probe reads THROUGH is inside the directory the review + // sandbox hands the reviewed code read-write, so `-C ` is + // not by itself a scoping guarantee: a `.git` rewritten to a planted admin + // entry makes every answer below the plant's own. That is not a hypothetical + // degradation — `changedSinceComment: false` from a planted repository is + // this command certifying that the code behind a blocker thread did not + // move, which is exactly the fact a reviewer acts on. Gated at the one place + // that owns all three spawns rather than at each of them. + // + // The existing `!inRepo` degradation carries it: every thread's code facts + // become `unknown`, which the report already distinguishes from "unchanged". const inRepo = + untrusted === null && gitOpt('-C', worktree, 'rev-parse', '--is-inside-work-tree') === 'true'; const memo = new Map>(); // Ancestry depends on the SHA alone (HEAD is fixed for the run); memoizing @@ -411,10 +434,22 @@ function writeCommentStatusReport( const { prAuthor, liveHeadBefore, liveHeadAfter, comments } = facts; const worktree = worktreePath(prNumber); - const worktreeHeadSha = gitOpt('-C', worktree, 'rev-parse', 'HEAD'); + // The gate the probe reads through, asked ONCE for the whole report and + // handed down to it: a HEAD sha read through a rewritten pointer is the + // planted repository's, and it feeds `worktreeStale` — the flag that + // decides whether this report's code facts are announced as describing a + // superseded checkout. Asked twice, the two answers can diverge between + // calls, and the report would certify a trust state its `threads[]` + // contradicts. + const worktreeUntrusted = untrustedGitfile(worktree); + const worktreeHeadSha = + worktreeUntrusted === null + ? gitOpt('-C', worktree, 'rev-parse', 'HEAD') + : null; // A null HEAD means the worktree is absent (comment-status run before - // fetch-pr, or after cleanup) — every thread's code facts then degrade to - // 'unknown', which must not pass silently as if the files were unchanged. + // fetch-pr, or after cleanup) or unusable — every thread's code facts then + // degrade to 'unknown', which must not pass silently as if the files were + // unchanged. const worktreeMissing = worktreeHeadSha === null; // Anchor facts (`line`, outdated) describe the LIVE head — the platform // maps comments against the latest diff it serves. Code facts @@ -469,7 +504,11 @@ function writeCommentStatusReport( const threads = buildThreadStatuses( comments, prAuthor, - makeGitProbe(worktree), + // The SAME verdict the report level just read: asking the gate a second + // time here would let the two answers diverge (a timeout on one side, a + // pointer swapped between them), and the report would certify a trust + // state its own thread facts contradict. + makeGitProbe(worktree, worktreeUntrusted), me, ); if (worktreeStale) { @@ -493,6 +532,7 @@ function writeCommentStatusReport( headDrift, headMovedDuringFetch, inlineComments: comments.length, + worktreeUntrusted, summary, threads, }; @@ -506,7 +546,17 @@ function writeCommentStatusReport( `${summary.changedSinceComment} on files changed since their comment, ` + `${summary.withReplies} with replies, ${summary.authorReplied} answered by the PR author)`, ); - if (worktreeMissing) { + if (worktreeUntrusted !== null) { + // Distinct from the missing-worktree warning below: the tree IS there, and + // saying "run fetch-pr first" would send a reader to re-create a worktree + // when what happened is that its pointer stopped being trustworthy. + writeStdoutLine( + `warning: the worktree at ${worktree} was not read — ${worktreeUntrusted}. ` + + `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + + `only the anchor and reply facts are usable. Sweep the tree ` + + `(\`qwen review cleanup\`) and re-fetch before trusting code facts for this PR.`, + ); + } else if (worktreeMissing) { writeStdoutLine( `warning: no worktree at ${worktree} — run \`qwen review fetch-pr\` first. ` + `Every thread's code facts (changedSinceComment, touchedBy) are \`unknown\`; ` + @@ -580,6 +630,11 @@ function writeDegradedCommentStatusReport( headDrift: false, headMovedDuringFetch: false, inlineComments: 0, + // Same shape as the success report: `null` is "the gate did not + // refuse", which is honest here — a degraded run never got as far as + // asking. The absent-worktree flag above already carries the + // unavailability. + worktreeUntrusted: null, summary: summarizeThreads([]), threads: [], }, diff --git a/packages/cli/src/commands/review/fetch-pr.test.ts b/packages/cli/src/commands/review/fetch-pr.test.ts index c8ddd8d475a..9d70b52ea84 100644 --- a/packages/cli/src/commands/review/fetch-pr.test.ts +++ b/packages/cli/src/commands/review/fetch-pr.test.ts @@ -278,6 +278,10 @@ const producerMocks = vi.hoisted(() => ({ buildDiffPlan: vi.fn(), actualBuildDiffPlan: undefined as unknown as (...a: unknown[]) => unknown, writeStderrLine: vi.fn(), + // Admits by default, which is what the real one answers for every checkout + // outside a review temp dir — the shape all but one of these tests are in. + // The ordering test steers it to a refusal; nothing else touches it. + untrustedRepositoryFrom: vi.fn((..._args: unknown[]): string | null => null), // The prebuild runs Agent 7's real build-test against the plan just // written; stubbed here because this suite's fs is a mock and the wiring — // when it runs, against what, and what lands in the plan — is the contract. @@ -333,15 +337,31 @@ vi.mock('../../utils/stdioHelpers.js', () => ({ writeStderrLineSafe: producerMocks.writeStderrLine, })); -vi.mock('../../services/review-worktree-lease.js', () => ({ - clearReviewWorktreeLease: vi.fn(), - clearReviewWorktreeLeaseIfOwned: vi.fn(), - createReviewWorktreeLease: vi.fn(), - readReviewWorktreeLease: vi.fn((): unknown => null), - reviewLeaseHeldByAnotherSession: vi.fn((): boolean => false), - reviewLeasePath: (repositoryRoot: string, target: string) => - `${repositoryRoot}/.qwen/tmp/qwen-review-lease-${target}.json`, -})); +vi.mock('../../services/review-worktree-lease.js', () => { + const readReviewWorktreeLease = vi.fn( + (_repositoryRoot: string, _target: string): unknown => null, + ); + return { + clearReviewWorktreeLease: vi.fn(), + clearReviewWorktreeLeaseIfOwned: vi.fn(), + createReviewWorktreeLease: vi.fn(), + readReviewWorktreeLease, + // The found-at variant the held-lease refusal uses: delegate so the + // `mockReturnValueOnce` steering above reaches both. + readReviewWorktreeLeaseAt: (repositoryRoot: string, target: string) => { + const lease = readReviewWorktreeLease(repositoryRoot, target); + return lease + ? { + lease, + path: `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`, + } + : null; + }, + reviewLeaseHeldByAnotherSession: vi.fn((): boolean => false), + reviewLeasePath: (repositoryRoot: string, target: string) => + `${repositoryRoot}/.qwen/review-leases/qwen-review-lease-${target}.json`, + }; +}); vi.mock('./lib/gh.js', async (importOriginal) => { const actual = await importOriginal(); @@ -363,6 +383,14 @@ vi.mock('./lib/git.js', () => ({ releaseWorktree: producerMocks.releaseWorktree, })); +// PARTIAL: only the launch-directory gate is steered. The rest of this module +// — `sanitizedGitEnv`, `untrustedGitfile` — has to stay real, because the +// worktree gate is one of the things the report-assembly path exercises. +vi.mock('./lib/worktree.js', async (importOriginal) => ({ + ...(await importOriginal()), + untrustedRepositoryFrom: producerMocks.untrustedRepositoryFrom, +})); + vi.mock('./lib/merge-base.js', () => ({ resolveMergeBase: producerMocks.resolveMergeBase, })); @@ -465,6 +493,7 @@ describe('fetch-pr report assembly', () => { throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); }); producerMocks.refExists.mockReturnValue(false); + producerMocks.untrustedRepositoryFrom.mockReturnValue(null); producerMocks.git.mockImplementation((...args: string[]) => args[0] === 'rev-parse' ? 'f00df00df00d' : '', ); @@ -1160,8 +1189,37 @@ describe('fetch-pr report assembly', () => { producerMocks.git.mock.invocationCallOrder[0]!, ); expect(leaseOrder).toBeLessThan( - producerMocks.execFileSync.mock.invocationCallOrder[0]!, + producerMocks.gitOpt.mock.invocationCallOrder[0]!, + ); + }); + + it('refuses a poisoned launch directory before the sweep and the fetch', async () => { + // The launch directory is where every git command here without an + // explicit `-C` finds its repository, and in the nested geometry that + // directory sits inside the OUTER review's read-write mount. Gating only + // the `worktree add` at step 4 left `cleanStale`'s force-remove and the + // PR fetch resolving through the very pointer the gate distrusts — a + // fetch through a planted repository loads its transport config and runs + // it on the host. + // + // So the assertion is ordering, not just refusal: NOTHING may have run. + // Not the lease read, not the sweep, not one git call. + producerMocks.untrustedRepositoryFrom.mockReturnValue( + 'the launch dir resolves to an admin entry inside the review temp dir', + ); + + await expect(reportFor({})).rejects.toThrow( + /refusing to review PR #42 from this directory/, + ); + + expect(producerMocks.untrustedRepositoryFrom).toHaveBeenCalledWith( + process.cwd(), ); + expect(producerMocks.releaseWorktree).not.toHaveBeenCalled(); + expect(producerMocks.git).not.toHaveBeenCalled(); + expect(producerMocks.execFileSync).not.toHaveBeenCalled(); + expect(vi.mocked(readReviewWorktreeLease)).not.toHaveBeenCalled(); + expect(vi.mocked(createReviewWorktreeLease)).not.toHaveBeenCalled(); }); }); @@ -1218,13 +1276,17 @@ describe('fetch-pr report assembly', () => { await expect(reportFor({})).rejects.toThrow( 'Failed to fetch PR #42 metadata', ); - expect(producerMocks.execFileSync).toHaveBeenCalledWith( - 'git', - ['branch', '-D', 'qwen-review/pr-42'], - // Sanitized env: a delete must land in the repository the caller - // named, not the one an exported `GIT_DIR` points at. - expect.objectContaining({ stdio: 'pipe', env: expect.any(Object) }), + // Through `lib/git`'s gated wrapper, not a direct `execFileSync`: the + // rollback runs from the launch directory, `branch -D` is a + // reference-transaction hook channel, and an ungated spawn there executes + // whatever hooks a planted pointer configures. The wrapper also carries + // the sanitized env and the timeout the direct spawn lacked. + expect(producerMocks.gitOpt).toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-42', ); + expect(producerMocks.execFileSync).not.toHaveBeenCalled(); expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalledWith( process.cwd(), 'pr-42', @@ -1235,13 +1297,36 @@ describe('fetch-pr report assembly', () => { // `branch -D` lets another session through the emptied gate while the // deletion is still pending. Compare the FIRST clear: the outer catch's // second clear fires after the branch leg anyway. - expect( - producerMocks.execFileSync.mock.invocationCallOrder[0]!, - ).toBeLessThan( + expect(producerMocks.gitOpt.mock.invocationCallOrder[0]!).toBeLessThan( vi.mocked(clearReviewWorktreeLeaseIfOwned).mock.invocationCallOrder[0]!, ); }); + it('lets the step-4 launch-dir refusal propagate unwrapped, with no rollback', async () => { + // The second ask sits OUTSIDE the try whose catch rolls the fetched ref + // back. Thrown inside, the refusal was caught by that rollback, which + // deleted the ref through the very pointer the refusal had just declared + // untrusted — `branch -D` is a reference-transaction hook channel — and + // then re-wrapped the refusal as `Failed to create worktree at …`, + // indistinguishable from an infrastructure failure. Outside, it reaches + // the lease rollback, which spawns no git at all, and the user unmangled. + producerMocks.untrustedRepositoryFrom + .mockReturnValueOnce(null) // the hoisted gate at the top of the run + .mockReturnValueOnce( + '/repo/.qwen/tmp/review-pr-42 resolves to an admin entry inside the review temp dir', + ); + + await expect(reportFor({})).rejects.toThrow( + /^refusing to create a review worktree: /, + ); + expect(producerMocks.gitOpt).not.toHaveBeenCalledWith( + 'branch', + '-D', + 'qwen-review/pr-42', + ); + expect(vi.mocked(clearReviewWorktreeLeaseIfOwned)).toHaveBeenCalled(); + }); + it('clears the lease when the worktree add fails', async () => { producerMocks.git.mockImplementation((...args: string[]) => { if (args[0] === 'worktree') throw new Error('disk full'); @@ -2176,10 +2261,10 @@ describe('fetch-pr report assembly', () => { .spyOn(mod, 'gitProbe') .mockImplementation((...args: string[]) => args[0] === 'merge-base' - ? { out: null, status: 128 } + ? { out: null, status: 128, refusal: null } : args[0] === 'rev-parse' - ? { out: ANCHOR, status: 0 } - : { out: '', status: 0 }, + ? { out: ANCHOR, status: 0, refusal: null } + : { out: '', status: 0, refusal: null }, ); try { const report = await reportFor({ since: ANCHOR }); @@ -2266,10 +2351,13 @@ describe('fetch-pr report assembly', () => { .spyOn(mod, 'gitProbe') .mockImplementation((...args: string[]) => args[0] === probe - ? (answer as { out: string | null; status: number }) + ? { + ...(answer as { out: string | null; status: number }), + refusal: null, + } : args[0] === 'rev-parse' - ? { out: ANCHOR, status: 0 } - : { out: '', status: 0 }, + ? { out: ANCHOR, status: 0, refusal: null } + : { out: '', status: 0, refusal: null }, ); try { const report = await reportFor({ since: ANCHOR }); diff --git a/packages/cli/src/commands/review/fetch-pr.ts b/packages/cli/src/commands/review/fetch-pr.ts index 8f54ebc3a99..ef2f645cd21 100644 --- a/packages/cli/src/commands/review/fetch-pr.ts +++ b/packages/cli/src/commands/review/fetch-pr.ts @@ -26,7 +26,6 @@ // LLM reads to drive the rest of Step 1. import type { CommandModule } from 'yargs'; -import { execFileSync } from 'node:child_process'; import { createHash } from 'node:crypto'; import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; @@ -34,11 +33,10 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { clearReviewWorktreeLeaseIfOwned, createReviewWorktreeLease, - readReviewWorktreeLease, + readReviewWorktreeLeaseAt, reviewLeaseHeldByAnotherSession, - reviewLeasePath, } from '../../services/review-worktree-lease.js'; -import { sanitizedGitEnv } from './lib/worktree.js'; +import { untrustedGitfile, untrustedRepositoryFrom } from './lib/worktree.js'; import { setGhHost } from './lib/gh.js'; import { getPlatformReader } from './lib/platform/registry.js'; import type { ReviewPlatformReader } from './lib/platform/types.js'; @@ -639,17 +637,11 @@ function cleanStale(prNumber: string): void { ); } const ref = reviewBranch(prNumber); - if (refExists(ref)) { - tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { - stdio: 'pipe', - // Same reason as every other git spawn in this pipeline: a delete must - // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. - env: sanitizedGitEnv(), - }), - ); - } + // Through `lib/git`'s wrapper, not a direct spawn: `branch -D` finds its + // repository from `process.cwd()` and is a reference-transaction hook + // channel, so an ungated one runs whatever hooks a planted pointer + // configures. `gitOpt` never throws, which is what `tryRemove` was for. + if (refExists(ref)) gitOpt('branch', '-D', ref); } /** sha256 of a file's raw bytes, or null when it cannot be read. */ @@ -709,11 +701,35 @@ function tryResume( const markerResumes = marker.resumes.filter( (r) => r.sessionId.toLowerCase() !== currentKey, ).length; + // Before reading anything through this tree's own pointer: `--resume` + // coexists with the sandbox on the very lane it polices, and the tree it + // reads is the one the previous run's containerized commands could write. + // `status` REFRESHES THE INDEX, so a planted `core.fsmonitor` runs on the + // host inside the very command that collects the ruling's evidence — the + // attack does not need the resume to succeed. See `untrustedGitfile`. + // REFUSE TO FRESH, not refuse to crash. Throwing here propagates out of + // `runFetchPr`, the enclosing catch rolls the lease back and re-throws, and + // `cleanStale` — the thing that would REMOVE the planted tree — is never + // reached. That contradicts what this file, the `--resume` describe and the + // docs all promise: the flag never fails a run that could start over. So + // this returns a refusal like every other one, the fresh path runs, and the + // planted tree is swept on the way. + if (untrustedGitfile(wt) !== null) { + return { + resumed: false, + reason: 'worktree-untrusted', + priorFetchedSha: null, + }; + } // `--porcelain` prints nothing on a clean tree; a null (the command could // not run) is treated as dirty. `--untracked-files=normal` explicitly, so // a `status.showUntrackedFiles=no` tuning cannot hide residue that is not - // in the PR. + // in the PR. `core.fsmonitor` inert here too: the gate above proves the + // pointer, this proves the command cannot be turned into an execution even + // if some future path reaches it ungated. const status = gitOpt( + '-c', + 'core.fsmonitor=', '-C', wt, 'status', @@ -826,6 +842,46 @@ async function runFetchPr(args: FetchPrArgs): Promise { const ref = reviewBranch(prNumber); const wt = worktreePath(prNumber); + // BEFORE the first git command of the run, not at step 4. Every git + // invocation this command makes without an explicit `-C` — `cleanStale`'s + // `worktree remove --force`, step 2's `git fetch`, the branch rollbacks — + // discovers its repository from `process.cwd()`, and in the nested geometry + // `mountRootFor` documents (a review running inside another review's + // worktree) that launch directory sits inside the outer review's read-write + // mount: the outer PR's containerized build can rewrite the pointer git + // resolves here. `git fetch` through a rewritten pointer loads the planted + // repository's transport config — `core.sshCommand`, `remote.*.uploadpack`, + // a `url.*.insteadOf` rewrite — and runs it on the host. Gating only the + // `worktree add` at step 4 left every command before it resolving through + // exactly the pointer the gate exists to distrust. + // + // Kept at this call site even though `lib/git`'s wrappers now ask the same + // question (see `assertTrustedLaunchDir`), because this command changes + // state that is NOT a git call before it makes one: the lease read and the + // lease write both land before `cleanStale`, and a run that registers a + // lease and then dies at the first git command has taken the lock for + // nothing. The ordering test pins exactly that — no lease read, no sweep, + // no git call. + // + // A throw, not a refusal-to-fresh like the `--resume` gate's: that one can + // fall back because the fresh path is the safe one, and here the fresh path + // is what has been poisoned. There is no command left to run in a + // repository this process cannot locate honestly — `cleanStale` would sweep + // through the same pointer. + // + // Says nothing outside a mount (`mountRootFor` answers null), so an ordinary + // `qwen review` from a normal checkout never reaches the question. + const launchUntrusted = untrustedRepositoryFrom(process.cwd()); + if (launchUntrusted !== null) { + throw new Error( + `refusing to review PR #${prNumber} from this directory: ` + + `${launchUntrusted}. Every git command below — the stale-worktree ` + + `sweep, the PR fetch, the worktree creation — would resolve through ` + + `that pointer and run whatever the repository it names configures. ` + + `Run the review from a checkout outside the review temp dir.`, + ); + } + // The lease is also a lock. The worktree path is fixed per PR number, so // the stale-clean below would remove a worktree ANOTHER session is actively // reviewing — that is precisely how #9205 destroyed a round-4 review mid-run. @@ -851,15 +907,15 @@ async function runFetchPr(args: FetchPrArgs): Promise { `concurrent session.`, ); } - const holder = readReviewWorktreeLease(process.cwd(), leaseTarget); - if (reviewLeaseHeldByAnotherSession(holder)) { + const holder = readReviewWorktreeLeaseAt(process.cwd(), leaseTarget); + if (holder && reviewLeaseHeldByAnotherSession(holder.lease)) { throw new Error( `PR #${prNumber} is already being reviewed by another session ` + - `(session ${holder.sessionId}). Same-PR reviews share one worktree ` + + `(session ${holder.lease.sessionId}). Same-PR reviews share one worktree ` + `path and cannot run concurrently, so this run refuses rather than ` + `destroy the other session's state. Wait for that session to finish ` + `— its cleanup releases the lease — or, only if that session is ` + - `gone, delete ${reviewLeasePath(process.cwd(), leaseTarget)} and ` + + `gone, delete ${holder.path} and ` + `re-run.`, ); } @@ -978,16 +1034,9 @@ async function runFetchPr(args: FetchPrArgs): Promise { ); } } catch (err) { - // Roll back the fetched ref so the next run starts clean. - tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { - stdio: 'pipe', - // Same reason as every other git spawn in this pipeline: a delete must - // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. - env: sanitizedGitEnv(), - }), - ); + // Roll back the fetched ref so the next run starts clean — through the + // gated wrapper, for the reason `cleanStale` gives. + gitOpt('branch', '-D', ref); throw new Error( `Failed to fetch PR #${prNumber} metadata: ${(err as Error).message}`, ); @@ -997,19 +1046,41 @@ async function runFetchPr(args: FetchPrArgs): Promise { const needsLocalStats = platform.kind !== 'github'; // 4. Create the ephemeral worktree. + // + // Asked a SECOND time, immediately before the write. The hoisted gate at + // the top of this function is what keeps the fetch and the sweep from + // running through a poisoned pointer; this one narrows the window between + // that answer and the checkout `worktree add` performs, which is the step + // that executes a planted filter. Neither closes the TOCTOU window a + // same-user writer has — `scratch-tree` documents the same residual — and + // one spawn is a cheap price for making the window one function long + // instead of one command long. + // + // The pointer it judges is the LAUNCH directory's, not the new tree's: + // `git()` sets no cwd, so `worktree add` finds the repository from + // `process.cwd()`. Gating `wt` was a no-op — `cleanStale` above has just + // removed whatever stood there, and a tree that does not exist has no + // pointer to distrust. + // + // OUTSIDE the try, because the catch below is a rollback. Thrown from + // inside, the refusal was caught by the path that deletes the fetched ref, + // and `branch -D` is a reference-transaction hook channel: the gate said + // "do not resolve through this pointer" and its own refusal immediately + // did, running the plant's hooks and then reporting the run as `Failed to + // create worktree at …` — indistinguishable from an infrastructure + // failure. Outside, it propagates to the lease rollback, which spawns no + // git at all, and reaches the user unmangled. + const freshUntrusted = untrustedRepositoryFrom(process.cwd()); + if (freshUntrusted !== null) { + throw new Error( + `refusing to create a review worktree: ${freshUntrusted}`, + ); + } try { mkdirSync(dirname(wt), { recursive: true }); git('worktree', 'add', wt, ref); } catch (err) { - tryRemove(() => - execFileSync('git', ['branch', '-D', ref], { - stdio: 'pipe', - // Same reason as every other git spawn in this pipeline: a delete must - // land in the repository the caller named, not the one the shell's - // `GIT_DIR` points at. - env: sanitizedGitEnv(), - }), - ); + gitOpt('branch', '-D', ref); throw new Error( `Failed to create worktree at ${wt}: ${(err as Error).message}`, ); diff --git a/packages/cli/src/commands/review/host-execution.canary.test.ts b/packages/cli/src/commands/review/host-execution.canary.test.ts new file mode 100644 index 00000000000..3a510b0c6ec --- /dev/null +++ b/packages/cli/src/commands/review/host-execution.canary.test.ts @@ -0,0 +1,665 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// One property, asked of every host-side entrance at once: with a planted +// repository in place, NOTHING the pipeline runs on the host may execute out of +// it. +// +// The gates each entrance carries are asserted in their own suites; this file +// exists because those gates were added one call site at a time, and a class +// closed call site by call site is a class that re-opens at the next call site +// somebody adds. So the fixture here is the attack, not a shape: a coherent +// plant — a rewritten gitfile naming an admin entry whose own `commondir` +// names a planted repository carrying `filter..clean`, which git EXECUTES +// during an ordinary index refresh — plus a canary file the filter writes on +// the host. Every case asserts the canary is absent afterwards, and the first +// case asserts it is PRESENT when the same command runs ungated, so a fixture +// that quietly stops being an attack fails here rather than certifying the +// gates that walked around it. +// +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { + appendFileSync, + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + renameSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { basename, dirname, join } from 'node:path'; +import { + isolateHostGitConfig, + plantAdminEntry, + plantRepository, +} from './lib/test-utils.js'; +import { + adminEntryInsideReviewTmp, + mountRootFor, + sanitizedGitEnv, + untrustedGitfile, + untrustedRepositoryFrom, + worktreeResidue, +} from './lib/worktree.js'; +import { makeGitProbe } from './comment-status.js'; +import { captureLocalDiff } from './lib/local-diff.js'; +import { git, gitOpt, gitProbe, releaseWorktree } from './lib/git.js'; +import { runBaseTree } from './base-tree.js'; +import { loadCombined } from './load-rules.js'; +import { runRevertHunk } from './revert-hunk.js'; +import type { BuildTestReport } from './build-test.js'; +import { baseWorktreePath } from './lib/paths.js'; +import { baseTreeTrustPath, runNonce } from './lib/base-tree-trust.js'; + +// On Windows `mountRootFor` refuses every absolute path (a drive letter is a +// colon), so containment cannot exist there and the question this file asks has +// no answer — the same block-level gate every containment suite uses. +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + +describe('a planted repository reaches no host-side execution', () => { + let gitIsolation: ReturnType; + const made: string[] = []; + const tmp = (prefix: string) => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), prefix))); + made.push(dir); + return dir; + }; + + beforeEach(() => { + gitIsolation = isolateHostGitConfig(); + }); + afterEach(() => { + for (const dir of made.splice(0)) + rmSync(dir, { recursive: true, force: true }); + gitIsolation.dispose(); + }); + + const g = (cwd: string, ...args: string[]) => + execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); + + /** A repository with one commit, and `.qwen/tmp` ready for its trees. */ + const repository = () => { + const repo = tmp('qwen-canary-repo-'); + g(repo, 'init', '-q', '-b', 'main'); + g(repo, 'config', 'user.email', 't@t.t'); + g(repo, 'config', 'user.name', 't'); + writeFileSync(join(repo, 'a.ts'), 'export const x = 1;\n'); + g(repo, 'add', 'a.ts'); + g(repo, 'commit', '-qm', 'init'); + mkdirSync(join(repo, '.qwen', 'tmp'), { recursive: true }); + return repo; + }; + + /** + * The single-layer geometry: a review worktree under `/.qwen/tmp`, with + * `plant()` to rewrite its gitfile the way the reviewed code would. + * + * Split in two because one arm needs the pointer poisoned AFTER a first call + * has answered clean — the shape a verdict memoized for the whole process + * gets wrong, and the one a fixture that plants before it chdirs can never + * reach. + */ + const mountedTree = () => { + const repo = repository(); + const canaryDir = tmp('qwen-canary-out-'); + const canary = join(canaryDir, 'PWNED'); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + g(repo, 'worktree', 'add', '-q', '--detach', tree, 'HEAD'); + const headSha = g(tree, 'rev-parse', 'HEAD'); + const realEntry = join(repo, '.git', 'worktrees', 'review-pr-1'); + const plant = () => { + const common = plantRepository( + join(repo, '.qwen', 'tmp', '.evil-common'), + join(repo, '.git'), + canary, + ); + const entry = plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + realEntry, + tree, + common, + ); + // The index refresh only re-hashes a file whose stat changed, and only a + // re-hash runs the clean filter. This is what a review's own build leaves + // behind, so it is the ordinary state, not a contrivance. + writeFileSync(join(tree, 'a.ts'), 'export const x = 2;\n'); + return { entry, common }; + }; + return { repo, tree, canary, headSha, plant }; + }; + + const poisoned = () => { + const layout = mountedTree(); + const { entry, common } = layout.plant(); + return { ...layout, entry, common }; + }; + + itWhereContainmentExists( + 'the fixture is a live attack: ungated, the index refresh runs the plant', + () => { + // The negative control the rest of the file rests on. This is the exact + // command `worktreeResidue` reaches once its identity gates pass — + // pinned to the admin entry the gitfile names, `core.fsmonitor` already + // emptied — so a green assertion below means a gate refused, not that + // the plant was inert. + const { tree, entry, canary } = poisoned(); + expect(existsSync(canary)).toBe(false); + execFileSync( + 'git', + [ + `--git-dir=${entry}`, + `--work-tree=${tree}`, + '-c', + 'core.fsmonitor=', + 'status', + '--porcelain', + ], + { cwd: tree, encoding: 'utf8' }, + ); + expect(existsSync(canary)).toBe(true); + }, + ); + + itWhereContainmentExists( + 'the residue probe refuses to measure through it (scratch-tree, agent-prompt)', + () => { + // Entrance 1: `worktreeResidue` runs BEFORE the reuse/rebuild gates at + // both of its call sites, so gating only the checkouts left the + // measurement itself as the execution. Every identity check it carries + // passes here — the plant writes both halves of the round trip, HEAD is + // the copied entry's so the sha pin matches, and no symlink is involved. + const { tree, canary, headSha } = poisoned(); + const residue = worktreeResidue(tree, 12, headSha); + expect(existsSync(canary)).toBe(false); + expect(residue.paths).toEqual([]); + expect(residue.unmeasured).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + 'a launch directory that resolves into the plant is refused (fetch-pr)', + () => { + // Entrance 2: `cleanStale` and step 2's `git fetch` discover their + // repository from `process.cwd()`, so a poisoned launch directory made + // them run the plant's transport config long before the step-4 gate. The + // same answer from a SUBDIRECTORY, which has no `.git` of its own — the + // nested/dogfood shape a review actually launches from. + const { tree, canary } = poisoned(); + expect(untrustedRepositoryFrom(tree, mountRootFor)).toContain( + 'review temp dir', + ); + const sub = join(tree, 'packages', 'cli'); + mkdirSync(sub, { recursive: true }); + expect(untrustedRepositoryFrom(sub, mountRootFor)).toContain( + 'review temp dir', + ); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'the comment-status code probe degrades to unknown rather than reading it', + () => { + // Entrance 3: four ungated `git -C ` probes. No + // execution witness is claimed for `log`/`rev-parse` — what is at stake + // is the answer: `changedSinceComment: false` sourced from a repository + // the PR author planted is this command certifying that the code behind + // a blocker thread did not move. + const { tree, canary, headSha } = poisoned(); + const probe = makeGitProbe(tree); + expect(probe('a.ts', headSha)).toEqual({ + changed: 'unknown', + touchedBy: [], + touchedByTotal: 0, + }); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'every wrapper in lib/git refuses to run from a poisoned launch directory', + () => { + // The launch-directory half, closed where all of it goes through rather + // than at each command that happens to run git: `load-rules` reading this + // review's own rules with `show :`, `submit` reading where a + // submission goes, `match-remote` choosing the remote, `cleanup` pruning + // — none of them knows it is asking this question, and none of them + // should have to. + const { tree, canary } = poisoned(); + const saved = process.cwd(); + try { + process.chdir(tree); + // The throwing wrapper throws... + expect(() => git('rev-parse', 'HEAD')).toThrow(/review temp dir/); + // ...and the probing wrapper answers its own "could not run" value, + // which every caller already handles, rather than the plant's answer. + expect(gitOpt('remote', 'get-url', 'origin')).toBeNull(); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'the local-review capture refuses a poisoned launch directory (capture-local)', + () => { + // The local lane's copy of the fetch-pr entrance. `captureLocalDiff` + // finds the repository from `process.cwd()` with `--show-toplevel`, and + // the `git diff` that follows is not a safe read through a plant: it + // refreshes the index (clean filters) and honours + // `diff..command`. + const { tree, canary } = poisoned(); + const saved = process.cwd(); + try { + process.chdir(tree); + expect(() => captureLocalDiff({})).toThrow(/review temp dir/); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'the base-tree reuse fast path does not certify a planted tree (base-tree)', + () => { + // The entrance a census found rather than the review: the reuse branch + // RETURNS before the gate that guards the rebuild, on two facts that both + // live inside the mount — a marker file in the base tree, and + // `rev-parse HEAD` resolved through that tree's own `.git`. Reused, the + // A/B's BASE side is the reviewed code's own tree, and every "the base + // behaves differently" verdict belongs to its author. + const repo = repository(); + const canaryDir = tmp('qwen-canary-out-'); + const canary = join(canaryDir, 'PWNED'); + const worktree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + g(repo, 'worktree', 'add', '-q', '--detach', worktree, 'HEAD'); + const baseSha = g(worktree, 'rev-parse', 'HEAD'); + // The base tree the pipeline would build, standing and marked. Through + // the real path helper, so the fixture cannot drift from the location + // the command actually reuses. + const tree = baseWorktreePath(worktree); + g(repo, 'worktree', 'add', '-q', '--detach', tree, 'HEAD'); + const plan = join(repo, 'plan.json'); + writeFileSync(plan, `${JSON.stringify({ mergeBaseSha: baseSha })}\n`); + // Stamped with THIS run's secret, so the reuse branch reaches the pointer + // gate this test is about instead of being turned away by the run fence + // that keeps an earlier run's tree — or a forged stamp — from being + // reused at all. + writeFileSync( + join(tree, '.qwen-review-base-ok'), + `${baseSha}\n${runNonce(baseTreeTrustPath(worktree, plan))}\n`, + ); + const common = plantRepository( + join(repo, '.qwen', 'tmp', '.evil-common'), + join(repo, '.git'), + canary, + ); + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + join(repo, '.git', 'worktrees', basename(tree)), + tree, + common, + ); + + const report = runBaseTree({ + plan, + worktree, + timeout: 60_000, + install: false, + // The build seam: reaching it at all means the tree was REBUILT, which + // is the acceptable outcome here — what must not happen is the reuse + // branch returning the planted tree as an already-built base. + build: () => + ({ + ok: true, + toolchain: 'npm', + build: [], + note: 'built', + }) as unknown as BuildTestReport, + }); + + expect(JSON.stringify(report)).not.toContain('reusing it'); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'revert-hunk refuses a scratch tree whose pointer was rewritten', + () => { + // `git apply` writes into the tree without running a filter, so this arm + // claims no execution — what it refuses is deciding a probe's outcome on + // `check-attr` and `ls-files` answers that come out of the plant. The + // refusal is a harness fact: nothing is claimed about the hunk. + const { tree, canary } = poisoned(); + const patch = join(tmp('qwen-canary-patch-'), 'p.diff'); + writeFileSync( + patch, + [ + 'diff --git a/a.ts b/a.ts', + 'index 0000000..1111111 100644', + '--- a/a.ts', + '+++ b/a.ts', + '@@ -1 +1 @@', + '-export const x = 1;', + '+export const x = 2;', + '', + ].join('\n'), + ); + const report = runRevertHunk({ + diff: patch, + tree, + hunk: 'a.ts:1', + } as unknown as Parameters[0]); + expect(report.applied).toBe(false); + expect(JSON.stringify(report)).toContain('review temp dir'); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'an ancestor layer of the nested geometry is inside the writable surface too', + () => { + // Entrance 4: a review running inside another review's worktree nests one + // `.qwen/tmp` inside another. The MOUNT is the innermost one — widening + // it would pull `/.git` and every sibling checkout into the + // container — but the OUTER review's containerized phase held the outer + // temp dir read-write, so a plant one layer up is reachable and, judged + // against the innermost root alone, was admitted by every gate. + const repo = repository(); + const canaryDir = tmp('qwen-canary-out-'); + const canary = join(canaryDir, 'PWNED'); + const outer = join(repo, '.qwen', 'tmp', 'review-pr-1'); + g(repo, 'worktree', 'add', '-q', '--detach', outer, 'HEAD'); + const inner = join(outer, '.qwen', 'tmp', 'review-pr-2'); + mkdirSync(dirname(inner), { recursive: true }); + g(outer, 'worktree', 'add', '-q', '--detach', inner, 'HEAD'); + // git puts a linked worktree's admin entry under the MAIN repository's + // `.git/worktrees/` even when `worktree add` runs from inside another + // linked worktree — which is why widening the judgement costs the honest + // layout nothing. + const realEntry = join(repo, '.git', 'worktrees', 'review-pr-2'); + expect(existsSync(realEntry)).toBe(true); + + const innerRoot = mountRootFor(inner); + expect(innerRoot).toBe(realpathSync(join(outer, '.qwen', 'tmp'))); + + const common = plantRepository( + join(repo, '.qwen', 'tmp', '.evil-common'), + join(repo, '.git'), + canary, + ); + // Planted in the OUTER temp dir: outside the inner root, inside the + // outer review's read-write mount. + const entry = plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + realEntry, + inner, + common, + ); + expect(mountRootFor(entry)).toBe( + realpathSync(join(repo, '.qwen', 'tmp')), + ); + + // The judgement, asked against the innermost root the mount was cut at: + // it must still refuse, because the question is where reviewed code can + // write, not where the bind mount starts. + expect(adminEntryInsideReviewTmp(entry, () => innerRoot, inner)).toBe( + true, + ); + expect(untrustedGitfile(inner, mountRootFor)).toContain( + 'review temp dir', + ); + writeFileSync(join(inner, 'a.ts'), 'export const x = 2;\n'); + expect( + worktreeResidue(inner, 12, g(outer, 'rev-parse', 'HEAD')).unmeasured, + ).toContain('review temp dir'); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'and the honest nested layout still measures, resolves and probes', + () => { + // The admit arm, without which every assertion above is satisfied by a + // gate that refuses everything — and the layout it admits is the + // pipeline's own dogfood geometry, two layers deep. + const repo = repository(); + const outer = join(repo, '.qwen', 'tmp', 'review-pr-1'); + g(repo, 'worktree', 'add', '-q', '--detach', outer, 'HEAD'); + const inner = join(outer, '.qwen', 'tmp', 'review-pr-2'); + mkdirSync(dirname(inner), { recursive: true }); + g(outer, 'worktree', 'add', '-q', '--detach', inner, 'HEAD'); + const headSha = g(inner, 'rev-parse', 'HEAD'); + + expect(untrustedGitfile(inner, mountRootFor)).toBeNull(); + expect(untrustedRepositoryFrom(inner, mountRootFor)).toBeNull(); + writeFileSync(join(inner, '__probe__.test.ts'), 'probe\n'); + const residue = worktreeResidue(inner, 12, headSha); + expect(residue.unmeasured).toBeUndefined(); + expect(residue.paths).toEqual(['__probe__.test.ts']); + expect(makeGitProbe(inner)('a.ts', headSha).changed).not.toBe('unknown'); + }, + ); + + itWhereContainmentExists( + 'a pointer planted AFTER the first clean answer is refused too (lib/git)', + () => { + // The window is one CLI command rather than the whole review — each + // `qwen review ` is its own process — but it is the whole command, + // and in the nested geometry the OUTER review's containerized build/test + // phase holds this directory read-write for that long. A verdict memoized + // for the process lifetime served the first clean answer to every later + // wrapper, so the run's own `status` refreshed the plant's index and ran + // its clean filter on the host. Re-asked inside a review temp dir; still + // memoized outside one, where the answer costs no syscall. + const { tree, canary, plant } = mountedTree(); + const saved = process.cwd(); + try { + process.chdir(tree); + // Clean, and the answer a process-lifetime memo would keep serving. + expect(gitOpt('rev-parse', 'HEAD')).not.toBeNull(); + plant(); + // `status`, not `rev-parse`: this is the command that refreshes the + // index, so the canary below is a witness and not a decoration. + expect(() => git('status', '--porcelain')).toThrow(/review temp dir/); + expect(gitOpt('remote', 'get-url', 'origin')).toBeNull(); + // ...and the refusal is still distinguishable from git's own answer. + expect(gitProbe('remote', 'get-url', 'origin')).toEqual({ + out: null, + status: null, + refusal: expect.stringContaining('review temp dir'), + }); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'an ancestor rename does not memoize TRUSTED over the re-stood-up spelling (lib/git)', + () => { + // The rename attack on the launch-dir memo. Judged once while clean, + // the directory is inside a review temp dir — but `mv .qwen + // .qwen-real` makes `mountRootFor` answer null for the live cwd's + // stale spelling, and a memo keyed on "no mount root" recorded TRUSTED + // for it. The spelling re-stood-up over a filter-carrying plant then + // inherited the verdict and the next `status` ran the planted clean + // filter on the host. The memo's key is the LEXICAL marker scan now: + // a spelling that carries the marker is never memoized, and the + // renamed-away state is itself a refusal. + const { repo, tree, canary, plant } = mountedTree(); + const saved = process.cwd(); + try { + process.chdir(tree); + // Clean, and — inside a review temp dir — never memoized. + expect(gitOpt('rev-parse', 'HEAD')).not.toBeNull(); + + // The ancestor is renamed out from under the live cwd. The stale + // spelling still matches the marker lexically, so this call is a + // refusal — not the "nothing to police" the old gate memoized. + renameSync(join(repo, '.qwen'), join(repo, '.qwen-real')); + expect(gitProbe('rev-parse', 'HEAD')).toEqual({ + out: null, + status: null, + refusal: expect.stringContaining('review temp dir'), + }); + + // The same spelling is stood back up, with the plant under it. + mkdirSync(join(repo, '.qwen', 'tmp'), { recursive: true }); + renameSync(join(repo, '.qwen-real', 'tmp', 'review-pr-1'), tree); + plant(); + // `status`, not `rev-parse`: the command that refreshes the index, + // so the canary is a witness and not a decoration. + expect(() => git('status', '--porcelain')).toThrow(/review temp dir/); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'releaseWorktree reports the release it could not make (cleanup)', + () => { + // The remedy the other refusals tell a user to run. Its git calls are + // refused while its `rmSync` still clears the DIRECTORY, and reporting + // `freed: true` over that certified a release whose registration under + // `/.git/worktrees/` and whose branch both survived — so the next + // `worktree add` fails with "missing but already registered": the exact + // wedge this function's docstring exists to prevent, reported as fixed, + // with the lease released over an unswept review. + const { repo, tree, canary } = poisoned(); + const registration = join(repo, '.git', 'worktrees', 'review-pr-1'); + const saved = process.cwd(); + let released: ReturnType; + try { + process.chdir(tree); + released = releaseWorktree(tree); + } finally { + process.chdir(saved); + } + expect(released.existed).toBe(true); + expect(released.freed).toBe(false); + // A reason, because `reason` is what cleanup prints — unset, it reports + // `Failed to remove worktree : undefined`. + expect(released.reason).toContain('review temp dir'); + // The path IS gone, which is what made the false `freed` plausible, and + // what survives is the half that wedges the next run. + expect(existsSync(tree)).toBe(false); + expect(existsSync(registration)).toBe(true); + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'load-rules says a source could not be READ, not that there are none', + () => { + // `show :` is how this review gets its OWN rules. Refused, + // every source came back null, `loadCombined` read each as absent, and + // `runLoadRules` wrote an empty file and exited 0 — which + // `agent-prompt --roster` stapled into every agent brief, so the whole + // fan-out ran with none of the project's `## Code Review` rules and + // nothing in the run said why. Git answers a path absent at the ref with + // 128, so `status === null` is the only key that separates the two. + const { repo, tree, canary } = poisoned(); + const saved = process.cwd(); + try { + process.chdir(tree); + const refused = loadCombined('HEAD'); + expect(refused.loaded).toEqual([]); + expect(refused.unread).toContain('AGENTS.md'); + // The honest arm, one directory up and outside the mount: nothing was + // found either, but nothing was refused, and the two must not print the + // same sentence. + process.chdir(repo); + expect(loadCombined('HEAD').unread).toEqual([]); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'the rollback delete a refusal used to trigger runs nothing (fetch-pr)', + () => { + // `branch -D` is a reference-transaction hook channel, and fetch-pr's + // step-4 refusal was thrown INSIDE the try whose catch rolled the fetched + // ref back with an ungated `execFileSync('git', ['branch','-D', ref])` + // from the launch directory that refusal had just declared untrusted — so + // the gate's own answer executed the plant's hooks, `tryRemove` swallowed + // it, and the run died with `Failed to create worktree at …`, + // indistinguishable from an infrastructure failure. + const { tree, common, canary } = poisoned(); + const hookCanary = join(tmp('qwen-canary-hook-'), 'HOOKED'); + const hooks = join(dirname(tree), '.evil-hooks'); + mkdirSync(hooks, { recursive: true }); + const hook = join(hooks, 'reference-transaction'); + writeFileSync(hook, `#!/bin/sh\necho PWNED >> ${hookCanary}\nexit 0\n`); + chmodSync(hook, 0o755); + // The ref the rollback deletes, created BEFORE the hook is armed so + // arming it is not what writes the canary. + execFileSync( + 'git', + [`--git-dir=${common}`, 'branch', 'qwen-review/pr-1', 'HEAD'], + { encoding: 'utf8', stdio: 'pipe' }, + ); + // Into the plant's OWN config: `sanitizedGitEnv` strips `GIT_*` + // redirects, but `core.hooksPath` is read from the resolved repository. + appendFileSync( + join(common, 'config'), + `\n[core]\n\thooksPath = ${hooks}\n`, + ); + + const saved = process.cwd(); + try { + process.chdir(tree); + // The routed rollback: refused before it spawns, so the hook never runs. + expect(gitOpt('branch', '-D', 'qwen-review/pr-1')).toBeNull(); + expect(existsSync(hookCanary)).toBe(false); + // The ungated spawn the refusal path used instead — the negative + // control, so a fixture that stops being an attack fails HERE rather + // than certifying the gate that walks around it. + execFileSync('git', ['branch', '-D', 'qwen-review/pr-1'], { + stdio: 'pipe', + env: sanitizedGitEnv(), + }); + expect(existsSync(hookCanary)).toBe(true); + } finally { + process.chdir(saved); + } + expect(existsSync(canary)).toBe(false); + }, + ); + + itWhereContainmentExists( + 'the gitfile the pipeline itself writes is what the fixture rewrites', + () => { + // Coherence: if `git worktree add` ever stopped writing a gitfile, or the + // admin entry moved, the plant above would be building a shape the + // pipeline never produces and every refusal would be about nothing. + const repo = repository(); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + g(repo, 'worktree', 'add', '-q', '--detach', tree, 'HEAD'); + const real = join(repo, '.git', 'worktrees', 'review-pr-1'); + expect(readFileSync(join(tree, '.git'), 'utf8').trim()).toBe( + `gitdir: ${real}`, + ); + // And git's own backpointer is bare — the format the fixtures use. + expect(readFileSync(join(real, 'gitdir'), 'utf8').trim()).toBe( + join(tree, '.git'), + ); + }, + ); +}); diff --git a/packages/cli/src/commands/review/lib/base-tree-trust.test.ts b/packages/cli/src/commands/review/lib/base-tree-trust.test.ts new file mode 100644 index 00000000000..eb1e4ce7f09 --- /dev/null +++ b/packages/cli/src/commands/review/lib/base-tree-trust.test.ts @@ -0,0 +1,104 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The trust store is the fence's load-bearing half: if the nonce could be +// read or refreshed from inside the mount, or the recorded untracked set +// could be lost or rewritten, every base-tree test that exercises the fence +// would still pass while the property they exist for is gone. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, sep } from 'node:path'; +import { + baseTreeTrustPath, + builtTreeRecord, + recordBuiltTree, + runNonce, +} from './base-tree-trust.js'; + +describe('base-tree trust store', () => { + let repo: string; + let worktree: string; + let plan: string; + + beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'qwen-base-tree-trust-')); + worktree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(worktree, { recursive: true }); + // Production geometry: the plan lives INSIDE the mounted tmp dir. + plan = join(repo, '.qwen', 'tmp', 'qwen-review-pr-1-fetch.json'); + writeFileSync(plan, '{}'); + }); + + afterEach(() => rmSync(repo, { recursive: true, force: true })); + + it('lives beside the leases — outside the mounted tmp dir — keyed by plan identity', () => { + const p = baseTreeTrustPath(worktree, plan); + expect(p.startsWith(join(repo, '.qwen', 'review-leases') + sep)).toBe(true); + expect(p.startsWith(join(repo, '.qwen', 'tmp') + sep)).toBe(false); + // Same plan, same identity: stable within a run. + expect(baseTreeTrustPath(worktree, plan)).toBe(p); + // A re-captured plan — a new run — keys a new file. + const later = new Date(Date.now() + 60_000); + utimesSync(plan, later, later); + expect(baseTreeTrustPath(worktree, plan)).not.toBe(p); + }); + + it('creates the run secret once and hands every later asker the same one', () => { + const p = baseTreeTrustPath(worktree, plan); + const first = runNonce(p); + expect(first).toMatch(/^[0-9a-f]{32}$/); + expect(runNonce(p)).toBe(first); + // A different run (a re-captured plan) gets a different secret. + const later = new Date(Date.now() + 60_000); + utimesSync(plan, later, later); + expect(runNonce(baseTreeTrustPath(worktree, plan))).not.toBe(first); + }); + + it('heals a torn file a crashed writer left instead of wedging the run', () => { + const p = baseTreeTrustPath(worktree, plan); + mkdirSync(join(p, '..'), { recursive: true }); + writeFileSync(p, ''); // open()ed, never written: the crash window + const nonce = runNonce(p); + expect(nonce).toMatch(/^[0-9a-f]{32}$/); + expect(JSON.parse(readFileSync(p, 'utf8')).nonce).toBe(nonce); + }, 10_000); + + it('records the untracked set per tree and preserves the nonce across records', () => { + const p = baseTreeTrustPath(worktree, plan); + const nonce = runNonce(p); + const tree = `${worktree}-base`; + expect(builtTreeRecord(p, tree)).toBeNull(); + + recordBuiltTree(p, tree, 'a'.repeat(40), ['.qwen-review-base-ok', 'dist/']); + expect(builtTreeRecord(p, tree)).toEqual({ + baseSha: 'a'.repeat(40), + untracked: ['.qwen-review-base-ok', 'dist/'], + }); + expect(JSON.parse(readFileSync(p, 'utf8')).nonce).toBe(nonce); + + // A second record for another tree keeps the first. + recordBuiltTree(p, `${tree}-2`, 'b'.repeat(40), []); + expect(builtTreeRecord(p, tree)?.baseSha).toBe('a'.repeat(40)); + expect(builtTreeRecord(p, `${tree}-2`)?.baseSha).toBe('b'.repeat(40)); + }); + + it('records nothing when the trust file is unreadable — never clobbers the nonce', () => { + const p = baseTreeTrustPath(worktree, plan); + recordBuiltTree(p, `${worktree}-base`, 'a'.repeat(40), []); + expect(existsSync(p)).toBe(false); + expect(builtTreeRecord(p, `${worktree}-base`)).toBeNull(); + }); +}); diff --git a/packages/cli/src/commands/review/lib/base-tree-trust.ts b/packages/cli/src/commands/review/lib/base-tree-trust.ts new file mode 100644 index 00000000000..6f0347fdcf6 --- /dev/null +++ b/packages/cli/src/commands/review/lib/base-tree-trust.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The base-tree reuse fence's trust artifacts, persisted OUTSIDE the sandbox's +// read-write mount — beside the worktree leases, the existing "nothing mounts +// it" precedent (`REVIEW_LEASE_DIR` in lib/paths.ts). +// +// Everything the fence used to key on lives inside `.qwen/tmp`, the one +// directory reviewed code can still write: the markers are files in the base +// tree, and the run epoch was the plan's mtime, which reviewed code can stat +// and write into a forged stamp — after which the pipeline certified an +// attacker-shaped tree as the A/B base. So the stamp's second line is now a +// random per-run secret that never enters the mount: the first host process +// of a run creates it (atomically, `flag: 'wx'`), its sibling shards adopt +// it, and a mount-local writer can neither read it to forge a stamp nor +// write it to refresh one. The file is keyed by the plan's path AND mtime, +// so a re-captured plan — a new run — gets a new secret, and an earlier +// run's tree fails the fence exactly as a forged one does. +// +// The same file records, per built tree, the untracked path set the build +// legitimately left (`node_modules/`, `dist/`) — recorded host-side because +// the marker itself is attacker-writable and vouches for nothing on its own. +// At reuse, the tree's current untracked set must be a subset of the recorded +// one; anything more is a plant dropped while reviewed code held the mount, +// and the tree is rebuilt, sweeping it. + +import { createHash, randomBytes } from 'node:crypto'; +import { mkdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs'; +import { basename, dirname, join, resolve } from 'node:path'; +import { REVIEW_LEASE_DIR } from './paths.js'; +import { runEpochMs } from './prompt-record.js'; + +/** What a successful build legitimately left behind, recorded host-side. */ +export interface BuiltTreeRecord { + baseSha: string; + /** The tree's untracked AND ignored paths, as `git status` collapses them. */ + untracked: string[]; +} + +interface TrustFile { + nonce: string; + trees?: Record; +} + +/** + * The one file holding a run's base-tree trust state, named by a digest of + * the plan's path and mtime — the run identity the rest of the pipeline + * already keys on, so same-run shards share the file and a re-captured plan + * starts a fresh one. + */ +export function baseTreeTrustPath(worktree: string, planPath: string): string { + // The worktree is `/.qwen/tmp/` by construction (paths.ts's + // `worktreePath`), so two directories up is `/.qwen`. Derived + // lexically, never through git: the worktree's own pointer lives inside + // the mount, and asking git for the root would let a planted pointer + // choose where the run's secret is written — and who can read it back. + const qwenDir = dirname(dirname(resolve(worktree))); + const key = createHash('sha256') + .update(resolve(planPath)) + .update('\0') + .update(String(runEpochMs(planPath))) + .digest('hex') + .slice(0, 16); + return join(qwenDir, basename(REVIEW_LEASE_DIR), 'base-tree', `${key}.json`); +} + +function readTrust(trustPath: string): TrustFile | null { + try { + const value = JSON.parse(readFileSync(trustPath, 'utf8')) as TrustFile; + if (typeof value.nonce !== 'string' || value.nonce === '') return null; + return value; + } catch { + return null; + } +} + +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); +} + +/** + * This run's stamp secret: created on first ask, adopted on every later one, + * so every shard of the run stamps — and accepts — the same nonce. + */ +export function runNonce(trustPath: string): string { + mkdirSync(dirname(trustPath), { recursive: true }); + const fresh = randomBytes(16).toString('hex'); + try { + // `flag: 'wx'`, the lease's atomic-create shape: two shards asking + // together must not both "create" and then stamp different secrets. + writeFileSync(trustPath, `${JSON.stringify({ nonce: fresh })}\n`, { + flag: 'wx', + }); + return fresh; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; + } + // Lost the create race, or the file predates this process: adopt it. A + // read can land between the winner's open and its write, so retry briefly + // before calling the file torn. + for (let attempt = 0; attempt < 20; attempt++) { + const existing = readTrust(trustPath); + if (existing) return existing.nonce; + sleepSync(25); + } + // A crashed writer left a torn file. Healing it is what the lease's + // same-session rewrite does, for the same reason: an unreadable file is + // already read as no secret by every reader, and leaving it wedges the + // run's shards on a nonce none of them can adopt. + writeFileSync(trustPath, `${JSON.stringify({ nonce: fresh })}\n`); + return fresh; +} + +/** + * Record, host-side, the untracked path set a successful build left — the + * baseline the reuse fence's subset check compares against. Called with the + * build lock held, so the read-modify-write cannot race a sibling builder; + * the rename makes the update atomic against lock-free readers on the reuse + * fast path. + */ +export function recordBuiltTree( + trustPath: string, + tree: string, + baseSha: string, + untracked: string[], +): void { + const trust = readTrust(trustPath); + // No readable file, no record: rewriting from scratch could clobber the + // nonce a concurrent shard is stamping with. The reuse fence treats a + // missing record as a fence failure and rebuilds, which re-records. + if (!trust) return; + const trees = { ...trust.trees, [tree]: { baseSha, untracked } }; + const tmp = `${trustPath}.${process.pid}.tmp`; + writeFileSync(tmp, `${JSON.stringify({ ...trust, trees })}\n`); + renameSync(tmp, trustPath); +} + +/** What {@link recordBuiltTree} stored for a tree, or null. */ +export function builtTreeRecord( + trustPath: string, + tree: string, +): BuiltTreeRecord | null { + return readTrust(trustPath)?.trees?.[tree] ?? null; +} diff --git a/packages/cli/src/commands/review/lib/git.integration.test.ts b/packages/cli/src/commands/review/lib/git.integration.test.ts index ba0fd76f989..9d74c57d062 100644 --- a/packages/cli/src/commands/review/lib/git.integration.test.ts +++ b/packages/cli/src/commands/review/lib/git.integration.test.ts @@ -20,7 +20,12 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { gitProbe, gitRawTolerateDiff, releaseWorktree } from './git.js'; +import { + gitOpt, + gitProbe, + gitRawTolerateDiff, + releaseWorktree, +} from './git.js'; import { NULL_DEVICE } from './diff-flags.js'; import { isolateHostGitConfig } from './test-utils.js'; @@ -221,6 +226,74 @@ describe('releaseWorktree', () => { git('worktree', 'add', '-q', 'wt-link', '-b', 'topic2'), ).not.toThrow(); }); + + it('reports the release it could not even ASK git to make', () => { + // `{status: null, refusal: null}` is the probe's third shape: spawn + // ENOENT or the timeout kill — git was never asked. Keying "not freed" + // on the refusal alone read that as "no objection", while the rmSync + // fallback cleared the DIRECTORY anyway, so the result certified + // `freed: true` over a registration and branch that both survived — and + // the next `worktree add` met "missing but already registered". Driven + // with a stripped PATH, the pattern this file's gitProbe block uses. + git('worktree', 'add', '-q', 'wt', '-b', 'topic'); + const savedPath = process.env['PATH']; + let got: ReturnType | undefined; + try { + process.env['PATH'] = join(repo, 'no-such-bin'); + got = releaseWorktree(join(repo, 'wt')); + } finally { + process.env['PATH'] = savedPath; + } + + expect(got).toMatchObject({ existed: true, freed: false }); + // `reason` set whenever `existed && !freed` — cleanup prints it. + expect(got?.reason).toBeTruthy(); + // The directory IS gone (rmSync is not git); the registration survived. + expect(existsSync(join(repo, 'wt'))).toBe(false); + expect(fwd(git('worktree', 'list'))).toContain( + fwd(join(realpathSync(repo), 'wt')), + ); + // Restored, a real prune clears the registration and the path is + // reusable — the wedge the false `freed` would have hidden. + expect(releaseWorktree(join(repo, 'wt'))).toMatchObject({ + existed: false, + freed: false, + }); + expect(() => + git('worktree', 'add', '-q', 'wt', '-b', 'topic2'), + ).not.toThrow(); + }); + + it('degrades through the result — never throws — when the cwd is deleted mid-release', () => { + // The never-throws contract starts before the first git call: `resolve` + // of the RELATIVE path production callers pass reads the cwd, and so does + // `redirectedAncestor`'s default `stopAt = process.cwd()` — both threw + // `uv_cwd` ENOENT here once the directory was gone, ahead of every + // degradation the probe carries. + git('worktree', 'add', '-q', 'wt', '-b', 'topic'); + const gone = join(repo, 'gone'); + mkdirSync(gone); + process.chdir(gone); + let got: ReturnType | undefined; + try { + rmSync(gone, { recursive: true, force: true }); + // The precondition, asserted rather than assumed. + expect(process.cwd).toThrow(/uv_cwd/); + expect(() => { + got = releaseWorktree('wt'); + }).not.toThrow(); + } finally { + process.chdir(repo); + } + expect(got).toMatchObject({ existed: true, freed: false }); + expect(got?.reason).toBeTruthy(); + // Nothing was removed — and the release retried from a live cwd completes. + expect(existsSync(join(repo, 'wt'))).toBe(true); + expect(releaseWorktree(join(repo, 'wt'))).toMatchObject({ + existed: true, + freed: true, + }); + }); }); describe('gitRawTolerateDiff', () => { @@ -362,6 +435,7 @@ describe('gitProbe — the exit status the anchor taxonomy rests on', () => { expect(gitProbe('-C', repo, 'rev-parse', 'HEAD')).toEqual({ out: null, status: null, + refusal: null, }); } finally { process.env['PATH'] = savedPath; @@ -370,4 +444,36 @@ describe('gitProbe — the exit status the anchor taxonomy rests on', () => { rmSync(repo, { recursive: true, force: true }); } }); + + it('answers "could not be run" when the process cwd no longer exists', () => { + // The launch-dir pre-check reads `process.cwd()`, which throws ENOENT once + // the directory is gone. Left outside the try it took the whole probe with + // it — and `releaseWorktree`'s documented never-throws contract, cleanup's + // `report()` and fetch-pr's `cleanStale` all call this with no catch of + // their own, so a review whose worktree was swept out from under it (the + // nested geometry, an operator `rm -rf` mid-run) aborted the sweep before + // the branch delete and the lease release, leaving the stale lease that + // refuses every later cleanup of that target. The pre-gate spawn degraded + // here instead of throwing: its own ENOENT landed in the catch. + const gone = join(repo, 'deleted-out-from-under'); + mkdirSync(gone, { recursive: true }); + process.chdir(gone); + try { + rmSync(gone, { recursive: true, force: true }); + // The precondition, asserted rather than assumed: this is the throw the + // pre-check used to let escape. + expect(process.cwd).toThrow(/uv_cwd/); + + expect(gitProbe('worktree', 'prune')).toEqual({ + out: null, + status: null, + refusal: null, + }); + // `gitOpt` and `refExists` delegate here, and every degradation route + // reads their null rather than catching a throw. + expect(gitOpt('worktree', 'prune')).toBeNull(); + } finally { + process.chdir(cwd); + } + }); }); diff --git a/packages/cli/src/commands/review/lib/git.ts b/packages/cli/src/commands/review/lib/git.ts index 07f4a344425..4d3fb206672 100644 --- a/packages/cli/src/commands/review/lib/git.ts +++ b/packages/cli/src/commands/review/lib/git.ts @@ -9,7 +9,12 @@ // across platforms. import { execFileSync } from 'node:child_process'; -import { redirectedAncestor, sanitizedGitEnv } from './worktree.js'; +import { + insideReviewTmpLexically, + redirectedAncestor, + sanitizedGitEnv, + untrustedRepositoryFrom, +} from './worktree.js'; import { existsSync, lstatSync, rmSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; @@ -45,8 +50,81 @@ function gitOpts() { }; } +/** + * The launch directory, judged once per directory this process runs git from. + * + * Every wrapper below runs git with NO `cwd`, so each one discovers its + * repository from `process.cwd()` — and in the nested geometry `mountRootFor` + * documents, that directory is a review worktree inside the OUTER review's + * read-write mount, where the reviewed code can rewrite the `.git` pointer git + * follows. What the individual commands then do through it is not a short list: + * `fetch` loads the plant's transport config, `diff` refreshes the index and + * honours `diff..command`, `show :` hands back the plant's + * content as this review's own rules, `remote get-url` names where a submission + * goes. Gating those command by command is how this class kept re-opening, so + * the question is asked HERE, at the four functions every one of them goes + * through. + * + * A `-C ` argument does not change the answer's relevance: that scopes + * the command to another tree, and THAT tree's pointer is the subject of + * `untrustedGitfile`, which its callers ask separately. This one is only ever + * about where the process stands. + * + * Only the NEGATIVE is memoized, and only where repeating it costs nothing: + * outside a review temp dir `untrustedRepositoryFrom` returns on its first line + * — a string scan, no spawn — so caching that keeps `gitProbe`, called per + * thread by `comment-status`, off a per-call syscall and an ordinary checkout + * never pays for this gate at all. + * + * The memo's key is the LEXICAL marker scan, never `mountRootFor`'s answer. + * That null is overloaded (see `insideReviewTmpLexically`), and the rename + * attack turns on the difference: `mv .qwen .qwen-real` from inside the outer + * mount leaves the live cwd's stale spelling matching the marker while the + * mount stops answering, so a memo keyed on "no mount root" recorded TRUSTED + * for a spelling inside the review temp dir — and served it to whatever tree + * the same spelling was later stood back up over, filter-carrying gitfile + * included. A spelling that carries the marker is therefore NEVER memoized: + * inside a review temp dir the question is RE-ASKED on every call. That is + * not the TOCTOU residual this design documents and does not close, which is + * about a pointer rewritten between a check and its own use: here the OUTER + * review's containerized build/test phase holds the directory read-write for + * the length of a whole command, so it rewrites the pointer BETWEEN calls, + * and a verdict memoized for the process served the first clean answer to + * every later wrapper — `status` refreshing the plant's index and running its + * clean filter, `show :` handing the plant's content back as this + * review's own rules. `fetch-pr` asks twice for the same reason; this asks + * every time, and the spawn it costs exists only in the one geometry where + * the gate speaks. + */ +let trustedLaunchDir: string | null = null; + +/** Why this process must not run git from where it stands, or null. */ +function launchDirRefusal(): string | null { + const cwd = process.cwd(); + if (trustedLaunchDir === cwd) return null; + const refusal = untrustedRepositoryFrom(cwd); + if (refusal === null) { + if (!insideReviewTmpLexically(cwd)) trustedLaunchDir = cwd; + return null; + } + return refusal; +} + +function assertTrustedLaunchDir(): void { + const refusal = launchDirRefusal(); + if (refusal !== null) { + throw new Error( + `refusing to run git from this directory: ${refusal}. ` + + `The command would resolve through that pointer and act on whatever ` + + `repository it names. Run the review from a checkout outside the ` + + `review temp dir.`, + ); + } +} + /** Run `git` with args. Returns stdout, trimmed and CRLF-normalised. */ export function git(...args: string[]): string { + assertTrustedLaunchDir(); return execFileSync('git', args, { ...gitOpts(), encoding: 'utf8' }) .replace(/\r\n/g, '\n') .trim(); @@ -78,6 +156,7 @@ export function gitWithInput(input: Buffer, args: string[]): string { * identity exists to track goes invisible. */ export function gitWithInputRaw(input: Buffer, args: string[]): string { + assertTrustedLaunchDir(); return execFileSync('git', args, { ...gitOpts(), encoding: 'utf8', @@ -122,12 +201,29 @@ export function gitOpt(...args: string[]): string | null { * `{status: null, signal: 'SIGTERM'}`. A timeout is therefore a null status, * not a high one; both route to the same "surface unavailable" handling, but * the distinction matters to anyone reading this to predict a value. + * + * A launch directory this process must not resolve through lands in the same + * `status: null` — the command genuinely could not be run — but it is the one + * case with a reason a caller can hand to a user, so it also comes back in + * `refusal`. Callers that REPORT rather than retry need the split: reading a + * null `out` as git's answer made `releaseWorktree` certify a release whose + * registration and branch both survived, and `load-rules` write an empty rules + * file into every agent brief while printing "no review rules found". */ export function gitProbe(...args: string[]): { out: string | null; status: number | null; + refusal: string | null; } { try { + // INSIDE the try, which is where the spawn's own failures land. The + // pre-check reads `process.cwd()`, and a cwd deleted out from under this + // process makes that throw ENOENT — the same "git could not be run" the + // catch already answers, and one this probe must answer rather than throw: + // `releaseWorktree`'s never-throws contract and every degradation route + // built on a null `out` go through here. + const refusal = launchDirRefusal(); + if (refusal !== null) return { out: null, status: null, refusal }; return { out: execFileSync('git', args, { ...gitOpts(), @@ -137,10 +233,15 @@ export function gitProbe(...args: string[]): { .replace(/\r\n/g, '\n') .trim(), status: 0, + refusal: null, }; } catch (err) { const status = (err as { status?: unknown }).status; - return { out: null, status: typeof status === 'number' ? status : null }; + return { + out: null, + status: typeof status === 'number' ? status : null, + refusal: null, + }; } } @@ -232,7 +333,22 @@ export function releaseWorktree(worktreePath: string): WorktreeRelease { // `dirname`, because the LEAF is the branch below: a link AT the path is // unlinked rather than refused, which is what clears it out of the next // `worktree add`'s way. - const redirected = redirectedAncestor(dirname(resolve(worktreePath))); + // + // In its own try, because the never-throws contract starts HERE, before any + // `gitProbe` call: `resolve` reads the cwd for the RELATIVE path production + // callers pass (`worktreePath()` returns `.qwen/tmp/review-pr-`), and so + // does `redirectedAncestor`'s default `stopAt = process.cwd()` — evaluated + // at the call, outside that function's own try. A cwd deleted out from + // under the process (an operator `rm -rf` mid-run, the nested geometry's + // outer sweep) throws `uv_cwd` ENOENT on both reads, and this function + // degrades through the result the way the probe does: `existed`, not freed, + // and the errno as the reason. + let redirected: string | null; + try { + redirected = redirectedAncestor(dirname(resolve(worktreePath))); + } catch (err) { + return worktreeReleaseResult(true, true, err); + } if (redirected !== null) { // `existed`/`stillThere` both true: the contract's `reason` is only carried // when something is still there, and a refusal is exactly that — the @@ -265,8 +381,10 @@ export function releaseWorktree(worktreePath: string): WorktreeRelease { } // prune still runs: a registration whose tree once stood at this // path must not wedge the next `worktree add` or hold the branch - // checked out. - gitOpt('worktree', 'prune'); + // checked out. `{status: null, refusal: null}` — spawn failure, + // timeout kill — is git never ASKED, so the registration may survive: + // that is not freed, keyed the same way the main path below keys it. + const pruned = gitProbe('worktree', 'prune'); let stillThere = false; try { lstatSync(worktreePath); @@ -274,15 +392,30 @@ export function releaseWorktree(worktreePath: string): WorktreeRelease { } catch { // The link is gone — freed. } - return worktreeReleaseResult(true, stillThere, removeError); + return worktreeReleaseResult( + true, + stillThere || pruned.refusal !== null || pruned.status === null, + removeError ?? + refusalError(pruned.refusal) ?? + (pruned.status === null ? couldNotRunError() : undefined), + ); } } catch { // Nothing at the path: the `existsSync` below answers that case. } const existed = existsSync(worktreePath); let removeError: unknown; + // `status === null` is "the command could not be run at all"; git answers a + // path it does not know with 128. So a null is never "nothing to remove" — + // and reading it that way let the `rmSync` below delete the DIRECTORY while + // the registration under `/.git/worktrees/` and the branch both + // survived, reporting `freed: true` over a path the next `worktree add` + // still refuses with "missing but already registered": the exact wedge this + // function's docstring exists to prevent, reported as fixed. + const removed = existed + ? gitProbe('worktree', 'remove', worktreePath, '--force') + : null; if (existed) { - gitOpt('worktree', 'remove', worktreePath, '--force'); // `worktree remove` only clears a tree git still tracks. A directory left at // the path after metadata loss or a partial cleanup is reported "not a // working tree" and left in place — and a non-empty one then blocks the next @@ -302,8 +435,57 @@ export function releaseWorktree(worktreePath: string): WorktreeRelease { removeError = e; } } - gitOpt('worktree', 'prune'); - return worktreeReleaseResult(existed, existsSync(worktreePath), removeError); + const pruned = gitProbe('worktree', 'prune'); + const refusal = removed?.refusal ?? pruned.refusal; + // `{status: null, refusal: null}` is the third shape a probe answers: git + // could not be run AT ALL (spawn ENOENT, the timeout kill, a cwd deleted + // underneath). Keying "not freed" on the refusal alone read that as "no + // objection", while the `rmSync` above had already cleared the DIRECTORY — + // so the result certified `freed: true` over a registration under + // `/.git/worktrees/` and a branch that both survived, and the next + // `worktree add` met "missing but already registered": the wedge this + // function's docstring exists to prevent. `status === 128` stays on the + // rmSync-fallback path above (git answered, the answer was "not a working + // tree", and the fallback owns it); a null status means nobody answered. + const couldNotRun = + (removed !== null && removed.status === null) || pruned.status === null; + const stillThere = existsSync(worktreePath); + // A path that IS gone but a release that did not happen: git never ran, so + // the registration and the branch survive. `stillThere` is how the result + // says "not freed", and the reason must be set or cleanup prints `Failed to + // remove worktree : undefined`. + return worktreeReleaseResult( + existed, + stillThere || (existed && (refusal !== null || couldNotRun)), + removeError ?? + refusalError(refusal) ?? + (couldNotRun ? couldNotRunError() : undefined), + ); +} + +/** + * The `reason` for a release git was never even asked to make: the spawn + * failed or the timeout killed it, so the prune that clears the registration + * and frees the branch did not happen — however the directory itself fared. + */ +function couldNotRunError(): Error { + return new Error( + 'git could not be run at all (a spawn failure or the timeout kill), so ' + + "this worktree's registration and branch were not pruned — the next " + + '`git worktree add` over the path will still fail with "missing but ' + + 'already registered". Re-run `qwen review cleanup`.', + ); +} + +/** The `reason` for a release git was refused, or undefined when it was not. */ +function refusalError(refusal: string | null): Error | undefined { + if (refusal === null) return undefined; + return new Error( + `git could not run from this directory — ${refusal}. The directory is ` + + `cleared but this worktree's registration and branch were not, so the ` + + `next \`git worktree add\` over it will still fail. Run \`qwen review ` + + `cleanup\` from a checkout outside the review temp dir.`, + ); } /** @@ -316,6 +498,7 @@ export function releaseWorktree(worktreePath: string): WorktreeRelease { * ENOBUFS rather than returning a short read. Diff capture uses this instead. */ export function gitRaw(...args: string[]): Buffer { + assertTrustedLaunchDir(); return execFileSync('git', args, { ...gitOpts(), maxBuffer: 512 * 1024 * 1024, 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 7378aaec8c7..94696559aea 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,26 @@ describe('captureLocalDiff — untracked files', () => { expect(res.text).not.toContain('.qwen/tmp'); }); + it('does not capture the worktree leases, which moved out of .qwen/tmp', () => { + // The lease moved to `.qwen/review-leases` to get host-trusted state out of + // the directory the sandbox mounts read-write, and the filter kept naming + // its directories by hand — so a checkout holding a lease captured that + // lease JSON (session id, prompt id, worktree path, branch) as the user's + // untracked change. A lease is rewritten on every acquisition, so an + // interleaved local round re-reviewed the churned lease forever instead of + // ever reporting "no changes": the pathology the `.qwen/tmp` arm exists to + // prevent, on the directory that moved out of it. + write( + '.qwen/review-leases/qwen-review-lease-pr-1.json', + '{"sessionId":"s","promptId":"p"}\n', + ); + write('real.ts', 'export const r = 1;\n'); + + const res = capture(); + expect(res.untracked).toEqual(['real.ts']); + expect(res.text).not.toContain('review-leases'); + }); + 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 diff --git a/packages/cli/src/commands/review/lib/local-diff.ts b/packages/cli/src/commands/review/lib/local-diff.ts index 29f7ba407f9..da8c8abf8a6 100644 --- a/packages/cli/src/commands/review/lib/local-diff.ts +++ b/packages/cli/src/commands/review/lib/local-diff.ts @@ -26,7 +26,13 @@ import { lstatSync, statSync, type Stats } from 'node:fs'; import { join, sep } from 'node:path'; -import { repoRelativeOf } from './paths.js'; +import { + REVIEW_CACHE_DIR, + REVIEW_LEASE_DIR, + REVIEW_TMP_DIR, + REVIEWS_DIR, + repoRelativeOf, +} from './paths.js'; import { parseDiff, sliceDiffByLines } from './diff-plan.js'; import { LITERAL_PATHSPECS, @@ -299,8 +305,8 @@ function diffUntracked(repoRoot: string, path: string): Buffer { /** * 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 + * Segment-exact at ANY depth, not anchored to the cwd. The `paths.ts` + * constants 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 @@ -311,11 +317,28 @@ function diffUntracked(repoRoot: string, path: string): Buffer { * * Segment-exact matters for the same reason `toRepoPathspec` records: a * directory named `.qwen-notes` or `tmpfiles` is the user's, not ours. + * + * Built from the constants rather than spelled out beside them: the lease + * directory moved once already — out of `.qwen/tmp`, which the review sandbox + * mounts read-write — and a hand-written alternation stayed behind, so a + * checkout holding a lease captured that churned lease JSON as the user's + * untracked change, and an incremental round could never again report "no + * changes". `sep` becomes `/` because git spells every path with forward + * slashes on every platform, which is the spelling `repoRelPath` arrives in. */ +const REVIEW_PLUMBING = new RegExp( + `(?:^|/)(?:${[REVIEW_TMP_DIR, REVIEW_CACHE_DIR, REVIEWS_DIR, REVIEW_LEASE_DIR] + .map((dir) => + dir + .split(sep) + .join('/') + .replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), + ) + .join('|')})(?:/|$)`, +); + export function isReviewPlumbing(repoRelPath: string): boolean { - return /(?:^|\/)\.qwen\/(?:tmp|review-cache|reviews)(?:\/|$)/.test( - repoRelPath, - ); + return REVIEW_PLUMBING.test(repoRelPath); } /** @@ -368,6 +391,11 @@ export function captureLocalDiff(opts: { includeUntracked?: boolean; }): LocalDiffCapture { const { file, includeUntracked = true } = opts; + // The launch directory is judged by `lib/git`'s wrappers, so the `rev-parse` + // below refuses before `git diff` can refresh the index through a planted + // pointer. No second gate here: a duplicate whose only contribution is a + // nicer message is one more thing to keep in step with the real one. + // // Everything below runs against the repo *root*, not the process's cwd. A // capture started from a subdirectory must still see the whole working tree — // and, more subtly, must label its files the same way `git diff --no-relative` diff --git a/packages/cli/src/commands/review/lib/paths.ts b/packages/cli/src/commands/review/lib/paths.ts index 350aaa9923f..8151ce62efc 100644 --- a/packages/cli/src/commands/review/lib/paths.ts +++ b/packages/cli/src/commands/review/lib/paths.ts @@ -46,6 +46,23 @@ export function assertWritableOutPath(out: string): void { } export const REVIEW_TMP_DIR = join('.qwen', 'tmp'); + +/** + * Where worktree leases live — a SIBLING of the review temp dir, not a child. + * + * The leases are host-trusted state: `cleanupReviewWorktreeLeases` matches one + * by session ids alone and then force-removes whatever worktree and deletes + * whatever branch it names. `REVIEW_TMP_DIR` is the directory the review + * sandbox bind-mounts read-write, so keeping them there put that state inside + * the container's writable surface — and inside reach of an unsandboxed run + * too, which is how it stood before the sandbox existed. Reviewed code that + * can edit a lease can make another session's cleanup destroy the wrong tree, + * or plant one no session will ever sweep and wedge that PR on that machine. + * + * One directory over is the whole fix: nothing mounts it, and the lease + * lifecycle is unchanged. + */ +export const REVIEW_LEASE_DIR = join('.qwen', 'review-leases'); export const REVIEWS_DIR = join('.qwen', 'reviews'); export const REVIEW_CACHE_DIR = join('.qwen', 'review-cache'); @@ -204,7 +221,7 @@ export function reviewWorkflowScriptPath( } /** - * Filename prefix for review-worktree lease files under `REVIEW_TMP_DIR`. + * Filename prefix for review-worktree lease files under `REVIEW_LEASE_DIR`. * Lives here, not in `review-worktree-lease.ts`, because the review * workflow's cleanup sweep deletes leases by glob — the sweep pattern and * the lease writer must share one definition (the cleanup spec pins both). diff --git a/packages/cli/src/commands/review/lib/resume.ts b/packages/cli/src/commands/review/lib/resume.ts index 1d765130bdb..e3467e911ac 100644 --- a/packages/cli/src/commands/review/lib/resume.ts +++ b/packages/cli/src/commands/review/lib/resume.ts @@ -37,7 +37,8 @@ export type ResumeRefusal = | 'diff-unreadable' // the captured diff is gone or cannot be read | 'diff-hash-mismatch' // the diff file changed since it was captured | 'head-moved' // the PR head advanced — the once-per-review restart case - | 'resume-cap'; // this review has already resumed RESUME_MAX times + | 'resume-cap' // this review has already resumed RESUME_MAX times + | 'worktree-untrusted'; // the tree's gitfile no longer resolves to its own admin entry export type ResumeAssessment = | { ok: true } diff --git a/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts b/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts index 9b636df8693..19aca57614d 100644 --- a/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts +++ b/packages/cli/src/commands/review/lib/sandboxed-exec.test.ts @@ -907,6 +907,25 @@ describe('mountRootFor', () => { }, ); + itWhereRootsCanMount( + 'answers for the review temp dir ITSELF, not only for its children', + () => { + // The marker ends in a separator, so a path ending AT `.qwen/tmp` + // matched no temp dir at all: `untrustedRepositoryFrom` short-circuited + // on its first line, the launch-directory question went unpolicied for a + // process standing there, and `lib/git` memoized that negative as + // trusted for the rest of the process. + const root = tmp(); + const tmpDir = join(root, '.qwen', 'tmp'); + mkdirSync(tmpDir, { recursive: true }); + expect(mountRootFor(tmpDir)).toBe(realpathSync(tmpDir)); + // ...and a child of it answers the same root, as before. + const tree = join(tmpDir, 'review-pr-1'); + mkdirSync(tree, { recursive: true }); + expect(mountRootFor(tree)).toBe(realpathSync(tmpDir)); + }, + ); + it('is null outside a temp dir, so a local checkout is never mounted', () => { // `/review` of a local checkout has no sibling layout: the tree under test // IS the user's working copy. diff --git a/packages/cli/src/commands/review/lib/sandboxed-exec.ts b/packages/cli/src/commands/review/lib/sandboxed-exec.ts index 394bd0001b2..ab2cb51ee06 100644 --- a/packages/cli/src/commands/review/lib/sandboxed-exec.ts +++ b/packages/cli/src/commands/review/lib/sandboxed-exec.ts @@ -47,16 +47,24 @@ import { spawnSync } from 'node:child_process'; import { realpathSync } from 'node:fs'; -import { basename, dirname, join, resolve, sep } from 'node:path'; +import { basename, dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { operatorReviewSettings } from './review-settings.js'; -import { REVIEW_TMP_DIR } from './paths.js'; -import { redirectedAncestor } from './worktree.js'; +import { mountRootFor } from './worktree.js'; import { CUSTOM_SANDBOX_IMAGE_ENV_VAR } from '../../../utils/processUtils.js'; import { isFileSourcedEnvKey } from '../../../config/environment.js'; import { readPackageUpSync } from 'read-package-up'; import { CLI_VERSION } from '../../../generated/git-commit.js'; +// Re-exported from its old home: `mountRootFor` moved into `worktree.ts` so +// the gates there — `untrustedGitfile`, `untrustedRepositoryFrom` and the +// residue probe's own location check — can DEFAULT to it instead of taking +// it as an argument every call site had to remember to pass. It stays +// importable from here because every one of this pipeline's commands +// already reaches it by this name, and the mount is still this module's +// subject. +export { mountRootFor }; + /** * Last resort only: the real default is the CLI's own `config.sandboxImageUri` * — see `cliSandboxImage`. This literal covers the case where the package @@ -432,73 +440,6 @@ export function containerEnv(cacheDir: string): string[] { ]; } -/** - * The directory to mount for a command running in `cwd`, or null when `cwd` is - * not one of the pipeline's trees. - * - * Not the tree: the dependency farm links out of every tree into the review - * worktree's `node_modules`, so a per-tree mount leaves every link dangling. - * Every tree the pipeline builds is a sibling under the review temp dir, so - * that directory covers both ends of every link while `/.git` stays - * outside it. - * - * `lastIndexOf`, because a review run from inside another review's worktree — - * this pipeline's own dogfood geometry — nests one `.qwen/tmp` inside another, - * and the FIRST occurrence would widen the mount to the outer temp dir, - * pulling `/.git` and every sibling checkout in with it. Tree names - * cannot contain a separator (scratch labels flatten to `[A-Za-z0-9._-]`), so - * the deepest occurrence is always the tree's own parent. - * - * Null for a cwd outside any temp dir — a `/review` of a local checkout, where - * the tree under test IS the user's working copy and there is no sibling - * layout to mount. - */ -export function mountRootFor(cwd: string): string | null { - const resolved = resolve(cwd); - const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; - const at = resolved.lastIndexOf(marker); - if (at < 0) return null; - const root = resolved.slice(0, at + marker.length - 1); - // A LEXICAL root is not a safe mount target. `resolve` never touches the - // filesystem, so a symlink at or above `.qwen/tmp` — committable as mode - // 120000 and materialised by a fresh clone — silently widens a read-write - // bind mount to wherever it points. Every other creating or destroying path - // in this pipeline refuses that (`runCleanup`, `releaseWorktree`, - // `resetScratchTree`); the mount is the one place a redirect would hand the - // reviewed code a directory nobody chose. - try { - if (redirectedAncestor(root, dirname(resolve(root, '..', '..'))) !== null) { - return null; - } - const real = realpathSync(root); - // `-v src:dst` separates its fields with `:`, so a root that contains one - // cannot be spelled in that grammar at all: docker answers `invalid spec - // ... too many colons` and every command in the phase hard-fails with a - // raw mount error. Under `auto` those land as build/test failures the - // report attributes to the PR; under `required` the gate passes and the - // refusal that should have explained it never happens. Both designed - // degradations are bypassed because this said "mountable" about a root - // that is not. - // - // Refusing it here puts such a checkout back on the path every other - // unmountable root already takes. That is the whole fix: the `-v` grammar - // has exactly one separator, and `:` in a repository path — legal, if - // rare — is the only way to write it. - // - // On Windows this refuses EVERY absolute path, and that is the right - // answer rather than a casualty of it: a drive letter is a colon, and the - // mount this builds uses one path as both source and target, which a - // Windows path cannot be — the container side has no `C:`. So containment - // is not available there, and saying so gives `auto` its direct fallback - // and `required` its refusal instead of the runtime's parse error on every - // single command. - if (real.includes(':')) return null; - return real; - } catch { - return null; - } -} - /** * Whether a hand-off report must become a refusal. * diff --git a/packages/cli/src/commands/review/lib/test-utils.ts b/packages/cli/src/commands/review/lib/test-utils.ts index 051e9d51e70..fd66bb4dc46 100644 --- a/packages/cli/src/commands/review/lib/test-utils.ts +++ b/packages/cli/src/commands/review/lib/test-utils.ts @@ -5,8 +5,11 @@ */ import { + appendFileSync, + cpSync, mkdirSync, mkdtempSync, + readFileSync, realpathSync, rmSync, writeFileSync, @@ -176,3 +179,79 @@ export const FOREIGN_DIGEST = 'ab'.repeat(32); export function stampDigest(fs: FixtureFs, repo: string, digest: string): void { fs.writeFileSync(join(repo, 'dist', DIGEST_FILE), digest); } + +/** + * The admin entry `/.git` names, spelled the way git spelled it. + * + * Read rather than derived: the gates ask git where the repository is, so a + * fixture that guesses the entry's path can build a plant git never resolves. + */ +export function adminEntryOf(tree: string): string { + return readFileSync(join(tree, '.git'), 'utf8') + .trim() + .replace('gitdir: ', ''); +} + +/** + * A repository the reviewed code planted, carrying a content filter git + * EXECUTES. The oracle every host-execution witness shares, so a fixture that + * quietly stops being an attack fails in one place instead of in each suite. + * + * `filter` names the driver to arm, because the routes differ in what makes + * git run one: `clean` (the default) for the READ routes, where an index + * refresh re-hashes a file whose stat changed and re-hashing runs the clean + * filter; `smudge` for the CHECKOUT routes; `process` where a suite has to + * slip past the repo-local `smudge|clean` screen and still execute. `clean` + * and `smudge` pass the content through — a filter that swallows it turns the + * command into a different failure than the one under test. + * + * The marker goes in `info/attributes`, not a `.gitattributes`: the tree's own + * working files are not the subject, and a tracked one would show up as residue. + */ +export function plantRepository( + dir: string, + from: string, + canary: string, + filter: 'clean' | 'smudge' | 'process' = 'clean', +): string { + cpSync(from, dir, { recursive: true }); + // The copy carries the real repository's admin entries; leaving them makes + // the plant answer for trees it was never given. + rmSync(join(dir, 'worktrees'), { recursive: true, force: true }); + const command = + filter === 'process' + ? `sh -c "echo PWNED > ${canary}"` + : `sh -c "echo PWNED > ${canary}; cat"`; + appendFileSync( + join(dir, 'config'), + `\n[filter "evil"]\n\t${filter} = ${command}\n`, + ); + mkdirSync(join(dir, 'info'), { recursive: true }); + writeFileSync(join(dir, 'info', 'attributes'), '* filter=evil\n'); + return dir; +} + +/** + * Rewrite `/.git` to name an admin entry the reviewed code planted, + * copying `from` so the plant is coherent: it round-trips, and `rev-parse` + * through it answers the shas the identity gates pin. + * + * The backpointer in `/gitdir` is BARE, which is the only form git + * writes there. The `gitdir: ` prefix belongs to the TREE's own `.git` file; a + * prefixed backpointer fails the round-trip identity check the residue and + * probe-tree routes run first, which turns a witness into a green statement + * about the wrong refusal. + */ +export function plantAdminEntry( + entry: string, + from: string, + tree: string, + commonDir: string, +): string { + cpSync(from, entry, { recursive: true }); + writeFileSync(join(entry, 'commondir'), `${commonDir}\n`); + writeFileSync(join(entry, 'gitdir'), `${join(tree, '.git')}\n`); + rmSync(join(tree, '.git'), { force: true }); + writeFileSync(join(tree, '.git'), `gitdir: ${entry}\n`); + return entry; +} diff --git a/packages/cli/src/commands/review/lib/worktree.test.ts b/packages/cli/src/commands/review/lib/worktree.test.ts index 4b76be5ff3f..1e7a30c8462 100644 --- a/packages/cli/src/commands/review/lib/worktree.test.ts +++ b/packages/cli/src/commands/review/lib/worktree.test.ts @@ -16,6 +16,7 @@ import { appendFileSync, chmodSync, copyFileSync, + cpSync, existsSync, lstatSync, mkdirSync, @@ -31,8 +32,18 @@ import { } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, join } from 'node:path'; -import { isolateHostGitConfig } from './test-utils.js'; import { + adminEntryOf, + isolateHostGitConfig, + plantAdminEntry, +} from './test-utils.js'; +import { + adminEntryInsideReviewTmp, + insideReviewTmpLexically, + mountRootFor, + unmountableRootSpelling, + untrustedGitfile, + untrustedRepositoryFrom, discardWorktree, exposeDependencies, filterBlankEnv, @@ -357,31 +368,75 @@ describe('worktreeResidue', () => { ).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', - ]); + it('gives a mount that is NO repository the walk-up reason, not the gate’s', () => { + // The location gate below fails closed on a git it could not run, and a + // genuine "not a repository" is not that: it belongs to the walk-up check, + // whose reason says what was actually found. Folding the two would report + // a `.git` gitfile that does not exist — and would refuse a directory the + // caller's own error path already owns. + const root = mkdtempSync(join(tmpdir(), 'qwen-plain-')); + const plain = join(root, '.qwen', 'tmp', 'x'); + mkdirSync(plain, { recursive: true }); + try { + const got = worktreeResidue(plain); + expect(got.paths).toEqual([]); + expect(got.unmeasured).toBeTruthy(); + expect(got.unmeasured).not.toContain('could not resolve its own git dir'); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); - gitRepo('config', 'core.worktree', tree); - overwriteGitfile(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); + // The common-dir reason is the LOCATION gate's, and on Windows + // `mountRootFor` refuses every absolute path (a drive letter is the colon it + // refuses), so no location gate speaks there and the identity gate answers + // instead. Gated rather than branched on `process.platform` inside one + // assertion, so each lane asserts one unconditional expectation — gating + // case by case inside a shared assertion is how the same lane surfaced four + // times in this pull request. + it.skipIf(process.platform === 'win32')( + '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', + ]); - const got = worktreeResidue(tree); + gitRepo('config', 'core.worktree', tree); + overwriteGitfile(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); - 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'); + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + // Inside a mount the common-dir gate answers this shape first, and with + // the more useful reason: it says what the pointer IS (the repository's + // own common dir) rather than what the entry behind it lacks. + expect(got.unmeasured).toContain('common dir'); + }, + ); + + it('says UNMEASURED for that swap outside a mount, where no location gate speaks', () => { + // The identity gate's own reason is what answers where the location gates + // say nothing — outside a mount, where the same swap is a stale pointer + // rather than a plantable one, and on Windows, where `mountRootFor` + // refuses every absolute path and the gated case above takes this route. + // 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. + const outside = join(repo, 'wt-outside'); + gitRepo('worktree', 'add', '--detach', '-q', outside, 'HEAD'); + gitRepo('config', 'core.worktree', outside); + overwriteGitfile(join(outside, '.git'), `gitdir: ${join(repo, '.git')}\n`); + expect(worktreeResidue(outside).unmeasured).toContain('no admin entry'); }); it('says UNMEASURED for a forged admin entry when the caller pins the expected head', () => { @@ -452,10 +507,12 @@ describe('worktreeResidue', () => { // 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. + // self-equality would pass while the round trip catches the borrow. Inside + // a mount the LOCATION gate's own round-trip (`untrustedPointer`, R23-2) + // speaks first; this residue's identical check stays the answer outside + // one. Either way the verdict is a refusal, and removing the round trip + // from both layers turns this red — 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') @@ -466,7 +523,9 @@ describe('worktreeResidue', () => { const got = worktreeResidue(tree); expect(got.paths).toEqual([]); - expect(got.unmeasured).toContain('does not point back'); + expect(got.unmeasured).toMatch( + /does not point back|a different tree's admin entry/, + ); }); it('says UNMEASURED — not "not a git worktree" — for a dangling backpointer', () => { @@ -535,10 +594,15 @@ describe('worktreeResidue', () => { 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'); + // The location gate answers this shape FIRST now: a symlinked + // `.qwen/tmp` is a REFUSED mount, and "inside the review temp dir by + // spelling with no mount root" fails closed — reading the refusal as + // "nothing to police" was the fail-open inversion that let the + // measurement run through the redirect. (The walk below refuses it + // too, one check later; the intermediate-ancestor case keeps that arm + // witnessed, where the mount answers and only the walk can see the + // link.) + expect(got.unmeasured).toContain('review temp dir'); } finally { rmSync(outside, { recursive: true, force: true }); } @@ -574,16 +638,17 @@ describe('worktreeResidue', () => { }); 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. + // The walk's own witness. The sibling redirect shape (a link AT + // `.qwen/tmp`) is now refused by the location gate before the walk runs — + // a refused mount fails closed — and the leaf-link shape refuses at the + // leaf lstat. What remains the walk's alone is a link BETWEEN the mount + // root and the leaf: `.qwen/tmp` itself stays real, so the mount answers + // and the gate passes, and deleting the walk turns this test red. The + // shape passes every other check: 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, @@ -591,19 +656,19 @@ describe('worktreeResidue', () => { 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. + // Move the worktree out and re-hang it one level deeper, behind a link + // planted INSIDE the mount. 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')); overwriteGitfile( join(outside, 'review-wt', '.git'), `gitdir: ${join(repo, '.git', 'worktrees', 'review-wt')}\n`, ); - rmSync(dirname(tree), { recursive: true, force: true }); - symlinkSync(outside, dirname(tree)); + symlinkSync(outside, join(dirname(tree), 'link')); + const spelled = join(dirname(tree), 'link', 'review-wt'); - const got = worktreeResidue(tree); + const got = worktreeResidue(spelled); expect(got.paths).toEqual([]); expect(got.unmeasured).toContain('resolves through a symlink'); @@ -960,6 +1025,41 @@ describe('worktreeResidue', () => { }, ); + it.skipIf(process.platform === 'win32')( + 'says UNMEASURED when the location gate cannot resolve, and never reaches status', + () => { + // The gate above asks `rev-parse --absolute-git-dir` and read EVERY null + // as "no objection" — so a git that timed out, failed to spawn, or died + // on any other fatal fell through to the `status` below, which refreshes + // the index and runs whatever clean filter the resolved repository + // configures. Its own 30s budget is a quarter of the protected + // commands' and the spawns below carry none at all, so a config sized + // between the two was measured reaching host-side filter execution. + // Only that one call is broken here: the shim proves the rest of the + // probe still answers, so the verdict comes from this gate and not from + // a repo the test broke wholesale. + const shim = mkdtempSync(join(tmpdir(), 'qwen-git-shim-')); + const realGit = execFileSync('sh', ['-c', 'command -v git'], { + encoding: 'utf8', + }).trim(); + const statusRan = join(shim, 'status-ran'); + writeFileSync( + join(shim, 'git'), + `#!/bin/sh\nfor a in "$@"; do\n [ "$a" = --absolute-git-dir ] && { echo "fatal: simulated config parse failure" >&2; exit 128; }\n [ "$a" = status ] && echo x >> "${statusRan}"\ndone\nexec ${realGit} "$@"\n`, + { mode: 0o755 }, + ); + process.env['PATH'] = `${shim}:${realPath}`; + + const got = worktreeResidue(tree); + + expect(got.paths).toEqual([]); + expect(got.unmeasured).toContain('could not resolve its own git dir'); + // The point of failing closed, and the half a message assertion cannot + // show: the measurement that would have run the plant's filter never ran. + expect(existsSync(statusRan)).toBe(false); + }, + ); + 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 @@ -2208,3 +2308,588 @@ describe('worktreeCreateFailureDetail', () => { ); }); }); + +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + +// One case below plants a name holding a raw invalid-UTF-8 byte, which only a +// filesystem that stores such names allows — NTFS is UTF-16 and APFS rejects +// invalid UTF-8 with EILSEQ, so the shape the case pins cannot exist there +// (and neither can the attack: the plant itself is uncreateable). +const itWhereRawByteNamesExist = it.skipIf(process.platform !== 'linux'); + +// Every case in this block builds a layout under `.qwen/tmp` and asks a +// question that only has an answer where containment can exist. On Windows +// `mountRootFor` refuses every absolute path (a drive letter is a colon), so +// the gate never speaks — and the fixtures cannot even be built there: a +// planted name carrying a drive letter mid-path is rejected by NTFS. Gated as +// a BLOCK, because gating case by case is how the same lane surfaced four +// times in this pull request. +describe('untrustedGitfile', () => { + // Real git runs here, so the host's own config must not reach it — the same + // isolation every other real-git describe in this file installs. Without it + // a host carrying `commit.gpgsign=true` and no usable key fails the fixture + // commit and the whole block goes red for a reason unrelated to the gate. + let gitIsolation: ReturnType; + beforeEach(() => { + gitIsolation = isolateHostGitConfig(); + }); + afterEach(() => gitIsolation.dispose()); + + const made: string[] = []; + const tmp = () => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-gitfile-'))); + made.push(dir); + return dir; + }; + afterEach(() => { + for (const dir of made.splice(0)) + rmSync(dir, { recursive: true, force: true }); + }); + + /** + * A REAL repository with a real linked worktree under `.qwen/tmp` — the + * pipeline's own geometry. Real, because the gate asks git to resolve the + * pointer rather than parsing it, so a fixture git cannot read proves + * nothing about either answer. + */ + const pipelineTree = () => { + const repo = tmp(); + const g = (cwd: string, ...args: string[]) => + execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); + g(repo, 'init', '-q', '-b', 'main'); + g(repo, 'config', 'user.email', 't@t.t'); + g(repo, 'config', 'user.name', 't'); + writeFileSync(join(repo, 'a.txt'), 'a\n'); + g(repo, 'add', 'a.txt'); + g(repo, 'commit', '-q', '-m', 'init'); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(dirname(tree), { recursive: true }); + g(repo, 'worktree', 'add', '-q', '--detach', tree, 'HEAD'); + return { repo, tree, mount: () => join(repo, '.qwen', 'tmp') }; + }; + + itWhereContainmentExists('ADMITS an intact pipeline gitfile', () => { + // The admit path, which nothing exercised: every refusal case would also + // refuse under a mutation that breaks the resolution, so only asserting + // the admit tells a working gate from one that refuses everything. + const { tree, mount } = pipelineTree(); + expect(untrustedGitfile(tree, mount)).toBeNull(); + }); + + itWhereContainmentExists( + 'refuses a pointer git resolves INTO the mount, however it is spelled', + () => { + // Spelled with a non-breaking space, which JS `trim()` strips and git's + // `read_gitfile` does not — the divergence that let the first cut resolve + // the REAL entry, outside the mount, and admit a tree git resolves to a + // planted one inside it. The gate asks git now, so the spelling stops + // mattering: whatever git answers is what gets located. + const { repo, tree, mount } = pipelineTree(); + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + adminEntryOf(tree), + tree, + join(repo, '.git'), + ); + expect(untrustedGitfile(tree, mount)).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + "refuses a gitfile borrowing a SIBLING worktree's legitimate admin entry", + () => { + // The third shape (R23-2): the entry is REAL — outside the mount, so + // the location question passes, and not the common dir, so the shape + // question passes — but it belongs to a sibling, and every command + // through it measures and mutates THAT tree. Only the entry's own + // `gitdir` backpointer names its owner, so the round-trip is the arm + // that speaks here: removed, this tree is admitted and the assertion + // goes red. + const { repo, tree, mount } = pipelineTree(); + const sibling = join(repo, '.qwen', 'tmp', 'review-pr-2'); + execFileSync( + 'git', + ['worktree', 'add', '-q', '--detach', sibling, 'HEAD'], + { + cwd: repo, + }, + ); + const admin = readFileSync(join(sibling, '.git'), 'utf8') + .trim() + .replace(/^gitdir:\s*/, ''); + writeFileSync(join(tree, '.git'), `gitdir: ${admin}\n`); + expect(untrustedGitfile(tree, mount)).toContain( + "a different tree's admin entry", + ); + }, + ); + + itWhereRawByteNamesExist( + 'refuses a gitfile whose target git prints and JS cannot read alike (invalid UTF-8, R8-2)', + () => { + // The gitfile names a planted entry under a RAW 0xFF byte in its path. + // Node's `encoding: 'utf8'` render of git's byte-exact answer maps that + // byte to U+FFFD — and the decoy planted under the U+FFFD spelling, a + // symlink to the tree's REAL admin entry (outside the mount), is what + // the location question would then judge: outside, admitted, while + // every gated command resolves the raw-byte plant inside the mount. + // A lossy answer counts as not given, so the gate refuses; without the + // U+FFFD guard every question below passes (the round-trip included — + // the decoy's backpointer is the real entry's) and this returns null. + const { repo, tree, mount } = pipelineTree(); + const tmpRoot = join(repo, '.qwen', 'tmp'); + const common = join(tmpRoot, '.evil-common'); + execFileSync('git', ['init', '-q', common]); + const evil = Buffer.concat([ + Buffer.from(`${tmpRoot}/ent`), + Buffer.from([0xff]), + Buffer.from('ry'), + ]); + mkdirSync(evil); + writeFileSync( + Buffer.concat([evil, Buffer.from('/commondir')]), + `${common}\n`, + ); + const decoy = join(tmpRoot, 'ent�ry'); + symlinkSync(adminEntryOf(tree), decoy); + writeFileSync( + join(tree, '.git'), + Buffer.concat([Buffer.from('gitdir: '), evil, Buffer.from('\n')]), + ); + expect(untrustedGitfile(tree, mount)).toContain('could not resolve'); + }, + ); + + itWhereContainmentExists( + 'refuses a `.git` that is not the pipeline gitfile at all', + () => { + // `rm .git && git init .` inside the mount: a repository of the writer's + // own, which skips every gate written for the gitfile shape. + const { tree, mount } = pipelineTree(); + rmSync(join(tree, '.git'), { force: true }); + execFileSync('git', ['init', '-q'], { cwd: tree }); + expect(untrustedGitfile(tree, mount)).toContain('not the gitfile'); + }, + ); + + itWhereContainmentExists( + 'follows GIT through a spelling only git and JS read differently', + () => { + // The divergence that made parsing here unsafe: JS `trim()` strips U+00A0, + // git's `read_gitfile` trims only C-locale space. Spelled with a leading + // NBSP, the pointer resolves — in Node — to the REAL entry outside the + // mount and would be admitted, while git reads the NBSP as part of a + // RELATIVE path and lands on the planted entry inside the mount, which is + // what the checkout would then run through. + const { repo, tree, mount } = pipelineTree(); + const real = readFileSync(join(tree, '.git'), 'utf8') + .trim() + .replace('gitdir: ', ''); + // The planted entry sits where git will look: under the tree, at a name + // beginning with the NBSP. + const planted = join(tree, `\u00a0${real}`); + mkdirSync(dirname(planted), { recursive: true }); + cpSync(real, planted, { recursive: true }); + writeFileSync(join(planted, 'commondir'), `${join(repo, '.git')}\n`); + writeFileSync(join(planted, 'gitdir'), `${join(tree, '.git')}\n`); + writeFileSync(join(tree, '.git'), `gitdir: \u00a0${real}\n`); + + // Node would resolve the real entry here; git resolves the planted one. + expect(untrustedGitfile(tree, mount)).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + "takes git's answer unedited, trailing NBSP and all", + () => { + // The trap the ASK GIT fix walked back into: `.trim()` on git's stdout + // removes U+00A0 too, so an entry whose directory NAME ends in one is + // resolved by git with it and judged here without it — and a twin of + // that name minus the character, symlinked outside the mount, is what + // the judgment then lands on. Only the terminator may be stripped. + const { repo, tree, mount } = pipelineTree(); + const twin = join(repo, '.qwen', 'tmp', 'entry'); + // The twin points OUTSIDE the mount; trimming the NBSP lands here. + symlinkSync(repo, twin); + plantAdminEntry( + join(repo, '.qwen', 'tmp', 'entry\u00a0'), + adminEntryOf(tree), + tree, + join(repo, '.git'), + ); + + expect(untrustedGitfile(tree, mount)).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + "takes git's answer unedited, trailing CR and all", + () => { + // The NBSP trap's sibling, one regex character away: `/\r?\n$/` also + // removes a `\r` that is the LAST BYTE OF THE PATH ITSELF, and git + // plumbing terminates with `\n` on every platform — so the plant + // `entry\r` was judged at its twin `entry`, symlinked outside the + // mount, and admitted while every gated command resolved through the + // plant. The gitfile's trailing `/` is load-bearing: git trims C-locale + // whitespace off a gitdir line, which would eat the `\r` first. + const { repo, tree, mount } = pipelineTree(); + const planted = join(repo, '.qwen', 'tmp', 'entry\r'); + // The twin points OUTSIDE the mount; stripping the CR lands here. + symlinkSync(repo, join(repo, '.qwen', 'tmp', 'entry')); + plantAdminEntry(planted, adminEntryOf(tree), tree, join(repo, '.git')); + overwriteGitfile(join(tree, '.git'), `gitdir: ${planted}/\n`); + + expect(untrustedGitfile(tree, mount)).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + 'lets a SUBDIRECTORY of a mounted checkout resolve the way git does', + () => { + // The geometry my own checkout hid: a review running inside a review + // worktree launches from, say, `/packages/cli`, which has no + // `.git` of its own — git walks up. Demanding one there refused every + // worktree creation in that geometry, and a repository NOT under + // `.qwen/tmp` (mine) never reaches the question at all, so the suite + // stayed green while the pipeline's own dogfood lane broke. + const { tree, mount } = pipelineTree(); + const sub = join(tree, 'packages', 'cli'); + mkdirSync(sub, { recursive: true }); + expect(untrustedRepositoryFrom(sub, mount)).toBeNull(); + }, + ); + + itWhereContainmentExists( + 'refuses a launch directory git could not be run in, and passes one that is no repository', + () => { + // A directory that genuinely is no repository keeps passing through — + // refusing there would answer a question nobody asked, and the caller's + // own error path owns it. Asserted BEFORE the shim, which cannot tell + // the two apart. + const nowhere = join(tmp(), 'nowhere'); + mkdirSync(nowhere, { recursive: true }); + expect(untrustedRepositoryFrom(nowhere, () => nowhere)).toBeNull(); + + // Every OTHER null used to pass through as that same "not a repository": + // a timeout, a spawn failure, any other fatal. This gate's budget is a + // quarter of the protected commands' (`GIT_TIMEOUT_MS` in lib/git.ts), so + // a planted config sized to parse between the two left the gate silent + // while the command resolved through the pointer it never judged — + // `untrustedGitfile` fails closed on the same null, which is what made + // the asymmetry visible. Only `--absolute-git-dir` is broken here. + const { tree, mount } = pipelineTree(); + const shim = mkdtempSync(join(tmpdir(), 'qwen-git-shim-')); + const realGit = execFileSync('sh', ['-c', 'command -v git'], { + encoding: 'utf8', + }).trim(); + writeFileSync( + join(shim, 'git'), + `#!/bin/sh\nfor a in "$@"; do\n [ "$a" = --absolute-git-dir ] && { echo "fatal: simulated config parse failure" >&2; exit 128; }\ndone\nexec ${realGit} "$@"\n`, + { mode: 0o755 }, + ); + const savedPath = process.env['PATH']; + process.env['PATH'] = `${shim}:${savedPath}`; + try { + expect(untrustedRepositoryFrom(tree, mount)).toContain( + 'could not resolve its own git dir', + ); + } finally { + process.env['PATH'] = savedPath; + } + }, + ); + + itWhereContainmentExists( + 'works where `realpathSync` carries no `.native`', + () => { + // A suite that mocks `node:fs.realpathSync` as a bare `vi.fn` gives it + // no `.native`, and reaching through it threw a TypeError into the + // fail-closed catch — refusing every worktree creation in any checkout + // sitting under `.qwen/tmp`. Deleting the property here asks that shape + // directly, because the suite that HAS the mock never reaches this gate + // from an unmounted checkout. + const { tree, mount } = pipelineTree(); + const holder = realpathSync as unknown as { native?: unknown }; + const saved = holder.native; + delete holder.native; + try { + expect(untrustedGitfile(tree, mount)).toBeNull(); + } finally { + holder.native = saved; + } + }, + ); + + itWhereContainmentExists( + 'refuses a gitfile rewritten to the repository own common dir', + () => { + // The shape the location question cannot see: `gitdir: /.git` + // resolves OUTSIDE the mount, so every gate that asked only where the + // answer lives admitted it and then acted on the MAIN repository + // through it. Measured at this head: `status` rewrote the main index and + // `rev-parse HEAD` answered the main head, so the reads a review treats + // as fact came from a tree nobody verified. No `worktree add` writes + // that pointer — a linked worktree's admin entry is always + // `/worktrees/` — which is what makes it answerable. + const { repo, tree, mount } = pipelineTree(); + overwriteGitfile(join(tree, '.git'), `gitdir: ${join(repo, '.git')}\n`); + + expect(untrustedGitfile(tree, mount)).toContain('common dir'); + expect(untrustedRepositoryFrom(tree, mount)).toContain('common dir'); + // The residue probe is the route that was measured rewriting the main + // index: `status` refreshes it, and a refresh runs the clean filter. + expect(worktreeResidue(tree, 12).unmeasured).toContain('common dir'); + }, + ); + + it('says nothing about a tree that does not exist yet', () => { + // `--resume` asks about a worktree before deciding whether to build one. + // Absence is that caller's question; answering it here refused every + // ordinary resume — 26 of them, measured. + const { repo, mount } = pipelineTree(); + expect( + untrustedGitfile(join(repo, '.qwen', 'tmp', 'review-pr-999'), mount), + ).toBeNull(); + }); + + it('says nothing at all outside a mount', () => { + // A plain checkout's `.git` IS a directory; refusing that would refuse + // every ordinary repository. + const repo = tmp(); + execFileSync('git', ['init', '-q'], { cwd: repo }); + expect(untrustedGitfile(repo, () => null)).toBeNull(); + }); + + itWhereContainmentExists( + 'REFUSES a tree whose mount was refused — a refusal is not "nothing to police"', + () => { + // `mountRootFor`'s null is overloaded: "outside any temp dir" and + // "inside one but REFUSED" were the same value, and every gate read + // both as "nothing to police" — the fail-open inversion, measured + // end-to-end with a symlinked `.qwen/tmp`. Lexically inside plus no + // mount root is fail-closed, at each gate that consumes the mount. + // Asked with the REAL `mountRootFor`, because an injected root can + // only ever answer one of the two nulls. + const { repo, tree } = pipelineTree(); + const elsewhere = join(repo, 'elsewhere'); + renameSync(join(repo, '.qwen', 'tmp'), elsewhere); + symlinkSync(elsewhere, join(repo, '.qwen', 'tmp')); + // The tree still resolves — through the link the mount refuses. + expect(existsSync(tree)).toBe(true); + expect(mountRootFor(tree)).toBeNull(); + + expect(untrustedGitfile(tree)).toContain('review temp dir'); + expect(untrustedRepositoryFrom(tree)).toContain('review temp dir'); + const residue = worktreeResidue(tree); + expect(residue.paths).toEqual([]); + expect(residue.unmeasured).toContain('review temp dir'); + }, + ); + + itWhereContainmentExists( + 'refuses a launch directory that no longer exists at its spelling inside the review temp dir', + () => { + // The rename attack's other half: `mv .qwen .qwen-real` from inside the + // outer mount leaves the stale spelling matching lexically while the + // directory is gone. "No objection" there is what the launch-dir memo + // recorded TRUSTED over, so inside the spelling a non-existent cwd is + // a refusal — while outside it absence stays the caller's own error + // path, exactly as before. + const { repo, mount } = pipelineTree(); + const gone = join(repo, '.qwen', 'tmp', 'review-pr-2'); + expect(untrustedRepositoryFrom(gone, mount)).toContain( + 'no directory exists', + ); + expect( + untrustedRepositoryFrom(join(repo, 'gone'), () => repo), + ).toBeNull(); + }, + ); +}); + +describe('adminEntryInsideReviewTmp', () => { + const made: string[] = []; + const tmp = () => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-admin-'))); + made.push(dir); + return dir; + }; + afterEach(() => { + for (const dir of made.splice(0)) + rmSync(dir, { recursive: true, force: true }); + }); + + it('refuses an entry planted inside the mounted directory', () => { + // The shape the round-trip gate cannot see: the writer chose BOTH the + // gitfile's target and that target's backpointer, so the two agree, no + // symlink is involved, and `--show-toplevel` still names the tree. Only + // the entry's location tells them apart. + const repo = tmp(); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe'); + const planted = join(repo, '.qwen', 'tmp', '.evil-git'); + mkdirSync(tree, { recursive: true }); + mkdirSync(planted, { recursive: true }); + expect( + adminEntryInsideReviewTmp( + planted, + () => join(repo, '.qwen', 'tmp'), + tree, + ), + ).toBe(true); + }); + + it('counts the mount root itself, and a child that looks like an escape', () => { + // The two shapes a hand-rolled prefix test gets wrong, and both are places + // a planted entry can actually sit. The mount ROOT is as writable as + // anything under it, and `..evil-git` is a legal filename whose relative + // path starts with the characters an escape would. + const repo = tmp(); + const mount = join(repo, '.qwen', 'tmp'); + const tree = join(mount, 'review-pr-1-probe'); + const oddly = join(mount, '..evil-git'); + mkdirSync(tree, { recursive: true }); + mkdirSync(oddly, { recursive: true }); + expect(adminEntryInsideReviewTmp(mount, () => mount, tree)).toBe(true); + expect(adminEntryInsideReviewTmp(oddly, () => mount, tree)).toBe(true); + // ...while a genuine sibling of the mount is still outside. + const outside = join(repo, '.qwen', 'review-leases'); + mkdirSync(outside, { recursive: true }); + expect(adminEntryInsideReviewTmp(outside, () => mount, tree)).toBe(false); + }); + + it('admits the real admin entry, which lives under the repository git dir', () => { + const repo = tmp(); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe'); + const real = join(repo, '.git', 'worktrees', 'review-pr-1-probe'); + mkdirSync(tree, { recursive: true }); + mkdirSync(real, { recursive: true }); + expect( + adminEntryInsideReviewTmp(real, () => join(repo, '.qwen', 'tmp'), tree), + ).toBe(false); + }); + + it('refuses rather than guesses when the entry cannot be resolved', () => { + // Fails CLOSED: "not inside" would be a guess, and the guess that lets a + // planted entry through is the one that ends in host execution. + const repo = tmp(); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1-probe'); + mkdirSync(tree, { recursive: true }); + expect( + adminEntryInsideReviewTmp( + join(repo, 'gone'), + () => join(repo, '.qwen', 'tmp'), + tree, + ), + ).toBe(true); + }); + + it('has nothing to say about a tree outside any mounted directory', () => { + // A local checkout is never mounted, so there is no writable surface for + // this question to be about — and answering `true` there would refuse + // every ordinary repository. + const repo = tmp(); + expect(adminEntryInsideReviewTmp(repo, () => null, repo)).toBe(false); + }); +}); + +describe('unmountableRootSpelling', () => { + it('refuses the drive-letter colon AND the colon-less UNC shape, and mounts a POSIX root', () => { + // Pure cases, because a UNC path cannot be constructed off Windows and a + // colon in a repository name is legal but rare: the refusal class is + // pinned arm by arm here. Removing the colon arm turns the first and + // third cases red; removing the UNC arm turns the second. + expect(unmountableRootSpelling('C:\\repo\\.qwen\\tmp')).toBe(true); + expect(unmountableRootSpelling('\\\\server\\share\\repo\\.qwen\\tmp')).toBe( + true, + ); + expect(unmountableRootSpelling('/repo/my:checkout/.qwen/tmp')).toBe(true); + expect(unmountableRootSpelling('/repo/.qwen/tmp')).toBe(false); + }); +}); + +describe('insideReviewTmpLexically', () => { + it('answers containment from the spelling alone, for a path nothing created', () => { + // NO filesystem access is the whole point: the gates pair this with + // `mountRootFor`'s overloaded null to tell "outside any temp dir" from + // "inside one, but refused" without paying a syscall outside. + const base = join(tmpdir(), 'qwen-lexical-no-such'); + expect(insideReviewTmpLexically(join(base, '.qwen', 'tmp', 'wt'))).toBe( + true, + ); + // The review temp dir ITSELF is inside, matching `mountRootFor`'s + // `+ sep`. + expect(insideReviewTmpLexically(join(base, '.qwen', 'tmp'))).toBe(true); + expect(insideReviewTmpLexically(join(base, 'checkout'))).toBe(false); + // A marker that is not a whole component is not containment. + expect(insideReviewTmpLexically(join(base, '.qwen', 'tmpl'))).toBe(false); + }); +}); + +describe('mountRootFor — the walk bound is geometry-aware', () => { + const made: string[] = []; + const tmp = () => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-bound-'))); + made.push(dir); + return dir; + }; + afterEach(() => { + for (const dir of made.splice(0)) + rmSync(dir, { recursive: true, force: true }); + }); + + itWhereContainmentExists( + 'mounts a checkout whose DIRECT parent is a symlink', + () => { + // The bound used to sit one component ABOVE the checkout, and + // `redirectedAncestor` lstats the stop directory before the stop test + // fires — so a link at the checkout's direct parent (a checkout one + // hop below a linked directory) was read as a redirect in a path the + // pipeline owns, and `--sandbox=auto` silently degraded to unsandboxed + // execution over the false refusal. Bounded at the repository root, + // the walk never looks at the user's own layout above the checkout. + const anchor = tmp(); + const realParent = tmp(); + symlinkSync(realParent, join(anchor, 'link')); + const repo = join(anchor, 'link', 'repo'); + const tree = join(repo, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(tree, { recursive: true }); + expect(mountRootFor(tree)).toBe(realpathSync(join(repo, '.qwen', 'tmp'))); + }, + ); + + itWhereContainmentExists( + 'still refuses a NESTED root whose OUTER review temp dir is a symlink', + () => { + // The nested bound is the outermost enclosing review temp root, and it + // stays INSIDE the walk: the outer review's containerized phase held + // that directory read-write, so a link planted there is a redirect in + // a writable surface, not the user's own layout. + const anchor = tmp(); + const elsewhere = tmp(); + mkdirSync(join(elsewhere, 'tmp', 'outer-wt', '.qwen', 'tmp', 'wt2'), { + recursive: true, + }); + const repo = join(anchor, 'repo'); + mkdirSync(join(repo, '.qwen'), { recursive: true }); + symlinkSync(join(elsewhere, 'tmp'), join(repo, '.qwen', 'tmp')); + const inner = join(repo, '.qwen', 'tmp', 'outer-wt', '.qwen', 'tmp'); + expect(mountRootFor(join(inner, 'wt2'))).toBeNull(); + + // ...and the same nested shape without the link mounts at the DEEPEST + // temp dir, so the refusal is about the redirect and not the geometry. + const honest = tmp(); + const honestInner = join( + honest, + '.qwen', + 'tmp', + 'outer-wt', + '.qwen', + 'tmp', + ); + mkdirSync(join(honestInner, 'wt2'), { recursive: true }); + expect(mountRootFor(join(honestInner, 'wt2'))).toBe( + realpathSync(honestInner), + ); + }, + ); +}); diff --git a/packages/cli/src/commands/review/lib/worktree.ts b/packages/cli/src/commands/review/lib/worktree.ts index 915c25c39a5..18786218271 100644 --- a/packages/cli/src/commands/review/lib/worktree.ts +++ b/packages/cli/src/commands/review/lib/worktree.ts @@ -36,6 +36,8 @@ import { } from 'node:fs'; import { homedir, tmpdir } from 'node:os'; import { basename, dirname, isAbsolute, join, resolve, sep } from 'node:path'; +import { isSubpath } from '@qwen-code/qwen-code-core'; +import { REVIEW_TMP_DIR } from './paths.js'; import { readWorkspacePackages } from './workspaces.js'; export type SweepResult = ReturnType; @@ -119,6 +121,604 @@ const GIT_ENV_EXEC = [ 'XDG_CONFIG_HOME', ]; +/** + * The directory to mount for a command running in `cwd`, or null when `cwd` is + * not one of the pipeline's trees. + * + * Not the tree: the dependency farm links out of every tree into the review + * worktree's `node_modules`, so a per-tree mount leaves every link dangling. + * Every tree the pipeline builds is a sibling under the review temp dir, so + * that directory covers both ends of every link while `/.git` stays + * outside it. + * + * `lastIndexOf`, because a review run from inside another review's worktree — + * this pipeline's own dogfood geometry — nests one `.qwen/tmp` inside another, + * and the FIRST occurrence would widen the mount to the outer temp dir, + * pulling `/.git` and every sibling checkout in with it. Tree names + * cannot contain a separator (scratch labels flatten to `[A-Za-z0-9._-]`), so + * the deepest occurrence is always the tree's own parent. + * + * The MOUNT is the deepest one; the DISTRUST judgement is not. Which directory + * may be handed to a container read-write and which directories reviewed code + * may already have written are two different questions, and in the nested + * geometry above they have two different answers — see + * `adminEntryInsideReviewTmp`, which widens only the second. + * + * Null for a cwd outside any temp dir — a `/review` of a local checkout, where + * the tree under test IS the user's working copy and there is no sibling + * layout to mount. + */ +export function mountRootFor(cwd: string): string | null { + const resolved = resolve(cwd); + const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; + // `+ sep` so a process standing AT the review temp dir is judged inside it. + // The marker ends in a separator, so the root itself matched no temp dir: + // the launch-directory question went unpolicied there, and the negative was + // memoized as trusted. + const at = (resolved + sep).lastIndexOf(marker); + if (at < 0) return null; + const root = resolved.slice(0, at + marker.length - 1); + // A LEXICAL root is not a safe mount target. `resolve` never touches the + // filesystem, so a symlink at or above `.qwen/tmp` — committable as mode + // 120000 and materialised by a fresh clone — silently widens a read-write + // bind mount to wherever it points. Every other creating or destroying path + // in this pipeline refuses that (`runCleanup`, `releaseWorktree`, + // `resetScratchTree`); the mount is the one place a redirect would hand the + // reviewed code a directory nobody chose. + // + // The walk's bound is geometry-aware. `outermostReviewTmpRoot(root) === + // root` is the flat case — one review temp dir on the path — and the bound + // is the repository root: the root itself stays inside the walk (a link at + // `.qwen` redirects the whole path), but the walk does not climb past the + // checkout into the user's own layout. A bound one component higher lstats + // the checkout's DIRECT parent before the stop test fires, so a link there + // — a checkout one hop below a linked directory, the everyday macOS shape — + // was read as a redirect in a path the pipeline owns, and `--sandbox=auto` + // silently degraded to unsandboxed execution of the reviewed code over a + // false refusal. The sibling walks (scratch-tree's, `worktreeResidue`'s) + // bound at the repository root for exactly this reason. + // + // The nested case — one review's worktree inside another's, the dogfood + // geometry — bounds at the OUTERMOST enclosing review temp root instead: + // the outer review's containerized phase held that directory read-write, so + // a link planted there sits inside a writable surface and must stay inside + // the walk, exactly as `adminEntryInsideReviewTmp` judges distrust against + // the outermost root rather than the one the mount was cut at. + const outermost = outermostReviewTmpRoot(root); + const bound = outermost === root ? resolve(root, '..', '..') : outermost; + try { + if (redirectedAncestor(root, bound) !== null) { + return null; + } + const real = realpathSync(root); + // `-v src:dst` separates its fields with `:`, so a root that contains one + // cannot be spelled in that grammar at all: docker answers `invalid spec + // ... too many colons` and every command in the phase hard-fails with a + // raw mount error. Under `auto` those land as build/test failures the + // report attributes to the PR; under `required` the gate passes and the + // refusal that should have explained it never happens. Both designed + // degradations are bypassed because this said "mountable" about a root + // that is not. `unmountableRootSpelling` owns the refusal class; saying + // so here gives `auto` its direct fallback and `required` its refusal + // instead of the runtime's parse error on every single command. + if (unmountableRootSpelling(real)) return null; + return real; + } catch { + return null; + } +} + +/** + * Whether a mount root's SPELLING is one the container's mount grammar cannot + * write. Pure, so both arms of the refusal class are pinnable on every host — + * a UNC path cannot be constructed off Windows, and a colon in a repository + * path is legal but rare. + * + * `-v src:dst` has exactly one separator. The colon arm covers a `:` anywhere + * in the root — which on Windows is EVERY absolute path, and that is the + * right answer rather than a casualty of it: a drive letter is a colon, and + * the mount this builds uses one path as both source and target, which a + * Windows path cannot be — the container side has no `C:`. So containment is + * not available there. + * + * The UNC arm is the same class with no colon in it: `\\server\share\...` is + * an absolute Windows path a colon-only check declares mountable, and the + * mount grammar cannot spell it either. Refusing it keeps the "on Windows + * this refuses EVERY absolute path" claim true as written, instead of sending + * a UNC checkout's every sandboxed command into a raw mount error the report + * attributes to the PR. + */ +export function unmountableRootSpelling(root: string): boolean { + return root.includes(':') || root.startsWith('\\\\'); +} + +/** + * Whether `cwd`'s spelling places it inside a review temp dir — the same + * deepest-marker arithmetic `mountRootFor` cuts with, and NOTHING else: no + * realpath, no lstat, no filesystem question of any kind. + * + * The gates consume this beside `mountRootFor`, not instead of it, because + * that function's null is overloaded: "outside any temp dir, nothing to + * police" and "inside one, but REFUSED — a symlink redirects it, or the + * spelling cannot be mounted" were the same value, and every gate read a + * refusal as "nothing to police". That is the fail-open direction inverted: + * the shapes `mountRootFor` refuses are exactly the ones a host-side command + * must not resolve through. So where this lexical scan says INSIDE and + * `mountRootFor` says null, the gate fails closed with a refusal of its own — + * and where the spelling carries no marker at all, the answer costs no + * syscall, which is what keeps an ordinary checkout off this machinery. + */ +export function insideReviewTmpLexically(cwd: string): boolean { + const resolved = resolve(cwd); + const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; + return (resolved + sep).lastIndexOf(marker) >= 0; +} + +/** + * A `rev-parse` answer, and which kind of no-answer a null was. + * + * `notARepository` separates the one null a gate may pass through — git ran + * and said this is not a repository, which is the caller's own error path to + * own — from every other null: a timeout, a spawn failure, any other fatal. + * Reading the second as the first made two of the three gates below answer + * "no objection" to a question git was never asked, and `revParse`'s budget + * is a quarter of the protected commands' (or, in `worktreeResidue`, of + * nothing at all), so a config sized to parse between the two reached the + * command the gate had just declined to judge. + */ +type RevParse = { value: string | null; notARepository: boolean }; + +/** + * git's own answer to "which repository does this path resolve to". + * + * ASK GIT, rather than parsing the pointer here. The first cut read the + * gitfile and resolved it in Node, and the two resolvers disagree in ways + * that are individually small and collectively unbounded: JS `trim()` strips + * U+00A0 (and U+FEFF, the U+2000 block, …) while git's `read_gitfile` trims + * only C-locale space, so `gitdir: ` resolves to the real + * entry here — outside the mount, admitted — and to a planted one inside it + * there; and `resolve()` is lexical while a spawned git resolves a relative + * target against the tree's PHYSICAL path after chdir. Every such divergence + * admits a pointer whose real referent only git sees, and the list of them + * has no last entry. + * + * So the resolver that decides is the resolver that acts. This asks git for + * the same answer the gated command will use, and the callers then make the + * only judgment that is theirs to make: WHERE that answer lives. Reading is safe — + * no checkout, no index refresh — and hooks and fsmonitor are inert arguments + * anyway, so a planted config cannot turn the question into an execution. + * + * The terminator `rev-parse` adds is stripped, and NOTHING else. `.trim()` + * here re-opened the very divergence the paragraph above closes: it also + * removes U+00A0, U+FEFF and the U+2000 block, so an admin entry whose + * directory name ENDS in one of those is resolved by git with the character + * kept and judged by the caller with it gone — and a twin of that name without + * it, symlinked outside the mount, is what the judgment then lands on. The + * resolver that decides has to receive git's answer unedited. + */ +function resolvedGitDir(cwd: string): RevParse { + return revParse(cwd, '--absolute-git-dir'); +} + +/** + * The common dir of the repository `cwd` resolves to, or null when git does + * not answer. + * + * One value per invocation, like every other `rev-parse` in this file: a + * combined answer would have to be split on a newline, and a POSIX path may + * carry one. + */ +function resolvedCommonDir(cwd: string): string | null { + return revParse(cwd, '--path-format=absolute', '--git-common-dir').value; +} + +/** + * The sanitized single-value `rev-parse` both location questions share. + * + * Strips git's terminal record delimiter and NOTHING else — never `.trim()`, + * for the reason `resolvedGitDir` gives. + */ +function revParse(cwd: string, ...flags: string[]): RevParse { + const resolved = spawnSync( + 'git', + [ + '-c', + 'core.hooksPath=/dev/null/no-hooks', + '-c', + 'core.fsmonitor=', + 'rev-parse', + ...flags, + ], + { + cwd, + encoding: 'utf8', + timeout: 30_000, + // `LC_ALL`, because `notARepository` below reads git's own sentence: a + // localized one classifies a launch directory that genuinely is no + // repository as "could not be run" and refuses it. The pin + // `core/utils/git-branches.ts` sets for the same reason. + env: { ...sanitizedGitEnv(), LC_ALL: 'C' }, + }, + ); + const value = + resolved.error || resolved.status !== 0 || !resolved.stdout + ? null + : resolved.stdout.replace(/\n$/, ''); + return { + // `\n` only, not `\r?\n`: git plumbing terminates with `\n` on every + // platform, so a `\r` in that regex is a `\r` that was the LAST BYTE OF + // THE PATH — and the judgment then landed on the twin of a plant named + // `evil\r`, symlinked outside the mount, while every gated command + // resolved through the plant. + // + // U+FFFD is the lossy-decode class, failed closed the same way: + // `encoding: 'utf8'` maps an undecodable byte in git's byte-exact answer + // to U+FFFD, so a path holding an invalid UTF-8 byte is judged at the + // U+FFFD spelling while git acts on the original bytes — a plant under + // the raw-byte name and a decoy under the U+FFFD one, symlinked outside + // the mount, is exactly what the location question is for. No gate may + // judge a spelling git never printed, so the answer counts as not given. + value: value !== null && value.includes('�') ? null : value, + notARepository: + resolved.status === 128 && + resolved.stderr.includes('not a git repository'), + }; +} + +/** + * Why a `.git` pointer is not one a host-side command may act through, or null + * when it is. + * + * Two questions, because a rewritten gitfile has two shapes and the first + * cannot see the second: + * + * - WHERE the admin entry lives — `adminEntryInsideReviewTmp`, the surface the + * reviewed code can write; + * - WHETHER it is an admin entry at all. `gitdir: /.git` names the + * repository's OWN common dir, which resolves outside the mount and so + * passes the location question, while every command through it acts on the + * MAIN repository: measured, `status` rewrote the main index and `rev-parse + * HEAD` answered the main head, so the reads a review treats as fact came + * from a tree nobody verified. No `worktree add` writes that pointer — a + * linked worktree's admin entry is always `/worktrees/` — which + * is why `resetScratchTree` already refuses the same equality. + * + * git not answering the second question is not a refusal: it answered the + * first one from the same directory with the same sanitized environment, and + * the location question has already spoken. The realistic case is a git too old + * for `--path-format`, and this pipeline's scratch and residue routes already + * require it — so that host has no working review inside a mount either way. + */ +function untrustedPointer( + dir: string, + gitDir: string, + mountRoot: (cwd: string) => string | null, +): string | null { + if (mountRoot(dir) === null) return null; + if (adminEntryInsideReviewTmp(gitDir, mountRoot, dir)) { + return 'resolves to an admin entry inside the review temp dir, where the reviewed code can rewrite it'; + } + const common = resolvedCommonDir(dir); + if (common !== null && common === gitDir) { + return ( + "resolves to the repository's own common dir rather than a linked " + + "worktree's admin entry, so every command through it acts on the main " + + 'repository instead of this tree' + ); + } + // The backpointer round-trip the write paths already carry + // (scratch-tree.ts), because a rewritten gitfile has a THIRD shape the two + // questions above cannot see: a SIBLING worktree's legitimate admin entry. + // That entry is outside the mount (question 1 passes) and is not the common + // dir (question 2 passes — a linked worktree's common dir differs from its + // git dir), while every host-side command through it measures and mutates + // the SIBLING tree. The entry's `gitdir` file names the `.git` of the tree + // it belongs to; a borrowed entry names the sibling's. `isSubpath`, because + // `dir` here may be any directory inside the tree (the launch-directory + // form of this question) — the entry must own the tree `dir` stands in, + // not `dir` itself. + // + // Only a definitive MISMATCH is this arm's to name. A backpointer that + // cannot be read or resolved — a moved tree, a dangling `gitdir` file — + // redirects nothing (the tree still resolves through its own gitfile), and + // the downstream identity checks refuse that shape with their own reasons; + // refusing it here too would shadow exactly those witnesses. And the one + // writer who could corrupt a backpointer to duck the mismatch — reviewed + // code — cannot reach an entry outside the mount at all, which question 1 + // has already established about this one. + let backpointer: string; + try { + // `\n` only, never `.trim()`: the same divergence `resolvedGitDir` + // documents — a path ending in U+00A0 or U+FEFF is resolved by git with + // the character kept and would be judged here with it gone. + backpointer = readFileSync(join(gitDir, 'gitdir'), 'utf8').replace( + /\n$/, + '', + ); + } catch { + return null; + } + let ownerTree: string; + let here: string; + try { + ownerTree = realpathSync(dirname(resolve(gitDir, backpointer))); + here = realpathSync(dir); + } catch { + return null; + } + if (ownerTree !== here && !isSubpath(ownerTree, here)) { + return ( + "resolves to a different tree's admin entry — its gitdir " + + 'backpointer names that tree, not this one, so every command ' + + 'through it would measure and mutate the tree it actually belongs to' + ); + } + return null; +} + +/** + * The same location question, asked from a directory that need not be a + * worktree ROOT. + * + * `untrustedGitfile` demands `/.git`, which is right for the trees this + * pipeline builds and wrong for the directory a command was launched from: any + * subdirectory of a checkout has no `.git` of its own, and git finds the + * repository by walking up. Requiring one there refused every worktree + * creation whose cwd sat inside a review temp dir without being its root — the + * nested/dogfood geometry this pipeline runs in, and 77 tests in one suite. + * + * So this asks git the same question `worktree add` will ask, from the same + * place, and judges only WHERE the answer lives. No shape check: the shape + * belongs to the tree, and this is not one. + */ +export function untrustedRepositoryFrom( + cwd: string, + mountRoot: (dir: string) => string | null = mountRootFor, +): string | null { + // INSIDE-by-spelling and mount-null together are a refusal, not "nothing to + // police": `mountRootFor`'s null is overloaded (see + // `insideReviewTmpLexically`), and a launch directory it REFUSED — a + // symlinked `.qwen/tmp`, an unspellable root — is precisely one no host-side + // command may resolve through. Reading that null as "outside any temp dir" + // inverted the gate's fail direction, measured end-to-end with a symlinked + // `.qwen/tmp`. + if (mountRoot(cwd) === null) { + if (!insideReviewTmpLexically(cwd)) return null; + return ( + `${cwd}: the path is inside the review temp dir by spelling, but no ` + + `mount root answered for it — a redirect or an unmountable shape ` + + `refused it, so where a command run from here would land is unmeasured` + ); + } + if (!existsSync(cwd)) { + // Inside the same spelling, a launch directory that is not there is the + // rename attack, not "no objection": `mv .qwen .qwen-real` from inside + // the outer mount leaves the stale spelling matching lexically while + // `mountRootFor` stops answering, and the next call at the re-stood-up + // spelling must be judged fresh, never inherited. Outside the spelling + // nothing changed: absence stays the caller's own error path. + if (!insideReviewTmpLexically(cwd)) return null; + return ( + `${cwd}: no directory exists at this spelling inside the review temp ` + + `dir — an ancestor was renamed out from under it, so where a command ` + + `run from here would land is unmeasured` + ); + } + const target = resolvedGitDir(cwd); + if (target.value === null) { + // Not a repository from here at all — the caller's own error path owns + // that, and refusing would answer a question nobody asked. Any OTHER null + // is git not answering, and "could not be run" is not "no objection": the + // command this gates resolves through a pointer nobody judged. + return target.notARepository + ? null + : `${cwd}: git could not resolve its own git dir, so where a command run from here would land is unmeasured`; + } + const why = untrustedPointer(cwd, target.value, mountRoot); + return why === null ? null : `${cwd} ${why}`; +} + +/** + * Why a tree's own gitfile cannot be trusted to resolve a host-side git command. + * + * Returns a refusal, or null when there is nothing to police. Every host-side + * git command resolves the repository through the `.git` of the tree it runs + * in, and for every tree this pipeline builds that `.git` sits inside the + * directory the sandbox mounts read-write. Two shapes reach a planted + * repository from there, and both end in a `filter.` command running on the + * host: + * + * - the gitfile rewritten to an admin entry planted under the same mount, and + * - the gitfile REPLACED by a `.git` directory, which is a repository of the + * writer's own and skips every identity gate written for the gitfile shape. + * + * Neither is a shape the pipeline creates under the mount — its trees come + * from `git worktree add`, which writes a gitfile pointing at + * `/.git/worktrees/` — so refusing both costs nothing. Outside a mount + * this says nothing at all: a plain checkout's `.git` IS a directory, and + * refusing that would refuse every ordinary repository. + * + * WHERE IT IS ASKED — kept as a list because this gate was first added one + * call site at a time, and a class closed call site by call site re-opens at + * the next call site somebody adds: + * + * - writes: `scratch-tree`'s reuse and rebuild, `base-tree`, `test-efficacy`'s + * probe-tree restore and worktree creation; + * - reads that the pipeline later TREATS as fact: `worktreeResidue` (below, + * inline, because `status` refreshes the index and a refresh runs the clean + * filter), `comment-status`'s code probes, `repo-context`'s merge-base + * identity reads, and `--resume`'s pre-flight; + * - launch directories rather than trees, via `untrustedRepositoryFrom`: + * `fetch-pr` before its stale sweep, `captureLocalDiff` before its first + * `rev-parse`. + * + * What it does NOT close: the TOCTOU window between this answer and the + * command that uses it (`scratch-tree` documents the same residual), and + * nothing at all where containment cannot exist — Windows, where + * `mountRootFor` refuses every absolute path because a drive letter is a + * colon. `host-execution.canary.test.ts` holds the property for the routes + * above as one test, with a live plant as its oracle. + */ +export function untrustedGitfile( + tree: string, + // Defaulted, but still injectable: the tests drive the judgement with a + // fixture's own root, and `sandboxed-exec` — which used to own + // `mountRootFor` — reaches it from here now, so no import cycle stands in + // the way of a default any more. + mountRoot: (cwd: string) => string | null = mountRootFor, +): string | null { + // No tree, no pointer, nothing to resolve through — and callers reach this + // with a path that may not exist yet (`--resume` asks about a worktree + // before deciding whether to build one). Absence is their question, not + // this one's; answering it here refused every ordinary resume. + if (!existsSync(tree)) return null; + const root = mountRoot(tree); + if (root === null) { + // The tree IS there and its spelling is inside the review temp dir, yet + // no mount root answered: `mountRootFor` REFUSED the path (a redirect, an + // unspellable root), and a refusal is not "nothing to police" — see + // `insideReviewTmpLexically`. Outside the spelling, null really does mean + // there is no writable surface for this question to be about. + if (!insideReviewTmpLexically(tree)) return null; + return ( + `${tree}: the path is inside the review temp dir by spelling, but no ` + + `mount root answered for it — a redirect or an unmountable shape ` + + `refused it, so where a command through its .git would land is unmeasured` + ); + } + const dotGit = join(tree, '.git'); + let stat; + try { + stat = lstatSync(dotGit); + } catch { + return `${tree} has no .git to resolve`; + } + if (!stat.isFile()) { + return `${tree}'s .git is not the gitfile the pipeline created, and the tree is inside the review temp dir`; + } + // The repository git itself would use — see `resolvedGitDir` for why the + // pointer is not parsed here. + const target = resolvedGitDir(tree); + if (target.value === null) { + return `${tree}: git could not resolve its own git dir`; + } + const why = untrustedPointer(tree, target.value, mountRoot); + return why === null ? null : `${tree} ${why}`; +} + +/** + * Whether a linked worktree's admin entry sits where reviewed code can write it. + * + * The gitfile inside a pipeline tree cannot move — git requires `/.git` — + * and that tree is inside the directory the sandbox bind-mounts read-write, so + * containerized code (and, before the sandbox, any code the review ran) can + * rewrite it to `gitdir: `. Put a + * `config` carrying `filter..smudge` in that directory and the next + * HOST-side `git checkout` executes it — out of the container, as the review + * user, on CI with the runner's tokens. + * + * The identity gates that already exist do not catch this, and cannot: the + * planted directory's own backpointer round-trips because the same writer + * chose it, `--show-toplevel` still prints the tree the gitfile sits in, and + * no symlink is involved anywhere. Asking git for the common dir does not help + * either — that answer is resolved THROUGH the rewritten gitfile, so it comes + * from the same hand. + * + * The question that does have an honest answer is about location, not + * content: a real linked worktree's admin entry lives under + * `/.git/worktrees/`, which is outside the mounted directory; a planted + * one has to be inside it, because that is the only place the writer can + * reach. So refuse an admin entry that resolves inside the review temp dir, + * and let every other check stand as it is. + */ +export function adminEntryInsideReviewTmp( + gitDir: string, + mountRoot: (cwd: string) => string | null, + tree: string, +): boolean { + const root = mountRoot(tree); + if (root === null) return false; + let real: string; + let realRoot: string; + try { + // `.native`, because the comparison below is a string comparison and this + // is what decides which strings it gets. Node's JS `realpathSync` resolves + // links in userspace and hands back the spelling it was ASKED for whenever + // the lookup succeeded — on a case-insensitive filesystem (default macOS + // APFS, a host this pipeline supports) a planted entry spelled with + // different case resolves INSIDE the mount and compares OUTSIDE it. The + // native call asks the operating system for the stored name, so both sides + // arrive spelled the way the filesystem actually holds them. + // `.native` when it is there, plain otherwise. The native call is what + // makes the comparison below case-correct (see above), but a suite that + // mocks `node:fs.realpathSync` as a bare `vi.fn` has no `.native` on it, + // and reaching through it threw a TypeError straight into the fail-closed + // catch — refusing every worktree creation in any checkout that happens to + // sit under `.qwen/tmp`. + const canonical = realpathSync.native ?? realpathSync; + real = canonical(resolve(gitDir)); + realRoot = canonical(root); + } catch { + // Unresolvable is not a licence to proceed: the caller's other gates + // report it, and answering "not inside" here would be a guess. + return true; + } + // `isSubpath`, not a hand-rolled prefix test. The first cut was hand-rolled + // and failed open at both boundaries of the question it was asking: an entry + // whose realpath IS the mount root produced an empty relative path and was + // read as "outside" — while the root is exactly as writable as anything + // under it — and a direct child legitimately named `..evil-git` produced a + // relative path starting with `..`, which the prefix test read as an escape. + // Both are places a planted entry can sit, and both are why path containment + // belongs in one tested helper rather than in each caller's arithmetic. + // + // And judged against the OUTERMOST review temp root on that path, not the + // one the mount was cut at. `mountRootFor` takes the deepest `.qwen/tmp` + // deliberately — the first occurrence would widen the MOUNT to the outer + // temp dir and pull `/.git` and every sibling checkout into the + // container — but "which directory may a container have been given + // read-write" and "where may reviewed code already have written" are not the + // same question. In the nested geometry `mountRootFor` documents, the OUTER + // review's containerized phase held the outer `.qwen/tmp` read-write, so an + // admin entry planted one layer up sits outside the inner root and inside a + // writable surface all the same — and judging only against the inner root + // admitted it, after which the very `worktree add` this gate protects ran + // the planted filter. + // + // Widening costs the honest layouts nothing, because git does not put a + // linked worktree's admin entry beside its tree: `worktree add` run from + // INSIDE a linked worktree still writes its entry under the MAIN + // repository's `.git/worktrees/`, which is outside every layer's + // `.qwen/tmp` rather than merely outside the deepest. What it does refuse is + // a repository whose own `.git` lives under a review temp dir — a checkout + // cloned into one. Nothing this pipeline builds has that shape (every tree + // it makes is a linked worktree), and a repository sitting inside the + // directory reviewed code is handed read-write is not one a host-side write + // should resolve through anyway. + return isSubpath(outermostReviewTmpRoot(realRoot), real); +} + +/** + * The shallowest review temp root on a canonical review temp root's own path. + * + * `mountRootFor` already cut at the DEEPEST `.qwen/tmp`, so the string handed + * in ends at one; any earlier occurrence in it is an enclosing review's temp + * dir, and the shallowest is the widest writable surface this tree sits in. + * Because the layers nest, the outermost alone answers for all of them — an + * entry inside any inner layer is inside this one too. + * + * Lexical on purpose: the input is already canonical (`adminEntryInsideReviewTmp` + * realpaths it before asking), so there is no link left to resolve, and a + * separator-delimited marker cannot match half a path component. A root with + * no `.qwen/tmp` inside it — an injected mount in a fixture — comes back + * unchanged, so the judgement is never widened past what was handed in. + */ +function outermostReviewTmpRoot(root: string): string { + const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; + const at = root.indexOf(marker); + return at < 0 ? root : root.slice(0, at + marker.length - 1); +} + /** * The first symlink at or above `dir`, or null when every component is real. * @@ -1029,6 +1629,68 @@ export function worktreeResidue( // No `.git` at all: the walk-up check below fails closed with its own // reason. } + // WHERE the repository lives, asked BEFORE the first spawn that can execute. + // `status` refreshes the index, and an index refresh runs the repository's + // configured content filters — so a gitfile rewritten to an admin entry + // planted under the same read-write mount, whose common dir carries + // `filter..process`, turns this tripwire into host-side code execution. + // The measurement is the attack. + // + // Every identity check below is honest about WHICH repository answers and + // blind to where that repository lives, and it cannot be otherwise: a + // coherent plant writes both halves of the round trip, so the entry points + // back; `--show-toplevel` prints the tree the gitfile sits in; no symlink is + // involved; and the sha pin can only compare against a record the same + // writer can rewrite. Location is the question that has an honest answer — + // `worktree add` puts a real admin entry under the MAIN repository's + // `.git/worktrees/`, outside the mount, and a planted one has to be inside + // it. See `untrustedGitfile`, which asks the same question for the writes. + // + // Unmeasured-with-reason, not a throw: this is a tripwire, and a tree whose + // pointer cannot be trusted is precisely what the `unmeasured` channel + // exists to report — every caller already treats it as "not clean". + // + // The mount's null is overloaded (see `insideReviewTmpLexically`): outside + // any review temp dir it means the question does not arise and an ordinary + // checkout never pays for the extra spawn — but INSIDE the spelling it is a + // REFUSAL (a redirect, an unspellable root), and reading it as "nothing to + // police" ran the measurement through exactly the redirect `mountRootFor` + // had just declined to mount. Distinguish lexically, fail closed. + if (mountRootFor(cwd) === null && insideReviewTmpLexically(cwd)) { + return { + paths: [], + total: 0, + unmeasured: + 'the path is inside the review temp dir by spelling, but no mount ' + + 'root answered for it — a redirect or an unmountable shape refused ' + + 'it, and the status below would measure whichever repository the ' + + 'pointer here names', + }; + } + if (mountRootFor(cwd) !== null) { + const target = resolvedGitDir(cwd); + // git not answering is not "no objection", and widening this gate's budget + // cannot make it so: the spawns below carry none at all, so a config sized + // past it used to reach a `status` that refreshes the planted index and + // runs its clean filter on the host. A genuine not-a-repository still + // falls through — the walk-up check below fails closed on it with its own + // reason. + let why: string | null; + if (target.value !== null) { + why = untrustedPointer(cwd, target.value, mountRootFor); + } else { + why = target.notARepository + ? null + : 'git could not resolve its own git dir'; + } + if (why !== null) { + return { + paths: [], + total: 0, + unmeasured: `the .git gitfile ${why} — the status below would measure whichever repository it names, and refresh that index through whatever filters it configures`, + }; + } + } // git's discovery WALKS UP: with the `.git` file gone — a crash mid-`worktree // add`, a cleanup whose `rmSync` failed — `status` exits 0 against the // enclosing user checkout: the wrong tree's dirty state answered as this diff --git a/packages/cli/src/commands/review/load-rules.ts b/packages/cli/src/commands/review/load-rules.ts index 1be50d2d210..d3eb0a27619 100644 --- a/packages/cli/src/commands/review/load-rules.ts +++ b/packages/cli/src/commands/review/load-rules.ts @@ -25,17 +25,13 @@ import type { CommandModule } from 'yargs'; import { mkdirSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; import { writeStdoutLine } from '../../utils/stdioHelpers.js'; -import { gitOpt } from './lib/git.js'; +import { gitProbe } from './lib/git.js'; interface LoadRulesArgs { base_ref: string; out: string; } -function showFile(baseRef: string, path: string): string | null { - return gitOpt('show', `${baseRef}:${path}`); -} - export function extractCodeReviewSection(content: string): string | null { // Find `## Code Review` heading and return everything up to the next // top-level `## ` heading, or end of file. Done with line-based scanning @@ -56,15 +52,31 @@ export function extractCodeReviewSection(content: string): string | null { return lines.slice(start, end).join('\n').trim(); } -function loadCombined(baseRef: string): { +export function loadCombined(baseRef: string): { combined: string; loaded: string[]; + unread: string[]; } { const sections: string[] = []; const loaded: string[] = []; + // A source git could not be ASKED about is not an absent source. `status === + // null` is `gitProbe`'s "the command could not be run at all" — a launch + // directory this process must not resolve through, a spawn failure, a timeout + // — while git answers a path absent at the ref with 128. Collapsing the two + // wrote an empty rules file and printed "No review rules found", which + // `agent-prompt --roster` stapled into every agent brief: the whole fan-out + // ran with none of the project's `## Code Review` rules and nothing in the + // run said why. `comment-status` gives its own untrusted-worktree degradation + // a distinct warning for exactly this reason. + const unread: string[] = []; + const showFile = (path: string): string | null => { + const { out, status } = gitProbe('show', `${baseRef}:${path}`); + if (status === null) unread.push(path); + return out; + }; // 1. Qwen-native rules. - const qwenRules = showFile(baseRef, '.qwen/review-rules.md'); + const qwenRules = showFile('.qwen/review-rules.md'); if (qwenRules) { sections.push(`### From .qwen/review-rules.md\n\n${qwenRules.trim()}`); loaded.push('.qwen/review-rules.md'); @@ -73,14 +85,14 @@ function loadCombined(baseRef: string): { // 2. Copilot-compatible rules: prefer .github/copilot-instructions.md; // only fall back to root-level copilot-instructions.md if the // preferred one doesn't exist on the base branch. - const copilotPreferred = showFile(baseRef, '.github/copilot-instructions.md'); + const copilotPreferred = showFile('.github/copilot-instructions.md'); if (copilotPreferred) { sections.push( `### From .github/copilot-instructions.md\n\n${copilotPreferred.trim()}`, ); loaded.push('.github/copilot-instructions.md'); } else { - const copilotFallback = showFile(baseRef, 'copilot-instructions.md'); + const copilotFallback = showFile('copilot-instructions.md'); if (copilotFallback) { sections.push( `### From copilot-instructions.md\n\n${copilotFallback.trim()}`, @@ -90,7 +102,7 @@ function loadCombined(baseRef: string): { } // 3. AGENTS.md — extract Code Review section only. - const agentsMd = showFile(baseRef, 'AGENTS.md'); + const agentsMd = showFile('AGENTS.md'); if (agentsMd) { const section = extractCodeReviewSection(agentsMd); if (section) { @@ -100,7 +112,7 @@ function loadCombined(baseRef: string): { } // 4. QWEN.md — extract Code Review section only. - const qwenMd = showFile(baseRef, 'QWEN.md'); + const qwenMd = showFile('QWEN.md'); if (qwenMd) { const section = extractCodeReviewSection(qwenMd); if (section) { @@ -112,19 +124,30 @@ function loadCombined(baseRef: string): { return { combined: sections.join('\n\n---\n\n'), loaded, + unread, }; } async function runLoadRules(args: LoadRulesArgs): Promise { const { base_ref: baseRef, out } = args; - const { combined, loaded } = loadCombined(baseRef); + const { combined, loaded, unread } = loadCombined(baseRef); mkdirSync(dirname(out), { recursive: true }); writeFileSync(out, combined, 'utf8'); + if (unread.length > 0) { + writeStdoutLine( + `warning: could not read ${unread.length} rule source(s) from ${baseRef} — ` + + `${unread.join(', ')}. ${out} is INCOMPLETE rather than empty by ` + + `choice, so every agent brief built from it is missing them. Run the ` + + `review from a checkout outside the review temp dir.`, + ); + } if (loaded.length === 0) { writeStdoutLine( - `No review rules found on ${baseRef}; wrote empty file to ${out}`, + unread.length === 0 + ? `No review rules found on ${baseRef}; wrote empty file to ${out}` + : `No review rules could be read on ${baseRef}; wrote an incomplete file to ${out}`, ); } else { writeStdoutLine( diff --git a/packages/cli/src/commands/review/repo-context.test.ts b/packages/cli/src/commands/review/repo-context.test.ts index e9573a25907..b21aa20c10d 100644 --- a/packages/cli/src/commands/review/repo-context.test.ts +++ b/packages/cli/src/commands/review/repo-context.test.ts @@ -33,7 +33,7 @@ import { runRepoContext, } from './repo-context.js'; import { stringifyPlanReport } from './lib/report.js'; -import { isolateHostGitConfig } from './lib/test-utils.js'; +import { isolateHostGitConfig, plantAdminEntry } from './lib/test-utils.js'; import { appendRunSession, priorSessionIds, @@ -744,6 +744,94 @@ describe('repo-context providers and trust boundary', () => { expect(readJson(planPath)).toHaveProperty('repositoryContext', context()); }); + // On Windows `mountRootFor` refuses every absolute path (a drive letter is a + // colon), so containment cannot exist there and this pair of cases has no + // answer to assert. + const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + + itWhereContainmentExists( + 'refuses to read identity files through a rewritten review-worktree gitfile', + () => { + // This command's whole point is that the identity files come from the + // MERGE BASE rather than from the PR head — and every read it makes + // (`--git-common-dir`, `cat-file -e`, `ls-tree`, `show :`) + // resolves the repository through the review worktree's own `.git`, + // which sits inside the directory the sandbox hands the reviewed code + // read-write. Read through a rewritten pointer, the "merge base" + // identity is whatever the PR author committed into a repository they + // planted, and the boundary is decoration. + const root = temp(); + const repository = join(root, 'repository'); + initGit(repository); + write(join(repository, 'src', 'change.ts'), 'base\n'); + const base = commitAll(repository); + const worktree = join(repository, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(dirname(worktree), { recursive: true }); + execFileSync('git', [ + '-C', + repository, + 'worktree', + 'add', + '-q', + '--detach', + worktree, + 'HEAD', + ]); + // The plant: an admin entry copied beside the tree, inside the same + // temp dir, with git's own BARE backpointer so the round-trip gates + // this command inherits all agree with it. + plantAdminEntry( + join(repository, '.qwen', 'tmp', '.evil-git'), + join(repository, '.git', 'worktrees', 'review-pr-1'), + worktree, + join(repository, '.git'), + ); + + expect(() => + run(root, worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }), + ).toThrow(/refusing to read the repository context/); + }, + ); + + itWhereContainmentExists( + 'still reads a review worktree whose pointer is the one the pipeline wrote', + () => { + // The admit arm: the geometry the gate above refuses is the SAME + // geometry every PR review runs in, so a gate that refused on location + // alone would refuse every review. + const root = temp(); + const repository = join(root, 'repository'); + initGit(repository); + // Committed, because the identity read this command makes is + // `git show :` — a manifest that exists only in the + // working tree is the HEAD-side one the trust boundary refuses. + writeManifest(repository); + write(join(repository, 'src', 'change.ts'), 'base\n'); + const base = commitAll(repository); + const worktree = join(repository, '.qwen', 'tmp', 'review-pr-1'); + mkdirSync(dirname(worktree), { recursive: true }); + execFileSync('git', [ + '-C', + repository, + 'worktree', + 'add', + '-q', + '--detach', + worktree, + 'HEAD', + ]); + + const { outPath } = run(root, worktree, { + files: [{ path: 'src/change.ts' }], + mergeBaseSha: base, + }); + expect(readJson(outPath)).toEqual(manifestContext()); + }, + ); + it('rejects a recorded worktree path that matches no checkout', () => { // The guard's rejection branch: a plan recorded for one checkout must // not be served identity reads from a different worktree. diff --git a/packages/cli/src/commands/review/repo-context.ts b/packages/cli/src/commands/review/repo-context.ts index 54d966a8e1e..ddc0f912fc3 100644 --- a/packages/cli/src/commands/review/repo-context.ts +++ b/packages/cli/src/commands/review/repo-context.ts @@ -26,6 +26,7 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { git, gitOpt, gitRaw } from './lib/git.js'; import { manifestRepositoryContextProvider } from './lib/manifest-repository-context.js'; import { isSameFile } from './lib/same-file.js'; +import { untrustedGitfile } from './lib/worktree.js'; import { isSafeRepositoryRelativePath, MAX_IDENTITY_BYTES, @@ -390,6 +391,23 @@ export function runRepoContext( } throw err; } + // Every read below — `rev-parse --git-common-dir`, `cat-file -e`, `ls-tree`, + // `show :` — resolves the repository through this worktree's own + // `.git`, which sits inside the directory the review sandbox hands the + // reviewed code read-write. The identity files this command extracts are the + // ones the trust boundary exists to take from the MERGE BASE rather than + // from the PR head; read through a rewritten pointer they come from a + // repository the PR author planted, and the whole boundary is decorative. + // Same gate, same reason, as the writes in `scratch-tree` and `fetch-pr`. + const untrusted = untrustedGitfile(worktree); + if (untrusted !== null) { + throw new Error( + `repo-context: refusing to read the repository context — ${untrusted}. ` + + `The merge-base identity files would come from whichever repository ` + + `that pointer names. Sweep the tree (\`qwen review cleanup\`) and ` + + `re-fetch before re-running.`, + ); + } // The plan's identity, captured BEFORE the provider work. The providers // take real time, the plan path is shared per PR, and a concurrent capture diff --git a/packages/cli/src/commands/review/revert-hunk.test.ts b/packages/cli/src/commands/review/revert-hunk.test.ts index bb29b9d7b1c..f59a887e42f 100644 --- a/packages/cli/src/commands/review/revert-hunk.test.ts +++ b/packages/cli/src/commands/review/revert-hunk.test.ts @@ -23,8 +23,10 @@ import { mkdtempSync, readdirSync, readFileSync, + renameSync, rmSync, statSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { dirname, join } from 'node:path'; @@ -106,6 +108,42 @@ function twoHunkFixture(trailingNewline = true) { return { dir, diffPath }; } +/** + * A scratch tree in the pipeline's real geometry — a genuine linked worktree + * at `/.qwen/tmp/shard/scratch`, one commit deep, with the PR diff + * beside it. The `shard` component is the ancestor between the review temp + * root and the leaf that the R24-1 witness swaps for a symlink. + */ +function scratchTreeFixture() { + const repo = join(tempDir('rh-scratch-'), 'repo'); + mkdirSync(repo, { recursive: true }); + git(repo, 'init', '-q', '-b', 'main'); + git(repo, 'config', 'user.email', 't@t'); + git(repo, 'config', 'user.name', 't'); + writeFileSync(join(repo, 'a.ts'), 'export const x = 2;\n'); + git(repo, 'add', '.'); + git(repo, 'commit', '-qm', 'pr head'); + const diffPath = join(repo, 'p.diff'); + writeFileSync( + diffPath, + [ + 'diff --git a/a.ts b/a.ts', + 'index 1111111..2222222 100644', + '--- a/a.ts', + '+++ b/a.ts', + '@@ -1 +1 @@', + '-export const x = 1;', + '+export const x = 2;', + '', + ].join('\n'), + ); + const shard = join(repo, '.qwen', 'tmp', 'shard'); + mkdirSync(shard, { recursive: true }); + const scratch = join(shard, 'scratch'); + git(repo, 'worktree', 'add', '--detach', scratch); + return { diffPath, shard, scratch }; +} + describe('listHunks', () => { it('enumerates hunks under stable : ids with their headers and counts', () => { const { diffPath } = twoHunkFixture(); @@ -716,6 +754,94 @@ describe('runRevertHunk', () => { expect(readFileSync(join(dir, 'y'), 'utf8')).toBe(before); // decoy untouched }); + it('refuses a --tree that IS a symlink, and leaves its target untouched (R15-5)', () => { + // `resolve(args.tree)` is lexical, `gitTreeState` realpaths BOTH sides on + // purpose so a linked tree answers 'root', and `untrustedGitfile`'s + // existsSync/lstatSync pair dereferences every component except the last — + // so a link AT the path is invisible to all three while the `.git` they + // inspect is the TARGET's genuine gitfile, whose admin entry sits outside + // the mount and is admitted. `gitApply` then spawns with `cwd: tree` and + // reverse-applies into whichever tree the link names — in the pipeline, the + // shared review worktree other agents are reading, or the A/B's base side — + // while the report certifies the scratch path. None of the suite's other + // `runRevertHunk` call sites can reach this: their fixtures are outside any + // `.qwen/tmp`, so the location gate never speaks there. + const dir = tempDir('rh-tree-target-'); + git(dir, 'init', '-q', '-b', 'main'); + git(dir, 'config', 'user.email', 't@t'); + git(dir, 'config', 'user.name', 't'); + writeFileSync(join(dir, 'a.ts'), 'export const x = 2;\n'); + git(dir, 'add', '.'); + git(dir, 'commit', '-qm', 'head'); + const diffPath = join(dir, 'p.diff'); + writeFileSync( + diffPath, + [ + 'diff --git a/a.ts b/a.ts', + 'index 1111111..2222222 100644', + '--- a/a.ts', + '+++ b/a.ts', + '@@ -1 +1 @@', + '-export const x = 1;', + '+export const x = 2;', + '', + ].join('\n'), + ); + const link = join(tempDir('rh-tree-link-'), 'scratch'); + symlinkSync(dir, link); + + const r = runRevertHunk({ diff: diffPath, tree: link, hunk: 'a.ts:1' }); + expect(r.applied).toBe(false); + expect(r.harnessFailure).toBe(true); + expect(r.note).toContain('is a symlink'); + // The tree the link names is byte-identical, which is the part the report + // would otherwise have lied about. + expect(readFileSync(join(dir, 'a.ts'), 'utf8')).toBe( + 'export const x = 2;\n', + ); + }); + + it('refuses a --tree whose ANCESTOR under the review temp root is a symlink (R24-1)', () => { + // The R15-5 leaf check lstats the scratch path itself; a link swapped in + // one component ABOVE it — anywhere between the review temp root and the + // leaf, the writable surface the sandbox hands the reviewed code — is + // invisible to that lstat, to `gitTreeState`'s realpath-both-sides + // compare (both sides resolve THROUGH the link), and to + // `untrustedGitfile` (the gitfile it reaches is the target's genuine + // one). `git apply -R` would then spawn with `cwd: tree` and reverse into + // whichever tree the link names while the report certifies the scratch + // path. The swap here keeps the link's target a real directory holding + // the same tree, so every resolving check still passes and only the + // bounded ancestor walk can speak. + const { diffPath, shard, scratch } = scratchTreeFixture(); + const shardReal = join(dirname(shard), 'shard-real'); + renameSync(shard, shardReal); + symlinkSync(shardReal, shard); + + const r = runRevertHunk({ diff: diffPath, tree: scratch, hunk: 'a.ts:1' }); + expect(r.applied).toBe(false); + expect(r.harnessFailure).toBe(true); + expect(r.note).toContain('ancestor'); + expect(r.note).toContain('is a symlink'); + // The tree the link resolves into is byte-identical — the part the + // report would otherwise have lied about. + expect(readFileSync(join(scratch, 'a.ts'), 'utf8')).toBe( + 'export const x = 2;\n', + ); + }); + + it('still applies in a scratch tree under the review temp root with no redirect (R24-1 control)', () => { + // The bounded walk must not buy its refusal at the price of the ordinary + // path: a genuine linked worktree under `.qwen/tmp` has every ancestor + // between the temp root and the leaf real, and the revert applies. + const { diffPath, scratch } = scratchTreeFixture(); + const r = runRevertHunk({ diff: diffPath, tree: scratch, hunk: 'a.ts:1' }); + expect(r.applied).toBe(true); + expect(readFileSync(join(scratch, 'a.ts'), 'utf8')).toBe( + 'export const x = 1;\n', + ); + }); + it('refuses a symlink type-change section — git apply -R restores content, not TYPE (R16-6)', () => { // A single-section symlink→file capture applies with exit 0 leaving a // REGULAR FILE holding the old target string where base had a link. Same diff --git a/packages/cli/src/commands/review/revert-hunk.ts b/packages/cli/src/commands/review/revert-hunk.ts index b80fe89a74b..c12ffedf36e 100644 --- a/packages/cli/src/commands/review/revert-hunk.ts +++ b/packages/cli/src/commands/review/revert-hunk.ts @@ -64,8 +64,12 @@ import { unquote, stripHeaderTimestamp, } from './lib/diff-plan.js'; -import { sanitizedGitEnv } from './lib/worktree.js'; -import { assertWritableOutPath } from './lib/paths.js'; +import { + redirectedAncestor, + sanitizedGitEnv, + untrustedGitfile, +} from './lib/worktree.js'; +import { assertWritableOutPath, REVIEW_TMP_DIR } from './lib/paths.js'; import { ignoreBrokenPipe, writeStdoutLineSafe, @@ -790,6 +794,72 @@ export function runRevertHunk(args: RevertHunkArgs): RevertHunkReport { } const tree = resolve(args.tree); + // ...and `--tree` must still BE the tree every check below judges. `resolve` + // is lexical, `gitTreeState` realpaths BOTH sides on purpose, and + // `untrustedGitfile`'s `existsSync`/`lstatSync(join(tree,'.git'))` pair + // dereferences every component except the last — so a link AT the scratch + // path is invisible to all three, and the `.git` they inspect is the TARGET's + // genuine gitfile, whose admin entry sits outside the mount and is admitted. + // `gitApply` then spawns with `cwd: tree` and reverse-applies into whichever + // tree the link names: the shared worktree other agents are reading, or the + // A/B's base side. `resetScratchTree` refuses this leaf for the same reason, + // and re-reads it immediately before its own mutation because a link swapped + // in during a gate that is many spawns long aims the write at whatever it + // names. + // + // The leaf AND its ancestors, bounded at the outermost review temp root — + // the walk `resetScratchTree` makes, with the bound this command can know. + // `--tree` is whatever the caller passed, so there is no common dir to stop + // at; but the tree this command exists to mutate is the scratch tree under + // `.qwen/tmp`, and everything between that root and the leaf is the + // directory the sandbox hands the reviewed code read-write — exactly where + // a writer can swap a component for a link AFTER the create/sweep checks + // (`mountRootFor`, `releaseWorktree`, `runCleanup`) ran, and a link there is + // as invisible to the gates below as one at the leaf: they all resolve + // THROUGH it. Above the temp root is the user's own layout — an unbounded + // walk would refuse every tree under a linked `/tmp`, and `/var` is a + // symlink on every macOS box — so a `--tree` outside any review temp root + // gets the leaf check alone. + const treeRedirected = (): string | null => { + try { + if (lstatSync(tree).isSymbolicLink()) { + return `--tree ${JSON.stringify(args.tree)} is a symlink`; + } + // Outermost `.qwen/tmp` on the path, the bound + // `outermostReviewTmpRoot` computes: the FIRST occurrence, so a nested + // review temp root still walks the inner components. `redirectedAncestor` + // lstats each component from the leaf's parent up to and including the + // bound, and stops there. + const marker = `${sep}${REVIEW_TMP_DIR}${sep}`; + const at = tree.indexOf(marker); + if (at >= 0) { + const bound = tree.slice(0, at + marker.length - 1); + const ancestor = redirectedAncestor(dirname(tree), bound); + if (ancestor !== null) { + return `an ancestor of --tree ${JSON.stringify(args.tree)} is a symlink: ${JSON.stringify(ancestor)}`; + } + } + } catch { + // Unreadable: the apply path's own spawn-error classification answers it. + } + return null; + }; + const redirectReport = (swapped?: true): RevertHunkReport | null => { + const redirected = treeRedirected(); + if (redirected === null) return null; + return { + applied: false, + hunk: entry, + harnessFailure: true, + note: `${redirected}, so the apply would reverse into whichever tree the link names while the report certified ${JSON.stringify(args.tree)}${ + swapped + ? ' — no link was there when the gates above ran, and the write is what it would aim' + : '' + }. Reset the scratch tree (\`qwen review scratch-tree\`) and retry; nothing was changed.`, + }; + }; + const redirectedTree = redirectReport(); + if (redirectedTree !== null) return redirectedTree; // git apply needs no repository, so a --tree that is a plain (non-repo) // directory would either fabricate a coupling fact on a content mismatch or // silently mutate the wrong directory on a match — and a bare clone or a @@ -815,6 +885,26 @@ export function runRevertHunk(args: RevertHunkArgs): RevertHunkReport { note: `--tree ${JSON.stringify(args.tree)} is a SUBDIRECTORY of a work tree, not its root — git apply resolves the patch's paths against the toplevel and silently SKIPS any that fall outside this subdirectory, exiting 0 without touching them, so applied:true would be a false witness over an unchanged file. Point --tree at the work-tree root (the scratch worktree); nothing was changed.`, }; } + // ...and the repository the reads below resolve to must be the tree's own. + // `--show-toplevel` above still prints this tree when the gitfile has been + // rewritten to a planted admin entry — that is exactly the shape the + // location gate exists for — while `check-attr` and `ls-files` below then + // answer out of the plant: the EOL regime this function refuses a conversion + // on, and the target's own shape, both decided by whoever wrote the pointer. + // A scratch tree is inside the directory the sandbox hands the reviewed code + // read-write, so that writer is the code under review. + // + // A harness fact, like every other refusal here: nothing about the hunk is + // being claimed, and the tree is untouched. + const untrusted = untrustedGitfile(tree); + if (untrusted !== null) { + return { + applied: false, + hunk: entry, + harnessFailure: true, + note: `--tree ${JSON.stringify(args.tree)}: ${untrusted} — the attribute and index reads this needs would come from whichever repository that pointer names, so no revert is attempted. Reset the scratch tree (\`qwen review scratch-tree\`) and retry; nothing was changed.`, + }; + } const targetPath = treePath(tree, targetBytes); // Line-ending regime, pinned like whitespace is. `git apply -R` re-runs EOL // conversion when it writes the restored bytes: `core.autocrlf` is disabled @@ -1021,6 +1111,11 @@ export function runRevertHunk(args: RevertHunkArgs): RevertHunkReport { } }; const before = snapshot(); + // Re-read the leaf and its ancestors immediately before the write, the + // second half of the shape `resetScratchTree` refuses twice: the gate + // above is many spawns long, and this is the call that mutates a tree. + const swapped = redirectReport(true); + if (swapped !== null) return swapped; const apply = exec(tree, [ ...gitEol, 'apply', diff --git a/packages/cli/src/commands/review/scratch-tree.test.ts b/packages/cli/src/commands/review/scratch-tree.test.ts index ae3553d4e08..84f250962f3 100644 --- a/packages/cli/src/commands/review/scratch-tree.test.ts +++ b/packages/cli/src/commands/review/scratch-tree.test.ts @@ -45,9 +45,21 @@ import { type ScratchTreeArgs, } from './scratch-tree.js'; import { scratchWorktreePath } from './lib/paths.js'; -import { isolateHostGitConfig } from './lib/test-utils.js'; +import { + adminEntryOf, + isolateHostGitConfig, + plantAdminEntry, + plantRepository, +} from './lib/test-utils.js'; import { shellQuotePath } from './lib/shell-quote.js'; +// Skipped on win32 for the same reason as the sibling suites: `mountRootFor` +// refuses every absolute Windows path (a drive letter is a colon), so +// containment is unavailable there by design and this gate never speaks. The +// assertion would fail for that reason and nothing else — first inside the +// merge queue, where that lane actually runs. +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + describe('runScratchTree', () => { let repo: string; // See `lib/worktree.test.ts`: a polluted host gitconfig makes the fixture @@ -86,6 +98,134 @@ describe('runScratchTree', () => { gitIsolation.dispose(); }); + itWhereContainmentExists( + 'runs nothing on the host when BOTH trees point at a planted repository', + () => { + // What this pins is the OUTCOME — no host execution, and a refusal — + // for the hardest shape reviewed code can build: both the scratch tree + // and the review worktree rewritten to admin entries under the mount + // that name one planted common dir, carrying `filter.*.process`. + // + // HONEST LIMIT: it does not isolate the reuse-path gate. Removing that + // gate leaves this green, because the reset declines this tree for its + // own reasons before reaching a checkout — so the gate is defence in + // depth here, not a demonstrated load-bearing check. Said out loud + // rather than left to look proven. + const first = run(); + expect(first.available).toBe(true); + const tree = scratchWorktreePath(worktree, 'verify--round-1--abc123'); + // A COHERENT planted repository carrying a real filter, so the oracle is + // the property itself: if the reset's `checkout --force` runs through this + // pointer, the filter executes on the host and writes the canary. No + // file-based marker can serve here — the reset's `clean -ffdx` removes + // untracked files, so reuse and rebuild leave the tree looking the same. + // + // `process`, not `smudge`: any filter shape serves, because which + // refusal speaks first is not the pin. The repo-local filter screen + // resolves through the same planted pointer, so it sees the plant's + // config and refuses before the location gate is ever asked — either + // refusal certifies the property, and the sibling tests that carry no + // filter are where the location gate is load-bearing. + const canary = join(repo, 'PWNED-SCRATCH'); + const fakeCommon = plantRepository( + join(repo, '.qwen', 'tmp', '.evil-common'), + join(repo, '.git'), + canary, + 'process', + ); + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-scratch'), + adminEntryOf(tree), + tree, + fakeCommon, + ); + // BOTH trees, pointing at ONE planted common dir. The reset's own gate + // compares the two trees' common dirs, so poisoning the scratch tree + // alone makes it refuse for a reason unrelated to this one — the shape + // that actually reaches its checkout is the pair. + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-wt'), + adminEntryOf(worktree), + worktree, + fakeCommon, + ); + + const second = run(); + // Nothing checked out through the planted pointer — the property. The + // command's own verdict may be a refusal (the rebuild gate refuses the + // poisoned review worktree next), but what must never happen is the + // reset running first and executing the filter. Either layer's refusal + // certifies it: the filter screen (which resolves through the same + // plant and sees its config), or the location gate (the admin entry is + // inside the review temp dir). + expect(existsSync(canary)).toBe(false); + expect(second.available).toBe(false); + expect(JSON.stringify(second)).toMatch(/content filter|review temp dir/); + }, + ); + + itWhereContainmentExists( + 'refuses to stand one up through a rewritten review-worktree gitfile', + () => { + // The sibling screen below catches a repo-local `filter.*.smudge|clean` in + // the REAL config. It cannot catch a pointer that names a different + // repository entirely — nor `filter..process`, which its regex does not + // match and which also executes — so the pointer itself has to be checked. + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + adminEntryOf(worktree), + worktree, + join(repo, '.git'), + ); + + const r = run(); + expect(JSON.stringify(r)).toContain('review temp dir'); + expect( + existsSync(scratchWorktreePath(worktree, 'verify--round-1--abc123')), + ).toBe(false); + }, + ); + + itWhereContainmentExists( + 'does not REUSE a scratch tree through a review-worktree gitfile rewritten to the common dir', + () => { + // The reuse route asked the location question of the SCRATCH pointer + // only, never of the review worktree's — which is what `headSha` and + // every comparison inside the reset resolve through. Rewritten to the + // repository's own common dir it resolves OUTSIDE the mount, so the + // location question admitted it, and an unpinned run answered + // `available: true, reused: true` with the MAIN head and reset the + // verifier's tree to the user's own commit: a wrong verdict carrying a + // deterministic source tag. + const first = run(); + expect(first.available).toBe(true); + const tree = scratchWorktreePath(worktree, 'verify--round-1--abc123'); + expect(git(tree, 'rev-parse', 'HEAD')).toBe(headSha); + + // Main moves on, so the head a rewritten pointer answers is + // distinguishable from the one this review is pinned to. + writeFileSync(join(repo, 'a.ts'), 'export const x = 999;\n'); + git(repo, 'commit', '-qam', 'main moved on'); + const mainHead = git(repo, 'rev-parse', 'HEAD'); + rmSync(join(worktree, '.git'), { force: true }); + writeFileSync(join(worktree, '.git'), `gitdir: ${join(repo, '.git')}\n`); + + const second = run(); + expect(second.available).toBe(false); + expect(second.reused).toBe(false); + expect(second.headSha).toBeUndefined(); + expect(JSON.stringify(second)).toContain('common dir'); + // The fixture really does distinguish the two heads, so the verdict + // above is about the rewrite and not about a no-op commit. + expect(mainHead).not.toBe(headSha); + // Swept for a rebuild the gate then refused is the expected outcome; if + // a tree does survive, it is still at the reviewed head, not the user's. + if (existsSync(tree)) { + expect(git(tree, 'rev-parse', 'HEAD')).toBe(headSha); + } + }, + ); + it('stands up a sibling tree at the commit under review', () => { const r = run(); expect(r.available).toBe(true); diff --git a/packages/cli/src/commands/review/scratch-tree.ts b/packages/cli/src/commands/review/scratch-tree.ts index 8fbcb0227a1..6898e774e40 100644 --- a/packages/cli/src/commands/review/scratch-tree.ts +++ b/packages/cli/src/commands/review/scratch-tree.ts @@ -98,6 +98,7 @@ import { } from './lib/paths.js'; import { shellQuotePath } from './lib/shell-quote.js'; import { + untrustedGitfile, RESIDUE_PATH_CAP, describeFilterScreen, discardWorktree, @@ -718,7 +719,28 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport { 'instead of clearing one).' : ''; - if (existsSync(tree) && resetScratchTree(tree, headSha, worktree)) { + // The REUSE path reaches a `checkout --force` of its own, through the SCRATCH + // tree's gitfile — inside the mount just like the review worktree's — so + // gating only the rebuild below left the cheaper route open. + // + // Not a refusal, though: an unusable leftover is exactly what the rebuild + // exists for, and refusing here turned "rebuild over it" into "fail the + // command". A pointer that cannot be trusted simply is not reusable — the + // rebuild below discards the tree (removing the plant with it) and creates a + // fresh one through the review worktree's pointer, which the gate after this + // one checks. + if ( + existsSync(tree) && + untrustedGitfile(tree) === null && + // ...and of the REVIEW worktree's pointer, which is what `headSha` and + // every comparison inside the reset resolve through. Rewritten to the + // repository's own common dir, the route answered `available: true` with + // the MAIN head and reset this tree to the user's own commit. Suspicion + // falls through to the rebuild, whose gate refuses — not a refusal here: + // an unusable leftover is what the rebuild exists for. + untrustedGitfile(worktree) === null && + resetScratchTree(tree, headSha, worktree) + ) { // The reset clears the ignored state too, so the farm went with it: this // re-links it. `rebuild` rather than trusting a marker, because // `node_modules` is where a probe is told it may install, and anything a @@ -758,6 +780,16 @@ export function runScratchTree(args: ScratchTreeArgs): ScratchTreeReport { // Clears both a leftover from a crashed run and a tree the reset above // could not rescue; either would fail `add` with `already exists`. sweep = discardWorktree(worktree, tree); + // The same question the probe phase asks before its own `worktree add`: + // this resolves the repository through the REVIEW worktree's gitfile, + // which lives inside the directory the sandbox mounts read-write and + // which the build/test phase already gave the reviewed code a chance to + // rewrite. `worktree add` checks files out, so it runs whatever that + // pointer leads to, on the host. See `untrustedGitfile`. + const untrusted = untrustedGitfile(worktree); + if (untrusted !== null) { + throw new Error(`refusing to create a scratch tree: ${untrusted}`); + } git(worktree, 'worktree', 'add', '--detach', tree, headSha); } catch (e) { // Not `unavailable()`: the residue was already measured, and a report whose diff --git a/packages/cli/src/commands/review/test-efficacy.integration.test.ts b/packages/cli/src/commands/review/test-efficacy.integration.test.ts index 815c156023f..86600809126 100644 --- a/packages/cli/src/commands/review/test-efficacy.integration.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.integration.test.ts @@ -15,6 +15,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; import { mkdtempSync, + cpSync, mkdirSync, writeFileSync, readFileSync, @@ -25,7 +26,7 @@ import { lstatSync, } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; import { runOneMutant, @@ -34,8 +35,11 @@ import { testEfficacyCommand, } from './test-efficacy.js'; import { + adminEntryOf, isolateHostGitConfig, isolateOperatorReviewSettings, + plantAdminEntry, + plantRepository, } from './lib/test-utils.js'; type Handler = (args: { @@ -87,7 +91,14 @@ function treeState(wt: string): string { * passes regardless (so a revert probe reads it as inert). Returns the shared * worktree and base SHA, with the report already written to `report.json`. */ -function scaffoldModifiedPr(): { wt: string; base: string } { +function scaffoldModifiedPr( + // The gate tests need the worktree UNDER `.qwen/tmp`, because that is the + // only place `mountRootFor` answers and so the only place the gates speak. + // A parameter rather than a fork: the fixture's shape — the report schema, + // the workspace layout the handler requires, the fake runner's contract — + // stays in one place when any of them moves. + wtPath: string = join(repo, 'wt'), +): { wt: string; base: string } { write('package.json', '{"private":true,"workspaces":["packages/*"]}\n'); write('packages/lib/src/f.ts', 'export const f = () => 1;\n'); const base = commitAll('base'); @@ -97,8 +108,8 @@ function scaffoldModifiedPr(): { wt: string; base: string } { 'import { f } from "./f.js"; import { it, expect } from "vitest"; it("t", () => expect(typeof f).toBe("function"));\n', ); commitAll('pr'); - const wt = join(repo, 'wt'); - git(repo, 'worktree', 'add', '-q', '--detach', wt, 'HEAD'); + mkdirSync(dirname(wtPath), { recursive: true }); + git(repo, 'worktree', 'add', '-q', '--detach', wtPath, 'HEAD'); writeFileSync( join(repo, 'report.json'), JSON.stringify({ @@ -108,7 +119,7 @@ function scaffoldModifiedPr(): { wt: string; base: string } { ], }), ); - return { wt, base }; + return { wt: wtPath, base }; } function vitestScript(): string { @@ -256,6 +267,13 @@ afterEach(() => { reviewSettingsIsolation?.dispose(); }); +// Skipped on win32, and not for convenience: `mountRootFor` refuses every +// absolute Windows path (a drive letter is a colon, which the `-v` grammar +// cannot spell), so containment is unavailable there BY DESIGN and these gates +// never speak. The assertions would fail for that reason and nothing else — +// first inside the merge queue, where the Windows lane actually runs. +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + describe('fixture git-config isolation', () => { it('spawned git reads the throwaway global config, not the host user config', () => { // Tripwire for every leg of the beforeEach isolation. Global leg: if @@ -291,6 +309,141 @@ describe('fixture git-config isolation', () => { }); }); +describe('the review worktree is the first pointer a probe run trusts', () => { + itWhereContainmentExists( + 'refuses to create a probe tree through a rewritten gitfile', + async () => { + // `worktree add` is the first host-side git write of the probe phase that + // CHECKS FILES OUT — `discardWorktree` above writes too, but materialises + // nothing, so no filter runs there — and it resolves the repository + // through the REVIEW worktree's own gitfile — + // which lives inside the directory the sandbox mounts read-write, and + // which the build/test phase already gave the PR's code a chance to + // rewrite. It checks files out, so it runs whatever filter the pointer + // leads to, on the host, before any gate inside the restore could fire. + // + // The fixture has to sit under `.qwen/tmp`: everywhere else `mountRootFor` + // answers null and the gate short-circuits, which is how deleting it + // shipped green. + const { wt, base } = scaffoldModifiedPr( + join(repo, '.qwen', 'tmp', 'review-pr-1'), + ); + + // The rewrite reviewed code can make from inside the mount — and a + // COHERENT one, which is the point: the planted entry answers `rev-parse + // HEAD` with the real sha, so every read before the write agrees and the + // run reaches the checkout. An empty directory would fail earlier for a + // reason that has nothing to do with the gate. + plantAdminEntry( + join(repo, '.qwen', 'tmp', '.evil-git'), + adminEntryOf(wt), + wt, + join(repo, '.git'), + ); + // Coherence check, so a fixture that breaks silently fails HERE and not as + // a green assertion below. + expect(git(wt, 'rev-parse', 'HEAD').trim()).toBe( + git(repo, 'rev-parse', 'HEAD').trim(), + ); + + await runHandler({ + report: join(repo, 'report.json'), + worktree: wt, + base, + out: join(repo, 'out.json'), + }); + + const out = JSON.parse( + readFileSync(join(repo, 'out.json'), 'utf8'), + ) as Record; + expect(JSON.stringify(out)).toContain('review temp dir'); + // ORDER, which the message alone cannot show: the same detail is + // serialized whether the gate throws before the spawn or after it, so a + // refactor that moves `worktree add` above the gate — the very thing the + // comment beside it warns against — would check files out through the + // planted pointer, execute its filters on the host, and still produce + // this string. The tree not existing is what says the spawn never ran. + expect(existsSync(`${wt}-probe`)).toBe(false); + }, + ); +}); + +describe('the revert phase is reached after the gates refuse', () => { + itWhereContainmentExists( + 'refuses to revert through a gitfile the probe run rewrote', + async () => { + // The route R1-13 names: a restore refusal becomes `inconclusive` without + // throwing, and the mutation phase's catch continues on purpose so the + // revert probe still runs — so the revert is reached PRECISELY WHEN the + // earlier gates fired. Guarding the first two writes and not this one + // leaves the route open exactly where the others closed it. + // + // The rewrite is done by the PR's own suite, which is who does it in + // production: the fake runner below runs with the probe tree as its cwd. + const { wt, base } = scaffoldModifiedPr( + join(repo, '.qwen', 'tmp', 'review-pr-1'), + ); + + // A runner that rewrites its own tree's `.git` the first time it is + // asked to run anything, then answers normally. + // A COHERENT planted repository carrying a real smudge filter, so the + // oracle can be the thing itself — a canary the host writes only if a + // checkout ran through this pointer — rather than the refusal message, + // which reads the same whichever side of the write the gate fires on. + const planted = join(repo, '.qwen', 'tmp', '.evil-git'); + const canary = join(repo, 'PWNED'); + const fakeCommon = plantRepository( + join(repo, '.qwen', 'tmp', '.evil-common'), + join(repo, '.git'), + canary, + 'smudge', + ); + // Staged but not pointed at yet: the runner script below rewrites + // `/.git` itself, the first time it is asked to run anything. + cpSync(adminEntryOf(wt), planted, { recursive: true }); + writeFileSync(join(planted, 'commondir'), `${fakeCommon}\n`); + writeFileSync(join(planted, 'gitdir'), `${join(wt, '.git')}\n`); + writeFileSync( + vitestScript(), + `#!/usr/bin/env node +import fs from 'node:fs'; +import path from 'node:path'; +try { + const dotGit = path.join(process.cwd(), '.git'); + if (fs.existsSync(dotGit) && fs.lstatSync(dotGit).isFile()) { + fs.writeFileSync(dotGit, ${JSON.stringify(`gitdir: ${planted}\n`)}); + } +} catch {} +const files = process.argv.slice(2).filter((a) => a.includes('.test.')); +process.stdout.write(JSON.stringify({ + numPassedTests: files.length, + numFailedTests: 0, + testResults: files.map((f) => ({ + name: path.resolve(f), + assertionResults: [{ status: 'passed' }], + })), +})); +`, + ); + + await runHandler({ + report: join(repo, 'report.json'), + worktree: wt, + base, + out: join(repo, 'out.json'), + }); + + const out = readFileSync(join(repo, 'out.json'), 'utf8'); + // The refusal names why... + expect(out).toContain('review temp dir'); + // ...and nothing checked out through the planted pointer. This is the + // property; the message above is only its explanation, and it reads the + // same whether the gate fires before the write or after it. + expect(existsSync(canary)).toBe(false); + }, + ); +}); + describe('test-efficacy probe isolation (#6832)', () => { it('probes in a disposable worktree and never mutates the shared one', async () => { const { wt, base } = scaffoldModifiedPr(); diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 7a90f3bc792..577f79b7dc4 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -37,6 +37,7 @@ import { isolateHostGitConfig } from './lib/test-utils.js'; import { sanitizedGitEnv } from './lib/worktree.js'; import { mkdtempSync, + realpathSync, mkdirSync, writeFileSync, symlinkSync, @@ -72,6 +73,13 @@ const GLOBS = [ '!packages/desktop-shell', ]; +// Skipped on win32, and not for convenience: `mountRootFor` refuses every +// absolute Windows path (a drive letter is a colon, which the `-v` grammar +// cannot spell), so containment is unavailable there BY DESIGN and these gates +// never speak. The assertions would fail for that reason and nothing else — +// first inside the merge queue, where the Windows lane actually runs. +const itWhereContainmentExists = it.skipIf(process.platform === 'win32'); + describe('isWorkspaceMember', () => { it('places the integration-tests directory outside every workspace', () => { // The whole of the PR #6486 unreachability finding, decided without running @@ -565,6 +573,41 @@ describe('restoreProbeTreeTracked, through runOneMutant', () => { } }); + itWhereContainmentExists( + 'refuses a probe tree whose admin entry sits inside the mounted dir', + () => { + // THROUGH the production call site, not at predicate level. Every other + // fixture here builds its tree under a bare `tmpdir()`, which contains no + // `.qwen/tmp` segment, so `mountRootFor` answers null and the gate + // short-circuits before its logic runs — the wiring could be deleted, or + // handed the wrong arguments, and the whole suite would stay green. + const root = realpathSync(mkdtempSync(join(tmpdir(), 'qwen-mounted-'))); + const probeTree = join(root, '.qwen', 'tmp', 'review-pr-1-probe'); + try { + mkdirSync(probeTree, { recursive: true }); + writeFileSync(join(probeTree, 'a.ts'), 'gone.clear();\n'); + asCheckout(probeTree); + // The shape the round-trip gate cannot see: `.git` replaced by a + // pointer into the mount. `asCheckout` leaves a `.git` DIRECTORY, which + // is itself one of the two refused shapes — so this covers the branch + // the identity gates all sit behind. + const r = runOneMutant( + probeTree, + { file: 'a.ts', line: 1, statement: 'gone.clear();' }, + ['a.test.ts'], + ); + + expect(r.verdict).toBe('inconclusive'); + expect(r.detail).toContain('review temp dir'); + expect(readFileSync(join(probeTree, 'a.ts'), 'utf8')).toBe( + 'gone.clear();\n', + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, + ); + it('refuses to run when the index hides a tracked file from the restore', () => { // `checkout --force` SILENTLY skips a file carrying skip-worktree, and // `clean` never touches a tracked file — so a bit the guest suite sets with diff --git a/packages/cli/src/commands/review/test-efficacy.ts b/packages/cli/src/commands/review/test-efficacy.ts index 7b84d43245c..83ba70be657 100644 --- a/packages/cli/src/commands/review/test-efficacy.ts +++ b/packages/cli/src/commands/review/test-efficacy.ts @@ -86,6 +86,7 @@ import { exposeDependencies, redirectedAncestor, sanitizedGitEnv, + untrustedGitfile, worktreeCreateFailureDetail, type SweepResult, } from './lib/worktree.js'; @@ -1613,6 +1614,13 @@ function restoreProbeTreeTracked(probeTree: string): string | null { // and carries its `.git` as a gitfile; a plain checkout has a `.git` // DIRECTORY and no admin entry to round-trip, and demanding one there // would refuse every ordinary repository. + // Both mount-relative questions in one place — the shape of the `.git` and + // the location of what it names — so the probe tree and the review + // worktree that creates it are held to the same rule. Before the + // `isFile()` branch below, because every gate inside that branch is + // skipped by the very shape this refuses. + const untrusted = untrustedGitfile(probeTree); + if (untrusted !== null) return untrusted; if (lstatSync(join(probeTree, '.git')).isFile()) { const backpointer = readFileSync(join(gitDir, 'gitdir'), 'utf8').trim(); if ( @@ -2654,6 +2662,24 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { // Clear a stale probe tree left by a crashed run — it would fail `add`. // Its stderr is kept to explain a subsequent `add` failure. sweep = discardWorktree(worktree, probeTree); + // The first host-side git write of this phase that CHECKS FILES OUT, + // and it resolves the repository through the REVIEW worktree's own + // gitfile — a second rewritable pointer inside the same read-write + // mount, written by the build/test phase that already ran the PR's code. + // Checking out is what runs `filter..smudge`, so this is where a + // planted pointer becomes host execution, before any gate inside the + // restore below could fire. + // + // Not "the first git write": `discardWorktree` above already runs + // `worktree remove --force` and `worktree unlock` with this same cwd. + // Those materialise nothing, so no filter and no hook runs — but the + // distinction is the whole reason this gate can sit here rather than + // above them, and a maintainer adding a checkout above it on the + // strength of a looser sentence would reopen the route. + const untrusted = untrustedGitfile(worktree); + if (untrusted !== null) { + throw new Error(`refusing to create a probe tree: ${untrusted}`); + } git(worktree, 'worktree', 'add', '--detach', probeTree, headSha); created = true; } catch (e) { @@ -2969,6 +2995,22 @@ async function runTestEfficacy(args: TestEfficacyArgs): Promise { 'root), so the revert would run against whatever it points at', ); } + // ...and the tree's REPOSITORY must still be its own. The check above + // asks the same question of the directory and answers it with an + // lstat walk, which a rewritten gitfile passes untouched: no symlink + // is involved, the tree resolves to itself, and the `checkout` below + // still runs through whatever repository that pointer names — + // executing its filters on the host. + // + // This phase is reached PRECISELY WHEN the gates fired: a restore + // refusal becomes `inconclusive` without throwing, and the mutation + // phase's catch continues on purpose "so the revert probe below still + // runs". Guarding the first two writes and not this one leaves the + // route open exactly where the other two closed it. + const untrusted = untrustedGitfile(probeTree); + if (untrusted !== null) { + throw new Error(`refusing to revert: ${untrusted}`); + } // "Revert to base" is two operations, confined to the throwaway tree. A // file the PR MODIFIED is checked out from base; a file the PR ADDED did // not exist at base, so it is removed — through `safeRmWithin`, which diff --git a/packages/cli/src/services/review-worktree-lease.test.ts b/packages/cli/src/services/review-worktree-lease.test.ts index 1f2d9206b8d..e23a782b838 100644 --- a/packages/cli/src/services/review-worktree-lease.test.ts +++ b/packages/cli/src/services/review-worktree-lease.test.ts @@ -9,23 +9,62 @@ import { readFileSync, renameSync, rmSync, + utimesSync, writeFileSync, } from 'node:fs'; +import type { PathOrFileDescriptor, WriteFileOptions } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { dirname, join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { cleanupReviewWorktreeLeases, clearReviewWorktreeLease, clearReviewWorktreeLeaseIfOwned, createReviewWorktreeLease, isReviewLeaseFile, + LEGACY_LEASE_CUTOFF_MS, readReviewWorktreeLease, reviewLeaseHeldByAnotherSession, reviewLeasePath, type ReviewWorktreeLease, } from './review-worktree-lease.js'; +// Set from exactly one test: plants an honored pre-move lease at the legacy +// path at the moment the new-path lease write happens — the "appears between +// the gate read and the mirror write" interleaving the mirror's EEXIST arm +// exists for, which no in-process fixture can otherwise produce because the +// acquisition sequence is synchronous. +const fsMockState = vi.hoisted(() => ({ + plantBeforeNewPathWrite: null as { + newLeasePath: string; + plantDir: string; + plantPath: string; + plantContents: string; + plantMtime: Date; + } | null, +})); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeFileSync: ( + path: PathOrFileDescriptor, + data: string | NodeJS.ArrayBufferView, + options?: WriteFileOptions, + ) => { + const plant = fsMockState.plantBeforeNewPathWrite; + if (plant && String(path) === plant.newLeasePath) { + fsMockState.plantBeforeNewPathWrite = null; + actual.mkdirSync(plant.plantDir, { recursive: true }); + actual.writeFileSync(plant.plantPath, plant.plantContents); + actual.utimesSync(plant.plantPath, plant.plantMtime, plant.plantMtime); + } + return actual.writeFileSync(path, data, options); + }, + }; +}); + const roots: string[] = []; function createRepository(): string { @@ -39,11 +78,26 @@ function createRepository(): string { } afterEach(() => { + fsMockState.plantBeforeNewPathWrite = null; for (const root of roots.splice(0)) { rmSync(root, { recursive: true, force: true }); } }); +/** Write a lease at the pre-move path, optionally backdating its mtime. */ +function writeLegacyLease(lease: ReviewWorktreeLease, mtime?: Date): string { + const path = join( + lease.repositoryRoot, + '.qwen', + 'tmp', + `qwen-review-lease-${lease.target}.json`, + ); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, `${JSON.stringify(lease)}\n`); + if (mtime) utimesSync(path, mtime, mtime); + return path; +} + describe('review worktree leases', () => { it('protects a worktree created after the lease is registered', () => { const root = createRepository(); @@ -82,7 +136,9 @@ describe('review worktree leases', () => { ).trim(), ).toBe(''); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(false); }); @@ -116,7 +172,9 @@ describe('review worktree leases', () => { ).trim(), ).toBe(''); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(false); }); @@ -143,7 +201,9 @@ describe('review worktree leases', () => { expect(existsSync(worktree)).toBe(false); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(true); }); @@ -206,7 +266,7 @@ describe('review worktree leases', () => { ).toBe(''); expect( readFileSync( - join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-2.json'), + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-2.json'), 'utf8', ), ).toContain('session-b'); @@ -242,7 +302,9 @@ describe('review worktree leases', () => { expect(existsSync(worktree)).toBe(true); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(true); }); @@ -268,7 +330,9 @@ describe('review worktree leases', () => { expect(readFileSync(join(outside, 'marker'), 'utf8')).toBe('keep'); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(true); }); @@ -302,7 +366,9 @@ describe('review worktree leases', () => { expect(existsSync(worktree)).toBe(true); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(true); }); @@ -349,7 +415,9 @@ describe('review worktree leases', () => { clearReviewWorktreeLease(root, 'pr-1'); expect( - existsSync(join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json')), + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), ).toBe(false); cleanupReviewWorktreeLeases({ sessionId: 'session-a', @@ -368,6 +436,350 @@ describe('review worktree leases', () => { }); }); +describe('the move out of the mounted directory', () => { + it('replaces the superseded legacy lease with the mirror, directory or not', () => { + const root = createRepository(); + const legacy = (t: string) => + join(root, '.qwen', 'tmp', `qwen-review-lease-${t}.json`); + mkdirSync(join(root, '.qwen', 'tmp'), { recursive: true }); + writeFileSync(legacy('pr-2'), '{}'); + // The wedge shape: a DIRECTORY where the old lease file was. A + // non-recursive remove throws EISDIR out of acquisition, and nothing + // else removes it — the sweep skips the lease shape and `rm -f` cannot + // remove a directory — so every review of that PR used to fail on this + // machine. + mkdirSync(legacy('pr-1'), { recursive: true }); + + createReviewWorktreeLease({ + sessionId: 's', + promptId: 'p', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }); + + // The wedge directory is gone, replaced by this session's mirror so + // pre-move builds can see the lock. + const mirror = JSON.parse(readFileSync(legacy('pr-1'), 'utf8')) as { + sessionId?: string; + }; + expect(mirror.sessionId).toBe('s'); + // Scoped: another target's legacy lease is not this call's to touch. + expect(readFileSync(legacy('pr-2'), 'utf8')).toBe('{}'); + // ...and the new one is written where nothing mounts. + expect( + existsSync( + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), + ), + ).toBe(true); + }); +}); + +describe('a pre-move lease another session is still holding', () => { + it('is read by the gate and left in place by acquisition', () => { + // The move changed where the gate READS with no fallback for the population + // already on disk, so for the length of a rollout an older build's live lock + // was invisible: `reviewLeaseHeldByAnotherSession(null)` answers false, the + // newer run proceeds, its acquisition deletes the lock, and `cleanStale` + // force-removes the older session's worktree and deletes its branch mid-run. + // That is #9205 — the incident this lease exists to prevent — with the older + // session's rollback then clearing nothing, so the destruction goes + // unannounced. Unread is not inert when the file IS another session's lock. + const root = createRepository(); + const worktreePath = join(root, '.qwen', 'tmp', 'review-pr-1'); + const legacy = writeLegacyLease( + { + sessionId: 'older-build-session', + promptId: 'older-prompt', + target: 'pr-1', + repositoryRoot: root, + worktreePath, + branch: 'qwen-review/pr-1', + }, + // Written before the first build carrying the move shipped: the bound + // honors it as a genuine pre-move lock. + new Date(LEGACY_LEASE_CUTOFF_MS - 60_000), + ); + + const read = readReviewWorktreeLease(root, 'pr-1'); + expect(read?.sessionId).toBe('older-build-session'); + expect(reviewLeaseHeldByAnotherSession(read)).toBe(true); + // Acquisition refuses rather than leaving two leases for one target, which + // is what deleting this one and writing a new one would have done. + let thrown: Error | null = null; + try { + createReviewWorktreeLease({ + sessionId: 'newer-build-session', + promptId: 'newer-prompt', + target: 'pr-1', + repositoryRoot: root, + worktreePath, + branch: 'qwen-review/pr-1', + }); + } catch (error) { + thrown = error as Error; + } + expect(thrown?.message).toMatch(/held by another/); + // The refusal must name the path the lease was actually found at: a + // recovery instruction citing only the new path deletes a file that does + // not exist and leaves this wedge in place. + expect(thrown?.message).toContain(legacy); + expect(existsSync(legacy)).toBe(true); + }); +}); + +describe('the one-release rollout window', () => { + const acquire = (root: string) => ({ + sessionId: 'session-a', + promptId: 'prompt-a', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }); + const legacyPathFor = (root: string) => + join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json'); + + it('mirrors the lease at the legacy path for pre-move builds', () => { + // A build from before the move reads ONLY `.qwen/tmp`; without the mirror + // it passes its own gate over this live lease for the whole rollout + // window, and its cleanStale force-removes this session's worktree and + // deletes its branch mid-run — #9205 in the mirrored direction, and + // unannounced, because this session's rollback clears only the new path. + const root = createRepository(); + createReviewWorktreeLease(acquire(root)); + + const mirror = JSON.parse( + readFileSync(legacyPathFor(root), 'utf8'), + ) as ReviewWorktreeLease; + expect(mirror.sessionId).toBe('session-a'); + expect(mirror.promptId).toBe('prompt-a'); + expect(mirror.worktreePath).toBe(join(root, '.qwen', 'tmp', 'review-pr-1')); + }); + + it('backs out the acquisition when an older build takes the legacy path mid-acquisition', () => { + // An honored pre-move lease (foreign session, mtime predating the cutoff) + // appearing between the gate read and the mirror write — an old build + // that cannot see the new path at all — must fail the acquisition and + // release the new-path lease: never clobber the older build's lock, + // never leave two sessions each believing they hold the target. + const root = createRepository(); + const legacy = legacyPathFor(root); + fsMockState.plantBeforeNewPathWrite = { + newLeasePath: reviewLeasePath(root, 'pr-1'), + plantDir: dirname(legacy), + plantPath: legacy, + plantContents: `${JSON.stringify({ + sessionId: 'older-build-session', + promptId: 'older-prompt', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + })}\n`, + plantMtime: new Date(LEGACY_LEASE_CUTOFF_MS - 60_000), + }; + + let thrown: Error | null = null; + try { + createReviewWorktreeLease(acquire(root)); + } catch (error) { + thrown = error as Error; + } + + expect(thrown?.message).toMatch(/held by another/); + expect(thrown?.message).toContain(legacy); + // The new-path lease was released... + expect(existsSync(reviewLeasePath(root, 'pr-1'))).toBe(false); + // ...and the older build's lock was not clobbered. + const surviving = JSON.parse( + readFileSync(legacy, 'utf8'), + ) as ReviewWorktreeLease; + expect(surviving.sessionId).toBe('older-build-session'); + }); + + it('grants a fresh-mtime legacy plant no gate authority and replaces it', () => { + // A lease-shaped file written inside the mounted directory AFTER the move + // (mtime past the cutoff) cannot be told apart from a plant naming a + // foreign session, so it must not block acquisition — that refusal would + // be a denial of service delivered from the writable surface the move + // exists to escape. Acquisition proceeds and the mirror overwrites the + // plant with the winner's own lease. + const root = createRepository(); + const legacy = writeLegacyLease( + { + sessionId: 'planted-foreign-session', + promptId: 'planted-prompt', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }, + // "Now" once the first build carrying the move has shipped — this test + // runs before that release date, so the fresh mtime is set explicitly. + new Date(LEGACY_LEASE_CUTOFF_MS + 60_000), + ); + + createReviewWorktreeLease(acquire(root)); + + expect(readReviewWorktreeLease(root, 'pr-1')?.sessionId).toBe('session-a'); + const mirror = JSON.parse( + readFileSync(legacy, 'utf8'), + ) as ReviewWorktreeLease; + expect(mirror.sessionId).toBe('session-a'); + }); + + it.skipIf(process.platform === 'win32')( + 'treats a FIFO planted at the legacy lease path as no lease instead of hanging', + { timeout: 10_000 }, + () => { + // `readFileSync` blocks in open(2) on a FIFO with no timeout, so + // without the lstat guard every host-side gate read of this target — + // acquisition, cleanup's holder check — would hang forever. + const root = createRepository(); + const legacy = legacyPathFor(root); + mkdirSync(dirname(legacy), { recursive: true }); + execFileSync('mkfifo', [legacy]); + + expect(readReviewWorktreeLease(root, 'pr-1')).toBeNull(); + // Removed, not merely ignored: nothing else ever would. + expect(existsSync(legacy)).toBe(false); + + createReviewWorktreeLease(acquire(root)); + expect(readReviewWorktreeLease(root, 'pr-1')?.sessionId).toBe( + 'session-a', + ); + }, + ); + + it('clearReviewWorktreeLeaseIfOwned removes an owned pre-move lease', () => { + // Before the dual-location clear this deleted only the nonexistent + // new-path file and left the legacy wedge in place. The ownership rule + // still gates the legacy delete — a foreign pre-move lease is covered by + // 'a pre-move lease another session is still holding'. + const root = createRepository(); + const legacy = writeLegacyLease( + { + sessionId: 'session-a', + promptId: 'prompt-a', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }, + new Date(LEGACY_LEASE_CUTOFF_MS - 60_000), + ); + + clearReviewWorktreeLeaseIfOwned(root, 'pr-1', { + sessionId: 'session-a', + promptId: 'prompt-a', + }); + expect(existsSync(legacy)).toBe(false); + }); + + it("the finalizer sweep finalizes this session's own mirror at the pre-move path", () => { + // The acquisition mirror is content-identical to the new-path lease, so + // the sweep's mirror check passes and the legacy copy is finalized + // together with it. + const root = createRepository(); + const worktree = join(root, '.qwen', 'tmp', 'review-pr-1'); + execFileSync('git', ['-C', root, 'branch', 'qwen-review/pr-1']); + execFileSync('git', [ + '-C', + root, + 'worktree', + 'add', + '-q', + worktree, + 'qwen-review/pr-1', + ]); + createReviewWorktreeLease({ + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-1', + repositoryRoot: root, + worktreePath: worktree, + branch: 'qwen-review/pr-1', + }); + const mirror = legacyPathFor(root); + + cleanupReviewWorktreeLeases({ + sessionId: 'session-a', + promptId: 'prompt-parent', + repositoryRoot: root, + }); + + expect(existsSync(worktree)).toBe(false); + expect(existsSync(mirror)).toBe(false); + expect(existsSync(reviewLeasePath(root, 'pr-1'))).toBe(false); + expect( + execFileSync( + 'git', + ['-C', root, 'branch', '--list', 'qwen-review/pr-1'], + { encoding: 'utf8' }, + ).trim(), + ).toBe(''); + }); + + it("never acts on a planted legacy lease that copies the mirror's credentials", () => { + // The mirror is READABLE inside the mounted directory, so reviewed code + // can learn this session's sessionId/promptId from it and plant a lease + // passing every ownership check — but naming a victim worktreePath. + // Only content-equality with the new-path lease (outside the mount) + // proves acquisition wrote a legacy lease, so the plant must be left + // inert: the victim and its branch survive. The plant file itself is + // left in place — ignoring it is enough, and acquisition overwrites it + // with the real mirror if the target is ever genuinely taken. + const root = createRepository(); + // The copy source: this session's genuine lease + mirror for pr-1. + createReviewWorktreeLease({ + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-1', + repositoryRoot: root, + worktreePath: join(root, '.qwen', 'tmp', 'review-pr-1'), + branch: 'qwen-review/pr-1', + }); + // The victim: another review tree under the same temp dir. + const victim = join(root, '.qwen', 'tmp', 'review-pr-2'); + execFileSync('git', ['-C', root, 'branch', 'qwen-review/pr-2']); + execFileSync('git', [ + '-C', + root, + 'worktree', + 'add', + '-q', + victim, + 'qwen-review/pr-2', + ]); + const plant = writeLegacyLease({ + sessionId: 'session-a', + promptId: 'prompt-parent', + target: 'pr-2', + repositoryRoot: root, + worktreePath: victim, + branch: 'qwen-review/pr-2', + }); + + cleanupReviewWorktreeLeases({ + sessionId: 'session-a', + promptId: 'prompt-parent', + repositoryRoot: root, + }); + + expect(existsSync(victim)).toBe(true); + expect( + execFileSync( + 'git', + ['-C', root, 'branch', '--list', 'qwen-review/pr-2'], + { encoding: 'utf8' }, + ).trim(), + ).toContain('qwen-review/pr-2'); + expect(existsSync(plant)).toBe(true); + }); +}); + describe('readReviewWorktreeLease', () => { it('returns the lease createReviewWorktreeLease wrote', () => { const root = createRepository(); @@ -385,7 +797,7 @@ describe('readReviewWorktreeLease', () => { expect(lease?.promptId).toBe('prompt-parent'); expect(lease?.worktreePath).toBe(join(root, '.qwen', 'tmp', 'review-pr-1')); expect(reviewLeasePath(root, 'pr-1')).toBe( - join(root, '.qwen', 'tmp', 'qwen-review-lease-pr-1.json'), + join(root, '.qwen', 'review-leases', 'qwen-review-lease-pr-1.json'), ); }); @@ -443,6 +855,7 @@ describe('lease acquisition is atomic (#9205)', () => { // writer rewriting it is self-heal, not clobber. const root = createRepository(); mkdirSync(join(root, '.qwen', 'tmp'), { recursive: true }); + mkdirSync(join(root, '.qwen', 'review-leases'), { recursive: true }); writeFileSync(reviewLeasePath(root, 'pr-1'), '{"truncated'); createReviewWorktreeLease(leaseParams(root)); expect(readReviewWorktreeLease(root, 'pr-1')?.sessionId).toBe('session-a'); @@ -511,7 +924,13 @@ describe('cleanupReviewWorktreeLeases scan', () => { worktree, 'qwen-review/pr-1', ]); - const stray = join(root, '.qwen', 'tmp', 'qwen-review-lease-local.json'); + const stray = join( + root, + '.qwen', + 'review-leases', + 'qwen-review-lease-local.json', + ); + mkdirSync(dirname(stray), { recursive: true }); writeFileSync( stray, JSON.stringify({ diff --git a/packages/cli/src/services/review-worktree-lease.ts b/packages/cli/src/services/review-worktree-lease.ts index 3bda53be2e5..70e85b2e3c1 100644 --- a/packages/cli/src/services/review-worktree-lease.ts +++ b/packages/cli/src/services/review-worktree-lease.ts @@ -4,17 +4,26 @@ import { execFileSync } from 'node:child_process'; import { existsSync, + lstatSync, mkdirSync, readFileSync, readdirSync, rmSync, writeFileSync, } from 'node:fs'; -import { basename, isAbsolute, join, relative, resolve } from 'node:path'; +import { + basename, + dirname, + isAbsolute, + join, + relative, + resolve, +} from 'node:path'; import { createDebugLogger } from '@qwen-code/qwen-code-core'; import { LEASE_PREFIX, REVIEW_TMP_DIR, + REVIEW_LEASE_DIR, reviewBranch, } from '../commands/review/lib/paths.js'; @@ -34,7 +43,7 @@ function validTarget(target: string): boolean { } /** - * Whether a filename under `REVIEW_TMP_DIR` is a review-worktree lease. + * Whether a filename under `REVIEW_LEASE_DIR` is a review-worktree lease. * Derived from `validTarget` so the writer, `cleanup`'s sweep guard, and the * `cleanupReviewWorktreeLeases` scan share one definition of the lease shape * (see the `LEASE_PREFIX` comment in `lib/paths.ts`). @@ -60,9 +69,40 @@ export interface ReviewWorktreeLease { } function leaseDirectory(repositoryRoot: string): string { - return join(repositoryRoot, REVIEW_TMP_DIR); + return join(repositoryRoot, REVIEW_LEASE_DIR); } +/** + * Where leases lived before they moved out of the mounted directory. Still + * READ as well as deleted, for one release: a runner whose workspace persists + * can be holding a live lock an older build wrote here. + */ +function legacyLeasePath(repositoryRoot: string, target: string): string { + return join(repositoryRoot, REVIEW_TMP_DIR, `${LEASE_PREFIX}${target}.json`); +} + +/** + * The release date of the first build carrying the lease move out of + * `.qwen/tmp` — MUST be reset to that release's actual date before shipping. + * A legacy-path lease is honored in gate/acquisition reads only while its + * mtime says it was written before this date: the legacy path lives in the + * one directory reviewed code can still write, so a lease-shaped file + * appearing there after the move could equally be a plant naming a foreign + * session, and honoring it would hand that writable surface a permanent + * denial of service against the pipeline meant to distrust it. + * + * One-release-window semantics: mirrors new builds write at the legacy path + * are for OLD builds' benefit (old builds read the legacy path directly, + * without this bound), so the mirror and the bound do not conflict. The + * residual window this accepts: an old build acquiring AFTER this date on a + * machine no new build has mirrored on yet writes a fresh-mtime legacy lease + * that cannot be distinguished from a plant, and new builds will not honor + * it. mtime is the only signal available at that path, and reviewed code can + * backdate it with `utimes` — a forged-mtime plant is the other residual + * this bound cannot close. + */ +export const LEGACY_LEASE_CUTOFF_MS = Date.UTC(2026, 8, 15); + function leasePath(repositoryRoot: string, target: string): string { return join(leaseDirectory(repositoryRoot), `${LEASE_PREFIX}${target}.json`); } @@ -80,7 +120,17 @@ export function clearReviewWorktreeLease( target: string, ): void { if (!validTarget(target)) return; - rmSync(leasePath(resolve(repositoryRoot), target), { force: true }); + const root = resolve(repositoryRoot); + rmSync(leasePath(root, target), { force: true }); + // The pre-move path too, for the same one-release window the read fallback + // covers: a stale legacy lease would otherwise wedge this target for old + // builds forever — nothing else removes it, and a recovery instruction + // naming only the new path deletes a file that does not exist. `recursive` + // because a DIRECTORY at the lease's name would throw EISDIR (the + // acquisition-side wedge shape); `force` because absence is the common + // case. Deletion only — the mirror in `createReviewWorktreeLease` is the + // sole legacy write path. + rmSync(legacyLeasePath(root, target), { force: true, recursive: true }); } /** @@ -130,6 +180,23 @@ export function createReviewWorktreeLease(params: { const data = `${JSON.stringify(lease, null, 2)}\n`; const path = leasePath(repositoryRoot, params.target); mkdirSync(leaseDirectory(repositoryRoot), { recursive: true }); + // A pre-move lease still holding this target blocks acquisition exactly as + // a new-path one does: taking the lock anyway would leave two leases for + // one target, and the older session's rollback would clear nothing while + // this run swept its tree. The bounded read is what keeps it safe to ask + // the question at a path inside the mounted directory: a legacy file + // younger than LEGACY_LEASE_CUTOFF_MS answers "no lease" here, so a plant + // naming a foreign session cannot turn this throw into a denial of + // service — acquisition proceeds and the mirror below replaces the plant. + const legacy = legacyLeasePath(repositoryRoot, params.target); + const legacyLease = readLegacyLease(legacy); + if (legacyLease !== null && legacyLease.sessionId !== params.sessionId) { + throw new Error( + `review worktree lease for ${params.target} is held by another ` + + `session (session ${legacyLease.sessionId}) at the pre-move path ` + + `${legacy} — an older build acquired it; retry`, + ); + } try { // `flag: 'wx'` fails EEXIST instead of overwriting: two concurrent // fetch-prs can both pass the gate's read, and a plain write would let @@ -143,8 +210,8 @@ export function createReviewWorktreeLease(params: { if (existing && existing.sessionId !== params.sessionId) { throw new Error( `review worktree lease for ${params.target} is held by another ` + - `session (session ${existing.sessionId}); it was acquired ` + - `between the gate read and the lease write — retry`, + `session (session ${existing.sessionId}) at ${path}; it was ` + + `acquired between the gate read and the lease write — retry`, ); } // Same-session re-fetch refreshes the lease (ownership is per session, @@ -152,9 +219,83 @@ export function createReviewWorktreeLease(params: { // every reader, so rewriting it heals a torn write instead of wedging. writeFileSync(path, data, 'utf8'); } + mirrorLeaseAtLegacyPath(legacy, path, data, params.sessionId, params.target); +} + +/** + * Mirror the just-acquired lease at the pre-move path, for the one release + * the read fallback assumes old builds exist in: a pre-move build reads ONLY + * that path, so without the mirror its fetch-pr passes its own gate over + * this live lease and its cleanStale force-removes this session's worktree + * and deletes its branch mid-run — #9205 in the mirrored direction, and + * unannounced, because this session's rollback clears only the new path. + * + * The mirror is NEVER an arbiter or a second acquisition path: it is written + * only after the new-path `wx` write has won, and new builds grant a + * fresh-mtime legacy file no gate authority (LEGACY_LEASE_CUTOFF_MS), so + * this write hands the mounted directory no authority over new builds. An + * EEXIST that reads as another session's honored lease backs the whole + * acquisition out rather than clobbering a concurrent writer's lock. + */ +function mirrorLeaseAtLegacyPath( + legacy: string, + path: string, + data: string, + sessionId: string, + target: string, +): void { + mkdirSync(dirname(legacy), { recursive: true }); + try { + writeFileSync(legacy, data, { encoding: 'utf8', flag: 'wx' }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + const existing = readLegacyLease(legacy); + if (existing && existing.sessionId !== sessionId) { + // An honored pre-move lease surfaced between the gate read and this + // mirror: an older build that cannot see the new path at all now + // believes it holds the target, so this run must back out entirely — + // release the new-path lease instead of leaving two sessions each + // believing the target is theirs. + rmSync(path, { force: true }); + throw new Error( + `review worktree lease for ${target} is held by another ` + + `session (session ${existing.sessionId}) at the pre-move path ` + + `${legacy} — an older build acquired it between the gate read ` + + `and the lease write; retry`, + ); + } + // Every other EEXIST is safe to overwrite: this session racing its own + // earlier mirror, a fresh-mtime plant the cutoff declines to honor, or + // a non-regular wedge (readLease has already removed a DIRECTORY at the + // lease name, so this plain write also heals the EISDIR shape). + writeFileSync(legacy, data, 'utf8'); + } } function readLease(path: string): ReviewWorktreeLease | null { + try { + // lstat BEFORE any open: either lease path can carry a planted FIFO — + // the legacy one sits in the one directory reviewed code can still + // write — and `readFileSync` blocks in open(2) on a FIFO with no + // timeout, so no catch below could ever run and every gate read of the + // target would hang forever. A non-regular file (FIFO, directory, + // socket) cannot be a lease: treat it as none and remove it, because + // nothing else will — a DIRECTORY at the lease's name otherwise keeps + // throwing EISDIR at every non-recursive removal (the wedge shape the + // recursive removes elsewhere in this file exist to escape). + if (!lstatSync(path).isFile()) { + rmSync(path, { force: true, recursive: true }); + return null; + } + } catch (error) { + // ENOENT is the ordinary "no lease" answer; anything else (a removal + // racing the lstat) is also read as no lease, the same torn-write + // healing the parse catch below performs. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + debugLogger.debug(`Failed to inspect review lease ${path}:`, error); + } + return null; + } try { const value = JSON.parse(readFileSync(path, 'utf8')) as ReviewWorktreeLease; if ( @@ -174,13 +315,66 @@ function readLease(path: string): ReviewWorktreeLease | null { } } +/** + * The legacy-path read for GATE authority: a parseable lease there speaks + * only when its mtime says it predates the first build carrying the move — + * see LEGACY_LEASE_CUTOFF_MS. Goes through `readLease` first so a planted + * non-regular file is still removed rather than merely ignored. + */ +function readLegacyLease(path: string): ReviewWorktreeLease | null { + const lease = readLease(path); + if (!lease) return null; + try { + if (lstatSync(path).mtimeMs > LEGACY_LEASE_CUTOFF_MS) return null; + } catch { + return null; + } + return lease; +} + /** The lease currently registered for a review target, or null. */ export function readReviewWorktreeLease( repositoryRoot: string, target: string, ): ReviewWorktreeLease | null { + return readReviewWorktreeLeaseAt(repositoryRoot, target)?.lease ?? null; +} + +/** + * The lease currently registered for a review target AND the path it was + * found at. A recovery instruction must name the file that actually holds + * the lock: during the one-release rollout that can be the pre-move path, + * and "delete and re-run" then points an operator at a file + * that does not exist while the wedge stands. + */ +export function readReviewWorktreeLeaseAt( + repositoryRoot: string, + target: string, +): { lease: ReviewWorktreeLease; path: string } | null { if (!validTarget(target)) return null; - return readLease(reviewLeasePath(repositoryRoot, target)); + const root = resolve(repositoryRoot); + // BOTH locations, for one release. The move changed where this reads with no + // fallback for the population already on disk, so for the length of a rollout + // a lock an older build was holding was invisible to the gate: + // `reviewLeaseHeldByAnotherSession(null)` answers false, the newer run + // proceeds, its acquisition deletes the older session's live lock, and + // `cleanStale` force-removes its worktree and deletes its branch mid-run — + // #9205, the incident this lease exists to prevent, with the older session's + // rollback then clearing nothing so the destruction goes unannounced. + // + // The legacy read stays a READ, bounded by LEGACY_LEASE_CUTOFF_MS so a + // fresh-mtime file inside the mounted directory exercises no gate + // authority. New builds' only legacy WRITE is the acquisition mirror in + // `createReviewWorktreeLease`, which exists solely so pre-move builds — + // reading only the legacy path, without the bound — can see the lock + // during the rollout; the new-path `wx` remains the one atomic + // acquisition. + const current = leasePath(root, target); + const lease = readLease(current); + if (lease) return { lease, path: current }; + const legacy = legacyLeasePath(root, target); + const legacyLease = readLegacyLease(legacy); + return legacyLease ? { lease: legacyLease, path: legacy } : null; } /** @@ -204,6 +398,26 @@ export function reviewLeaseHeldByAnotherSession( return !sessionId || lease.sessionId !== sessionId; } +/** + * Parsed-content equality: two lease files record the same lease regardless + * of formatting. The finalizer's mirror check keys on this rather than raw + * bytes so a genuinely identical mirror always passes. + */ +function sameLease( + a: ReviewWorktreeLease, + b: ReviewWorktreeLease | null, +): boolean { + return ( + b !== null && + a.sessionId === b.sessionId && + a.promptId === b.promptId && + a.target === b.target && + a.repositoryRoot === b.repositoryRoot && + a.worktreePath === b.worktreePath && + a.branch === b.branch + ); +} + function removeLeaseWorktree( lease: ReviewWorktreeLease, gitTimeout: number, @@ -306,23 +520,48 @@ export function cleanupReviewWorktreeLeases(params: { }): void { try { const repositoryRoot = resolve(params.repositoryRoot); - const directory = leaseDirectory(repositoryRoot); - if (!existsSync(directory)) return; + const newLeaseDirectory = leaseDirectory(repositoryRoot); + // The pre-move directory too, for the same one-release window: this + // build's own acquisition mirrors live there. It is scanned FIRST + // because the mirror check below reads the new-path lease, which the + // new-directory leg deletes when it finalizes. + // + // Acting on mounted-directory content is safe ONLY through the mirror + // check: the mirror is READABLE from inside the mount, so reviewed code + // can copy its sessionId/promptId into a planted lease naming a victim + // worktreePath (the A/B base tree, a sibling shard's worktree) and pass + // every ownership field below. Only equality with the new-path lease — + // outside the mount, undoctorable from there — proves acquisition wrote + // the content, so a plant is ignored, never wielded. A legacy lease + // with no new-path twin fails the same check; that is the deliberate + // cost of trusting nothing that lives only inside the mount. + for (const directory of [ + join(repositoryRoot, REVIEW_TMP_DIR), + newLeaseDirectory, + ]) { + if (!existsSync(directory)) continue; - for (const entry of readdirSync(directory)) { - if (!isReviewLeaseFile(entry)) continue; - const path = join(directory, basename(entry)); - const lease = readLease(path); - if ( - !lease || - lease.sessionId !== params.sessionId || - lease.promptId !== params.promptId || - resolve(lease.repositoryRoot) !== repositoryRoot - ) { - continue; - } - if (removeLeaseWorktree(lease, params.gitTimeout ?? GIT_TIMEOUT_MS)) { - rmSync(path, { force: true }); + for (const entry of readdirSync(directory)) { + if (!isReviewLeaseFile(entry)) continue; + const path = join(directory, basename(entry)); + const lease = readLease(path); + if ( + !lease || + lease.sessionId !== params.sessionId || + lease.promptId !== params.promptId || + resolve(lease.repositoryRoot) !== repositoryRoot + ) { + continue; + } + if ( + directory !== newLeaseDirectory && + !sameLease(lease, readLease(join(newLeaseDirectory, basename(entry)))) + ) { + continue; + } + if (removeLeaseWorktree(lease, params.gitTimeout ?? GIT_TIMEOUT_MS)) { + rmSync(path, { force: true }); + } } } } catch (error) { diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index 368f29be48d..a185bfa3415 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -23,6 +23,7 @@ import { describe, expect, it } from 'vitest'; import { parse } from 'yaml'; import { LEASE_PREFIX, + REVIEW_LEASE_DIR, REVIEW_TMP_DIR, reviewBranch, worktreePath, @@ -576,8 +577,19 @@ describe('review worktree cleanup steps', () => { expect(reviewCleanStep).toContain(`for leftover in ${worktreePrefix}*; do`); // Leases are session+prompt scoped so a stale one is inert, but the glob // must stay in sync with LEASE_PREFIX or it silently never matches. + // + // BOTH locations, because leases moved out of the mounted directory: the + // new one is where this job's own runs leave them, and the old one is + // where a persisted workspace can still be holding one written by an + // earlier build. `-r`, because the OLD path is inside the directory the + // review sandbox mounts read-write and a reviewed PR can leave a + // DIRECTORY at the lease's name — which plain `rm -f` cannot remove, and + // which then wedges every later review of that PR on that runner. expect(reviewCleanStep).toContain( - `rm -f ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`, + `rm -rf ${toPosix(REVIEW_LEASE_DIR)}/${LEASE_PREFIX}pr-*.json`, + ); + expect(reviewCleanStep).toContain( + `rm -rf ${toPosix(REVIEW_TMP_DIR)}/${LEASE_PREFIX}pr-*.json`, ); // A failed rm must not be left to poison the next job's checkout: the // sweep owns its own permission repair — chmod, then passwordless sudo