From d01f25314d68e59c848a88e0ed49b8b1e47f589c Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 05:20:25 -0500 Subject: [PATCH 1/2] fix(threads): a thread starts in another thread's checkout when its folder is behind a symlink Starting a thread in another thread's checkout failed with "is no longer a worktree of this project" whenever the data folder sat behind a symlink (macOS /var for /private/var): git lists a worktree by its real path, and the thread had recorded it through the symlink. The setup now compares the two as the disk resolves them. Found in a live run with a real agent. --- .../Layers/ThreadBootstrap.test.ts | 40 +++++++++++++++++++ .../orchestration/Layers/ThreadBootstrap.ts | 13 +++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/apps/server/src/orchestration/Layers/ThreadBootstrap.test.ts b/apps/server/src/orchestration/Layers/ThreadBootstrap.test.ts index af355975a..983107092 100644 --- a/apps/server/src/orchestration/Layers/ThreadBootstrap.test.ts +++ b/apps/server/src/orchestration/Layers/ThreadBootstrap.test.ts @@ -1,3 +1,7 @@ +import * as NodeFs from "node:fs"; +import * as NodeOs from "node:os"; +import * as NodePath from "node:path"; + import { describe, expect, it } from "@effect/vitest"; import { ChildRequestId, @@ -491,6 +495,42 @@ describe("ThreadBootstrap.startChild", () => { }).pipe(Effect.provide(world.layer)); }); + it.effect("finds a lent worktree that was recorded through a symlink", () => { + // git lists a worktree by its real path; the thread it belongs to may + // have recorded another spelling of it (macOS's /var for /private/var). + const base = NodeFs.realpathSync(NodeFs.mkdtempSync(NodePath.join(NodeOs.tmpdir(), "lent-"))); + const realPath = NodePath.join(base, "coder"); + const linkedPath = NodePath.join(base, "linked"); + NodeFs.mkdirSync(realPath); + NodeFs.symlinkSync(realPath, linkedPath); + const world = makeWorld(); + world.worktrees.push({ path: realPath, branch: "feature/retry" }); + return Effect.gen(function* () { + yield* startChild( + input({ + launch: launch({ + workspace: { kind: "project_folder" }, + runSetup: false, + checkout: { + threadId: "coder" as OrchestrationThread["id"], + title: "Build the retry", + worktreePath: linkedPath, + branch: "feature/retry", + }, + }), + }), + ); + // The thread keeps the spelling the checkout was lent under. + expect(world.starts()[0]?.create).toMatchObject({ + branch: "feature/retry", + worktreePath: linkedPath, + }); + }).pipe( + Effect.provide(world.layer), + Effect.ensuring(Effect.sync(() => NodeFs.rmSync(base, { recursive: true, force: true }))), + ); + }); + it.effect( "finishes a child an older build made without its first turn, never claiming its setup twice", () => { diff --git a/apps/server/src/orchestration/Layers/ThreadBootstrap.ts b/apps/server/src/orchestration/Layers/ThreadBootstrap.ts index 571fb505a..2386dbbd5 100644 --- a/apps/server/src/orchestration/Layers/ThreadBootstrap.ts +++ b/apps/server/src/orchestration/Layers/ThreadBootstrap.ts @@ -38,6 +38,7 @@ import { import { worktreeFolderNameForBranch } from "@threadlines/shared/git"; import { areFilesystemPathsEqual } from "@threadlines/shared/path"; import { setupProjectScript } from "@threadlines/shared/projectScripts"; +import * as NodeFs from "node:fs/promises"; import { basename, join } from "node:path"; import * as Cause from "effect/Cause"; @@ -316,7 +317,10 @@ const make = Effect.gen(function* () { /** * Stage 1 for a thread put in another thread's worktree: nothing is made. * The folder must still be one of this project's worktrees; its branch is - * read now, whatever it was when the thread was asked for. + * read now, whatever it was when the thread was asked for. Folders are + * compared as the disk resolves them: git lists a worktree by its real + * path, and a thread may have recorded it through a symlink (macOS's + * `/var` for `/private/var`). */ const findLentWorktree = (projectId: ProjectId, lent: NonNullable) => Effect.gen(function* () { @@ -327,7 +331,12 @@ const make = Effect.gen(function* () { project === undefined ? [] : yield* gitWorkflow.listWorktrees({ cwd: project.workspaceRoot }); - const found = worktrees.find((entry) => areFilesystemPathsEqual(entry.path, lent.path)); + const real = (path: string) => Effect.promise(() => NodeFs.realpath(path).catch(() => path)); + const lentPath = yield* real(lent.path); + const listed = yield* Effect.forEach(worktrees, (entry) => + Effect.map(real(entry.path), (path) => ({ entry, path })), + ); + const found = listed.find(({ path }) => areFilesystemPathsEqual(path, lentPath))?.entry; if (found === undefined) { return yield* new LentWorktreeGoneError(lent); } From d77868702504a5c21bde64810bb43eff36340518 Mon Sep 17 00:00:00 2001 From: Badcuban <108198679+badcuban@users.noreply.github.com> Date: Sat, 14 Nov 2026 05:20:25 -0500 Subject: [PATCH 2/2] fix(checkpoints): a revert never applies one checkout's snapshots to another A thread can move to another checkout between turns (the user's own switch, an agent's move, a session that settled somewhere else), and every checkout of a repository shares its checkpoint refs. A revert to a turn from before the move then applied snapshots of the folder the thread had left to the folder it was in, and for a thread alone in its worktree restored the other folder's whole tree over it. - Every snapshot now records the checkout it was taken in, as a `threadlines-checkout` trailer: `.` for the main checkout, else `worktrees/`, so a renamed or symlinked folder is still the same checkout. - `checkpointCheckouts` answers, per ref, here, elsewhere or unknown. - A revert is refused when a turn it would undo has a completion or starting snapshot from another checkout. When only its target is from another checkout, it restores from the first undone turn's own starting snapshot, which is this folder right before that turn. - The move an agent made, on the thread's record, now only stands in for snapshots that don't say where they were taken. --- .../Layers/CheckpointRevert.test.ts | 92 ++++++++++++++++--- .../checkpointing/Layers/CheckpointRevert.ts | 79 +++++++++++++--- .../Layers/CheckpointStore.test.ts | 57 ++++++++++++ .../checkpointing/Layers/CheckpointStore.ts | 8 ++ .../checkpointing/Services/CheckpointStore.ts | 22 ++++- .../testing/CheckpointStoreStub.ts | 1 + apps/server/src/vcs/GitVcsDriver.ts | 76 ++++++++++++++- apps/server/src/vcs/VcsDriver.ts | 16 ++++ docs/design/child-threads.md | 4 +- 9 files changed, 321 insertions(+), 34 deletions(-) diff --git a/apps/server/src/checkpointing/Layers/CheckpointRevert.test.ts b/apps/server/src/checkpointing/Layers/CheckpointRevert.test.ts index 32106a66c..ec5631eab 100644 --- a/apps/server/src/checkpointing/Layers/CheckpointRevert.test.ts +++ b/apps/server/src/checkpointing/Layers/CheckpointRevert.test.ts @@ -8,6 +8,7 @@ import { ThreadCheckoutMovedActivityKind, type ThreadCheckoutMovedPayload, ThreadId, + TurnId, } from "@threadlines/contracts"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; @@ -25,7 +26,8 @@ import { import { CheckpointRevert } from "../Services/CheckpointRevert.ts"; import { CheckpointStore } from "../Services/CheckpointStore.ts"; import { makeCheckpointStoreStub } from "../testing/CheckpointStoreStub.ts"; -import { checkpointRefForThreadTurn } from "../Utils.ts"; +import type { VcsCheckpointCheckout } from "../../vcs/VcsDriver.ts"; +import { checkpointPreTurnRefForThreadTurnCount, checkpointRefForThreadTurn } from "../Utils.ts"; import { CheckpointRevertLive } from "./CheckpointRevert.ts"; const PROJECT = ProjectId.make("project-1"); @@ -45,14 +47,36 @@ const thread = (overrides: Partial = {}) => worktreePath: worktree, activities: [], checkpoints: [1, 2, 3].map((turnCount) => ({ + turnId: TurnId.make(`turn-${turnCount}`), checkpointTurnCount: turnCount, checkpointRef: checkpointRefForThreadTurn(THREAD, turnCount), })), ...overrides, }) as unknown as OrchestrationThread; -/** Where a revert to `turnCount` would work, and how, for a thread no one else is near. */ -const resolve = (subject: OrchestrationThread, turnCount: number) => +/** A turn's completion snapshot, and the one taken right before it started. */ +const done = (turn: number) => checkpointRefForThreadTurn(THREAD, turn); +const start = (turn: number) => checkpointPreTurnRefForThreadTurnCount(THREAD, turn); + +/** Every snapshot of these turns, as taken in the folder the thread has since left. */ +const leftBehind = (...turns: ReadonlyArray) => + Object.fromEntries( + turns.flatMap((turn) => [ + [done(turn), "elsewhere"], + [start(turn), "elsewhere"], + ]), + ) as Record; + +/** + * Where a revert to `turnCount` would work, and how, for a thread no one else + * is near. `takenIn` says where a snapshot was taken; any other one was taken + * here, in a thread whose snapshots all record their checkout. + */ +const resolve = ( + subject: OrchestrationThread, + turnCount: number, + takenIn: Record | "unrecorded" = {}, +) => Effect.gen(function* () { const revert = yield* CheckpointRevert; return yield* revert.resolveContext({ threadId: THREAD, turnCount }); @@ -61,7 +85,21 @@ const resolve = (subject: OrchestrationThread, turnCount: number) => CheckpointRevertLive.pipe( Layer.provide( Layer.mergeAll( - Layer.succeed(CheckpointStore, makeCheckpointStoreStub()), + Layer.succeed( + CheckpointStore, + makeCheckpointStoreStub({ + checkpointCheckouts: (input) => + Effect.succeed( + input.checkpointRefs.map((ref) => + takenIn === "unrecorded" + ? "unknown" + : (takenIn[ref] ?? + // The older per-turn-id refs don't exist on these threads. + (ref.includes("pre-turn/") ? "unknown" : "here")), + ), + ), + }), + ), Layer.succeed(ProjectionSnapshotQuery, { getThreadDetailById: () => Effect.succeed(Option.some(subject)), getProjectShellById: () => @@ -78,11 +116,16 @@ const resolve = (subject: OrchestrationThread, turnCount: number) => Effect.runPromise, ); +const movedSince = { + kind: "unavailable", + detail: expect.stringContaining("moved to another checkout"), +}; + describe("CheckpointRevert.resolveContext", () => { it("restores the whole folder only for a thread whose worktree is its own", async () => { expect(await resolve(thread(), 1)).toMatchObject({ kind: "ready", - context: { mode: "workspace" }, + context: { mode: "workspace", targetCheckpointRef: done(1) }, }); // A guest in another thread's worktree never does, even with that thread // gone and nothing else pointing at the folder. @@ -93,7 +136,31 @@ describe("CheckpointRevert.resolveContext", () => { }); }); - it("never reaches back past the point an agent moved the thread into another checkout", async () => { + it("never undoes a turn whose snapshots were taken in another checkout", async () => { + // Turns one and two ran in the folder the thread has since left. + expect(await resolve(thread(), 1, leftBehind(1, 2))).toMatchObject(movedSince); + expect(await resolve(thread(), 0, leftBehind(1, 2))).toMatchObject(movedSince); + // A starting snapshot left over from the other folder spoils its turn too. + expect(await resolve(thread(), 2, { [start(3)]: "elsewhere" })).toMatchObject(movedSince); + }); + + it("undoes everything since the move from this folder's own snapshot, never the other folder's", async () => { + // Turn three is the only one that ran here. Reverting to turn two would + // restore from turn two's snapshot, which holds the other folder's files: + // the files come from this folder as it was right before turn three. + expect(await resolve(thread(), 2, leftBehind(1, 2))).toMatchObject({ + kind: "ready", + context: { targetCheckpointRef: start(3) }, + }); + // With no snapshot of this folder from before that turn, nothing is safe to restore from. + expect( + await resolve(thread(), 2, { ...leftBehind(1, 2), [start(3)]: "unknown" }), + ).toMatchObject(movedSince); + // And reverting to the newest turn undoes nothing, so there is no such snapshot at all. + expect(await resolve(thread(), 3, leftBehind(3))).toMatchObject(movedSince); + }); + + it("falls back to the move on the thread's record only for snapshots that don't say where they were taken", async () => { const moved = thread({ worktreeOrigin: { kind: "thread", threadId: CODER }, activities: [ @@ -109,13 +176,10 @@ describe("CheckpointRevert.resolveContext", () => { }, ] as never, }); - // Turns one and two changed the folder it left. - const before = await resolve(moved, 1); - expect(before.kind).toBe("unavailable"); - expect(before).toMatchObject({ - detail: expect.stringContaining("moved into another checkout"), - }); - // Undoing only what it did since the move stays possible. - expect((await resolve(moved, 2)).kind).toBe("ready"); + // Older snapshots: turns one and two changed the folder it left. + expect(await resolve(moved, 1, "unrecorded")).toMatchObject(movedSince); + expect((await resolve(moved, 2, "unrecorded")).kind).toBe("ready"); + // Snapshots that all say "here" outrank the record: it moved away and came back. + expect((await resolve(moved, 1)).kind).toBe("ready"); }); }); diff --git a/apps/server/src/checkpointing/Layers/CheckpointRevert.ts b/apps/server/src/checkpointing/Layers/CheckpointRevert.ts index a7a6659d0..8d16675e5 100644 --- a/apps/server/src/checkpointing/Layers/CheckpointRevert.ts +++ b/apps/server/src/checkpointing/Layers/CheckpointRevert.ts @@ -208,15 +208,65 @@ const make = Effect.gen(function* () { }; } - // Turns from before an agent moved it into another checkout changed the - // folder it left: undoing them here would rewrite files it never touched. - const movedAtTurnCount = turnCountAtLastCheckoutMove(thread); - if (movedAtTurnCount !== null && input.turnCount < movedAtTurnCount) { - return { - kind: "unavailable" as const, - detail: - "This thread moved into another checkout after that point. Its earlier changes are in the folder it left, so they can't be undone from here.", - }; + // A thread can move to another checkout between turns (the user's own + // switch, an agent's move, a session that settled somewhere else), and + // every checkout of the repository shares these snapshots. Each one + // says where it was taken. A turn whose snapshots come from another + // checkout changed that folder: undoing it here would rewrite files it + // never touched, so such a revert is refused. + const undone = thread.checkpoints + .filter((checkpoint) => checkpoint.checkpointTurnCount > input.turnCount) + .toSorted((left, right) => left.checkpointTurnCount - right.checkpointTurnCount); + // Per turn: its completion snapshot, then the two refs its starting + // snapshot can be under (by count, or by turn id on older threads). + const snapshotsOf = (checkpoint: RevertCheckpointContext) => + [ + checkpoint.checkpointRef, + checkpointPreTurnRefForThreadTurnCount(input.threadId, checkpoint.checkpointTurnCount), + checkpointPreTurnRefForThreadTurn(input.threadId, checkpoint.turnId), + ] as const; + const [targetTakenIn = "unknown", ...undoneTakenIn] = + yield* checkpointStore.checkpointCheckouts({ + cwd, + checkpointRefs: [targetCheckpointRef, ...undone.flatMap(snapshotsOf)], + }); + const takenIn = (index: number, snapshot: 0 | 1 | 2) => + undoneTakenIn[index * 3 + snapshot] ?? "unknown"; + // Snapshots from before they recorded their checkout say nothing. For + // turns like that, the move an agent made is still on the thread's record. + const unrecorded = undone.some((_, index) => takenIn(index, 0) === "unknown"); + const movedAtTurnCount = unrecorded ? turnCountAtLastCheckoutMove(thread) : null; + const movedSince = { + kind: "unavailable" as const, + detail: + "This thread moved to another checkout after that point. Its earlier changes are in the folder it left, so they can't be undone from here.", + }; + if ( + undoneTakenIn.includes("elsewhere") || + (movedAtTurnCount !== null && input.turnCount < movedAtTurnCount) + ) { + return movedSince; + } + + // The target is what files are restored from. One taken in another + // checkout (the turn the thread moved after) holds that folder's files; + // this folder as it was right before the first turn being undone is + // that turn's own starting snapshot, so the revert restores from it. + let revertTargetRef = targetCheckpointRef; + if (targetTakenIn === "elsewhere") { + const first = undone[0]; + const start = + first === undefined || first.checkpointTurnCount !== input.turnCount + 1 + ? undefined + : takenIn(0, 1) === "here" + ? snapshotsOf(first)[1] + : takenIn(0, 2) === "here" + ? snapshotsOf(first)[2] + : undefined; + if (start === undefined) { + return movedSince; + } + revertTargetRef = start; } const isolated = yield* isIsolatedWorkspaceForThread({ @@ -236,7 +286,7 @@ const make = Effect.gen(function* () { cwd, hasProviderSession: sessionCwd !== undefined, mode: isolated ? ("workspace" as const) : ("selective" as const), - targetCheckpointRef, + targetCheckpointRef: revertTargetRef, thread, }, }; @@ -252,6 +302,8 @@ const make = Effect.gen(function* () { readonly checkpoints: ReadonlyArray; readonly cwd: string; readonly targetTurnCount: number; + /** What the revert restores from: where the first turn's gap starts. */ + readonly targetCheckpointRef: CheckpointRef; readonly currentTurnCount: number; }) { const contested = new Set(); @@ -285,8 +337,10 @@ const make = Effect.gen(function* () { } const previousCheckpointRef = - input.checkpoints.find((entry) => entry.checkpointTurnCount === turnCount - 1) - ?.checkpointRef ?? checkpointRefForThreadTurn(input.threadId, turnCount - 1); + turnCount === input.targetTurnCount + 1 + ? input.targetCheckpointRef + : (input.checkpoints.find((entry) => entry.checkpointTurnCount === turnCount - 1) + ?.checkpointRef ?? checkpointRefForThreadTurn(input.threadId, turnCount - 1)); const previousCommit = yield* checkpointStore.resolveCheckpointCommit({ cwd: input.cwd, checkpointRef: previousCheckpointRef, @@ -367,6 +421,7 @@ const make = Effect.gen(function* () { checkpoints: thread.checkpoints, cwd: context.cwd, targetTurnCount: context.turnCount, + targetCheckpointRef: context.targetCheckpointRef, currentTurnCount: context.currentTurnCount, }); diff --git a/apps/server/src/checkpointing/Layers/CheckpointStore.test.ts b/apps/server/src/checkpointing/Layers/CheckpointStore.test.ts index fc9fba78a..3b4c2c3cd 100644 --- a/apps/server/src/checkpointing/Layers/CheckpointStore.test.ts +++ b/apps/server/src/checkpointing/Layers/CheckpointStore.test.ts @@ -199,6 +199,63 @@ it.layer(TestLayer)("CheckpointStoreLive", (it) => { ); }); + describe("checkpointCheckouts", () => { + it.effect("says which checkout of the repository each snapshot was taken in", () => + Effect.gen(function* () { + const tmp = yield* makeTmpDir(); + const worktree = path.join(yield* makeTmpDir("checkpoint-store-worktree-"), "feature"); + yield* initRepoWithCommit(tmp); + yield* git(tmp, ["worktree", "add", "-b", "feature", worktree]); + const checkpointStore = yield* CheckpointStore; + const threadId = ThreadId.make("thread-two-checkouts"); + const inProject = checkpointRefForThreadTurn(threadId, 1); + const inWorktree = checkpointRefForThreadTurn(threadId, 2); + const older = checkpointRefForThreadTurn(threadId, 3); + const gone = checkpointRefForThreadTurn(threadId, 4); + + // A thread that worked in the project folder, then moved to the worktree. + yield* checkpointStore.captureCheckpoint({ cwd: tmp, checkpointRef: inProject }); + // Taken from a folder inside it: a checkout is its root, however deep the cwd. + yield* git(worktree, ["mv", "README.md", "docs.md"]); + const fileSystem = yield* FileSystem.FileSystem; + yield* fileSystem.makeDirectory(path.join(worktree, "src")); + yield* checkpointStore.captureCheckpoint({ + cwd: path.join(worktree, "src"), + checkpointRef: inWorktree, + }); + // A snapshot from before they recorded their checkout. + const tree = yield* git(tmp, ["rev-parse", "HEAD^{tree}"]); + const legacy = yield* git(tmp, [ + "commit-tree", + tree, + "-m", + `threadlines checkpoint ref=${older}`, + ]); + yield* git(tmp, ["update-ref", older, legacy]); + + const checkpointRefs = [inProject, inWorktree, older, gone]; + expect(yield* checkpointStore.checkpointCheckouts({ cwd: tmp, checkpointRefs })).toEqual([ + "here", + "elsewhere", + "unknown", + "unknown", + ]); + // The same snapshots, asked from the worktree it moved to. + expect( + yield* checkpointStore.checkpointCheckouts({ cwd: worktree, checkpointRefs }), + ).toEqual(["elsewhere", "here", "unknown", "unknown"]); + + // A checkout is still itself after its folder is renamed: its + // snapshots stay revertible there. + const renamed = path.join(path.dirname(worktree), "renamed"); + yield* git(tmp, ["worktree", "move", worktree, renamed]); + expect( + yield* checkpointStore.checkpointCheckouts({ cwd: renamed, checkpointRefs }), + ).toEqual(["elsewhere", "here", "unknown", "unknown"]); + }), + ); + }); + describe("diffCheckpoints", () => { it.effect("returns full oversized checkpoint diffs without truncation", () => Effect.gen(function* () { diff --git a/apps/server/src/checkpointing/Layers/CheckpointStore.ts b/apps/server/src/checkpointing/Layers/CheckpointStore.ts index 126a2dcc5..09254775e 100644 --- a/apps/server/src/checkpointing/Layers/CheckpointStore.ts +++ b/apps/server/src/checkpointing/Layers/CheckpointStore.ts @@ -69,6 +69,13 @@ const makeCheckpointStore = Effect.gen(function* () { return yield* checkpoints.resolveCheckpointCommit(input); }); + const checkpointCheckouts: CheckpointStoreShape["checkpointCheckouts"] = Effect.fn( + "checkpointCheckouts", + )(function* (input) { + const checkpoints = yield* resolveCheckpoints("CheckpointStore.checkpointCheckouts", input.cwd); + return yield* checkpoints.checkpointCheckouts(input); + }); + const diffCheckpoints: CheckpointStoreShape["diffCheckpoints"] = Effect.fn("diffCheckpoints")( function* (input) { const checkpoints = yield* resolveCheckpoints("CheckpointStore.diffCheckpoints", input.cwd); @@ -156,6 +163,7 @@ const makeCheckpointStore = Effect.gen(function* () { hasCheckpointRef, restoreCheckpoint, resolveCheckpointCommit, + checkpointCheckouts, diffCheckpoints, listHeadMovementPaths, diffCheckpointAgainstHead, diff --git a/apps/server/src/checkpointing/Services/CheckpointStore.ts b/apps/server/src/checkpointing/Services/CheckpointStore.ts index d2ed42758..f9087e354 100644 --- a/apps/server/src/checkpointing/Services/CheckpointStore.ts +++ b/apps/server/src/checkpointing/Services/CheckpointStore.ts @@ -14,7 +14,11 @@ import * as Context from "effect/Context"; import type * as Effect from "effect/Effect"; import type { CheckpointStoreError } from "../Errors.ts"; -import type { VcsReadRevisionFileInput, VcsRevisionFile } from "../../vcs/VcsDriver.ts"; +import type { + VcsCheckpointCheckout, + VcsReadRevisionFileInput, + VcsRevisionFile, +} from "../../vcs/VcsDriver.ts"; import { CheckpointRef } from "@threadlines/contracts"; export interface CaptureCheckpointInput { @@ -54,6 +58,11 @@ export interface CheckpointRefInput { readonly checkpointRef: CheckpointRef; } +export interface CheckpointCheckoutsInput { + readonly cwd: string; + readonly checkpointRefs: ReadonlyArray; +} + /** Line counts for one repository-root-relative path. */ export interface CheckpointFileStat { readonly path: string; @@ -183,6 +192,17 @@ export interface CheckpointStoreShape { input: ResolveCheckpointCommitInput, ) => Effect.Effect; + /** + * Where each snapshot was taken, seen from the checkout `cwd` is in, one + * answer per ref in order: `here`, `elsewhere` (another checkout of the + * repository: a thread that moved since), or `unknown` (a snapshot from + * before they recorded it, or a ref that is gone). A revert never applies + * a snapshot from elsewhere to this checkout. + */ + readonly checkpointCheckouts: ( + input: CheckpointCheckoutsInput, + ) => Effect.Effect, CheckpointStoreError>; + /** * Compute patch diff between two checkpoint refs. * diff --git a/apps/server/src/checkpointing/testing/CheckpointStoreStub.ts b/apps/server/src/checkpointing/testing/CheckpointStoreStub.ts index 092ed8e51..464466768 100644 --- a/apps/server/src/checkpointing/testing/CheckpointStoreStub.ts +++ b/apps/server/src/checkpointing/testing/CheckpointStoreStub.ts @@ -20,6 +20,7 @@ export function makeCheckpointStoreStub( hasCheckpointRef: () => Effect.succeed(false), restoreCheckpoint: () => Effect.succeed(true), resolveCheckpointCommit: () => Effect.succeed(null), + checkpointCheckouts: (input) => Effect.succeed(input.checkpointRefs.map(() => "here" as const)), diffCheckpoints: () => Effect.succeed(""), listHeadMovementPaths: () => Effect.succeed([]), diffCheckpointAgainstHead: () => Effect.succeed(null), diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts index 62109ec8e..cfec87f63 100644 --- a/apps/server/src/vcs/GitVcsDriver.ts +++ b/apps/server/src/vcs/GitVcsDriver.ts @@ -576,15 +576,33 @@ function parseDiffTreeEntries(stdout: string): RawCheckpointEntry[] { // snapshot was taken rides along as a trailer. Diffs use it to tell edits made // in the checkout apart from history that moved HEAD (a merge, a checkout). const CHECKPOINT_HEAD_TRAILER = "threadlines-head"; - -function checkpointCommitMessage(checkpointRef: string, headCommit: string | null): string { +// A repository's checkouts share its refs, so a snapshot says which one it +// was taken in (checkoutId). A thread can move to another checkout between +// turns, and a revert must never apply one checkout's snapshots to another +// (checkpointCheckouts). +const CHECKPOINT_CHECKOUT_TRAILER = "threadlines-checkout"; + +function checkpointCommitMessage( + checkpointRef: string, + headCommit: string | null, + checkoutId: string | null, +): string { const subject = `threadlines checkpoint ref=${checkpointRef}`; - return headCommit === null ? subject : `${subject}\n\n${CHECKPOINT_HEAD_TRAILER}: ${headCommit}`; + const trailers = [ + ...(headCommit === null ? [] : [`${CHECKPOINT_HEAD_TRAILER}: ${headCommit}`]), + // A trailer is one line: a worktree whose name can't be one goes unrecorded. + ...(checkoutId === null || /[\r\n]/u.test(checkoutId) + ? [] + : [`${CHECKPOINT_CHECKOUT_TRAILER}: ${checkoutId}`]), + ]; + return trailers.length === 0 ? subject : `${subject}\n\n${trailers.join("\n")}`; } interface CheckpointCommitMetadata { /** HEAD when the snapshot was taken; null on an unborn branch or for older snapshots. */ readonly head: string | null; + /** The checkout the snapshot was taken in (checkoutId); null for older snapshots. */ + readonly checkout: string | null; /** Committer time of the snapshot commit, in epoch seconds. */ readonly capturedAtSeconds: number | null; } @@ -596,8 +614,10 @@ function parseCheckpointCommitMetadata(raw: string): CheckpointCommitMetadata { const body = separator === -1 ? "" : raw.slice(separator + 2); const committer = /^committer .* (\d+) [+-]\d{4}$/mu.exec(headers); const trailer = new RegExp(`^${CHECKPOINT_HEAD_TRAILER}: ([0-9a-f]{40,64})$`, "mu").exec(body); + const checkout = new RegExp(`^${CHECKPOINT_CHECKOUT_TRAILER}: (.+)$`, "mu").exec(body); return { head: trailer?.[1] ?? null, + checkout: checkout?.[1] ?? null, capturedAtSeconds: committer?.[1] === undefined ? null : Number(committer[1]), }; } @@ -927,6 +947,29 @@ export const makeVcsDriverShape = Effect.fn("makeGitVcsDriverShape")(function* ( }), ); + // Which checkout of its repository `cwd` is in: `.` for the main one, else + // its worktree's place under the repository's git folder + // (`worktrees/`). Unlike its path, that survives the folder being + // renamed or moved, and reads the same through a symlink. Null when git + // can't say (a folder that is gone, a git too old for `--path-format`): + // the snapshot then records nothing, as older ones do. + const resolveCheckoutId = (cwd: string) => + execute({ + operation: "GitVcsDriver.checkpoints.resolveCheckoutId", + cwd, + args: ["rev-parse", "--path-format=absolute", "--git-dir", "--git-common-dir"], + allowNonZeroExit: true, + }).pipe( + Effect.map((result) => { + const [gitDir, commonDir] = result.exitCode === 0 ? result.stdout.split(/\r?\n/u) : []; + if (!gitDir || !commonDir) { + return null; + } + const place = nodePath.relative(commonDir, gitDir).replaceAll("\\", "/"); + return place.length === 0 ? "." : place; + }), + ); + const readCheckpointMetadata = (cwd: string, checkpointRef: string) => execute({ operation: "GitVcsDriver.checkpoints.readCheckpointMetadata", @@ -1205,7 +1248,11 @@ export const makeVcsDriverShape = Effect.fn("makeGitVcsDriverShape")(function* ( }; // Read by oid so the recorded head is exactly the commit the snapshot // was layered on, even if HEAD moves while the capture runs. - const headCommit = yield* resolveHeadCommit(input.cwd); + // Read together: a capture holds up the turn it is taken for. + const [headCommit, checkoutId] = yield* Effect.all( + [resolveHeadCommit(input.cwd), resolveCheckoutId(input.cwd)], + { concurrency: 2 }, + ); const commitOid = yield* scratchIndexes.withScratchIndex({ cwd: input.cwd, @@ -1239,7 +1286,7 @@ export const makeVcsDriverShape = Effect.fn("makeGitVcsDriverShape")(function* ( }); } - const message = checkpointCommitMessage(input.checkpointRef, headCommit); + const message = checkpointCommitMessage(input.checkpointRef, headCommit, checkoutId); const commitTreeResult = yield* execute({ operation, cwd: input.cwd, @@ -1312,6 +1359,25 @@ export const makeVcsDriverShape = Effect.fn("makeGitVcsDriverShape")(function* ( return true; }), + checkpointCheckouts: Effect.fn("GitVcsDriver.checkpoints.checkpointCheckouts")( + function* (input) { + yield* ensureLegacyCheckpointRefsMigrated(input.cwd); + const here = yield* resolveCheckoutId(input.cwd); + return yield* Effect.forEach( + input.checkpointRefs, + (checkpointRef) => + Effect.map(readCheckpointMetadata(input.cwd, checkpointRef), (metadata) => + here === null || metadata === null || metadata.checkout === null + ? ("unknown" as const) + : metadata.checkout === here + ? ("here" as const) + : ("elsewhere" as const), + ), + { concurrency: 4 }, + ); + }, + ), + resolveCheckpointCommit: Effect.fn("GitVcsDriver.checkpoints.resolveCheckpointCommit")( function* (input) { yield* ensureLegacyCheckpointRefsMigrated(input.cwd); diff --git a/apps/server/src/vcs/VcsDriver.ts b/apps/server/src/vcs/VcsDriver.ts index e46ee9215..5a6047940 100644 --- a/apps/server/src/vcs/VcsDriver.ts +++ b/apps/server/src/vcs/VcsDriver.ts @@ -125,6 +125,18 @@ export interface VcsCheckpointRefInput { readonly checkpointRef: CheckpointRef; } +export interface VcsCheckpointCheckoutsInput { + readonly cwd: string; + readonly checkpointRefs: ReadonlyArray; +} + +/** + * Where a snapshot was taken, seen from the checkout that asks: this one, + * another checkout of the repository, or not recorded (a snapshot from before + * snapshots recorded it, or a ref that is gone). + */ +export type VcsCheckpointCheckout = "here" | "elsewhere" | "unknown"; + /** Line counts for one repository-root-relative path. */ export interface VcsCheckpointFileStat { readonly path: string; @@ -162,6 +174,10 @@ export interface VcsCheckpointOps { readonly resolveCheckpointCommit: ( input: VcsResolveCheckpointCommitInput, ) => Effect.Effect; + /** One answer per ref, in order: see VcsCheckpointCheckout. */ + readonly checkpointCheckouts: ( + input: VcsCheckpointCheckoutsInput, + ) => Effect.Effect, VcsError>; readonly diffCheckpoints: (input: VcsDiffCheckpointsInput) => Effect.Effect; readonly listHeadMovementPaths: ( input: VcsCheckpointRangeInput, diff --git a/docs/design/child-threads.md b/docs/design/child-threads.md index 611a135c0..8a8bff56d 100644 --- a/docs/design/child-threads.md +++ b/docs/design/child-threads.md @@ -154,9 +154,9 @@ A chain like orchestrator, coder, reviewer needs the reviewer to read the coder' - **The setting.** "Threads can share a checkout" (`agentThreadsShareWorktrees`, Settings › Threads, under "Agents can start threads"), off unless chosen. Read at every call and again when the thread is set up. Off, the tools refuse and name the setting. - **The handle is a thread, never a path.** `thread_start`'s per-thread `checkout` and `thread_send`'s `checkout` take `self` (the caller's own checkout) or the id of a thread the caller started, attached or separated, open, on this computer, in the same project. An agent can't name a folder Threadlines doesn't know. - **Resolved when asked.** The handler reads the folder from the owner (where its session works, else its worktree, else the project folder), checks with git that it is a checkout of this project's repository, and reads its branch. The result is saved with the request as `ChildThreadLaunch.checkout` (`ChildThreadCheckout`: owner id and title, folder, branch), so an approval or a restart later puts the thread in the same place. `workspace` stays `project_folder`, which here means only "no worktree is made", so an older client reads the launch as a shared folder. -- **Not while its owner works.** A call is refused while the owning thread has live work (a turn, background work it awaits, a queued message, something waiting for the user), except `self`: the caller is in its own turn by definition. The user's yes can come much later, so a child's setup checks again: the reactor rereads the setting and the owner, and the bootstrap's worktree stage finds the folder among this project's worktrees again and records the branch it is on then (a folder that is gone, or no longer this project's, fails the start with a reason). A move by `thread_send` is checked against the owner in the decision itself. After the thread starts, the folder is live. Nothing stops the owner from being messaged again; the tool descriptions tell the agent to have one thread work there at a time. We chose this over a checkout lock: the user's own threads already share checkouts without one, and a lock can't cover terminals or other editors. +- **Not while its owner works.** A call is refused while the owning thread has live work (a turn, background work it awaits, a queued message, something waiting for the user), except `self`: the caller is in its own turn by definition. The user's yes can come much later, so a child's setup checks again: the reactor rereads the setting and the owner, and the bootstrap's worktree stage finds the folder among this project's worktrees again and records the branch it is on then (a folder that is gone, or no longer this project's, fails the start with a reason; folders are compared by their real paths, since git lists a worktree by its real path and a thread may have recorded it through a symlink). A move by `thread_send` is checked against the owner in the decision itself. After the thread starts, the folder is live. Nothing stops the owner from being messaged again; the tool descriptions tell the agent to have one thread work there at a time. We chose this over a checkout lock: the user's own threads already share checkouts without one, and a lock can't cover terminals or other editors. - **A guest, not an owner.** A thread started in another thread's worktree records it as `worktreeOrigin` (`ThreadWorktreeOrigin`, on the thread, its shell and `thread.created`). A guest's first turn never renames the branch, the project's setup script is never run for it (`runSetup` is forced off), deleting it never offers to remove the folder, even as the last thread pointing there, and a revert in it is always selective, never the whole-folder restore (CheckpointRevert), even once its owner is archived or deleted. The folder still blocks removal while any thread points at it (worktreeUsage.ts), and the cleanup list still shows it. The project folder is nobody's worktree: a thread put there has no `worktreeOrigin`. -- **Moving an existing thread.** `thread_send` with `checkout` moves an idle child there before it reads the message, with the same events the user's own checkout switch uses (threadCheckout.ts), so its session restarts in the new folder and resumes its conversation. A busy child is refused: a thread only moves between its turns. So is one with any background work left running (a dev server): its session can't restart while that runs, so it would read the message in the folder it is in. The move is written on the child as a `thread.checkout.moved` activity, which is its line in the chat and a boundary: a revert to a turn from before the move is refused, because those turns changed the folder it left. The worktree a moved child had of its own stays on disk. Any move to another checkout, the user's included, ends an earlier stay as a guest. +- **Moving an existing thread.** `thread_send` with `checkout` moves an idle child there before it reads the message, with the same events the user's own checkout switch uses (threadCheckout.ts), so its session restarts in the new folder and resumes its conversation. A busy child is refused: a thread only moves between its turns. So is one with any background work left running (a dev server): its session can't restart while that runs, so it would read the message in the folder it is in. The move is written on the child as a `thread.checkout.moved` activity, its line in the chat. A revert never crosses a move, the agent's or the user's own: every checkpoint snapshot records the checkout it was taken in (the `threadlines-checkout` trailer, GitVcsDriver: `.` for the main checkout, `worktrees/` for a worktree, so a renamed or symlinked folder is still the same checkout), and CheckpointRevert refuses to undo a turn whose snapshots come from another checkout, because that turn changed the folder the thread left. A revert to the turn the thread moved after restores from this folder's own snapshot (the first undone turn's starting one), never from the other folder's. Snapshots from before they recorded it fall back to the activity. The worktree a moved child had of its own stays on disk. Any move to another checkout, the user's included, ends an earlier stay as a guest. - **Access.** `thread_start`'s per-thread `access` asks for a lower level than the caller's; the child gets the safer of the two. There is no read-only level, and Supervised is enforced differently by each provider, so nothing here promises a guest can't write. - **Not covered.** Linked computers (`computer` with `checkout` is refused, and so is a child on another computer as owner or as the thread to move) and the tools outside agents and assistants use. - **On screen.** The approval card's line ends "in 's checkout" (or "in this thread's checkout"), with the access level when it is lower. The row under the message box reads "Shared worktree", and the sidebar's hover card names whose worktree it is.