From 2bcabb4efb9dcecfb515b522e7dbf33c1c0b872d Mon Sep 17 00:00:00 2001 From: ferueda Date: Tue, 14 Jul 2026 16:30:39 -0700 Subject: [PATCH 1/2] fix: harden worktree lifecycle correctness --- README.md | 2 + dev/plans/260714-destroy-path-layout.md | 48 ++++++ .../260714-preparing-acquire-recovery.md | 107 ++++++++++++++ dev/plans/260714-process-scan-failure.md | 22 +++ dev/plans/260714-repair-force-propagation.md | 20 +++ dev/plans/260714-repository-pool-identity.md | 21 +++ dev/plans/README.md | 7 + packages/grove/src/config.ts | 14 +- packages/grove/src/lease-acquire.ts | 41 +++++- packages/grove/src/lease-destroy.ts | 25 +++- packages/grove/src/lease-repair.ts | 4 +- packages/grove/src/process/detect.ts | 7 +- packages/grove/src/schemas.ts | 1 + packages/grove/src/target.ts | 8 +- packages/grove/src/transitions.ts | 33 ++++- packages/grove/test/config.test.ts | 16 +- .../test/lease-destroy.integration.test.ts | 31 ++++ .../test/lease-hooks.integration.test.ts | 2 +- .../test/lease-release.integration.test.ts | 43 ++++++ .../test/lease-repair.integration.test.ts | 139 ++++++++++++++++++ packages/grove/test/state-v1.test.ts | 39 +++++ packages/grove/test/transitions.test.ts | 57 ++++++- 22 files changed, 656 insertions(+), 31 deletions(-) create mode 100644 dev/plans/260714-destroy-path-layout.md create mode 100644 dev/plans/260714-preparing-acquire-recovery.md create mode 100644 dev/plans/260714-process-scan-failure.md create mode 100644 dev/plans/260714-repair-force-propagation.md create mode 100644 dev/plans/260714-repository-pool-identity.md create mode 100644 dev/plans/README.md diff --git a/README.md b/README.md index cdd6752..46d47cf 100644 --- a/README.md +++ b/README.md @@ -378,6 +378,8 @@ Configure shell commands in `createGrove({ hooks })`. Hook cwd is the worktree p | `postRelease` | After lease cleanup | | `preDestroy` | Before worktree removal | +Crash recovery may run `postCreate` more than once. Keep that hook idempotent; reused physical slots do not run it. + Lease hooks receive: `GROVE_LEASE_ID`, `GROVE_SLOT_NAME`, `GROVE_BRANCH`, `GROVE_REPO_ROOT`, `GROVE_WORKTREE_PATH`. Set `onHookFailure: "fail"` to throw `HOOK_FAILED` on hook errors. diff --git a/dev/plans/260714-destroy-path-layout.md b/dev/plans/260714-destroy-path-layout.md new file mode 100644 index 0000000..e5695a1 --- /dev/null +++ b/dev/plans/260714-destroy-path-layout.md @@ -0,0 +1,48 @@ +# Constrain destroy to its owned slot directory + +## Goal + +Prevent `destroy()` from recursively deleting the pool root or unrelated pool contents when +persisted, schema-valid state gives a slot a shallow or malformed path. Before either Git or +filesystem removal, the directory passed to recursive `rm` must be a non-pool direct child of +`poolDir` whose basename equals the persisted `slotName`. Keep the existing +`PATH_OUTSIDE_POOL` error contract and transition-driven failure quarantine. Canonical +`poolDir//` worktrees must continue to destroy normally. + +## Changes + +1. `packages/grove/test/lease-destroy.integration.test.ts` — add the failing regression first, + through the public SDK/state seams and real Git: + - Acquire a normal lease, then use `git worktree move` to relocate its registered worktree to + the schema-valid shallow path `poolDir/`. + - Read the acquired state with `readLeaseFirstState`, update the matching slot and lease paths, + and persist it with `writeLeaseFirstState`. This proves the fixture passes the existing schema + and joint-invariant boundary instead of bypassing validation with raw JSON. + - Place an unrelated sentinel file in `poolDir`, call `grove.destroy(leaseId, { force: true })`, + and assert `PATH_OUTSIDE_POOL`, an intact sentinel, an intact shallow worktree, and a + quarantined lease. These assertions prove rejection happens before both `removeWorktree` and + recursive `rm`, while preserving destroy failure transitions. + - Keep the existing `idempotent destroy resumes an in-progress destroying lease` case green as + positive coverage for canonical slot layout, worktree removal, and state finalization; do not + add a duplicate happy-path fixture. + +2. `packages/grove/src/lease-destroy.ts:completeDestroy` — add one private, destroy-specific helper + that derives `slotDir = dirname(wtPath)` and returns it only when all three conditions hold: + `slotDir !== normalize(poolDir)`, `dirname(slotDir) === normalize(poolDir)`, and + `basename(slotDir) === slot.slotName`. Otherwise throw `PathOutsidePoolError` with a concise + slot-layout message. Run the existing `assertPathWithinPool(poolDir, wtPath)` first, then this + helper, before `removeWorktree`; pass the returned directory to `rm` instead of recomputing + `dirname(wtPath)`. The current `completeDestroy` catch must remain the owner of quarantine and + rethrow behavior. + +## Verify + +- `pnpm exec vitest run packages/grove/test/lease-destroy.integration.test.ts` +- `pnpm check` + +## Boundaries + +- No persisted-state schema tightening or migration: malformed historical state must remain + readable and recover as a quarantined destroy failure. +- No generalized filesystem/path framework, repo-basename validation, or changes to release; + only destroy recursively removes the slot parent. diff --git a/dev/plans/260714-preparing-acquire-recovery.md b/dev/plans/260714-preparing-acquire-recovery.md new file mode 100644 index 0000000..84e8fef --- /dev/null +++ b/dev/plans/260714-preparing-acquire-recovery.md @@ -0,0 +1,107 @@ +# Recover Crash-Stuck Preparing Acquires + +## Goal + +Make the documented `repair({ action: "resume-acquire" })` path recover a lease +persisted in `preparing`, not only one quarantined after a caught failure. Preserve +the original checkout intent and existing branch-reuse behavior while adding the +minimum durable information needed to decide whether `postCreate` is still due. +Baseline: `38df31ba10bd06d1c48fe5cd20f0023743225840`. + +Acceptance semantics: + +- A `preparing` or `quarantined` lease with `pendingAcquire` can resume; other + states or missing intent still return `REPAIR_NOT_AVAILABLE`. +- Every new acquire explicitly persists `pendingAcquire.postCreatePending`: + `true` for a newly allocated physical slot, `false` for a reused slot. +- The marker stays `true` through materialization and hook execution, then is + durably set to `false` before checkout. A crash during the hook, or after its + side effects but before that state write, therefore retries `postCreate`. + This is intentionally at-least-once, not exactly-once. +- A present worktree with an explicit `false` marker never runs `postCreate`. + A missing worktree overrides the marker to `true`, because repair must create + a new physical worktree before continuing. +- Older persisted intents without the additive marker remain valid. On repair, + missing path or `diagnostics.failedPhase === "postCreate"` means pending; + otherwise an existing path means complete/not-required. This conservative + fallback avoids running `postCreate` on a legacy reused slot; an old, + ambiguous new-slot crash with an existing path cannot be distinguished and + also skips the hook. + +## Changes + +1. `packages/grove/src/schemas.ts:PendingAcquireSchema` and + `packages/grove/src/target.ts:buildPendingAcquire` — add optional boolean + `postCreatePending`. Keep it optional solely for persisted-state and exported + type compatibility; do not apply a Zod default because repair must distinguish + an old missing value from an explicit `false`. Require all newly built intents + to supply the boolean. + +2. `packages/grove/src/transitions.ts:LeaseEvent` and `transitionLease` — keep + lease mutations transition-owned. Allow `REPAIR_RESUME_ACQUIRE` from either + `quarantined` or `preparing`; retain its `pendingAcquire` guard and accept an + optional `postCreatePending` payload so existing external event construction + remains source-compatible while repair can normalize legacy intent. Add one + `ACQUIRE_POST_CREATE_COMPLETE` event that is valid only for a `preparing` + lease with pending post-create work and replaces the nested marker with + `false`. Do not change slot transitions: quarantined recovery still uses + `REPAIR_RESUME_LEASE`; an already-preparing lease already owns a leased slot. + +3. `packages/grove/src/lease-acquire.ts:acquireLease` — build `pendingAcquire` + after `findOrAllocateSlot()` reveals `isNew`, and persist the explicit marker + in the same write-ahead state save as the preparing lease. After a new slot's + `postCreate` step succeeds (including the no-hook case), persist + `ACQUIRE_POST_CREATE_COMPLETE` under the state lock before checkout. Leave + the marker true when materialization or `postCreate` fails so existing + quarantine-and-repair behavior retries the unfinished step; checkout failures + occur after the marker is false and must not rerun it. + +4. `packages/grove/src/lease-acquire.ts:resumeAcquireLease` — mirror + `resumeCleanupLease` state handling: transition quarantined records back to + preparing, accept preparing records directly, reject all others. While + holding the initial lock, derive and persist the repair marker in this exact + order: missing worktree => `true`; otherwise explicit marker => use it; + otherwise legacy `failedPhase: "postCreate"` => `true`; otherwise `false`. + Persist that decision through `REPAIR_RESUME_ACQUIRE` before materialization + or hooks, run/complete `postCreate` only when true, then use the existing + checkout/finalization flow. Keep `targetToAcquireOptions()` recovery semantics: + a pending create-from branch uses `ifExists: "reuse"`, allowing a branch + created before the crash to finish acquisition without weakening normal + acquire's fail-first policy. + +5. Test first with real Git fixtures: + + - `packages/grove/test/lease-repair.integration.test.ts` — seed valid + write-ahead crash snapshots and prove public `repair()` directly completes + a `preparing` lease; retries `postCreate` when the persisted marker is true; + and, for an actually reused physical slot with a false marker, skips a + failing `postCreate` hook while reusing a branch created before the crash. + Assert final leased target/head data and cleared `pendingAcquire`. + - `packages/grove/test/lease-hooks.integration.test.ts` — strengthen the + existing failed-`postCreate` case to assert the pending marker remains true + through quarantine and the existing repair retry still succeeds. + - `packages/grove/test/transitions.test.ts` — cover quarantined-to-preparing + and preparing-to-preparing repair, marker normalization, post-create + completion, missing intent, and invalid completion states. + - `packages/grove/test/state-v1.test.ts` — prove a pre-change preparing record + without `postCreatePending` still parses. No legacy worktree-shape migration + or state version is needed. + +6. `README.md:Lease states` and `README.md:Lifecycle hooks` — retain the existing + preparing repair instruction, now backed by behavior. Clarify that crash + recovery may retry `postCreate`, so commands requiring recovery safety must be + idempotent; reused physical slots do not run it. + +## Verify + +- `pnpm exec vitest run packages/grove/test/lease-repair.integration.test.ts packages/grove/test/lease-hooks.integration.test.ts packages/grove/test/transitions.test.ts packages/grove/test/state-v1.test.ts` +- `pnpm check` + +## Boundaries + +- No broader acquire phase enum or new state machine; one pending boolean is + sufficient for the only ambiguous side effect. +- No exactly-once hook guarantee, subprocess recovery, active-owner/liveness + policy, `postAcquire` replay, ref-pinning change, branch-ownership fix, CLI + change, or state-file version bump. +- No timestamp/path-layout inference for ambiguous legacy records. diff --git a/dev/plans/260714-process-scan-failure.md b/dev/plans/260714-process-scan-failure.md new file mode 100644 index 0000000..0aca677 --- /dev/null +++ b/dev/plans/260714-process-scan-failure.md @@ -0,0 +1,22 @@ +# Treat failed macOS process scans as unverified + +## Goal + +Restore the process-safety contract in `VISION.md` and `README.md`: a failed or unavailable macOS `lsof` scan reports `unverified`, so destructive cleanup without `force: true` stops before mutating lease state. Today `findInWorktree()` uses Execa with `reject: false` and parses failed results as a verified empty scan. + +Acceptance: failed spawn and nonzero `lsof` results produce `{ processes: [], unverified: true }`; successful output keeps the current parsing behavior; existing forced cleanup behavior remains unchanged. + +## Changes + +1. `packages/grove/test/lease-release.integration.test.ts` — test first with one focused regression using the real SDK and Git fixture. Acquire a lease, release with `preserve` to clear its owner reservation, then temporarily set `process.platform` to `darwin` and `PATH` to a nonexistent temporary directory inside a `try/finally`. Assert the real `findInWorktree()` result is unverified, a reset release with explicit `resetTo: "main"` fails with `UNSAFE_CLEANUP`, and after restoring both globals the lease is still `leased` and its path still exists. This deterministically exercises unavailable `lsof` on Linux and macOS CI, avoids mocking Git or Execa, and proves cleanup stopped before the write-ahead transition. Keep the existing force-bypass coverage rather than duplicating it. +2. `packages/grove/src/process/detect.ts:findInWorktree` — in the Darwin branch, retain the full Execa result and return the existing empty/unverified result immediately when `result.failed` is true; parse `stdout` only on success. Execa's `failed` flag covers both spawn failures such as `ENOENT` and nonzero exits, avoiding separate status/error branches or a new abstraction. Keep the catch as the fallback for thrown scan errors. + +## Verify + +- `pnpm vitest run packages/grove/test/lease-release.integration.test.ts` +- `pnpm check` + +## Boundaries + +- No Linux `/proc` scan redesign, process-detector refactor, new error type, schema/API change, or documentation change. +- Do not preserve or parse partial `lsof` output from a failed command; its completeness is unverified. diff --git a/dev/plans/260714-repair-force-propagation.md b/dev/plans/260714-repair-force-propagation.md new file mode 100644 index 0000000..6ea270e --- /dev/null +++ b/dev/plans/260714-repair-force-propagation.md @@ -0,0 +1,20 @@ +# Preserve repair force intent through destroy + +## Goal + +Make `repair({ action: "force-destroy" })` honor the caller's optional `force` value for the entire destroy operation. A process that appears during `preDestroy` must make the fresh pre-removal scan fail with `UNSAFE_CLEANUP` when force was omitted or false; explicit `force: true` must remain the only override. Preserve the current destroy/quarantine lifecycle, API, and error contracts with the smallest data-flow correction. + +## Changes + +1. `packages/grove/test/lease-repair.integration.test.ts` — test first by expanding the existing force-destroy integration case into the regression and positive path. Acquire a real worktree, release it with `cleanup: "quarantine"` through the SDK so its owner reservation is cleared, then call exported `repairLease` without force and inject a `preDestroy` callback that starts a long-running Node child with the worktree as its CWD. Wait for the child to become visible using the established process-test timing pattern, assert the fresh scan rejects with `UNSAFE_CLEANUP`, the worktree still exists, and the failed destroy leaves the lease quarantined. Keep that hook-created child alive, retry through public `grove.repair` with `force: true`, and retain the existing destroyed result, missing lease, and removed-path assertions. Always kill and await the child in `finally` so failures cannot leak a process or race fixture cleanup. This single real-git case proves both sides of the contract without state-file mutation or mocked Git. +2. `packages/grove/src/lease-repair.ts:repairForceDestroy` — pass the existing `options` object directly to `destroyLease` instead of replacing it with `{ force: true }`, and remove the now-unused `DestroyLeaseOptions` type import. Keep the repair preflight, `destroyLease`'s begin scan, its post-hook fresh scan, hook wiring, and failure quarantine unchanged; `destroyLease` will carry the same caller value in `DestroyContext.force` to the final safety check. + +## Verify + +- `pnpm vitest run packages/grove/test/lease-repair.integration.test.ts` +- `pnpm check` + +## Boundaries + +- No changes to destroy/process-safety algorithms, transitions, schemas, public types, CLI behavior, docs, or error classes. +- Do not rename `force-destroy`, remove existing safety scans, or add a second force option; this fix only stops repair from manufacturing force authorization. diff --git a/dev/plans/260714-repository-pool-identity.md b/dev/plans/260714-repository-pool-identity.md new file mode 100644 index 0000000..67b14f6 --- /dev/null +++ b/dev/plans/260714-repository-pool-identity.md @@ -0,0 +1,21 @@ +# Key default pools by local repository path + +## Goal + +Ensure an implicitly named Grove pool belongs to one local repository checkout. Two clones with the same directory basename and `origin` URL must resolve to different pool directories, preventing one clone from reading or mutating the other clone's lease state. Keep the fix local to pool-name resolution: use the normalized absolute `repoRoot` as the identity, preserve configured pool placement, and leave explicit `groveDir` ownership unchanged. + +## Changes + +1. `packages/grove/test/config.test.ts:Config` — add the regression first using real Git: create two clone directories named `repo` that share one remote, then assert `resolveGroveDir()` returns different default directories. Reuse `setupRepo()` for the origin and first checkout, clone only the second checkout inside the same temporary fixture, and avoid a broader pool lifecycle test because directory separation is the highest seam that proves the invariant. +2. `packages/grove/src/config.ts:resolveGroveDir` — remove remote URL lookup from pool identity. Normalize the documented absolute repository input with `node:path.resolve`, then derive both the readable basename and existing short hash from that local path alone. Continue resolving `groveRoot` and environment expansion exactly as today. Do not add filesystem `realpath`, repository metadata, or a new state field: those add I/O and failure modes without improving separation between distinct checkout paths. +3. Treat the resulting pool-name change as a direct cutover. Do not probe, adopt, move, or delete the old remote-keyed directory because it may already be shared by multiple clones. Existing data remains untouched; a caller that intentionally needs an old pool can address it explicitly through `groveDir`. + +## Verify + +- `pnpm vitest run packages/grove/test/config.test.ts` +- `pnpm check` + +## Boundaries + +- No pool-state schema change, migration machinery, remote identity fallback, or cross-repository redesign. +- No change to explicit `groveDir`, `groveRoot` placement semantics, or the existing pool-name shape beyond its hash input. diff --git a/dev/plans/README.md b/dev/plans/README.md new file mode 100644 index 0000000..1b8d98f --- /dev/null +++ b/dev/plans/README.md @@ -0,0 +1,7 @@ +# Implementation plans + +- [Treat failed macOS process scans as unverified](260714-process-scan-failure.md) +- [Preserve repair force intent through destroy](260714-repair-force-propagation.md) +- [Recover crash-stuck preparing acquires](260714-preparing-acquire-recovery.md) +- [Key default pools by local repository path](260714-repository-pool-identity.md) +- [Constrain destroy to its owned slot directory](260714-destroy-path-layout.md) diff --git a/packages/grove/src/config.ts b/packages/grove/src/config.ts index 7bd1bc9..39ae92b 100644 --- a/packages/grove/src/config.ts +++ b/packages/grove/src/config.ts @@ -1,6 +1,6 @@ -import { join, basename, isAbsolute } from "node:path"; +import { join, basename, isAbsolute, resolve } from "node:path"; import { homedir } from "node:os"; -import { getRemoteUrl, shortHash } from "./git/index.js"; +import { shortHash } from "./git/index.js"; function expandEnv(str: string): string { return str.replace(/\$(?:{([A-Za-z_][A-Za-z0-9_]*)}|([A-Za-z_][A-Za-z0-9_]*))/g, (_, n1, n2) => { @@ -10,13 +10,9 @@ function expandEnv(str: string): string { } export async function resolveGroveDir(repoRoot: string, root?: string): Promise { - let hashInput = repoRoot; - try { - hashInput = await getRemoteUrl(repoRoot); - } catch {} - - const repoName = basename(repoRoot); - const hash = shortHash(hashInput); + const normalizedRepoRoot = resolve(repoRoot); + const repoName = basename(normalizedRepoRoot); + const hash = shortHash(normalizedRepoRoot); const poolName = `${repoName}-${hash}`; if (!root) { diff --git a/packages/grove/src/lease-acquire.ts b/packages/grove/src/lease-acquire.ts index a44e9b3..8d1defc 100644 --- a/packages/grove/src/lease-acquire.ts +++ b/packages/grove/src/lease-acquire.ts @@ -85,6 +85,27 @@ export async function finalizeLeaseCheckout( return enrichLeaseReadOnly(lease); } +async function persistPostCreatePending( + poolDir: string, + repoRoot: string, + leaseId: string, + postCreatePending: boolean, +): Promise { + await withStateLock(poolDir, async () => { + const state = await loadPoolState(poolDir, repoRoot); + const lease = findLease(state, leaseId); + if (!lease) { + throw new LeaseNotFoundError(`Lease ${leaseId} not found after postCreate`); + } + + const leaseIndex = state.leases.findIndex((entry) => entry.leaseId === leaseId); + state.leases[leaseIndex] = postCreatePending + ? transitionLease(lease, { type: "REPAIR_RESUME_ACQUIRE", postCreatePending: true })! + : transitionLease(lease, { type: "ACQUIRE_POST_CREATE_COMPLETE" })!; + await savePoolState(poolDir, state); + }); +} + export async function quarantineFailedAcquire( poolDir: string, repoRoot: string, @@ -127,7 +148,6 @@ export async function acquireLease( const pendingTarget = await buildAcquireTarget(options, repoRoot); const now = new Date().toISOString(); - const pendingAcquire = buildPendingAcquire(pendingTarget, now); let targetWtPath = ""; let isNewSlot = false; @@ -184,6 +204,7 @@ export async function acquireLease( } const { slot, isNew } = await findOrAllocateSlot(state, poolDir, config); + const pendingAcquire = buildPendingAcquire(pendingTarget, now, isNew); const reservedSlot = transitionSlot(slot, { type: "RESERVE_FOR_LEASE" }, now)!; const slotIndex = state.slots.findIndex((entry) => entry.slotName === slot.slotName); @@ -238,6 +259,7 @@ export async function acquireLease( await quarantineFailedAcquire(poolDir, repoRoot, leaseIdForCheckout, reason, "postCreate"); throw err; } + await persistPostCreatePending(poolDir, repoRoot, leaseIdForCheckout, false); } try { @@ -286,14 +308,21 @@ export async function resumeAcquireLease( if (!lease.pendingAcquire) { throw new RepairNotAvailableError("resume-acquire requires pendingAcquire"); } - if (lease.state !== "quarantined") { + if (lease.state !== "quarantined" && lease.state !== "preparing") { throw new RepairNotAvailableError( - `resume-acquire requires quarantined lease, got ${lease.state}`, + `resume-acquire requires quarantined or preparing lease, got ${lease.state}`, ); } + const postCreatePending = + !existsSync(lease.path) || + (lease.pendingAcquire.postCreatePending ?? lease.diagnostics?.failedPhase === "postCreate"); + const leaseIndex = state.leases.findIndex((entry) => entry.leaseId === leaseId); - state.leases[leaseIndex] = transitionLease(lease, { type: "REPAIR_RESUME_ACQUIRE" })!; + state.leases[leaseIndex] = transitionLease(lease, { + type: "REPAIR_RESUME_ACQUIRE", + postCreatePending, + })!; const slot = findSlot(state, lease.slotName); if (slot && slot.state === "quarantined") { @@ -304,7 +333,7 @@ export async function resumeAcquireLease( await savePoolState(poolDir, state); wtPath = lease.path; slotName = lease.slotName; - runPostCreate = lease.diagnostics?.failedPhase === "postCreate"; + runPostCreate = postCreatePending; pendingTarget = lease.pendingAcquire.target; }); @@ -312,6 +341,7 @@ export async function resumeAcquireLease( let lease: GroveLease; if (!existsSync(wtPath)) { try { + await persistPostCreatePending(poolDir, repoRoot, leaseId, true); const state = await loadPoolState(poolDir, repoRoot, { heal: false }); const slot = findSlot(state, slotName); if (!slot) { @@ -333,6 +363,7 @@ export async function resumeAcquireLease( await quarantineFailedAcquire(poolDir, repoRoot, leaseId, reason, "postCreate"); throw err; } + await persistPostCreatePending(poolDir, repoRoot, leaseId, false); } try { diff --git a/packages/grove/src/lease-destroy.ts b/packages/grove/src/lease-destroy.ts index e1b87ce..84665b3 100644 --- a/packages/grove/src/lease-destroy.ts +++ b/packages/grove/src/lease-destroy.ts @@ -1,4 +1,4 @@ -import { dirname } from "node:path"; +import { basename, dirname, normalize } from "node:path"; import { rm } from "node:fs/promises"; import type { GroveConfig, GroveLeaseRecord, GroveSlot } from "./schemas.js"; import type { DestroyLeaseOptions } from "./types.js"; @@ -7,7 +7,12 @@ import { withStateLock } from "./lock.js"; import { assertPathWithinPool } from "./path-boundary.js"; import { isWorktreeInUse } from "./process/detect.js"; import { assertWorktreeSafeForCleanup } from "./process/cleanup-safety.js"; -import { InvalidInputError, LeaseBusyError, LeaseNotFoundError } from "./errors.js"; +import { + InvalidInputError, + LeaseBusyError, + LeaseNotFoundError, + PathOutsidePoolError, +} from "./errors.js"; import { buildLeaseHookEnv, recordToGroveLease } from "./lease-view.js"; import { findLease, @@ -39,6 +44,19 @@ function assertDeleteBranchNotRequested(options?: DestroyLeaseOptions): void { } } +function destroySlotDirectory(poolDir: string, wtPath: string, slotName: string): string { + const normalizedPool = normalize(poolDir); + const slotDir = dirname(normalize(wtPath)); + if ( + slotDir === normalizedPool || + dirname(slotDir) !== normalizedPool || + basename(slotDir) !== slotName + ) { + throw new PathOutsidePoolError("Worktree is not inside its owned pool slot directory"); + } + return slotDir; +} + function assertLeaseDestroyable( lease: { leaseId: string; state: string }, resuming: boolean, @@ -254,8 +272,9 @@ async function completeDestroy( }); await assertPathWithinPool(poolDir, wtPath); + const slotDir = destroySlotDirectory(poolDir, wtPath, slot.slotName); await removeWorktree(config.repoRoot, wtPath); - await rm(dirname(wtPath), { recursive: true, force: true }); + await rm(slotDir, { recursive: true, force: true }); } catch (err) { const reason = err instanceof Error ? err.message : "destroy failed"; await quarantineFailedDestroy(poolDir, config.repoRoot, context.leaseId, reason); diff --git a/packages/grove/src/lease-repair.ts b/packages/grove/src/lease-repair.ts index fab7b44..0748773 100644 --- a/packages/grove/src/lease-repair.ts +++ b/packages/grove/src/lease-repair.ts @@ -1,6 +1,5 @@ import type { GroveConfig } from "./schemas.js"; import type { - DestroyLeaseOptions, GroveLease, ReleaseResult, RepairLeaseOptions, @@ -79,9 +78,8 @@ async function repairForceDestroy( } }); - const destroyOptions: DestroyLeaseOptions = { force: true }; const destroyHooks = hooks.preDestroy ? { preDestroy: hooks.preDestroy } : {}; - await destroyLease(poolDir, config, leaseId, destroyOptions, destroyHooks); + await destroyLease(poolDir, config, leaseId, options, destroyHooks); return { status: "destroyed", leaseId }; } diff --git a/packages/grove/src/process/detect.ts b/packages/grove/src/process/detect.ts index 63cdf35..959368f 100644 --- a/packages/grove/src/process/detect.ts +++ b/packages/grove/src/process/detect.ts @@ -130,8 +130,11 @@ export async function findInWorktree(worktreePath: string): Promise; export const PendingAcquireSchema = z.object({ target: GroveLeaseTargetSchema, startedAt: z.string(), + postCreatePending: z.boolean().optional(), }); export type PendingAcquire = z.infer; diff --git a/packages/grove/src/target.ts b/packages/grove/src/target.ts index 3ad0daf..671e0ca 100644 --- a/packages/grove/src/target.ts +++ b/packages/grove/src/target.ts @@ -74,8 +74,12 @@ export async function buildAcquireTarget( } } -export function buildPendingAcquire(target: GroveLeaseTarget, startedAt: string): PendingAcquire { - return { target, startedAt }; +export function buildPendingAcquire( + target: GroveLeaseTarget, + startedAt: string, + postCreatePending: boolean, +): PendingAcquire { + return { target, startedAt, postCreatePending }; } export function finalizeBranchTarget(target: GroveLeaseTarget, headSha: string): GroveLeaseTarget { diff --git a/packages/grove/src/transitions.ts b/packages/grove/src/transitions.ts index 6df0426..4e9f7bb 100644 --- a/packages/grove/src/transitions.ts +++ b/packages/grove/src/transitions.ts @@ -16,6 +16,7 @@ import { export type LeaseEvent = | { type: "ACQUIRE_COMPLETE"; target: GroveLeaseTarget; headSha: string } | { type: "ACQUIRE_FAILED"; reason: string; failedPhase?: GroveFailedPhase } + | { type: "ACQUIRE_POST_CREATE_COMPLETE" } | { type: "RELEASE_START"; cleanup: LeaseFirstCleanupIntent } | { type: "RELEASE_PRESERVE_COMPLETE" } | { type: "RELEASE_RESET_COMPLETE" } @@ -24,7 +25,7 @@ export type LeaseEvent = | { type: "DESTROY_START" } | { type: "DESTROY_COMPLETE" } | { type: "DESTROY_FAILED"; reason: string } - | { type: "REPAIR_RESUME_ACQUIRE" } + | { type: "REPAIR_RESUME_ACQUIRE"; postCreatePending?: boolean } | { type: "REPAIR_RESUME_CLEANUP" }; export type SlotEvent = @@ -81,6 +82,21 @@ export function transitionLease( diagnostics: quarantineDiagnostics(lease, event.reason, event.failedPhase), }; } + case "ACQUIRE_POST_CREATE_COMPLETE": { + if ( + lease.state !== "preparing" || + !lease.pendingAcquire || + lease.pendingAcquire.postCreatePending !== true + ) { + throw new InvalidTransitionError( + `ACQUIRE_POST_CREATE_COMPLETE invalid from lease state ${lease.state}`, + ); + } + return { + ...base, + pendingAcquire: { ...lease.pendingAcquire, postCreatePending: false }, + }; + } case "RELEASE_START": { if (lease.state !== "leased") { throw new InvalidTransitionError(`RELEASE_START invalid from lease state ${lease.state}`); @@ -172,7 +188,7 @@ export function transitionLease( }; } case "REPAIR_RESUME_ACQUIRE": { - if (lease.state !== "quarantined") { + if (lease.state !== "quarantined" && lease.state !== "preparing") { throw new InvalidTransitionError( `REPAIR_RESUME_ACQUIRE invalid from lease state ${lease.state}`, ); @@ -180,7 +196,18 @@ export function transitionLease( if (!lease.pendingAcquire) { throw new RepairNotAvailableError("resume-acquire requires pendingAcquire"); } - return { ...base, state: "preparing" }; + return { + ...base, + state: "preparing", + ...(event.postCreatePending === undefined + ? {} + : { + pendingAcquire: { + ...lease.pendingAcquire, + postCreatePending: event.postCreatePending, + }, + }), + }; } case "REPAIR_RESUME_CLEANUP": { if (lease.state !== "quarantined") { diff --git a/packages/grove/test/config.test.ts b/packages/grove/test/config.test.ts index 415955a..fb680ff 100644 --- a/packages/grove/test/config.test.ts +++ b/packages/grove/test/config.test.ts @@ -1,9 +1,10 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import { resolveGroveDir } from "../src/config.js"; -import { setupPathFixture } from "./helpers/git-repo.js"; -import { rm } from "node:fs/promises"; +import { setupPathFixture, setupRepo } from "./helpers/git-repo.js"; +import { mkdir, rm } from "node:fs/promises"; import { join, basename } from "node:path"; import { homedir } from "node:os"; +import { execa } from "execa"; describe("Config", () => { let tmpDirs: string[] = []; @@ -79,4 +80,15 @@ describe("Config", () => { delete process.env["TEST_GROVE_ROOT"]; delete process.env["var2"]; }); + + it("uses distinct pools for same-named clones of one remote", async () => { + const { repoDir, remoteDir, tmpDir } = await setupRepo(); + tmpDirs.push(tmpDir); + const secondParent = join(tmpDir, "second"); + const secondRepoDir = join(secondParent, basename(repoDir)); + await mkdir(secondParent, { recursive: true }); + await execa("git", ["clone", remoteDir, secondRepoDir]); + + await expect(resolveGroveDir(repoDir)).resolves.not.toBe(await resolveGroveDir(secondRepoDir)); + }); }); diff --git a/packages/grove/test/lease-destroy.integration.test.ts b/packages/grove/test/lease-destroy.integration.test.ts index f5f3081..d190663 100644 --- a/packages/grove/test/lease-destroy.integration.test.ts +++ b/packages/grove/test/lease-destroy.integration.test.ts @@ -6,6 +6,7 @@ import { join } from "node:path"; import { createTestGrove } from "./helpers/test-grove.js"; import { setupRepo } from "./helpers/git-repo.js"; import { registerLeaseIntegrationCleanup } from "./helpers/lease-integration.js"; +import { readLeaseFirstState, writeLeaseFirstState } from "../src/state-v1.js"; describe("lease destroy integration", () => { const cleanup = registerLeaseIntegrationCleanup(); @@ -57,6 +58,36 @@ describe("lease destroy integration", () => { expect(quarantined?.state).toBe("quarantined"); }); + it("destroy rejects a worktree whose parent is the pool root", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const lease = await grove.acquire({ + leaseId: "shallow-layout-lease", + mode: "branch", + branch: "shallow-layout-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + + const shallowPath = join(grove.poolDir, "repo"); + await execa("git", ["worktree", "move", lease.path, shallowPath], { cwd: repoDir }); + const state = await readLeaseFirstState(grove.poolDir); + state.slots[0]!.path = shallowPath; + state.leases[0]!.path = shallowPath; + await writeLeaseFirstState(grove.poolDir, state); + + const sentinelPath = join(grove.poolDir, "sentinel.txt"); + await writeFile(sentinelPath, "keep"); + + await expect(grove.destroy(lease.leaseId, { force: true })).rejects.toMatchObject({ + code: "PATH_OUTSIDE_POOL", + }); + expect(existsSync(sentinelPath)).toBe(true); + expect(existsSync(shallowPath)).toBe(true); + expect(await grove.inspect(lease.leaseId)).toMatchObject({ state: "quarantined" }); + }); + it("idempotent destroy resumes an in-progress destroying lease", async () => { const { repoDir, tmpDir, groveDir } = await setupRepo(); cleanup.tmpDirs.push(tmpDir); diff --git a/packages/grove/test/lease-hooks.integration.test.ts b/packages/grove/test/lease-hooks.integration.test.ts index 6a3d198..40c7341 100644 --- a/packages/grove/test/lease-hooks.integration.test.ts +++ b/packages/grove/test/lease-hooks.integration.test.ts @@ -64,7 +64,7 @@ describe("lease hooks integration", () => { expect(lease).toMatchObject({ leaseId: "post-create-fail", state: "quarantined", - pendingAcquire: expect.anything(), + pendingAcquire: expect.objectContaining({ postCreatePending: true }), diagnostics: { failedPhase: "postCreate" }, }); }); diff --git a/packages/grove/test/lease-release.integration.test.ts b/packages/grove/test/lease-release.integration.test.ts index 4881100..5e21145 100644 --- a/packages/grove/test/lease-release.integration.test.ts +++ b/packages/grove/test/lease-release.integration.test.ts @@ -6,6 +6,7 @@ import { join } from "node:path"; import { createTestGrove } from "./helpers/test-grove.js"; import { setupRepo } from "./helpers/git-repo.js"; import { registerLeaseIntegrationCleanup } from "./helpers/lease-integration.js"; +import { findInWorktree } from "../src/process/detect.js"; describe("lease release integration", () => { const cleanup = registerLeaseIntegrationCleanup(); @@ -44,6 +45,48 @@ describe("lease release integration", () => { } }); + it("treats an unavailable macOS process scan as unverified", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const lease = await grove.acquire({ + leaseId: "unverified-scan", + mode: "branch", + branch: "unverified-scan-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + await grove.release(lease.leaseId, { cleanup: "preserve" }); + + const originalPlatform = process.platform; + const originalPath = process.env.PATH; + Object.defineProperty(process, "platform", { value: "darwin", configurable: true }); + process.env.PATH = join(tmpDir, "missing-bin"); + + try { + await expect(findInWorktree(lease.path)).resolves.toEqual({ + processes: [], + unverified: true, + }); + await expect( + grove.release(lease.leaseId, { cleanup: "reset", resetTo: "main" }), + ).rejects.toMatchObject({ code: "UNSAFE_CLEANUP" }); + } finally { + Object.defineProperty(process, "platform", { + value: originalPlatform, + configurable: true, + }); + if (originalPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = originalPath; + } + } + + expect(await grove.inspect(lease.leaseId)).toMatchObject({ state: "leased" }); + expect(existsSync(lease.path)).toBe(true); + }); + it("preserve release keeps dirty files and returns preserved lease", async () => { const { repoDir, tmpDir, groveDir } = await setupRepo(); cleanup.tmpDirs.push(tmpDir); diff --git a/packages/grove/test/lease-repair.integration.test.ts b/packages/grove/test/lease-repair.integration.test.ts index ec1a918..0df3c39 100644 --- a/packages/grove/test/lease-repair.integration.test.ts +++ b/packages/grove/test/lease-repair.integration.test.ts @@ -6,6 +6,22 @@ import { join } from "node:path"; import { createTestGrove } from "./helpers/test-grove.js"; import { setupRepo } from "./helpers/git-repo.js"; import { registerLeaseIntegrationCleanup } from "./helpers/lease-integration.js"; +import { repairLease } from "../src/lease-repair.js"; + +async function markLeasePreparing(statePath: string, postCreatePending: boolean): Promise { + const state = JSON.parse(await readFile(statePath, "utf8")); + const lease = state.leases[0]; + lease.pendingAcquire = { + target: lease.target, + startedAt: lease.updatedAt, + postCreatePending, + }; + lease.state = "preparing"; + delete lease.target; + delete lease.acquiredHeadSha; + delete lease.currentHeadSha; + await writeFile(statePath, JSON.stringify(state)); +} describe("lease repair integration", () => { const cleanup = registerLeaseIntegrationCleanup(); @@ -259,6 +275,129 @@ describe("lease repair integration", () => { expect(existsSync(lease.path)).toBe(false); }); + it("repair force-destroy preserves caller force intent after preDestroy", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const lease = await grove.acquire({ + leaseId: "force-intent-lease", + mode: "branch", + branch: "force-intent-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + await grove.release(lease.leaseId, { cleanup: "quarantine" }); + + let stopChild = async (): Promise => {}; + try { + await expect( + repairLease( + grove.poolDir, + { repoRoot: repoDir }, + { leaseId: lease.leaseId, action: "force-destroy" }, + { + preDestroy: async (wtPath) => { + const child = execa("node", ["-e", "setInterval(() => {}, 1000)"], { + cwd: wtPath, + }); + stopChild = async () => { + child.kill(); + await child.catch(() => {}); + }; + await new Promise((resolve) => setTimeout(resolve, 500)); + }, + }, + ), + ).rejects.toMatchObject({ code: "UNSAFE_CLEANUP" }); + + expect(await grove.inspect(lease.leaseId)).toMatchObject({ state: "quarantined" }); + expect(existsSync(lease.path)).toBe(true); + + const result = await grove.repair({ + leaseId: lease.leaseId, + action: "force-destroy", + force: true, + }); + expect(result).toEqual({ status: "destroyed", leaseId: lease.leaseId }); + expect(await grove.inspect(lease.leaseId)).toBeNull(); + expect(existsSync(lease.path)).toBe(false); + } finally { + await stopChild(); + } + }); + + it("repair resume-acquire completes a preparing lease without replaying postCreate", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const firstLease = await grove.acquire({ + leaseId: "preparing-slot-source", + mode: "detached", + ref: "main", + }); + await grove.release(firstLease.leaseId, { + cleanup: "reset", + resetTo: "main", + force: true, + }); + + const lease = await grove.acquire({ + leaseId: "preparing-resume", + mode: "branch", + branch: "preparing-resume-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + expect(lease.slotName).toBe(firstLease.slotName); + await markLeasePreparing(join(grove.poolDir, "grove-state.json"), false); + + let postCreateRuns = 0; + const repaired = await repairLease( + grove.poolDir, + { repoRoot: repoDir }, + { leaseId: lease.leaseId, action: "resume-acquire" }, + { postCreate: async () => void postCreateRuns++ }, + ); + + expect(repaired).toMatchObject({ + state: "leased", + branch: "preparing-resume-branch", + pendingAcquire: undefined, + }); + expect(postCreateRuns).toBe(0); + }); + + it("repair resume-acquire recreates a missing worktree and replays postCreate", async () => { + const { repoDir, tmpDir, groveDir } = await setupRepo(); + cleanup.tmpDirs.push(tmpDir); + + const grove = await createTestGrove({ repoRoot: repoDir, groveRoot: groveDir }); + const lease = await grove.acquire({ + leaseId: "preparing-post-create", + mode: "branch", + branch: "preparing-post-create-branch", + createBranch: { from: "main", ifExists: "fail" }, + }); + await execa("git", ["worktree", "remove", "--force", lease.path], { cwd: repoDir }); + await markLeasePreparing(join(grove.poolDir, "grove-state.json"), false); + + let postCreateRuns = 0; + const repaired = await repairLease( + grove.poolDir, + { repoRoot: repoDir }, + { leaseId: lease.leaseId, action: "resume-acquire" }, + { postCreate: async () => void postCreateRuns++ }, + ); + + expect(repaired).toMatchObject({ + state: "leased", + branch: "preparing-post-create-branch", + pendingAcquire: undefined, + }); + expect(postCreateRuns).toBe(1); + expect(existsSync(lease.path)).toBe(true); + }); + it("repair resume-acquire completes a quarantined pending acquire", async () => { const { repoDir, tmpDir, groveDir } = await setupRepo(); cleanup.tmpDirs.push(tmpDir); diff --git a/packages/grove/test/state-v1.test.ts b/packages/grove/test/state-v1.test.ts index ded8b8e..64d1dd9 100644 --- a/packages/grove/test/state-v1.test.ts +++ b/packages/grove/test/state-v1.test.ts @@ -74,6 +74,45 @@ describe("Lease-first state", () => { expect(parsed.slots[0]?.state).toBe("leased"); }); + it.each([ + { name: "pre-change intent", postCreatePending: undefined }, + { name: "intent with pending postCreate", postCreatePending: true }, + ])("parses $name", ({ postCreatePending }) => { + const parsed = parseLeaseFirstState({ + slots: [ + { + slotName: "slot-1", + path: "/pool/slot-1", + state: "leased", + createdAt: NOW, + updatedAt: NOW, + }, + ], + leases: [ + { + leaseId: "job-1", + slotName: "slot-1", + path: "/pool/slot-1", + repoRoot: "/repo", + state: "preparing", + pendingAcquire: { + target: { + mode: "detached", + requestedRef: "main", + resolvedRefSha: "abc123", + }, + startedAt: NOW, + ...(postCreatePending === undefined ? {} : { postCreatePending }), + }, + createdAt: NOW, + updatedAt: NOW, + }, + ], + }); + + expect(parsed.leases[0]?.pendingAcquire?.postCreatePending).toBe(postCreatePending); + }); + it("migrates legacy worktrees state", () => { const legacy: GroveState = { worktrees: [ diff --git a/packages/grove/test/transitions.test.ts b/packages/grove/test/transitions.test.ts index cfe15d3..ff605f8 100644 --- a/packages/grove/test/transitions.test.ts +++ b/packages/grove/test/transitions.test.ts @@ -206,8 +206,46 @@ describe("transitionLease", () => { state: "quarantined", pendingAcquire: { target: TARGET, startedAt: NOW }, }); - const next = transitionLease(lease, { type: "REPAIR_RESUME_ACQUIRE" }, NOW); + const next = transitionLease( + lease, + { type: "REPAIR_RESUME_ACQUIRE", postCreatePending: false }, + NOW, + ); expect(next?.state).toBe("preparing"); + expect(next?.pendingAcquire?.postCreatePending).toBe(false); + }); + + it("preparing -> preparing on REPAIR_RESUME_ACQUIRE", () => { + const lease = createPreparingLease({ + leaseId: "job-1", + slotName: "slot-1", + path: "/pool/slot-1", + repoRoot: "/repo", + pendingAcquire: { target: TARGET, startedAt: NOW }, + now: NOW, + }); + + const next = transitionLease( + lease, + { type: "REPAIR_RESUME_ACQUIRE", postCreatePending: true }, + NOW, + ); + expect(next?.state).toBe("preparing"); + expect(next?.pendingAcquire?.postCreatePending).toBe(true); + }); + + it("records completed postCreate work", () => { + const lease = createPreparingLease({ + leaseId: "job-1", + slotName: "slot-1", + path: "/pool/slot-1", + repoRoot: "/repo", + pendingAcquire: { target: TARGET, startedAt: NOW, postCreatePending: true }, + now: NOW, + }); + + const next = transitionLease(lease, { type: "ACQUIRE_POST_CREATE_COMPLETE" }, NOW); + expect(next?.pendingAcquire?.postCreatePending).toBe(false); }); it("quarantined -> releasing on REPAIR_RESUME_CLEANUP when pendingCleanup exists", () => { @@ -291,6 +329,23 @@ describe("transitionLease", () => { }), event: { type: "DESTROY_START" }, }, + { + name: "ACQUIRE_POST_CREATE_COMPLETE from leased", + lease: leasedLease(), + event: { type: "ACQUIRE_POST_CREATE_COMPLETE" }, + }, + { + name: "ACQUIRE_POST_CREATE_COMPLETE without pending work", + lease: createPreparingLease({ + leaseId: "job-1", + slotName: "slot-1", + path: "/pool/slot-1", + repoRoot: "/repo", + pendingAcquire: { target: TARGET, startedAt: NOW, postCreatePending: false }, + now: NOW, + }), + event: { type: "ACQUIRE_POST_CREATE_COMPLETE" }, + }, { name: "RELEASE_PRESERVE_COMPLETE from leased", lease: leasedLease(), From 0c29895038024c944a27efc8714033f546bb34fb Mon Sep 17 00:00:00 2001 From: ferueda Date: Tue, 14 Jul 2026 16:44:26 -0700 Subject: [PATCH 2/2] chore: remove completed implementation plans --- dev/plans/260714-destroy-path-layout.md | 48 -------- .../260714-preparing-acquire-recovery.md | 107 ------------------ dev/plans/260714-process-scan-failure.md | 22 ---- dev/plans/260714-repair-force-propagation.md | 20 ---- dev/plans/260714-repository-pool-identity.md | 21 ---- dev/plans/README.md | 7 -- 6 files changed, 225 deletions(-) delete mode 100644 dev/plans/260714-destroy-path-layout.md delete mode 100644 dev/plans/260714-preparing-acquire-recovery.md delete mode 100644 dev/plans/260714-process-scan-failure.md delete mode 100644 dev/plans/260714-repair-force-propagation.md delete mode 100644 dev/plans/260714-repository-pool-identity.md delete mode 100644 dev/plans/README.md diff --git a/dev/plans/260714-destroy-path-layout.md b/dev/plans/260714-destroy-path-layout.md deleted file mode 100644 index e5695a1..0000000 --- a/dev/plans/260714-destroy-path-layout.md +++ /dev/null @@ -1,48 +0,0 @@ -# Constrain destroy to its owned slot directory - -## Goal - -Prevent `destroy()` from recursively deleting the pool root or unrelated pool contents when -persisted, schema-valid state gives a slot a shallow or malformed path. Before either Git or -filesystem removal, the directory passed to recursive `rm` must be a non-pool direct child of -`poolDir` whose basename equals the persisted `slotName`. Keep the existing -`PATH_OUTSIDE_POOL` error contract and transition-driven failure quarantine. Canonical -`poolDir//` worktrees must continue to destroy normally. - -## Changes - -1. `packages/grove/test/lease-destroy.integration.test.ts` — add the failing regression first, - through the public SDK/state seams and real Git: - - Acquire a normal lease, then use `git worktree move` to relocate its registered worktree to - the schema-valid shallow path `poolDir/`. - - Read the acquired state with `readLeaseFirstState`, update the matching slot and lease paths, - and persist it with `writeLeaseFirstState`. This proves the fixture passes the existing schema - and joint-invariant boundary instead of bypassing validation with raw JSON. - - Place an unrelated sentinel file in `poolDir`, call `grove.destroy(leaseId, { force: true })`, - and assert `PATH_OUTSIDE_POOL`, an intact sentinel, an intact shallow worktree, and a - quarantined lease. These assertions prove rejection happens before both `removeWorktree` and - recursive `rm`, while preserving destroy failure transitions. - - Keep the existing `idempotent destroy resumes an in-progress destroying lease` case green as - positive coverage for canonical slot layout, worktree removal, and state finalization; do not - add a duplicate happy-path fixture. - -2. `packages/grove/src/lease-destroy.ts:completeDestroy` — add one private, destroy-specific helper - that derives `slotDir = dirname(wtPath)` and returns it only when all three conditions hold: - `slotDir !== normalize(poolDir)`, `dirname(slotDir) === normalize(poolDir)`, and - `basename(slotDir) === slot.slotName`. Otherwise throw `PathOutsidePoolError` with a concise - slot-layout message. Run the existing `assertPathWithinPool(poolDir, wtPath)` first, then this - helper, before `removeWorktree`; pass the returned directory to `rm` instead of recomputing - `dirname(wtPath)`. The current `completeDestroy` catch must remain the owner of quarantine and - rethrow behavior. - -## Verify - -- `pnpm exec vitest run packages/grove/test/lease-destroy.integration.test.ts` -- `pnpm check` - -## Boundaries - -- No persisted-state schema tightening or migration: malformed historical state must remain - readable and recover as a quarantined destroy failure. -- No generalized filesystem/path framework, repo-basename validation, or changes to release; - only destroy recursively removes the slot parent. diff --git a/dev/plans/260714-preparing-acquire-recovery.md b/dev/plans/260714-preparing-acquire-recovery.md deleted file mode 100644 index 84e8fef..0000000 --- a/dev/plans/260714-preparing-acquire-recovery.md +++ /dev/null @@ -1,107 +0,0 @@ -# Recover Crash-Stuck Preparing Acquires - -## Goal - -Make the documented `repair({ action: "resume-acquire" })` path recover a lease -persisted in `preparing`, not only one quarantined after a caught failure. Preserve -the original checkout intent and existing branch-reuse behavior while adding the -minimum durable information needed to decide whether `postCreate` is still due. -Baseline: `38df31ba10bd06d1c48fe5cd20f0023743225840`. - -Acceptance semantics: - -- A `preparing` or `quarantined` lease with `pendingAcquire` can resume; other - states or missing intent still return `REPAIR_NOT_AVAILABLE`. -- Every new acquire explicitly persists `pendingAcquire.postCreatePending`: - `true` for a newly allocated physical slot, `false` for a reused slot. -- The marker stays `true` through materialization and hook execution, then is - durably set to `false` before checkout. A crash during the hook, or after its - side effects but before that state write, therefore retries `postCreate`. - This is intentionally at-least-once, not exactly-once. -- A present worktree with an explicit `false` marker never runs `postCreate`. - A missing worktree overrides the marker to `true`, because repair must create - a new physical worktree before continuing. -- Older persisted intents without the additive marker remain valid. On repair, - missing path or `diagnostics.failedPhase === "postCreate"` means pending; - otherwise an existing path means complete/not-required. This conservative - fallback avoids running `postCreate` on a legacy reused slot; an old, - ambiguous new-slot crash with an existing path cannot be distinguished and - also skips the hook. - -## Changes - -1. `packages/grove/src/schemas.ts:PendingAcquireSchema` and - `packages/grove/src/target.ts:buildPendingAcquire` — add optional boolean - `postCreatePending`. Keep it optional solely for persisted-state and exported - type compatibility; do not apply a Zod default because repair must distinguish - an old missing value from an explicit `false`. Require all newly built intents - to supply the boolean. - -2. `packages/grove/src/transitions.ts:LeaseEvent` and `transitionLease` — keep - lease mutations transition-owned. Allow `REPAIR_RESUME_ACQUIRE` from either - `quarantined` or `preparing`; retain its `pendingAcquire` guard and accept an - optional `postCreatePending` payload so existing external event construction - remains source-compatible while repair can normalize legacy intent. Add one - `ACQUIRE_POST_CREATE_COMPLETE` event that is valid only for a `preparing` - lease with pending post-create work and replaces the nested marker with - `false`. Do not change slot transitions: quarantined recovery still uses - `REPAIR_RESUME_LEASE`; an already-preparing lease already owns a leased slot. - -3. `packages/grove/src/lease-acquire.ts:acquireLease` — build `pendingAcquire` - after `findOrAllocateSlot()` reveals `isNew`, and persist the explicit marker - in the same write-ahead state save as the preparing lease. After a new slot's - `postCreate` step succeeds (including the no-hook case), persist - `ACQUIRE_POST_CREATE_COMPLETE` under the state lock before checkout. Leave - the marker true when materialization or `postCreate` fails so existing - quarantine-and-repair behavior retries the unfinished step; checkout failures - occur after the marker is false and must not rerun it. - -4. `packages/grove/src/lease-acquire.ts:resumeAcquireLease` — mirror - `resumeCleanupLease` state handling: transition quarantined records back to - preparing, accept preparing records directly, reject all others. While - holding the initial lock, derive and persist the repair marker in this exact - order: missing worktree => `true`; otherwise explicit marker => use it; - otherwise legacy `failedPhase: "postCreate"` => `true`; otherwise `false`. - Persist that decision through `REPAIR_RESUME_ACQUIRE` before materialization - or hooks, run/complete `postCreate` only when true, then use the existing - checkout/finalization flow. Keep `targetToAcquireOptions()` recovery semantics: - a pending create-from branch uses `ifExists: "reuse"`, allowing a branch - created before the crash to finish acquisition without weakening normal - acquire's fail-first policy. - -5. Test first with real Git fixtures: - - - `packages/grove/test/lease-repair.integration.test.ts` — seed valid - write-ahead crash snapshots and prove public `repair()` directly completes - a `preparing` lease; retries `postCreate` when the persisted marker is true; - and, for an actually reused physical slot with a false marker, skips a - failing `postCreate` hook while reusing a branch created before the crash. - Assert final leased target/head data and cleared `pendingAcquire`. - - `packages/grove/test/lease-hooks.integration.test.ts` — strengthen the - existing failed-`postCreate` case to assert the pending marker remains true - through quarantine and the existing repair retry still succeeds. - - `packages/grove/test/transitions.test.ts` — cover quarantined-to-preparing - and preparing-to-preparing repair, marker normalization, post-create - completion, missing intent, and invalid completion states. - - `packages/grove/test/state-v1.test.ts` — prove a pre-change preparing record - without `postCreatePending` still parses. No legacy worktree-shape migration - or state version is needed. - -6. `README.md:Lease states` and `README.md:Lifecycle hooks` — retain the existing - preparing repair instruction, now backed by behavior. Clarify that crash - recovery may retry `postCreate`, so commands requiring recovery safety must be - idempotent; reused physical slots do not run it. - -## Verify - -- `pnpm exec vitest run packages/grove/test/lease-repair.integration.test.ts packages/grove/test/lease-hooks.integration.test.ts packages/grove/test/transitions.test.ts packages/grove/test/state-v1.test.ts` -- `pnpm check` - -## Boundaries - -- No broader acquire phase enum or new state machine; one pending boolean is - sufficient for the only ambiguous side effect. -- No exactly-once hook guarantee, subprocess recovery, active-owner/liveness - policy, `postAcquire` replay, ref-pinning change, branch-ownership fix, CLI - change, or state-file version bump. -- No timestamp/path-layout inference for ambiguous legacy records. diff --git a/dev/plans/260714-process-scan-failure.md b/dev/plans/260714-process-scan-failure.md deleted file mode 100644 index 0aca677..0000000 --- a/dev/plans/260714-process-scan-failure.md +++ /dev/null @@ -1,22 +0,0 @@ -# Treat failed macOS process scans as unverified - -## Goal - -Restore the process-safety contract in `VISION.md` and `README.md`: a failed or unavailable macOS `lsof` scan reports `unverified`, so destructive cleanup without `force: true` stops before mutating lease state. Today `findInWorktree()` uses Execa with `reject: false` and parses failed results as a verified empty scan. - -Acceptance: failed spawn and nonzero `lsof` results produce `{ processes: [], unverified: true }`; successful output keeps the current parsing behavior; existing forced cleanup behavior remains unchanged. - -## Changes - -1. `packages/grove/test/lease-release.integration.test.ts` — test first with one focused regression using the real SDK and Git fixture. Acquire a lease, release with `preserve` to clear its owner reservation, then temporarily set `process.platform` to `darwin` and `PATH` to a nonexistent temporary directory inside a `try/finally`. Assert the real `findInWorktree()` result is unverified, a reset release with explicit `resetTo: "main"` fails with `UNSAFE_CLEANUP`, and after restoring both globals the lease is still `leased` and its path still exists. This deterministically exercises unavailable `lsof` on Linux and macOS CI, avoids mocking Git or Execa, and proves cleanup stopped before the write-ahead transition. Keep the existing force-bypass coverage rather than duplicating it. -2. `packages/grove/src/process/detect.ts:findInWorktree` — in the Darwin branch, retain the full Execa result and return the existing empty/unverified result immediately when `result.failed` is true; parse `stdout` only on success. Execa's `failed` flag covers both spawn failures such as `ENOENT` and nonzero exits, avoiding separate status/error branches or a new abstraction. Keep the catch as the fallback for thrown scan errors. - -## Verify - -- `pnpm vitest run packages/grove/test/lease-release.integration.test.ts` -- `pnpm check` - -## Boundaries - -- No Linux `/proc` scan redesign, process-detector refactor, new error type, schema/API change, or documentation change. -- Do not preserve or parse partial `lsof` output from a failed command; its completeness is unverified. diff --git a/dev/plans/260714-repair-force-propagation.md b/dev/plans/260714-repair-force-propagation.md deleted file mode 100644 index 6ea270e..0000000 --- a/dev/plans/260714-repair-force-propagation.md +++ /dev/null @@ -1,20 +0,0 @@ -# Preserve repair force intent through destroy - -## Goal - -Make `repair({ action: "force-destroy" })` honor the caller's optional `force` value for the entire destroy operation. A process that appears during `preDestroy` must make the fresh pre-removal scan fail with `UNSAFE_CLEANUP` when force was omitted or false; explicit `force: true` must remain the only override. Preserve the current destroy/quarantine lifecycle, API, and error contracts with the smallest data-flow correction. - -## Changes - -1. `packages/grove/test/lease-repair.integration.test.ts` — test first by expanding the existing force-destroy integration case into the regression and positive path. Acquire a real worktree, release it with `cleanup: "quarantine"` through the SDK so its owner reservation is cleared, then call exported `repairLease` without force and inject a `preDestroy` callback that starts a long-running Node child with the worktree as its CWD. Wait for the child to become visible using the established process-test timing pattern, assert the fresh scan rejects with `UNSAFE_CLEANUP`, the worktree still exists, and the failed destroy leaves the lease quarantined. Keep that hook-created child alive, retry through public `grove.repair` with `force: true`, and retain the existing destroyed result, missing lease, and removed-path assertions. Always kill and await the child in `finally` so failures cannot leak a process or race fixture cleanup. This single real-git case proves both sides of the contract without state-file mutation or mocked Git. -2. `packages/grove/src/lease-repair.ts:repairForceDestroy` — pass the existing `options` object directly to `destroyLease` instead of replacing it with `{ force: true }`, and remove the now-unused `DestroyLeaseOptions` type import. Keep the repair preflight, `destroyLease`'s begin scan, its post-hook fresh scan, hook wiring, and failure quarantine unchanged; `destroyLease` will carry the same caller value in `DestroyContext.force` to the final safety check. - -## Verify - -- `pnpm vitest run packages/grove/test/lease-repair.integration.test.ts` -- `pnpm check` - -## Boundaries - -- No changes to destroy/process-safety algorithms, transitions, schemas, public types, CLI behavior, docs, or error classes. -- Do not rename `force-destroy`, remove existing safety scans, or add a second force option; this fix only stops repair from manufacturing force authorization. diff --git a/dev/plans/260714-repository-pool-identity.md b/dev/plans/260714-repository-pool-identity.md deleted file mode 100644 index 67b14f6..0000000 --- a/dev/plans/260714-repository-pool-identity.md +++ /dev/null @@ -1,21 +0,0 @@ -# Key default pools by local repository path - -## Goal - -Ensure an implicitly named Grove pool belongs to one local repository checkout. Two clones with the same directory basename and `origin` URL must resolve to different pool directories, preventing one clone from reading or mutating the other clone's lease state. Keep the fix local to pool-name resolution: use the normalized absolute `repoRoot` as the identity, preserve configured pool placement, and leave explicit `groveDir` ownership unchanged. - -## Changes - -1. `packages/grove/test/config.test.ts:Config` — add the regression first using real Git: create two clone directories named `repo` that share one remote, then assert `resolveGroveDir()` returns different default directories. Reuse `setupRepo()` for the origin and first checkout, clone only the second checkout inside the same temporary fixture, and avoid a broader pool lifecycle test because directory separation is the highest seam that proves the invariant. -2. `packages/grove/src/config.ts:resolveGroveDir` — remove remote URL lookup from pool identity. Normalize the documented absolute repository input with `node:path.resolve`, then derive both the readable basename and existing short hash from that local path alone. Continue resolving `groveRoot` and environment expansion exactly as today. Do not add filesystem `realpath`, repository metadata, or a new state field: those add I/O and failure modes without improving separation between distinct checkout paths. -3. Treat the resulting pool-name change as a direct cutover. Do not probe, adopt, move, or delete the old remote-keyed directory because it may already be shared by multiple clones. Existing data remains untouched; a caller that intentionally needs an old pool can address it explicitly through `groveDir`. - -## Verify - -- `pnpm vitest run packages/grove/test/config.test.ts` -- `pnpm check` - -## Boundaries - -- No pool-state schema change, migration machinery, remote identity fallback, or cross-repository redesign. -- No change to explicit `groveDir`, `groveRoot` placement semantics, or the existing pool-name shape beyond its hash input. diff --git a/dev/plans/README.md b/dev/plans/README.md deleted file mode 100644 index 1b8d98f..0000000 --- a/dev/plans/README.md +++ /dev/null @@ -1,7 +0,0 @@ -# Implementation plans - -- [Treat failed macOS process scans as unverified](260714-process-scan-failure.md) -- [Preserve repair force intent through destroy](260714-repair-force-propagation.md) -- [Recover crash-stuck preparing acquires](260714-preparing-acquire-recovery.md) -- [Key default pools by local repository path](260714-repository-pool-identity.md) -- [Constrain destroy to its owned slot directory](260714-destroy-path-layout.md)