diff --git a/docs/design/WORKTREE_INDEX_SAFETY.md b/docs/design/WORKTREE_INDEX_SAFETY.md new file mode 100644 index 00000000..ffb6a85e --- /dev/null +++ b/docs/design/WORKTREE_INDEX_SAFETY.md @@ -0,0 +1,52 @@ +# Worktree Index Safety + +Status: validated +Created: 2026-09-08 +Verified: 2026-09-08 +Issue: [#472](https://github.com/openpi-dev/openpi/issues/472) + +## Decision + +Automatic worktree reclamation must treat the checkout index as an independent +source of safety evidence. Before `git worktree remove`, OpenPI reads the +worktree's tracked-file inventory with `git ls-files -v -z`. + +- `assume-unchanged` and `skip-worktree` entries preserve the worktree because + ordinary status may hide a tracked local change. +- A failed, truncated, or unrecognized inventory also preserves the worktree. +- The check is read-only. It does not clear flags, reset files, or infer that a + sparse checkout is safe to remove. + +The existing status, ignored-file, baseline, commit, detached-head, and +non-force removal checks remain in place. + +## Alternatives + +Copying the index to a temporary `GIT_INDEX_FILE`, clearing flags, and +recomputing the diff would detect more states, but it mutates the inspection +surface and requires a policy for sparse checkout entries. The conservative +inventory check is sufficient for the reported loss mode and fails closed when +it cannot establish a trustworthy answer. + +## Validation + +The regression suite uses temporary Git repositories and verifies both +`assume-unchanged` and `skip-worktree`, with and without a modified file. It +asserts that the checkout, file contents, branch, and index bytes remain +unchanged. It also covers NUL-delimited unusual paths and unreadable, +truncated, malformed, and over-limit inventory results. + +Validation commands: + +```text +node --test --experimental-strip-types tests/extensions/shared/worktree.test.ts +``` + +Full repository checks are recorded in the pull request before publication. + +## Ablation + +Removing the index inventory gate makes the existing `status` and non-force +`worktree remove` checks pass for both hidden-flag fixtures, deleting the only +copy of the modified tracked file. Removing the NUL-delimited parsing would +make unusual tracked paths ambiguous. Both elements are therefore retained. diff --git a/extensions/shared/worktree.ts b/extensions/shared/worktree.ts index 49b8a32a..1a4bfddd 100644 --- a/extensions/shared/worktree.ts +++ b/extensions/shared/worktree.ts @@ -37,12 +37,10 @@ * with a self-referential symlink — destroying the dependencies of the very * repo the session was working in. * - * - **Teardown never uses `--force`.** Git refuses to remove a worktree with - * modified or untracked files, and that refusal is exactly the policy we - * want: an isolated child that produced nothing is reclaimed automatically, - * and one that produced work keeps its directory and branch for the parent - * to inspect. Committed work is safe either way — the branch outlives the - * worktree — but uncommitted work would not be, so we let git veto. + * - **Teardown never uses `--force`.** Before asking Git to remove a checkout, + * inspect ignored files and index flags too: Git's ordinary dirty check can + * miss the only copy of local work. Unknown inspection results preserve the + * checkout; committed work keeps its branch for the parent to inspect. */ import { execFile } from "node:child_process"; @@ -110,6 +108,7 @@ function runGit( { cwd, encoding: "utf8", + maxBuffer: 1024 * 1024, timeout: Math.min(timeoutMs, WORKTREE_GIT_TIMEOUT_MS), }, (error, stdout, stderr) => { @@ -312,8 +311,8 @@ export interface WorktreeCommitCount { /** * Reclaim a worktree, keeping anything the child actually produced. * - * Deliberately no `--force`: git's own refusal on a dirty tree is the policy - * we want. A child that changed nothing costs nothing to discard; a child with + * Deliberately no `--force`, plus explicit ignored-file and index inspection. + * A child that changed nothing costs nothing to discard; a child with * uncommitted work keeps its directory so the parent can look at it. The * branch is deleted only when it holds no commits, so a child that committed * always leaves something to merge, and a child that did nothing leaves no @@ -380,7 +379,39 @@ export async function reclaimWorktree( const branch = headBranch || worktree.branch; const observed = { branch, headSha, detached }; + // Status and even non-force removal trust flags that can hide local edits. + // Do not clear flags (including sparse-checkout flags) to guess at safety. + const index = await run(["-C", worktree.path, "ls-files", "-v", "-z"]); + if (index.code !== 0) { + return preserve( + `could not inspect worktree index: ${firstLine(index.stderr) || "git ls-files failed"}`, + observed, + ); + } + if (index.stdout && !index.stdout.endsWith("\0")) { + return preserve("worktree index inventory is incomplete", observed); + } + for (const entry of index.stdout.split("\0").slice(0, -1)) { + if (!/^[HSMRCK?hsmrck] [\s\S]+$/.test(entry)) { + return preserve( + "worktree index inventory has an unknown format", + observed, + ); + } + const flag = entry[0]!; + if (flag === "S" || flag === flag.toLowerCase()) { + return preserve( + "worktree index flags (assume-unchanged or skip-worktree) may hide local changes", + observed, + ); + } + if (flag !== "H") { + return preserve("worktree index contains non-clean entries", observed); + } + } + const status = await run([ + "--no-optional-locks", "-C", worktree.path, "status", diff --git a/tests/extensions/shared/worktree.test.ts b/tests/extensions/shared/worktree.test.ts index 3086c4a7..a9ce3c26 100644 --- a/tests/extensions/shared/worktree.test.ts +++ b/tests/extensions/shared/worktree.test.ts @@ -1,8 +1,9 @@ import { strict as assert } from "node:assert"; -import { execFileSync } from "node:child_process"; +import childProcess, { execFileSync } from "node:child_process"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import { syncBuiltinESMExports } from "node:module"; import { after, before, describe, test } from "node:test"; import { createWorktree, @@ -294,6 +295,122 @@ describe("worktree lifecycle", () => { git(repo, "branch", "-D", result.worktree.branch); }); + for (const flag of ["assume-unchanged", "skip-worktree"]) { + for (const changed of [false, true]) { + test(`preserves ${flag} index and ${changed ? "modified" : "clean"} content`, async () => { + const result = await createWorktree({ + cwd: repo, + label: flag, + id: String(changed), + }); + assert.ok(result.ok); + const wt = result.worktree; + const file = path.join(wt.path, "a.txt"); + const content = changed + ? "only copy of local work\n" + : fs.readFileSync(file, "utf8"); + fs.writeFileSync(file, content); + git(wt.path, "update-index", `--${flag}`, "a.txt"); + assert.equal(git(wt.path, "status", "--porcelain"), ""); + const indexPath = path.resolve( + wt.path, + git(wt.path, "rev-parse", "--git-path", "index").trim(), + ); + const indexBefore = fs.readFileSync(indexPath); + const cleanup = await reclaimWorktree(repo, wt); + assert.equal(cleanup.removed, false); + assert.equal(cleanup.branchDeleted, false); + assert.equal( + cleanup.dirty, + undefined, + "hidden changes are unknown, not clean", + ); + assert.match(cleanup.reason ?? "", /index flags.*may hide/); + assert.equal(fs.readFileSync(file, "utf8"), content); + assert.deepEqual(fs.readFileSync(indexPath), indexBefore); + assert.equal(git(repo, "rev-parse", wt.branch).trim(), wt.baseSha); + }); + } + } + + test("accepts NUL-delimited tracked paths with whitespace and newlines", async () => { + const result = await createWorktree({ cwd: repo, label: "paths", id: "1" }); + assert.ok(result.ok); + fs.writeFileSync( + path.join(result.worktree.path, "space and\nnewline.txt"), + "fixture", + ); + git(result.worktree.path, "add", "-A"); + git(result.worktree.path, "commit", "--quiet", "-m", "unusual path"); + const cleanup = await reclaimWorktree(repo, result.worktree); + assert.equal(cleanup.removed, true, cleanup.reason ?? ""); + assert.equal(cleanup.branchDeleted, false); + }); + + for (const inventory of [ + "H a.txt", + "X a.txt\0", + "H \0", + "H a.txt\0\0", + "error", + "overflow", + ]) { + test(`preserves when index inventory is not trustworthy: ${JSON.stringify(inventory)}`, async (t) => { + const result = await createWorktree({ + cwd: repo, + label: "inventory", + id: "1", + }); + assert.ok(result.ok); + const original = childProcess.execFile; + t.mock.method( + childProcess, + "execFile", + (...args: Parameters) => { + const [file, argv] = args; + if (file === "git" && Array.isArray(argv) && argv.includes("-v")) { + const callback = args.at(-1) as ( + error: Error | null, + stdout: string, + stderr: string, + ) => void; + const error = inventory === "error" || inventory === "overflow"; + callback( + error + ? new Error( + inventory === "overflow" + ? "stdout maxBuffer length exceeded" + : "inventory unavailable", + ) + : null, + error ? "H a.txt\0" : inventory, + "", + ); + return undefined; + } + return Reflect.apply(original, childProcess, args); + }, + ); + syncBuiltinESMExports(); + t.after(() => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + }); + const cleanup = await reclaimWorktree(repo, result.worktree); + assert.equal(cleanup.removed, false); + assert.equal(cleanup.branchDeleted, false); + assert.match(cleanup.reason ?? "", /index/); + assert.equal( + fs.readFileSync(path.join(result.worktree.path, "a.txt"), "utf8"), + "hello\n", + ); + assert.equal( + git(repo, "rev-parse", result.worktree.branch).trim(), + result.worktree.baseSha, + ); + }); + } + test("preserves a clean detached checkout instead of guessing it is empty", async () => { const result = await createWorktree({ cwd: repo,