From a9f1fb8b7860f49e8a9c5481c36397725dc588be Mon Sep 17 00:00:00 2001 From: Hussain Phalasiya Date: Sun, 18 Oct 2026 09:06:50 +0300 Subject: [PATCH] VC-834: background worktree reads run read-only git (--no-optional-locks) The Change Set snapshot and path set, the rail status, the venue snapshot and diffStat ran a plain git status, which holds the checkout's index.lock for its whole untracked scan. These run in the background within a second of any write, so an agent's own git add/commit (or a pre-commit hook's git add) in the same checkout could fail with 'index.lock: File exists'. Each entry point now wraps its runner in readOnlyGitAsync, the orphan scan's existing wrapper. Mutations (commit, sync, worktree add/remove) are unchanged. --- packages/host-core/src/agent-commands.test.ts | 14 +- .../host-core/src/worktree/change-set.test.ts | 12 +- packages/host-core/src/worktree/change-set.ts | 13 +- packages/host-core/src/worktree/diff.test.ts | 22 +-- packages/host-core/src/worktree/diff.ts | 9 +- .../src/worktree/read-only-reads.test.ts | 177 ++++++++++++++++++ .../host-core/src/worktree/scripted-git.ts | 27 ++- .../host-core/src/worktree/snapshot.test.ts | 4 +- .../host-core/src/worktree/status.test.ts | 4 +- packages/host-core/src/worktree/status.ts | 13 +- packages/host-core/src/worktree/venue.test.ts | 8 +- packages/host-core/src/worktree/venue.ts | 8 +- 12 files changed, 271 insertions(+), 40 deletions(-) create mode 100644 packages/host-core/src/worktree/read-only-reads.test.ts diff --git a/packages/host-core/src/agent-commands.test.ts b/packages/host-core/src/agent-commands.test.ts index 99bfeb4ba..ebe0bcb16 100644 --- a/packages/host-core/src/agent-commands.test.ts +++ b/packages/host-core/src/agent-commands.test.ts @@ -61,7 +61,7 @@ import { testHostHandlers, type TestHandlerPorts } from "./testing/host-handlers import { writeModelAccessDefault } from "./session-runtime/model-access-preferences"; import { archiveTicketCommand, updateTicketFieldsCommand } from "./ticket-commands"; import { createSessionTokenRegistry } from "./session-tokens"; -import { scriptedGit } from "./worktree/scripted-git"; +import { scriptedGit, scriptedReadOnlyGit } from "./worktree/scripted-git"; import * as worktree from "./worktree"; import { getWorktreeSnapshots, resetWorktreeSnapshotsForTest } from "./worktree/snapshot"; import { createInMemoryTranscriptArtifactStore } from "@volli/session-engine"; @@ -4911,7 +4911,7 @@ describe("agent command service", () => { ticketPrefix: "VC", }), ); - const { git, gitAsync } = scriptedGit((args) => { + const { git, gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "status") return " M src/a.ts\n"; if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no origin ref"); if (args[0] === "rev-list" && args[1] === "--left-right") return "0\t3\n"; @@ -5111,7 +5111,7 @@ describe("agent command service", () => { ticketPrefix: "VC", }), ); - const { git, gitAsync } = scriptedGit((args) => { + const { git, gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no origin ref"); if (args[0] === "diff" && args.includes("main...HEAD")) return "3\t1\tsrc/a.ts\n"; if (args[0] === "diff" && args.includes("HEAD")) return "9\t0\tsrc/wip.ts\n"; @@ -5183,7 +5183,7 @@ describe("agent command service", () => { ); const numstat = Array.from({ length: 25 }, (_, i) => `1\t0\tsrc/file-${i}.ts`).join("\n") + "\n"; - const { git, gitAsync } = scriptedGit((args) => { + const { git, gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no origin ref"); if (args[0] === "diff") return numstat; return ""; @@ -5780,7 +5780,7 @@ describe("agent command service", () => { ); void paths; } - const { git, gitAsync } = scriptedGit((args, cwd) => { + const { git, gitAsync } = scriptedReadOnlyGit((args, cwd) => { if (args[0] === "rev-parse" && args[1] === "--verify") return "base-tip\n"; if (args[0] === "merge-base") return "base-sha\n"; if (args[0] === "diff" && args.includes("--raw")) { @@ -5860,7 +5860,7 @@ describe("agent command service", () => { }), ); } - const { git, gitAsync } = scriptedGit((args, cwd) => { + const { git, gitAsync } = scriptedReadOnlyGit((args, cwd) => { if (args[0] === "rev-parse" && args[1] === "--verify") return "base-tip\n"; if (args[0] === "merge-base") return "base-sha\n"; if (args[0] === "diff" && args.includes("--raw")) { @@ -5948,7 +5948,7 @@ describe("agent command service", () => { baseBranch: "main", }), ); - const { git, gitAsync } = scriptedGit((args) => { + const { git, gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") return "base-tip\n"; if (args[0] === "merge-base") return "base-sha\n"; if (args[0] === "diff" && args.includes("--raw")) return rawModified(["package.json"]); diff --git a/packages/host-core/src/worktree/change-set.test.ts b/packages/host-core/src/worktree/change-set.test.ts index 0fe331884..8ccb1ccca 100644 --- a/packages/host-core/src/worktree/change-set.test.ts +++ b/packages/host-core/src/worktree/change-set.test.ts @@ -19,7 +19,7 @@ import { CHANGE_SET_FILE_CAP } from "@volli/shared"; import { changeSetSnapshot, readChangeSetBaseFile } from "./change-set"; import { GitError, runGitCapturingAsync } from "./git"; -import { scriptedGit } from "./scripted-git"; +import { scriptedGit, scriptedReadOnlyGit } from "./scripted-git"; /** Join NUL-terminated git -z fields without octal-escape hazards (`\03` ≠ NUL+"3"). */ function z(...fields: string[]): string { @@ -65,7 +65,7 @@ function scriptedChangeSetGit(opts: { numstat?: string; status?: string; }): ReturnType { - return scriptedGit((args) => { + return scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); if (args[0] === "merge-base") return "basesha\n"; if (args[0] === "rev-parse" && args[1] === "main") return "tipsha\n"; @@ -97,7 +97,7 @@ describe("changeSetSnapshot — merge-base stamping", () => { }); it("reuses the remote-tracking probe's SHA rather than re-resolving the tip", async () => { - const { gitAsync, calls } = scriptedGit((args) => { + const { gitAsync, calls } = scriptedReadOnlyGit((args) => { // `rev-parse --verify` PRINTS the SHA it verified. if (args[0] === "rev-parse" && args[1] === "--verify") return "remotetip\n"; if (args[0] === "merge-base") throw new GitError("failed", "fatal: no merge base", args); @@ -117,7 +117,7 @@ describe("changeSetSnapshot — merge-base stamping", () => { }); it("falls back to the comparison ref tip when merge-base cannot be computed", async () => { - const { gitAsync } = scriptedGit((args) => { + const { gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); if (args[0] === "merge-base") throw new GitError("failed", "fatal: no merge base", args); if (args[0] === "rev-parse" && args[1] === "main") return "tipsha\n"; @@ -664,7 +664,7 @@ describe("changeSetSnapshot — file cap", () => { describe("changeSetSnapshot — failures", () => { it("errs when no base branch is known", async () => { - const { gitAsync, calls } = scriptedGit(() => ""); + const { gitAsync, calls } = scriptedReadOnlyGit(() => ""); const result = await changeSetSnapshot(gitAsync, { worktreePath: "/wt", baseBranch: null }); expect(result.ok).toBe(false); if (result.ok) return; @@ -673,7 +673,7 @@ describe("changeSetSnapshot — failures", () => { }); it("surfaces real git stderr when a read fails", async () => { - const { gitAsync } = scriptedGit((args) => { + const { gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); if (args[0] === "merge-base" || (args[0] === "rev-parse" && args[1] === "main")) { throw new GitError("failed", "fatal: bad object main", args); diff --git a/packages/host-core/src/worktree/change-set.ts b/packages/host-core/src/worktree/change-set.ts index 8fb28afa6..e19b02171 100644 --- a/packages/host-core/src/worktree/change-set.ts +++ b/packages/host-core/src/worktree/change-set.ts @@ -31,6 +31,7 @@ import { import { resolveChangeSetBaseRevision } from "./comparison-ref"; import { stderrOf } from "./git"; +import { readOnlyGitAsync } from "./scan"; import { err, ok, type RunGitAsync, type WorktreeResult } from "./types"; export interface ChangeSetInput { @@ -103,12 +104,18 @@ const UNTRACKED_STAT_CONCURRENCY = 16; /** * Builds a {@link ChangeSetSnapshot} for the worktree. Failures surface real * git stderr (never a silent empty snapshot). Missing base fails fast. + * + * Read-only git ({@link readOnlyGitAsync}): this is refetched within a second + * of any write in the worktree, and a plain `git status -uall` holds the + * worktree's `index.lock` for its whole untracked scan — exactly when the + * agent that wrote is likely to run its own `git add`/`commit`. */ export async function changeSetSnapshot( - git: RunGitAsync, + runGit: RunGitAsync, input: ChangeSetInput, readUntrackedFileStat: ReadUntrackedFileStat = readUntrackedStat, ): Promise> { + const git = readOnlyGitAsync(runGit); if (!input.baseBranch) { return err("No base branch is known for this worktree, so its Change Set cannot be computed."); } @@ -193,9 +200,11 @@ export async function changeSetSnapshot( * the resolved base and appends untracked and unmerged paths from porcelain. */ export async function changeSetPaths( - git: RunGitAsync, + runGit: RunGitAsync, input: ChangeSetInput, ): Promise> { + // Read-only for the reason the snapshot is ({@link changeSetSnapshot}). + const git = readOnlyGitAsync(runGit); if (!input.baseBranch) { return err("No base branch is known for this worktree, so its Change Set cannot be computed."); } diff --git a/packages/host-core/src/worktree/diff.test.ts b/packages/host-core/src/worktree/diff.test.ts index abb66cd73..0cd613fed 100644 --- a/packages/host-core/src/worktree/diff.test.ts +++ b/packages/host-core/src/worktree/diff.test.ts @@ -2,11 +2,11 @@ import { describe, expect, it } from "vite-plus/test"; import { diffStat } from "./diff"; import { GitError } from "./git"; -import { scriptedGit } from "./scripted-git"; +import { scriptedReadOnlyGit } from "./scripted-git"; describe("diffStat — working-tree mode", () => { it("sums tracked numstat and appends untracked files with null counts", async () => { - const { gitAsync: git, calls } = scriptedGit((args) => { + const { gitAsync: git, calls } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "3\t1\tsrc/a.ts\n10\t0\tsrc/b.ts\n"; if (args[0] === "status") return "?? src/new.ts\n M src/a.ts\n"; return ""; @@ -27,7 +27,7 @@ describe("diffStat — working-tree mode", () => { }); it("represents binary files (-\\t-) with null insertions/deletions, excluded from totals", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "-\t-\tassets/logo.png\n5\t2\tsrc/a.ts\n"; if (args[0] === "status") return ""; return ""; @@ -46,7 +46,7 @@ describe("diffStat — working-tree mode", () => { }); it("ignores non-untracked porcelain lines (only ?? entries are appended)", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return ""; if (args[0] === "status") return " M src/a.ts\nA src/staged.ts\n?? src/really-new.ts\n"; return ""; @@ -60,7 +60,7 @@ describe("diffStat — working-tree mode", () => { }); it("returns err carrying stderr when the diff read fails", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") throw new GitError("failed", "fatal: bad revision HEAD", args); return ""; }); @@ -73,7 +73,7 @@ describe("diffStat — working-tree mode", () => { describe("diffStat — merge-base mode", () => { it("diffs ...HEAD (three-dot) and never reads untracked", async () => { - const { gitAsync: git, calls } = scriptedGit((args) => { + const { gitAsync: git, calls } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "4\t0\tsrc/a.ts\n"; // No remote-tracking ref — comparisons stay on the local base. if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); @@ -96,7 +96,7 @@ describe("diffStat — merge-base mode", () => { }); it("diffs against origin/ when the remote-tracking ref exists", async () => { - const { gitAsync: git, calls } = scriptedGit((args) => { + const { gitAsync: git, calls } = scriptedReadOnlyGit((args) => { if (args[0] === "rev-parse" && args[1] === "--verify") return "abc123\n"; if (args[0] === "diff") return "4\t0\tsrc/a.ts\n"; return ""; @@ -111,7 +111,7 @@ describe("diffStat — merge-base mode", () => { }); it("errs when the base branch is unknown", async () => { - const { gitAsync: git, calls } = scriptedGit(() => ""); + const { gitAsync: git, calls } = scriptedReadOnlyGit(() => ""); const result = await diffStat(git, { worktreePath: "/wt", baseBranch: null }, "merge-base"); expect(result.ok).toBe(false); // No git spawned — it fails fast on the missing base. @@ -121,7 +121,7 @@ describe("diffStat — merge-base mode", () => { describe("diffStat — rename path handling", () => { it("keeps the new path for a braced rename (common prefix/suffix)", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "2\t2\tsrc/{old.ts => new.ts}\n"; return ""; }); @@ -132,7 +132,7 @@ describe("diffStat — rename path handling", () => { }); it("keeps the new path for a whole-path rename (old => new, no braces)", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "1\t1\told/a.ts => new/b.ts\n"; return ""; }); @@ -143,7 +143,7 @@ describe("diffStat — rename path handling", () => { }); it("resolves a braced rename embedded mid-path (prefix/{old => new}/suffix)", async () => { - const { gitAsync: git } = scriptedGit((args) => { + const { gitAsync: git } = scriptedReadOnlyGit((args) => { if (args[0] === "diff") return "0\t0\tsrc/{a => b}/index.ts\n"; return ""; }); diff --git a/packages/host-core/src/worktree/diff.ts b/packages/host-core/src/worktree/diff.ts index 0bef6cc1a..34cec383d 100644 --- a/packages/host-core/src/worktree/diff.ts +++ b/packages/host-core/src/worktree/diff.ts @@ -21,6 +21,7 @@ import type { WorktreeDiffMode } from "@volli/shared"; import { resolveComparisonRefAsync } from "./comparison-ref"; import { stderrOf } from "./git"; +import { readOnlyGitAsync } from "./scan"; import { err, ok, type RunGitAsync, type WorktreeResult } from "./types"; export interface DiffStatInput { @@ -104,12 +105,18 @@ export function total(files: readonly DiffFileStat[], key: "insertions" | "delet * IPC verb and the `volli worktree diff` CLI door, both of which run on the * Electron main process, where `execFileSync` froze every window for the length * of the spawn. + * + * Read-only git ({@link readOnlyGitAsync}): the working-tree `status` then never + * holds the worktree's `index.lock` out from under an agent's own git write. + * Porcelain `git diff` does not consult the switch — it may still refresh the + * index at its end when it finds stat-dirty entries (`diff.autoRefreshIndex`). */ export async function diffStat( - git: RunGitAsync, + runGit: RunGitAsync, input: DiffStatInput, mode: WorktreeDiffMode, ): Promise> { + const git = readOnlyGitAsync(runGit); if (mode === "merge-base" && !input.baseBranch) { return err("No base branch is known for this worktree, so its PR diff cannot be computed."); } diff --git a/packages/host-core/src/worktree/read-only-reads.test.ts b/packages/host-core/src/worktree/read-only-reads.test.ts new file mode 100644 index 000000000..919b0cdb7 --- /dev/null +++ b/packages/host-core/src/worktree/read-only-reads.test.ts @@ -0,0 +1,177 @@ +/** + * VC-834: the background worktree reads run read-only git. + * + * A plain `git status` holds the checkout's `index.lock` for its whole run so it + * can write back the stat data it refreshed (git-status(1), "BACKGROUND + * REFRESH"). These reads run in the background — the Change Set and its path + * set within a second of any write in the worktree, the rail status on every + * watch event, the venue snapshot against a Main checkout many Sessions share — + * so an agent's own `git add`/`commit` in the same checkout could find the lock + * taken and fail with "index.lock: File exists". `--no-optional-locks` is git's + * switch for exactly this: the read never takes the lock and never writes. + */ +import { execFileSync } from "node:child_process"; +import { + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vite-plus/test"; + +import { changeSetPaths, changeSetSnapshot } from "./change-set"; +import { diffStat } from "./diff"; +import { runGitCapturingAsync } from "./git"; +import { scriptedGit } from "./scripted-git"; +import { getWorktreeStatus } from "./status"; +import type { RunGitAsync } from "./types"; +import { venueSnapshot } from "./venue"; + +/** Plausible answers so each read walks its whole path, status included. */ +function answer(args: readonly string[]): string { + const [verb, first] = args[0] === "--no-optional-locks" ? args.slice(1) : args; + if (verb === "rev-parse" && first === "--verify") throw new Error("no such ref"); + if (verb === "merge-base") return "basesha\n"; + if (verb === "rev-parse") return "sha\n"; + if (verb === "rev-list") return "0\t0\n"; + if (verb === "branch") return "topic\n"; + return ""; +} + +describe("background worktree reads issue read-only git (VC-834)", () => { + const reads: Array< + [string, (git: ReturnType["gitAsync"]) => Promise] + > = [ + [ + "changeSetSnapshot", + (git) => changeSetSnapshot(git, { worktreePath: "/wt", baseBranch: "main" }), + ], + ["changeSetPaths", (git) => changeSetPaths(git, { worktreePath: "/wt", baseBranch: "main" })], + [ + "getWorktreeStatus", + (git) => getWorktreeStatus(git, { worktreePath: "/wt", branch: "topic", baseBranch: "main" }), + ], + [ + "venueSnapshot", + (git) => venueSnapshot(git, { path: "/wt", kind: "worktree", baseBranch: "main" }), + ], + [ + "diffStat (working-tree)", + (git) => diffStat(git, { worktreePath: "/wt", baseBranch: "main" }, "working-tree"), + ], + [ + "diffStat (merge-base)", + (git) => diffStat(git, { worktreePath: "/wt", baseBranch: "main" }, "merge-base"), + ], + ]; + + it.each(reads)("%s leads every git call with --no-optional-locks", async (_name, read) => { + const { gitAsync, calls } = scriptedGit(answer); + + await read(gitAsync); + + const issued = calls.map((call) => call.args.join(" ")); + expect(issued.length).toBeGreaterThan(0); + expect(issued.every((line) => line.startsWith("--no-optional-locks "))).toBe(true); + }); +}); + +// The scripted runner proves the argv; this proves what the argv buys against +// real git. Before every `status` a read issues, the fixture makes a tracked +// file stat-dirty (same bytes, new mtime) — exactly what a plain status +// refreshes under `index.lock` and writes back. A status that never takes the +// lock never writes, so the index bytes after it are the bytes before it. +// +// Calls are serialized so each `status` is measured on its own. `git diff` is +// deliberately not measured: porcelain diff refreshes the index at its end +// regardless of `--no-optional-locks` (it is gated by `diff.autoRefreshIndex` +// instead), which is a shorter lock window this switch cannot close. +describe("background worktree status reads leave the index untouched (real git)", () => { + let dir: string; + let tick = 0; + const raw = (args: readonly string[]): string => + execFileSync("git", [...args], { + cwd: dir, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + const index = (): Buffer => readFileSync(join(dir, ".git", "index")); + const makeStatDirty = (): void => { + tick += 1; + const past = new Date(Date.now() - tick * 3_600_000); + utimesSync(join(dir, "tracked.txt"), past, past); + }; + + /** Real git, one call at a time, recording whether each `status` wrote the index. */ + function measuringGit(): { git: RunGitAsync; statuses: Array<{ args: string; wrote: boolean }> } { + const statuses: Array<{ args: string; wrote: boolean }> = []; + let queue: Promise = Promise.resolve(); + const git: RunGitAsync = (args, cwd) => { + const run = queue.then(async () => { + if (!args.includes("status")) return runGitCapturingAsync(args, cwd); + makeStatDirty(); + const before = index(); + const out = await runGitCapturingAsync(args, cwd); + statuses.push({ args: args.join(" "), wrote: !index().equals(before) }); + return out; + }); + queue = run.catch(() => undefined); + return run; + }; + return { git, statuses }; + } + + beforeEach(() => { + dir = realpathSync(mkdtempSync(join(process.cwd(), ".git-read-only-test-"))); + raw(["init", "--quiet", "--initial-branch=main"]); + raw(["config", "user.name", "Test"]); + raw(["config", "user.email", "test@example.com"]); + raw(["config", "commit.gpgsign", "false"]); + writeFileSync(join(dir, "tracked.txt"), "tracked\n"); + raw(["add", "-A"]); + raw(["commit", "--quiet", "-m", "initial"]); + raw(["checkout", "--quiet", "-b", "topic"]); + writeFileSync(join(dir, "untracked.txt"), "untracked\n"); + }); + afterEach(() => rmSync(dir, { recursive: true, force: true })); + + it("a plain status writes the stat-dirty index back (the fixture can see the lock taken)", async () => { + const { git, statuses } = measuringGit(); + await git(["status", "--porcelain=v2", "-z", "-uall"], dir); + expect(statuses).toEqual([{ args: "status --porcelain=v2 -z -uall", wrote: true }]); + }); + + it("the Change Set, its path set, the rail status, the venue and diffStat never write it", async () => { + const { git, statuses } = measuringGit(); + + const snapshot = await changeSetSnapshot(git, { worktreePath: dir, baseBranch: "main" }); + expect(snapshot.ok && snapshot.value.files.map((file) => file.path)).toEqual(["untracked.txt"]); + expect(await changeSetPaths(git, { worktreePath: dir, baseBranch: "main" })).toEqual({ + ok: true, + value: ["untracked.txt"], + }); + const status = await getWorktreeStatus(git, { + worktreePath: dir, + branch: "topic", + baseBranch: "main", + }); + expect(status.uncommitted).toBe(true); + expect((await venueSnapshot(git, { path: dir, kind: "worktree", baseBranch: "main" })).ok).toBe( + true, + ); + expect( + (await diffStat(git, { worktreePath: dir, baseBranch: "main" }, "working-tree")).ok, + ).toBe(true); + + expect(statuses).toEqual([ + { args: "--no-optional-locks status --porcelain=v2 -z -uall", wrote: false }, + { args: "--no-optional-locks status --porcelain=v2 -z -uall", wrote: false }, + { args: "--no-optional-locks status --porcelain", wrote: false }, + { args: "--no-optional-locks status --porcelain=v2 -z -uall", wrote: false }, + { args: "--no-optional-locks status --porcelain", wrote: false }, + ]); + }); +}); diff --git a/packages/host-core/src/worktree/scripted-git.ts b/packages/host-core/src/worktree/scripted-git.ts index ee31f8337..1368f6349 100644 --- a/packages/host-core/src/worktree/scripted-git.ts +++ b/packages/host-core/src/worktree/scripted-git.ts @@ -35,6 +35,17 @@ export interface ScriptedGit { countMatching: (prefix: readonly string[]) => number; } +export interface ScriptedGitOptions { + /** + * The code under test runs read-only git (`readOnlyGitAsync`, VC-834), so + * every call leads with `--no-optional-locks`. Strip that switch before the + * handler and the recorded calls see the args, so a suite scripts the verb, + * not the switch. The switch itself is pinned by its own test against the + * raw args (`read-only-reads.test.ts`). + */ + readOnly?: boolean; +} + /** * Builds a recording `RunGit` from a handler. The handler receives the args and * cwd; return a string to act as stdout, or `throw` (optionally a @@ -42,17 +53,22 @@ export interface ScriptedGit { */ export function scriptedGit( handler: (args: readonly string[], cwd: string) => string, + options: ScriptedGitOptions = {}, ): ScriptedGit { const calls: GitCall[] = []; const syncCalls: GitCall[] = []; const asyncCalls: GitCall[] = []; - const git: RunGit = (args, cwd) => { + const verb = (args: readonly string[]): readonly string[] => + options.readOnly === true && args[0] === "--no-optional-locks" ? args.slice(1) : args; + const git: RunGit = (rawArgs, cwd) => { + const args = verb(rawArgs); const call = { args, cwd }; calls.push(call); syncCalls.push(call); return handler(args, cwd); }; - const gitAsync: RunGitAsync = async (args, cwd) => { + const gitAsync: RunGitAsync = async (rawArgs, cwd) => { + const args = verb(rawArgs); const call = { args, cwd }; calls.push(call); asyncCalls.push(call); @@ -68,3 +84,10 @@ export function scriptedGit( calls.filter((call) => prefix.every((token, i) => call.args[i] === token)).length, }; } + +/** {@link scriptedGit} for the read-only verbs: see {@link ScriptedGitOptions.readOnly}. */ +export function scriptedReadOnlyGit( + handler: (args: readonly string[], cwd: string) => string, +): ScriptedGit { + return scriptedGit(handler, { readOnly: true }); +} diff --git a/packages/host-core/src/worktree/snapshot.test.ts b/packages/host-core/src/worktree/snapshot.test.ts index 52a932335..225c9b185 100644 --- a/packages/host-core/src/worktree/snapshot.test.ts +++ b/packages/host-core/src/worktree/snapshot.test.ts @@ -29,7 +29,7 @@ import { type WorktreeChangeSetRead, type WorktreeStatusRead, } from "./read"; -import { scriptedGit, type ScriptedGit } from "./scripted-git"; +import { scriptedReadOnlyGit, type ScriptedGit } from "./scripted-git"; import { createWorktreeSnapshotCache, getWorktreeSnapshots, @@ -275,7 +275,7 @@ describe("the rail across a Now↔Diffs flip (VC-372)", () => { }), ); - const git = scriptedGit((args) => { + const git = scriptedReadOnlyGit((args) => { if (args[0] === "status") return ""; if (args[0] === "rev-parse" && args[1] === "--git-dir") return ".git\n"; if (args[0] === "rev-parse" && args[1] === "--verify") return "base-sha\n"; diff --git a/packages/host-core/src/worktree/status.test.ts b/packages/host-core/src/worktree/status.test.ts index 6d6910ff8..6f26a8518 100644 --- a/packages/host-core/src/worktree/status.test.ts +++ b/packages/host-core/src/worktree/status.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vite-plus/test"; -import { scriptedGit } from "./scripted-git"; +import { scriptedReadOnlyGit } from "./scripted-git"; import { getWorktreeStatus } from "./status"; let dirs: string[] = []; @@ -32,7 +32,7 @@ function statusGit( hasRemoteRef?: boolean; } = {}, ) { - return scriptedGit((args) => { + return scriptedReadOnlyGit((args) => { if (args[0] === "status") return (over.status ?? (() => ""))(); if (args[0] === "rev-parse" && args[1] === "--verify") { if (over.hasRemoteRef) return "abc123\n"; diff --git a/packages/host-core/src/worktree/status.ts b/packages/host-core/src/worktree/status.ts index 0c0f6f39b..62640b3cd 100644 --- a/packages/host-core/src/worktree/status.ts +++ b/packages/host-core/src/worktree/status.ts @@ -25,6 +25,7 @@ * on the loaded machine where this hurts most. */ import { resolveComparisonRefAsync } from "./comparison-ref"; +import { readOnlyGitAsync } from "./scan"; import { detectSequencerStateAsync } from "./sequencer"; import type { RunGitAsync } from "./types"; @@ -114,11 +115,19 @@ async function readUnpushed(git: RunGitAsync, input: WorktreeStatusInput): Promi } } -/** Computes the Details-rail worktree status report (see {@link WorktreeStatusReport}). */ +/** + * Computes the Details-rail worktree status report (see {@link WorktreeStatusReport}). + * + * Every read carries `--no-optional-locks` ({@link readOnlyGitAsync}): this runs + * in the background on every watch event, and a plain `git status` holds the + * worktree's `index.lock` for its whole run — long enough for an agent's own + * `git add`/`commit` in the same worktree to fail with "index.lock: File exists". + */ export async function getWorktreeStatus( - git: RunGitAsync, + runGit: RunGitAsync, input: WorktreeStatusInput, ): Promise { + const git = readOnlyGitAsync(runGit); return { uncommitted: await readUncommitted(git, input.worktreePath), sequencerActive: (await detectSequencerStateAsync(git, input.worktreePath)) === "active", diff --git a/packages/host-core/src/worktree/venue.test.ts b/packages/host-core/src/worktree/venue.test.ts index 24a435c4e..245d50c4d 100644 --- a/packages/host-core/src/worktree/venue.test.ts +++ b/packages/host-core/src/worktree/venue.test.ts @@ -11,7 +11,7 @@ import { openTestDb, testProject, testTicket } from "../db/test-helpers"; import type { TestDb } from "../db/test-helpers"; import { insertTicket, updateTicketFields } from "../db/tickets-repo"; import { GitError } from "./git"; -import { scriptedGit } from "./scripted-git"; +import { scriptedReadOnlyGit } from "./scripted-git"; import { readVenue, venueSnapshot } from "./venue"; /** Real git in a temp repo, with an identity so commits do not need the host's. */ @@ -44,7 +44,7 @@ function tracked(code: string, path: string): string { * status/numstat payloads are whatever the case under test needs. */ function scriptedVenueGit(opts: { branch?: string; status?: string; numstat?: string }) { - return scriptedGit((args) => { + return scriptedReadOnlyGit((args) => { if (args[0] === "branch") return `${opts.branch ?? "volli/VC-81-auto-title"}\n`; if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); if (args[0] === "merge-base") return "basesha\n"; @@ -164,7 +164,7 @@ describe("venueSnapshot", () => { }); it("drops the diff when the base branch cannot be resolved at all", async () => { - const { gitAsync } = scriptedGit((args) => { + const { gitAsync } = scriptedReadOnlyGit((args) => { if (args[0] === "branch") return "volli/VC-9\n"; if (args[0] === "rev-parse" && args[1] === "--verify") throw new Error("no such ref"); if (args[0] === "merge-base") throw new GitError("failed", "fatal: no merge base", args); @@ -224,7 +224,7 @@ describe("venueSnapshot", () => { }); it("surfaces git's own message rather than a silently empty venue", async () => { - const { gitAsync } = scriptedGit(() => { + const { gitAsync } = scriptedReadOnlyGit(() => { throw new GitError("failed", "fatal: not a git repository", ["status"]); }); diff --git a/packages/host-core/src/worktree/venue.ts b/packages/host-core/src/worktree/venue.ts index 74bae11ce..178631809 100644 --- a/packages/host-core/src/worktree/venue.ts +++ b/packages/host-core/src/worktree/venue.ts @@ -27,6 +27,7 @@ import { getProjectById } from "../db/projects-repo"; import { getTicketRow } from "../db/tickets-repo"; import { resolveChangeSetBaseRevision } from "./comparison-ref"; import { runGitCapturingAsync, stderrOf } from "./git"; +import { readOnlyGitAsync } from "./scan"; import { err, ok, type RunGitAsync, type WorktreeResult } from "./types"; export interface VenueSnapshotInput { @@ -145,11 +146,16 @@ interface DirtyEntry { * are independent of each other, so the snapshot costs one round trip rather * than two, and the tree cannot move between them any more than it could * between two sequential spawns. + * + * Read-only git ({@link readOnlyGitAsync}): a venue may be the Main checkout + * many Sessions share, and a plain `git status` holds its `index.lock` for the + * whole untracked scan — any of their `git add`/`commit` calls would fail. */ export async function venueSnapshot( - git: RunGitAsync, + runGit: RunGitAsync, input: VenueSnapshotInput, ): Promise> { + const git = readOnlyGitAsync(runGit); try { // `--show-current` rather than `rev-parse --abbrev-ref HEAD`: it prints // nothing on a detached HEAD (instead of the literal string "HEAD", which a