feat(worktree): snapshot-backed management and automatic cleanup - #46
Conversation
Add Phase 2 states, snapshot metadata, per-server policy fields, and inventory/preview/delete/restore/retry/permanent-delete/archive RPC contracts plus their channels, routing, and wire error codes. Refs #41
Extend the fixed registry with Phase 2 states (snapshotting, snapshotted, restoring, cleanup-failed, restore-failed, unowned), optional snapshot metadata / policy / archive / sanitized-error fields, and an exclusive cross-process transaction API for lifecycle operations. Settings gain per-server autoDeleteEnabled and retentionLimit policy with defaults and bounds validation. Refs #41
Add a canonical checkout-path lease manager with cross-process marker files (sessions protect their checkout from the first instant, even before registry owner persistence), an append-only durable lifecycle journal with idempotent steps and commit markers for crash classification, and export the process-liveness probe for stale-lease recovery. Refs #41
Streaming binary capture of staged/unstaged projections, untracked regular files, non-dereferenced symlink nodes, and .worktreeinclude files; preflight 10k-file/100MiB bounds; component hashing; CAS-created hidden snapshot refs pinning captured HEAD; atomic verified publish; exact restore that recreates only an absent branch and never force- resets; and ownership-proved permanent deletion. Real-Git tests cover clean/staged/unstaged/mixed/binary/rename/deletion/untracked/included/ executable/symlink state plus unsupported-state, limit, tamper, ref- conflict, and path-safety failures. Refs #41
Single entry for every V2 destructive path: fresh preview fingerprints binding owner/path/Git/content/policy state, snapshot-first manual delete with pre-capture and pre-release revalidation, exact restore with payload/ref removal only after the journaled commit, retry of safe failed steps, ownership-proved permanent deletion, per-owner archive with all-archived cleanup, LRU retention sweeps fenced at the policy-version boundary, session-delete integration (plain detach to unowned, final-owner snapshot-first removal), lastUsedAt activity hooks, and journal-based classification of interrupted transactions. Refs #41
…encing Startup reconciliation is now awaited: it repairs owners, leases every live checkout path (including sessions not yet in registry owners), classifies interrupted journal transactions, compacts the journal, and only then marks lifecycle readiness. Registers inventory/preview/ delete/restore/retry/permanent-delete/archive/unarchive RPCs with typed wire errors. Send, agent creation, Git mutations, and diffs stay fenced while a session's worktree record is not ready. Session deletion routes through the lifecycle (plain detach to unowned + sweep; delete-session- and-worktree is snapshot-first and staged); accepted user messages touch lastUsedAt; conversation-branch children lease their checkout. Refs #41
Worktrees settings now expose per-server auto-delete policy and materialized-worktree limit with last cleanup result, a full inventory (display name, branch, workspace/repository/server path, per-owner archived/active/flagged protection, created/last-used, lifecycle state, sanitized failure text, snapshot metadata), snapshot-first delete with fresh preview confirmation, restore, retry, per-owner archive/ unarchive, and permanent snapshot deletion behind a second irreversibility confirmation. i18n keys added to all seven locales. Refs #41
Add the snapshot-backed lifecycle ADR (single destructive entry, verified snapshot before release, CAS-owned hidden refs, path fences, journal recovery, event-driven LRU cleanup, permanent deletion) and document snapshot-first delete/restore/archive/retention, the inventory/recovery surface, and V2 session-deletion semantics in the online docs; update OKF index and logs. Refs #41
… and relink sessions on restore Preview and delete now bind the same current settings version, so a policy change between preview and confirmation invalidates it instead of producing a false stale mismatch (or a false match). Restore stamps the restored checkout path onto every owner session and re-leases owners to the live path; creation stamps policyVersion. Adds the real-Electron E2E for snapshot delete/restore and cleanup policy. Refs #41
The stability contract now covers the eight Phase 2 lifecycle channels (335 → 343 channel strings). Refs #41
- Reconcile never reclassifies lifecycle-owned states (snapshot-backed records were being turned into 'missing' at startup) and V2 skips the V1 branch-pruning reclamation path. - Every removal transaction now takes host lock, common-directory mutation lock, and registry lock in creation order; automatic and session-delete transactions verify a stability fingerprint before and after capture; owner writes re-read the registry set. - Restore persists a durable 'restoring' marker before touching the checkout and commits the ready record before payload/ref cleanup, retaining snapshot metadata until cleanup is provably done; startup reconciliation finishes pending restore cleanup and permanent-delete record removal with verified payload/ref evidence. - Capture cleans published payloads and refs when post-publish verification fails; payload paths and stored names are constrained to the snapshot root; untracked modes are preserved exactly; unresolved merge/rebase/cherry-pick/revert markers block capture; restore checks realpath containment. - Registry binds hidden refs to their snapshot IDs; missing-record removal releases owner leases; inventory counts unowned records; the enqueue slot actually releases after a sweep; the legacy V2 removal RPC routes through the lifecycle; session recovery state persists through bindCheckout-equivalent persistence. - UI: delete offered only where a preview fingerprint exists, stale cross-server responses are discarded, reset restores policy fields. - Regression tests cover each fix. Refs #41
…s, UI guards - enqueueCleanup keeps the raw sweep as its slot so later events always start a fresh sweep (regression-tested by counting sweeps). - Retry of a cleanup-failed record re-enters the full locked transaction (host + mutation locks, fences, quiescence, stability fingerprint) instead of removing under the host lock alone. - Restore commit #1 writes only this transaction's fields from the live registry record (owner sets are never clobbered), tracks its own commit so a concurrent restore cannot claim success, and startup reconciliation sweeps ready records that still carry snapshot metadata (crash between journal commit and payload/ref removal). - Startup GC removes unreferenced payloads and stale .tmp-* staging under the cross-process host lock; missing-record removal stamps owner recovery state; reconcile repairs owners on lifecycle-owned states too. - V2 legacy removal RPC enforces shared-owner blocking per requester. - Snapshot service rejects a symlinked materialization root on restore, preserves zero permission bits exactly, and restores exact untracked modes (covered by a new real-Git test). - UI guards use a live target-key ref and bind confirmations to their target so a server switch can never apply stale inventory or submit server A's fingerprint to server B. Refs #41
📝 WalkthroughWalkthroughWorktree V2 now supports server-local snapshots, lifecycle states, automatic cleanup, retention policies, ownership protection, recovery, inventory, lifecycle RPCs, and Electron management controls. The change also adds durable journals, path leases, startup reconciliation, documentation, localization, and end-to-end coverage. ChangesWorktree V2 lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SettingsPage
participant GitRPC
participant WorktreeLifecycleService
participant WorktreeSnapshotService
participant WorktreeRegistry
SettingsPage->>GitRPC: Request preview or lifecycle action
GitRPC->>WorktreeLifecycleService: Validate readiness, ownership, and fingerprint
WorktreeLifecycleService->>WorktreeSnapshotService: Capture, verify, restore, or delete snapshot
WorktreeLifecycleService->>WorktreeRegistry: Persist lifecycle state and cleanup results
GitRPC-->>SettingsPage: Return inventory or action result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0361c2fde8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
e2e/tests/git/worktree-v2-manage.spec.ts-117-121 (1)
117-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment claims the test disables auto-delete; the test never toggles it.
The test only fills
worktrees-retention-limit. It never interacts withworktrees-auto-delete, and it never asserts the last-cleanup row the comment refers to. The stated coverage of the auto-delete control does not exist.Either toggle
worktrees-auto-deleteand assert its persisted state after the save, or correct the comment to describe only the retention limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/git/worktree-v2-manage.spec.ts` around lines 117 - 121, Align the policy-controls test comment and implementation: either interact with worktrees-auto-delete and assert its persisted state after clicking worktrees-save, or revise the comment to describe only the worktrees-retention-limit update; do not claim auto-delete or cleanup coverage without exercising and validating it.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx-598-610 (1)
598-610: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe auto-delete label renders twice.
SettingsRowrenderslabelanddescriptionin its text column, then places children in the control slot.SettingsTogglerenders its ownlabelnext to the switch. Both aret('settings.worktrees.autoDelete'), so the row shows "Automatic cleanup" twice.Use
SettingsToggledirectly, as the other settings cards do.🐛 Proposed fix
<div data-testid="worktrees-auto-delete"> - <SettingsRow - label={t('settings.worktrees.autoDelete')} - description={t('settings.worktrees.autoDeleteDesc')} - > - <SettingsToggle - label={t('settings.worktrees.autoDelete')} - checked={autoDeleteEnabled} - onCheckedChange={setAutoDeleteEnabled} - disabled={isSaving} - /> - </SettingsRow> + <SettingsToggle + label={t('settings.worktrees.autoDelete')} + description={t('settings.worktrees.autoDeleteDesc')} + checked={autoDeleteEnabled} + onCheckedChange={setAutoDeleteEnabled} + disabled={isSaving} + /> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx` around lines 598 - 610, Replace the SettingsRow wrapper in the worktrees auto-delete section with SettingsToggle directly, preserving the existing label, checked state, change handler, disabled state, and surrounding test-id container so the label renders only once.apps/online-docs/core-concepts/git-worktrees.mdx-157-160 (1)
157-160: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winState the flagged condition consistently.
Line 115 says the checkout is removed when every owner is archived "and none is active or flagged". This passage omits "or flagged".
SessionManagerinstalls anisSessionFlaggedhook, so a flagged owner does block removal. Readers get two different rules for the same behavior.- archived (and none is active), the worktree is removed snapshot-first and can + archived (and none is active or flagged), the worktree is removed + snapshot-first and can🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/online-docs/core-concepts/git-worktrees.mdx` around lines 157 - 160, Update the Worktree V2 archiving rule in the session-removal documentation to include flagged owners in the blocking condition, matching the “every owner is archived and none is active or flagged” behavior described elsewhere. Preserve the existing snapshot-first removal and inventory restoration details.e2e/tests/git/worktree-v2-manage.spec.ts-133-134 (1)
133-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not assert a fixed OID length.
git init -b maininheritsinit.defaultObjectFormatfrom the host configuration. A SHA-256 repository produces a 64-character OID, and this assertion then fails for a correct result.Assert the shape instead of the length.
💚 Proposed fix
const branch = await git(repository, "rev-parse", "--verify", "refs/heads/kata-agent/manage-me"); - expect(branch).toHaveLength(40); + expect(branch).toMatch(/^[0-9a-f]{40,64}$/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/git/worktree-v2-manage.spec.ts` around lines 133 - 134, Update the assertion on the `branch` value in the worktree management test to validate that it has the expected hexadecimal object-ID shape rather than a fixed 40-character length, allowing both SHA-1 and SHA-256 repository formats.packages/server-core/src/sessions/SessionManager.ts-5927-5938 (1)
5927-5938: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not cast an arbitrary lifecycle reason code to
'agent_not_quiesced'.
outcome.reasonCodecarries any lifecycle blocking code (for example a stale preview or a foreign lease). The cast forwards those values inblockedReasonCode, whose type declares only'agent_not_quiesced'. A client that branches onblockedReasonCodethen treats an unrelated block as a wedged agent.Map the code explicitly and leave it undefined for every other value.
🐛 Proposed fix
- blockedReasonCode: outcome.reasonCode as 'agent_not_quiesced' | undefined, + blockedReasonCode: + outcome.reasonCode === 'agent_not_quiesced' ? 'agent_not_quiesced' : undefined,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/sessions/SessionManager.ts` around lines 5927 - 5938, In the blocked outcome handling, replace the unsafe cast of outcome.reasonCode with an explicit mapping that sets blockedReasonCode only when the code is exactly 'agent_not_quiesced'; leave it undefined for all other lifecycle blocking codes while preserving the existing blocked result fields.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx-611-623 (1)
611-623: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe retention input cannot be cleared and accepts values it silently rewrites.
Number('')is0and passesNumber.isFinite. Clearing the field therefore setsretentionLimitto0, and the input immediately redisplays0. The user cannot empty the field to retype a value.The same handler accepts
1.5and5000. Those values render in the field, mark the policy dirty, and are only corrected by the clamp inhandleSave. The displayed value disagrees with the value that is persisted until the server response arrives.Keep the raw text in state, and reject non-integer input for display.
🐛 Proposed fix
onChange={(value) => { - const parsed = Number(value) - if (Number.isFinite(parsed)) setRetentionLimit(parsed) + if (value.trim() === '') return + if (!/^\d+$/.test(value.trim())) return + setRetentionLimit(Number(value.trim())) }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx` around lines 611 - 623, Update the retention input state and handler in WorktreesSettingsPage so it preserves the raw text, allowing the field to remain empty while the user edits. Reject non-integer values such as decimals and out-of-range values instead of updating the policy state, and ensure accepted text stays synchronized with the value used for saving without relying on handleSave to silently clamp it.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx-905-911 (1)
905-911: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGive the confirmation input an accessible name.
The input has only a placeholder. A placeholder is not an accessible name, and it disappears once the user types. Assistive technology therefore announces an unnamed text field on an irreversible confirmation.
Add an
aria-label, or a visible<label>bound withhtmlFor.🐛 Proposed fix
<input data-testid="worktrees-permanent-confirm-input" + aria-label={t('settings.worktrees.confirmPermanentIrreversible')} className="w-full rounded-md border bg-background px-3 py-2 text-sm" placeholder="delete"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx` around lines 905 - 911, Add an accessible name to the input identified by data-testid="worktrees-permanent-confirm-input", preferably by associating a visible label via htmlFor or by adding a descriptive aria-label. Preserve its existing value and onChange behavior.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx-474-501 (1)
474-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBind the permanent-delete confirmation to its target, as the delete flow does.
ConfirmDeleteStaterecordstargetKeyandconfirmDeleteActionrefuses to run after a target switch.ConfirmPermanentStaterecords onlyrow. If the operator opens this dialog for server A, changes the selected server, and then confirms, the irreversibleWORKTREE_PERMANENT_DELETEis sent to server B with server A'smanagedWorktreeId.Add
targetKeytoConfirmPermanentStateand apply the sameisCurrentTargetcheck.🐛 Proposed fix
interface ConfirmPermanentState { row: WorktreeInventoryRow + /** Target the row was listed from; the permanent delete must still be current. */ + targetKey: string }const confirmPermanentAction = useCallback(async () => { if (!selectedTarget || !confirmPermanent) return + if (!isCurrentTarget(confirmPermanent.targetKey)) { + setConfirmPermanent(null) + setPermanentTyped('') + return + } if (permanentTyped.trim().toLowerCase() !== 'delete') returnonClick={() => { - setConfirmPermanent({ row }) + setConfirmPermanent({ row, targetKey: selectedTarget.key }) setPermanentTyped('') }}Add
isCurrentTargetto theuseCallbackdependency list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx` around lines 474 - 501, Update ConfirmPermanentState to store the targetKey when opening the permanent-delete confirmation, then apply the existing isCurrentTarget guard in confirmPermanentAction before invoking WORKTREE_PERMANENT_DELETE. Add isCurrentTarget to the callback dependencies and preserve the existing confirmation flow for the currently selected target.packages/server-core/src/git/worktree-registry.ts-1285-1293 (1)
1285-1293: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate
atbefore it reaches persistence.
updateLastUsedAtwritesatwithout any check.validateV2Recordrequires a finite number forlastUsedAton load. A non-finite or non-integerattherefore persists successfully and then makes every subsequentload()throwREGISTRY_INVALID_RECORD, which fences all worktree operations until the file is repaired by hand.
setStatealready validates its input againstVALID_STATES. Apply the same fail-fast rule here.🛡️ Proposed guard
updateLastUsedAt(id: string, at: number): void { + if (!Number.isFinite(at)) { + throw new WorktreeRegistryError( + 'REGISTRY_INVALID_RECORD', + 'Managed-worktree lastUsedAt must be a finite number.', + this.registryPath, + ) + } this.mutate((records) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-registry.ts` around lines 1285 - 1293, Update updateLastUsedAt to validate at before calling mutate, rejecting non-finite or non-integer values with the same fail-fast approach used by setState and its VALID_STATES validation. Only allow valid integer timestamps to reach persistence, while preserving the existing record lookup and unchanged-value behavior.packages/server-core/src/git/worktree-snapshot-service.ts-584-591 (1)
584-591: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClassify missing patch files as
SNAPSHOT_PAYLOAD_MISSING.
verifyPayloadguards the manifest withexistsSyncat Line 566, but readsstaged.patchandunstaged.patchdirectly. If either file is absent,readFileSyncthrows a rawENOENTerror. Callers then receive an unclassified error instead of aWorktreeSnapshotError, so restore and permanent-delete paths cannot map it to a recovery state.🛠️ Proposed fix
- const stagedPatch = readFileSync(join(meta.payloadPath, 'staged.patch')) + const readPatch = (name: string): Buffer => { + try { + return readFileSync(join(meta.payloadPath, name)) + } catch { + throw new WorktreeSnapshotError('SNAPSHOT_PAYLOAD_MISSING', `Snapshot payload is missing ${name}.`) + } + } + const stagedPatch = readPatch('staged.patch') if (sha256(stagedPatch) !== manifest.stagedPatch.sha256) { throw new WorktreeSnapshotError('SNAPSHOT_VERIFY_FAILED', 'Staged patch hash mismatch.') } - const unstagedPatch = readFileSync(join(meta.payloadPath, 'unstaged.patch')) + const unstagedPatch = readPatch('unstaged.patch')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 584 - 591, Update verifyPayload around the stagedPatch and unstagedPatch readFileSync calls to detect missing staged.patch or unstaged.patch files and throw WorktreeSnapshotError with code SNAPSHOT_PAYLOAD_MISSING instead of allowing raw ENOENT errors. Preserve the existing hash-mismatch validation for files that are present.packages/server-core/src/git/worktree-snapshot-service.ts-236-243 (1)
236-243: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThrow the classified error before the follow-up Git calls.
If the git-dir probe fails,
problemsrecords "the checkout is not a readable Git worktree", but execution continues. Line 240 then runsgit ls-files -uin the same unreadable path. That call rejects with a raw command error, so callers receive an unclassified error instead ofSNAPSHOT_UNSUPPORTED_STATE. Return the classified error as soon as the probe fails.🛠️ Proposed fix
} catch { problems.push('the checkout is not a readable Git worktree') } + + if (problems.length > 0) { + throw new WorktreeSnapshotError( + 'SNAPSHOT_UNSUPPORTED_STATE', + `Snapshot capture is blocked because ${problems.join('; ')}.`, + ) + } const unmerged = await runGit(['ls-files', '-u'], { cwd: checkoutPath })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 236 - 243, Update the git-dir probe error handling in the worktree validation flow to return or throw the classified SNAPSHOT_UNSUPPORTED_STATE error immediately when the probe fails, before invoking runGit for ls-files -u. Preserve the existing problem message and classification, and keep the follow-up unmerged-entry check only for successful probes.
🧹 Nitpick comments (24)
packages/shared/src/i18n/locales/en.json (1)
1246-1246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
{{count}}interpolation variable.i18next treats
countas a reserved plural option.settings.worktrees.confirmDeleteIgnoredhas no_one/_othervariants, so it interpolates through the base key today, but any future plural variant will select by count instead.Use a neutral name such as
{{included}}, and update theWorktreesSettingsPagecall site that passescountto this key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/i18n/locales/en.json` at line 1246, Rename the interpolation variable in settings.worktrees.confirmDeleteIgnored from count to a neutral name such as included, and update the WorktreesSettingsPage call site for this translation key to pass the renamed variable. Keep the displayed count value unchanged.Source: Coding guidelines
packages/server-core/src/git/worktree-settings-service.ts (1)
311-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWiden the write-failure message to cover the policy fields.
nextnow carriesautoDeleteEnabledandretentionLimit. The failure message still names only the root setting, so a failed policy save reports a misleading cause.♻️ Proposed message change
- 'Unable to persist the managed-worktree root setting.', + 'Unable to persist the managed-worktree root and cleanup policy settings.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-settings-service.ts` around lines 311 - 327, Update the WorktreeSettingsError message in the writeAtomically failure path within the settings update flow to describe persistence of the managed-worktree root and policy settings, including autoDeleteEnabled and retentionLimit. Keep the existing error code, settings path, and original error propagation unchanged.packages/server-core/src/git/worktree-registry.ts (2)
280-288: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
snapshotIdto its generated alphabet.
snapshotIdis validated only as a non-empty string, and thehiddenRefcheck is a pure equality against a string built from that same value. A hand-edited record can therefore carry an id with path separators or..while still passing both checks, and that id is documented as the payload directory leaf name.
verifyPayloadblocks the read path through the containment check onpayloadPath, so this is not currently exploitable.capturealways generatesrandomBytes(8).toString('hex'), so a hex check costs nothing and removes the class.♻️ Proposed validation
const snapshotId = requireString(value.snapshotId, 'snapshot.snapshotId', registryPath) + if (!/^[0-9a-f]{8,64}$/.test(snapshotId)) { + throw new WorktreeRegistryError('REGISTRY_INVALID_RECORD', 'Registry snapshot snapshotId must be a lowercase hex identifier.', registryPath) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-registry.ts` around lines 280 - 288, Update snapshot record validation in the function containing snapshotId, schemaVersion, and hiddenRef checks to require snapshotId to be a non-empty lowercase hexadecimal string matching the capture-generated 16-character format. Reject any value containing separators, traversal components, or other characters before validating hiddenRef, while preserving the existing snapshotId-to-hiddenRef equality check.
1327-1341: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winA second
commit()in one transaction fails with a misleading conflict.
expectedSourceis captured once, before the callback runs.persistLockedcompares the on-disk hash and identity against that captured value. After the firstcommit()succeeds, the file hash has changed, so a secondcommit()in the same callback throwsREGISTRY_CONFLICTwith the message "The registry source changed before the mutation could be committed." No other writer was involved.Refresh the expected source inside
commit()after each successful persist, so repeated commits in one transaction remain correct.♻️ Proposed fix: re-anchor the expected source after each commit
commit: () => { this.hooks.beforePersist?.() this.persistLocked(records.values(), { exists: source.exists, hash: authoritativeHash, identity: authoritativeIdentity, }) + // Re-anchor so a second commit in the same transaction compares + // against the bytes this transaction just wrote. + const committed = this.readSource() + source = { ...source, exists: committed.exists } + authoritativeHash = committed.hash + authoritativeIdentity = committed.identity },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-registry.ts` around lines 1327 - 1341, Update the transaction’s commit implementation in the WorktreeRegistryTransaction construction to refresh the expected source after each successful persist. Keep a mutable expected hash and identity for the transaction, pass those values to persistLocked, then replace them with the newly persisted source only after persistLocked succeeds so repeated commits do not report a self-induced REGISTRY_CONFLICT.packages/server-core/src/git/__tests__/worktree-settings.test.ts (1)
99-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a temp-directory root instead of
~/xin the rejection cases.
'~/x'expands to a path inside the developer's real home directory. The policy validators currently throw beforeensureRootUsableruns, so nothing is created today. That ordering is the only thing keeping the test from callingmkdirSyncon a real home path. Bind the fixture to the test temp root so the test cannot touch the developer environment if the validation order changes.♻️ Proposed fixture change
- test('rejects out-of-range retention limits and non-boolean auto-delete policy', () => { - const { settings } = makeSettings() + test('rejects out-of-range retention limits and non-boolean auto-delete policy', () => { + const { root, settings } = makeSettings() + const candidate = join(root, 'x') - expect(() => settings.update({ materializationRoot: '~/x', retentionLimit: 0 })).toThrow( + expect(() => settings.update({ materializationRoot: candidate, retentionLimit: 0 })).toThrow( WorktreeSettingsError, )Apply the same change to the remaining three cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-settings.test.ts` around lines 99 - 114, Update all four rejection cases in the test named “rejects out-of-range retention limits and non-boolean auto-delete policy” to use the fixture’s temporary-directory root from makeSettings instead of the literal ~/x for materializationRoot, ensuring validation-order changes cannot create directories in the developer’s home directory.packages/shared/src/protocol/git.ts (1)
132-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive
WorktreeRecoveryStatefromManagedWorktreeState.
SessionManager.setGitServicesassigns any non-readyrecord state toManagedWorktreeStatefields and casts it asWorktreeRecoveryState.ManagedWorktreeStateincludespreparing,removing, andblocked, butWorktreeRecoveryStatedoes not, so those states can bypass the recovery union. Derive the type with the intent states excluded, or narrow the cast at this assignment site and map unsupported states explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/protocol/git.ts` around lines 132 - 141, The WorktreeRecoveryState union is incomplete relative to ManagedWorktreeState, allowing setGitServices to cast unsupported non-ready states unsafely. Derive WorktreeRecoveryState from ManagedWorktreeState while excluding the intended lifecycle states, or update the assignment in SessionManager.setGitServices to explicitly map preparing, removing, and blocked before casting.packages/server-core/src/git/__tests__/path-leases.test.ts (1)
87-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
pruneStale().The suite does not exercise
pruneStale(). That method is the only stale-lease recovery path, and a lease left by a dead process blocks every destructive lifecycle transaction on its checkout. Write a marker whosepidrefers to a dead process and assert thatpruneStale()removes it and returns 1. Also assert that a marker with a livepidsurvives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/path-leases.test.ts` around lines 87 - 95, Add tests covering PathLeaseManager.pruneStale(): create stale and live lease marker files, using a dead pid for the stale marker and a live pid for the other, then assert pruneStale() removes only the stale marker and returns 1 while the live marker remains.packages/server-core/src/git/__tests__/worktree-journal.test.ts (1)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the torn-tail line and
recover().Two documented behaviors are untested:
readAll()must skip a malformed trailing line. Append a partial JSON line to the file and assert that the valid entries still load.recover()moves anin-progressentry torecoveredand setscommitMarker. Startup reconciliation depends on it. Assert both fields, and assert thatrecover()on acommittedentry is a no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-journal.test.ts` around lines 85 - 91, Add tests in the WorktreeJournal test suite covering malformed trailing JSON so readAll() preserves previously valid entries, and covering recover() so an in-progress entry becomes recovered with commitMarker set. Also verify recover() leaves a committed entry unchanged.packages/server-core/src/git/worktree-journal.ts (1)
209-215: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead the journal once in
compact().
compact()parses the whole file twice: once for the filter and once for the length comparison. Reuse the first read.Also confirm that a caller invokes
compact(). The provided lifecycle code never calls it, socommittedandrecoveredentries accumulate. Everystep()rewrites the full file, so the per-step cost grows with the retained entry count.♻️ Proposed fix
compact(): void { this.lock.runSync(() => { - const entries = this.readAll().filter((entry) => entry.status === 'failed') - if (entries.length === this.readAll().length) return - this.writeAll(entries) + const all = this.readAll() + const entries = all.filter((entry) => entry.status === 'failed') + if (entries.length === all.length) return + this.writeAll(entries) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-journal.ts` around lines 209 - 215, Update compact() to read the journal once, store the result, and reuse it for both filtering and length comparison. Also trace the journal lifecycle and ensure compact() is invoked after the appropriate step/commit/recovery operation so committed and recovered entries are removed while failed entries remain retained.packages/server-core/src/git/__tests__/test-helpers.ts (1)
51-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport a
GIT_ENV-bound helper instead of the rawrunGit.
initRepoat Line 29 andgitat Line 38 both passenv: GIT_ENV, which isolates tests from ambient Git configuration. The re-exportedrunGitcarries no such binding. The one consumer, worktree-snapshot.test.ts Line 288, calls it withoutenv: GIT_ENV, so that call inherits the developer's or CI runner's Git environment. Export a wrapper that suppliesGIT_ENVand still allowsinput, so stdin-based calls keep the same isolation.♻️ Proposed refactor
-export { runGit } +/** `runGit` with the test Git environment applied; supports `input` for stdin. */ +export function gitWith( + args: string[], + options: Omit<Parameters<typeof runGit>[1], 'env'> & { cwd: string }, +): ReturnType<typeof runGit> { + return runGit(args, { ...options, env: GIT_ENV }) +}Then update the consumer:
- await runGit(['update-index', '--index-info'], { + await gitWith(['update-index', '--index-info'], { cwd: worktreePath,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/test-helpers.ts` around lines 51 - 52, Replace the raw runGit export in test-helpers.ts with a GIT_ENV-bound wrapper that forwards input and any required arguments while always supplying env: GIT_ENV. Update the worktree-snapshot.test.ts consumer to use this helper without relying on ambient Git configuration, preserving stdin-based calls.packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts (4)
427-428: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a past timestamp instead of a future one.
Date.now() + 1000placesfirst.lastUsedAtin the future. No real activity produces that state, and any idle-window or freshness rule in the sweep could treat it differently from a recent timestamp. Setfirstto a recent past value so the LRU ordering under test stays realistic.♻️ Proposed change
- svc.registry.updateLastUsedAt(first.managedWorktreeId, Date.now() + 1000) + svc.registry.updateLastUsedAt(first.managedWorktreeId, Date.now() - 10) svc.registry.updateLastUsedAt(second.managedWorktreeId, Date.now() - 1000)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts` around lines 427 - 428, Update the timestamp setup in the worktree lifecycle test around updateLastUsedAt so first.managedWorktreeId receives a recent past timestamp rather than Date.now() + 1000, while keeping second older than first to preserve the intended LRU ordering.
736-752: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the race test with a content-only write.
The injected write creates a new file,
race.txt. A new path changes thegit status --porcelain=v2output, so the fingerprint changes and the sweep blocks. The test therefore passes even though the per-path content binding incomputeWorktreeFingerprintis inert; see worktree-snapshot-service.ts Line 1024. Change the injected write to modify a file that is already modified in the checkout, so the test proves content-level race detection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts` around lines 736 - 752, Update the race injection in the runCleanupSweep test around svc.snapshots.capture so it overwrites an already modified file in first.checkoutPath rather than creating race.txt. Preserve the existing assertions, ensuring the path set and git status remain unchanged while computeWorktreeFingerprint detects the content change and records cleanup-failed with the checkout intact.
484-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the materialization root with
join.
harness.root + '/worktrees'hard-codes the POSIX separator, while the harness builds the same path withjoin(root, 'worktrees')at Line 34 and Line 50. The same concatenation repeats at Line 529, Line 540, Line 669, and Line 784. Usejoin(harness.root, 'worktrees')at every site so the settings value matches the harness value exactly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts` at line 484, Replace every materialization root concatenation in the worktree lifecycle tests with join(harness.root, 'worktrees'), including the occurrences near the settings updates and the other repeated sites. Reuse the existing join import or add it if needed so the settings value exactly matches the harness-built path across platforms.
26-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for flagged-session protection.
flaggedSessionsis declared and wired intoisSessionFlagged, but no test ever adds a session to it. The flagged-owner protection path therefore has no coverage, while the active-owner path does. Issue#41requires protection when a session is flagged, so add a delete test and a sweep test with a flagged owner.💚 Proposed test
+ test('blocks delete while an owning session is flagged', async () => { + const { svc } = harness + const record = await makeManagedWorktree('feature-x', ['session-1']) + flaggedSessions.add('session-1') + + const result = await svc.lifecycle.deleteWorktree( + record.managedWorktreeId, + (await svc.lifecycle.preview(record.managedWorktreeId)).previewFingerprint, + ) + expect(result.deleted).toBe(false) + expect(existsSync(record.checkoutPath)).toBe(true) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts` around lines 26 - 45, Add test coverage using the existing makeHarness lifecycle hooks by inserting a session into flaggedSessions and verifying deletion refuses to remove its worktree. Add a separate sweep test with a flagged owner, asserting the flagged session remains protected. Reuse the existing active-owner test patterns and keep the isSessionFlagged behavior unchanged.packages/server-core/src/git/worktree-snapshot-service.ts (7)
804-814: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrune the worktree administrative entry after a failed restore.
If
worktree remove --forcefails,removeDirdeletes the directory but leaves the entry in.git/worktrees. Git then reports the path as already registered, so a retry that targets the same destination fails. Rungit worktree pruneafter the directory removal.🛠️ Proposed fix
this.removeDir(checkoutPath) + await runGit(['worktree', 'prune'], { + cwd: record.repositoryRoot, + okExitCodes: [1, 128], + }).catch(() => undefined) throw error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 804 - 814, Update the cleanup flow around the createdWorktree branch to run git worktree prune after removeDir(checkoutPath), ensuring stale administrative entries are removed when forced worktree removal fails. Use record.repositoryRoot as the command working directory and preserve the existing cleanup behavior.
652-656: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a containment check to
removeStagingDir.The method validates only the
.tmp-prefix before a recursivermSync. Directory entries from the snapshots root are leaf names, so the current callers are safe. A future caller that passes a relative name such as.tmp-../xwould delete outside the snapshots root. Reuse the existingisContainedguard.🛡️ Proposed hardening
removeStagingDir(name: string): void { if (!name.startsWith('.tmp-')) return - this.removeDir(join(this.snapshotsRoot, name)) + const target = resolvePath(join(this.snapshotsRoot, name)) + if (!isContained(this.snapshotsRoot, target)) return + this.removeDir(target) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 652 - 656, Update removeStagingDir to validate that the resolved staging path remains contained within snapshotsRoot using the existing isContained guard before calling removeDir. Keep the .tmp- prefix check and return without deletion when the containment check fails.
863-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an lstat-based existence check before writing the entry.
existsSync(dest)follows symlinks. A dangling symlink atdestreportsfalse, socopyFileSyncwould then write through the link to its target. Capture cannot produce this state today, becausegit ls-files --othersexcludes tracked paths and the manifest is hash-bound to the record, so this is hardening rather than a live defect.lstatSafeis already available in this file.🛡️ Proposed hardening
- if (existsSync(dest)) { + if (lstatSafe(dest) !== null) { throw new WorktreeSnapshotError('SNAPSHOT_PATH_UNSAFE', `Restore destination exists: ${entry.path}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 863 - 892, In the entry restore logic, replace the follows-symlink `existsSync(dest)` guard with the existing `lstatSafe`-based check so dangling symlinks are treated as existing destinations. Keep throwing `SNAPSHOT_PATH_UNSAFE` before symlink recreation or payload copying whenever any filesystem entry already occupies `dest`.
437-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the tautological manifest hash check.
this.manifestHash(manifest)is defined assha256(JSON.stringify(manifest)). The comparison at Line 439 compares the same expression to itself, so the branch can never fail. The real read-back verification already happens at Line 456 and Line 503. Delete these lines to avoid implying a guarantee that does not exist.♻️ Proposed refactor
const manifestHash = this.manifestHash(manifest) - if (manifestHash !== sha256(JSON.stringify(manifest))) { - throw new WorktreeSnapshotError('SNAPSHOT_VERIFY_FAILED', 'Manifest hash mismatch while preparing the snapshot.') - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 437 - 441, Remove the tautological manifest hash comparison and its associated SNAPSHOT_VERIFY_FAILED throw from the snapshot preparation flow. In the method containing manifestHash, rely on the existing read-back verification paths instead, leaving manifest creation and publication behavior unchanged.
364-384: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApply the capture limits to symlink entries too.
The symlink branch appends an entry and increases
totalByteswithout a limit check, thencontinues. Only the post-loop checks at Line 411 and Line 417 catch the excess. A checkout with many symlinks therefore accumulates unbounded entries in memory during the loop. Move the limit check above the symlink branch so both entry types are bounded as they are added.♻️ Proposed refactor
+ if (fileEntries.length + 1 > this.limits.maxFiles) { + throw new WorktreeSnapshotError( + 'SNAPSHOT_LIMIT', + `Snapshot exceeds the capture limit (${this.limits.maxFiles} files / ${this.limits.maxBytes} bytes).`, + ) + } if (stat.isSymbolicLink()) { const linkText = readlinkSync(abs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 364 - 384, Move the max-files and max-bytes validation in the worktree snapshot capture loop to run before the symbolic-link branch, accounting for the symlink’s entry and linkText.length. Ensure both symlink and regular-file entries are rejected immediately when limits would be exceeded, while preserving the existing WorktreeSnapshotError behavior.
668-712: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winVerify the payload once per restore.
verifyPayload(meta)runs at Line 670 and again at Line 712. Each call re-reads and re-hashes both patches and every stored file, so a payload near the 100 MiB limit is hashed twice before any checkout is created. No state changes between the two calls, so keep the first result.♻️ Proposed refactor
const { record, meta, checkoutPath } = input - this.verifyPayload(meta) + const manifest = this.verifyPayload(meta)this.prepareRestoreParents(resolvePath(record.materializationRoot), checkoutPath) - const manifest = this.verifyPayload(meta) let createdWorktree = false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 668 - 712, Update restore to verify the payload only once: retain the result returned by the initial verifyPayload(meta) call near the start of restore and reuse it where manifest is currently assigned later. Remove the second invocation while preserving the existing manifest-dependent restore flow.
837-851: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefine the minimum supported Git version for these CLI options.
git rev-parse --path-format=absoluteneeds Git 2.31, andgit apply --allow-emptyneeds Git 2.35. The code currently uses both without a supported-version declaration, so older Git clients fail intoSNAPSHOT_RESTORE_FAILEDinstead of reporting a clear Git version requirement. If a lower Git floor is intended, drop--allow-emptyhere because empty patches are already skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/worktree-snapshot-service.ts` around lines 837 - 851, Declare and enforce a minimum supported Git version of 2.35 wherever Git capability requirements are defined, covering both rev-parse --path-format=absolute and apply --allow-empty. Ensure older clients report the clear Git version requirement instead of surfacing SNAPSHOT_RESTORE_FAILED; if the project intentionally supports an older floor, remove --allow-empty from applyPatch because empty patches are already skipped.packages/server-core/src/git/__tests__/worktree-snapshot.test.ts (3)
492-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the leftover exploratory setup.
Line 492 and Line 493 move
record.expectedBranchtomain, and the comment at Line 494 then states the real intent before the same branch is moved again at Line 499. Only the second move matters. Deletekeepand the firstbranch -fso the test states one setup.♻️ Proposed cleanup
- // Advance the branch after capture. - const keep = await git(repo, ['rev-parse', '--verify', 'refs/heads/main']) - await git(repo, ['branch', '-f', record.expectedBranch, keep.trim()]) - // Actually: recreate the branch at a different commit (as if another actor - // pushed new work to it). + // Another actor advanced the branch after capture. writeFile(repo, 'new-work.txt', 'new work\n')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-snapshot.test.ts` around lines 492 - 499, In the worktree snapshot test setup, remove the unused keep assignment and the initial branch -f operation that moves record.expectedBranch to main. Preserve the subsequent commit creation and final branch -f operation, which alone establishes the intended advanced branch state.
643-689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a content-only edit case to the fingerprint test.
Every step in this test adds a path or stages a change, so the hashed
git status --porcelain=v2output changes each time. A second edit to a file that is already modified keeps the same status line, and porcelain v2 does not report the working-tree blob ID. Such a case currently produces an unchanged fingerprint, which is the defect flagged in worktree-snapshot-service.ts at Line 1024. Add that assertion so the parsing fix is enforced.💚 Proposed test addition
await git(record.checkoutPath, ['add', 'tracked.txt']) const afterStaged = await fingerprint(record) expect(afterStaged).not.toBe(afterDirty) + + // A content-only edit of an already-modified file must change the fingerprint. + writeFile(record.checkoutPath, 'tracked.txt', 'v3\n') + const afterFirstEdit = await fingerprint(record) + writeFile(record.checkoutPath, 'tracked.txt', 'v4\n') + expect(await fingerprint(record)).not.toBe(afterFirstEdit)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-snapshot.test.ts` around lines 643 - 689, Add a content-only edit assertion to the existing “binds owner set, policy, HEAD, index, worktree, and unique commits” test: after the staged-change fingerprint is captured, modify the already tracked file again without staging it, recompute the fingerprint, and assert it changes. Keep the existing owner, policy, and idempotence checks intact, ensuring the test exercises working-tree content changes that do not alter the porcelain-v2 status line.
554-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test and add real coverage for the path-escape guard.
The tampered manifest breaks
meta.manifestHash, soverifyPayloadrejects it at worktree-snapshot-service.ts Line 578 and the assertion expectsSNAPSHOT_VERIFY_FAILED. Restore never reachesrestoreFileEntry, so theSNAPSHOT_PATH_UNSAFEescape guard and the symlinked-parent guard stay uncovered, and the test name claims otherwise.Rename this test to state what it proves (the manifest is hash-authoritative). Then call
restoreFileEntryguards directly with a matchingmanifestHash, or add a separate test that recomputesmeta.manifestHashover the tampered manifest so the escaping entry reaches the restore loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/__tests__/worktree-snapshot.test.ts` around lines 554 - 579, Rename the existing test to describe that it verifies manifest-hash authority, since the tampered manifest is rejected by verifyPayload before restoreFileEntry. Add separate coverage for restoreFileEntry using a matching manifestHash (or invoke its guards directly) so ../escape.txt reaches restoration and asserts SNAPSHOT_PATH_UNSAFE; also cover a symlinked parent path and assert the same guard rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4354da19-9c9d-4eed-8e4b-3979c4dcf7f0
⛔ Files ignored due to path filters (7)
apps/electron/resources/release-notes/next.mdis excluded by!**/*.mddocs/adrs/2026-08-05-snapshot-backed-worktree-lifecycle.mdis excluded by!**/*.mddocs/adrs/index.mdis excluded by!**/*.mddocs/adrs/log.mdis excluded by!**/*.mddocs/index.mdis excluded by!**/*.mddocs/log.mdis excluded by!**/*.mddocs/specs/log.mdis excluded by!**/*.md
📒 Files selected for processing (35)
apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsxapps/electron/src/shared/__tests__/ipc-channels.test.tsapps/online-docs/core-concepts/git-worktrees.mdxe2e/tests/git/worktree-v2-manage.spec.tspackages/server-core/src/git/__tests__/path-leases.test.tspackages/server-core/src/git/__tests__/test-helpers.tspackages/server-core/src/git/__tests__/worktree-journal.test.tspackages/server-core/src/git/__tests__/worktree-lifecycle.test.tspackages/server-core/src/git/__tests__/worktree-registry.test.tspackages/server-core/src/git/__tests__/worktree-settings.test.tspackages/server-core/src/git/__tests__/worktree-snapshot.test.tspackages/server-core/src/git/index.tspackages/server-core/src/git/managed-worktree-service.tspackages/server-core/src/git/mutation-lock.tspackages/server-core/src/git/path-leases.tspackages/server-core/src/git/worktree-journal.tspackages/server-core/src/git/worktree-lifecycle-service.tspackages/server-core/src/git/worktree-registry.tspackages/server-core/src/git/worktree-settings-service.tspackages/server-core/src/git/worktree-snapshot-service.tspackages/server-core/src/handlers/rpc/git.test.tspackages/server-core/src/handlers/rpc/git.tspackages/server-core/src/sessions/SessionManager.tspackages/shared/src/i18n/locales/de.jsonpackages/shared/src/i18n/locales/en.jsonpackages/shared/src/i18n/locales/es.jsonpackages/shared/src/i18n/locales/hu.jsonpackages/shared/src/i18n/locales/ja.jsonpackages/shared/src/i18n/locales/pl.jsonpackages/shared/src/i18n/locales/zh-Hans.jsonpackages/shared/src/protocol/__tests__/git-contracts.test.tspackages/shared/src/protocol/channels.tspackages/shared/src/protocol/git.tspackages/shared/src/protocol/routing.tspackages/shared/src/protocol/types.ts
Verify-phase UAT (136 checks against real Git + restarts) and the real-app capture run surfaced five gaps against the approved acceptance criteria: - A flagged or active owner did not block manual deletion server-side. Flag state is deliberately not part of the preview fingerprint, so the removal transaction now enforces the protection for every caller (manual, retry, session-delete), matching the sweep (AC4, AC10). - Retry of a cleanup-failed record recomputed a working-tree fingerprint on a partially released checkout and always failed. The verified snapshot now governs the retry: if the checkout is still inspectable it must match the captured fingerprint, otherwise the snapshot wins (AC3, AC12). - Reconciliation classified missing/blocked records with no actionable recovery text (AC2, AC14). - The composer recovery badge ignored the persisted lifecycle recoveryState, showing a generic "missing" for snapshot-backed states and omitting the worktree name/branch (AC14). - The inventory refresh control rendered the raw key common.refresh: the key existed in no locale (AC1, AC21). Adds regression tests for each (7 server-core, 8 renderer) plus the seven new lifecycle recovery strings in all seven locales. Refs #41
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/electron/src/renderer/components/app-shell/input/checkout-controls.ts`:
- Around line 336-338: Update the checkout status decision flow to evaluate
checkout.recoveryState before the contextLoaded guard, ensuring persisted
lifecycle states return { kind: 'lifecycle', state } even when contextLoaded is
false. Add coverage for a resumed checkout with contextLoaded: false that
asserts the persisted lifecycle state is returned.
In `@packages/server-core/src/git/__tests__/worktree-lifecycle.test.ts`:
- Around line 953-962: The retry test must simulate an actual checkout mutation
rather than altering the stored snapshot fingerprint. Preserve the captured
fingerprint, modify a file under record.checkoutPath after capture, then call
lifecycle.retryWorktree and unconditionally assert retried is false and the
error contains “changed after its snapshot”; remove the .git existence guard.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ad16968-f8ca-4e7c-b445-85ac63eecaf7
⛔ Files ignored due to path filters (1)
AGENTS.mdis excluded by!**/*.md
📒 Files selected for processing (13)
apps/electron/src/renderer/components/app-shell/input/WorkspaceCheckoutBadge.tsxapps/electron/src/renderer/components/app-shell/input/__tests__/checkout-controls.test.tsapps/electron/src/renderer/components/app-shell/input/checkout-controls.tspackages/server-core/src/git/__tests__/worktree-lifecycle.test.tspackages/server-core/src/git/managed-worktree-service.tspackages/server-core/src/git/worktree-lifecycle-service.tspackages/shared/src/i18n/locales/de.jsonpackages/shared/src/i18n/locales/en.jsonpackages/shared/src/i18n/locales/es.jsonpackages/shared/src/i18n/locales/hu.jsonpackages/shared/src/i18n/locales/ja.jsonpackages/shared/src/i18n/locales/pl.jsonpackages/shared/src/i18n/locales/zh-Hans.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/shared/src/i18n/locales/ja.json
- packages/shared/src/i18n/locales/en.json
- packages/server-core/src/git/managed-worktree-service.ts
- packages/server-core/src/git/worktree-lifecycle-service.ts
AC14 requires any owner of a non-ready record to show its name/branch and recovery status. The server already persisted recoveryState on the session checkout, but nothing told the renderer to re-fetch the DTO after a lifecycle mutation, so a session whose worktree was just removed kept showing stale identity instead of the recovery badge (and, after restore, kept the old path). Adds a session_updated event to the protocol, emitted by SessionManager when the lifecycle applyOwnerSessionState hook mutates an owner's checkout (delete, restore, sweeps), and handled by the renderer exactly like session_created: re-fetch the session DTO and replace the atom. Adds an RPC-level test proving delete emits session_updated with recoveryState snapshotted and restore emits again with the state cleared and path moved. Also documents the Verify-phase UAT findings in the lifecycle ADR (flagged/ active enforcement in the removal transaction, snapshot-governed retry, recovery badge) and adds the release-notes bullet. Refs #41
…ontext The composer badge gated every recovery kind behind local Git context loading, but the persisted lifecycle recoveryState is authoritative server data: a fenced session whose checkout was removed may never load context, so it kept showing stale identity instead of the recovery badge. The persisted state now wins immediately (precedence lifecycle → blocked → missing → branch-drift); local inference stays suppressed until context loads so resumed sessions do not flash false drift warnings. Drops the temporary UAT capture spec (evidence captured in uat-evidence/electron-20260805-201815). Refs #41
- setArchived, detachSession, and recordFailure upserted full records
from a snapshot read outside the registry lock, overwriting concurrent
owner binds, detaches, and state transitions. Each now re-reads the
record inside registry.runExclusive and mutates only owned fields.
- runExclusive relabeled callback errors (LIFECYCLE_RECORD_MISSING,
LIFECYCLE_STATE_UNMANAGEABLE) as REGISTRY_LOCK_FAILED. Callback errors
now propagate unchanged; only lock acquisition and registry I/O are
wrapped.
- The cleanup sweep returned after the first removal, so a surplus beyond
the retention limit stayed materialized; the loop now drains every
candidate and reports the last removed ID. enqueueCleanup chains a
follow-up sweep for mid-sweep enqueues so callers await a sweep that
covers their candidate.
- reconcileJournal now prunes stale path leases, so markers from a
crashed process no longer fence deletions as foreign leases forever.
- The session-delete removal stamped owner state for the session being
deleted, persisting and recreating it at its original path after its
storage was staged away; only remaining owners are stamped now.
- quiesceRuntimes passed a precomputed false for preChatSettled, making
every processing owner unquiesceable and fencing all lifecycle
transactions with LIFECYCLE_NOT_QUIESCED; the pre-chat barrier is now
awaited within the bounded budget.
- The V2 removal branch dropped expectedConfirmation, so a destructive
confirmation no longer named what was removed; the fresh preview is
now compared against the confirmed counts before deleteWorktree.
- computeWorktreeFingerprint parsed porcelain-v2 records with
split('\t').pop(), which never resolved the path for space-separated
records and picked the origin path for renames, leaving content-only
edits undetected. Records are now parsed by type (1/2/u/?/!) and dirty
file content is bound, so the retry guard and race detection actually
see changed work.
- New user-facing lifecycle RPC errors use i18n keys with the state
interpolation preserved.
- The runExclusive child lock test now signals that it reached the
acquisition attempt, the snapshot CAS test actually collides a hidden
ref, the retry tests simulate a real post-capture checkout change, and
the sweep race test writes into an already-modified file. The
remove-worktree-cleanup-failure mock restores the real registry module
exports so its mock.module no longer leaks into other suites in the
same worker.
Refs #41
Translates the 68 settings.worktrees.* keys that shipped with English values in de, es, hu, ja, pl, and zh-Hans, preserving placeholders and the Worktree/.worktreeinclude/Git brand terms. Adds the two lifecycle RPC error keys (git.worktree.idRequired, git.worktree.usableFence) to all seven locales, alphabetically sorted. Refs #41
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/server-core/src/handlers/rpc/git.ts (1)
401-456: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFence lifecycle mutations until startup reconciliation completes.
Lines 401-456 call destructive lifecycle operations without
git.lifecycle.assertReady(). The handlers are available before the asynchronous reconciliation at Lines 305-336 completes. A request can delete, restore, archive, or permanently delete a worktree before journal recovery and path leases are reconciled.Call
git.lifecycle.assertReady()in every lifecycle mutation handler before delegating to the lifecycle service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/handlers/rpc/git.ts` around lines 401 - 456, Add git.lifecycle.assertReady() to each WORKTREE_DELETE, WORKTREE_RESTORE, WORKTREE_RETRY, WORKTREE_PERMANENT_DELETE, WORKTREE_ARCHIVE, and WORKTREE_UNARCHIVE handler immediately after assertWorktreeV2Enabled() and before invoking the lifecycle service. Preserve the existing delegation and error handling.packages/server-core/src/sessions/SessionManager.ts (1)
5741-5750: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLocalize the recovery fence error.
Line 5748 returns a raw English error to the user. Use
i18n.t('git.worktree.usableFence', { state }), as the RPC fence already does.As per coding guidelines, “Route all user-facing strings through
t()ori18n.t(); translation keys must exist in all seven locale files.”Proposed fix
if (state !== 'ready') { throw new Error( - `This session's worktree is ${state}. Open Worktrees settings to restore or resolve it before continuing.`, + i18n.t('git.worktree.usableFence', { state }), ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/sessions/SessionManager.ts` around lines 5741 - 5750, Update assertSessionCheckoutReady to replace the raw recovery-fence error text with i18n.t('git.worktree.usableFence', { state }), matching the existing RPC fence behavior. Ensure the git.worktree.usableFence translation key exists in all seven locale files.Source: Coding guidelines
apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx (1)
280-286: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPrevent stale mutations from updating another server target.
A user can change the selected server while a save or delete RPC is pending. The completed operation can then apply server A state to server B. A later save can persist those stale values to server B.
apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx#L280-L286: capture the target key before the RPC and discard the returned snapshot, toast, and refresh when the current target changed.apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx#L362-L375: do not call the callback captured for the previous target after deletion; clear only target-scoped busy state after confirming the target is still current.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx` around lines 280 - 286, Guard the save flow around the target-identifying state by capturing the selected server target before the RPC, then discard the returned snapshot, success toast, and inventory refresh if the target changed while it was pending. In the deletion flow around the delete RPC and its completion callback, do not invoke a callback captured for the previous target; after completion, clear target-scoped busy state only when the captured target is still current. Apply these changes at apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx lines 280-286 and 362-375.packages/server-core/src/git/managed-worktree-service.ts (2)
1014-1028: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve records in the
removingstate during reconciliation.
beginRemoval()sets the record toremovingbeforeremoveCheckoutFiles()runs. The lifecycle-owned exclusion list does not includeremoving.If reconciliation runs after the checkout disappears but before the registry record is removed, Lines 1085-1090 change
removingtomissingand write a recovery error. This can overwrite the in-flight removal state and make recovery classification inconsistent.Add
removingto the excluded states.Proposed fix
rec.state === 'restoring' || rec.state === 'cleanup-failed' || - rec.state === 'restore-failed' + rec.state === 'restore-failed' || + rec.state === 'removing'Also applies to: 1085-1090
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/managed-worktree-service.ts` around lines 1014 - 1028, Update the lifecycle-owned exclusion condition in the reconciliation logic to include the `removing` state alongside `snapshotting`, `snapshotted`, `restoring`, `cleanup-failed`, and `restore-failed`. Preserve `removing` records without reclassifying them as `missing` or writing a recovery error while removal is in flight.
1081-1107: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLocalize
lastErrorbefore exposing it to the Worktrees UI.These values are recovery text for the inventory row, but the code persists raw English strings. Store stable error codes and translate them at the renderer boundary, or use the shared i18n path in the server contract.
Add the required keys to
en,de,es,hu,ja,pl, andzh-Hans. Runbun run lint:i18n:parityandbun run lint:i18n:sorted.As per coding guidelines, user-facing strings must go through
t()ori18n.t(), and translation keys must exist in all seven locale files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server-core/src/git/managed-worktree-service.ts` around lines 1081 - 1107, Replace the raw English assignments to recV2.lastError in the reconciliation flow with stable error codes or the shared server-contract i18n path, then translate them at the Worktrees UI renderer boundary. Add the corresponding translation keys to en, de, es, hu, ja, pl, and zh-Hans, preserving the existing recovery messages and clearing behavior. Run bun run lint:i18n:parity and bun run lint:i18n:sorted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/electron/src/renderer/App.tsx`:
- Around line 926-940: Update the session event handling around the asynchronous
getSessionMessages call in the session_created/session_updated branch to track a
per-session event revision or deletion tombstone before starting the fetch. When
the promise resolves, apply replaceLoadedSession, addSession, or
initializeSessions only if the revision still matches and no later
session_deleted event was recorded; otherwise discard the stale response.
In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx`:
- Around line 382-387: Update the worktree management list around activeRows so
records in missing, cleanup-failed, snapshotted, and restore-failed states
remain accessible in a separate recovery section. Provide only the safe restore,
retry, or permanent-delete lifecycle actions for those records, while keeping
snapshot payloads hidden and preserving the existing active ready/unowned list
behavior.
In `@e2e/tests/git/worktree-v2-manage.spec.ts`:
- Around line 130-135: Update the worktrees auto-delete assertion in the test
around the worktrees-auto-delete switch to expect aria-checked "true" instead of
"false", and preserve the server-side initial settings so autoDeleteEnabled
defaults to enabled.
In `@packages/server-core/src/git/managed-worktree-service.ts`:
- Around line 198-199: Set autoDeleteEnabled to true in both fallback snapshot
objects returned by getSnapshot(), including the defaults near retentionLimit 15
and the corresponding snapshot around the additional referenced section, so V2
policy evaluation keeps automatic cleanup enabled by default.
---
Outside diff comments:
In `@apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx`:
- Around line 280-286: Guard the save flow around the target-identifying state
by capturing the selected server target before the RPC, then discard the
returned snapshot, success toast, and inventory refresh if the target changed
while it was pending. In the deletion flow around the delete RPC and its
completion callback, do not invoke a callback captured for the previous target;
after completion, clear target-scoped busy state only when the captured target
is still current. Apply these changes at
apps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsx lines
280-286 and 362-375.
In `@packages/server-core/src/git/managed-worktree-service.ts`:
- Around line 1014-1028: Update the lifecycle-owned exclusion condition in the
reconciliation logic to include the `removing` state alongside `snapshotting`,
`snapshotted`, `restoring`, `cleanup-failed`, and `restore-failed`. Preserve
`removing` records without reclassifying them as `missing` or writing a recovery
error while removal is in flight.
- Around line 1081-1107: Replace the raw English assignments to recV2.lastError
in the reconciliation flow with stable error codes or the shared server-contract
i18n path, then translate them at the Worktrees UI renderer boundary. Add the
corresponding translation keys to en, de, es, hu, ja, pl, and zh-Hans,
preserving the existing recovery messages and clearing behavior. Run bun run
lint:i18n:parity and bun run lint:i18n:sorted.
In `@packages/server-core/src/handlers/rpc/git.ts`:
- Around line 401-456: Add git.lifecycle.assertReady() to each WORKTREE_DELETE,
WORKTREE_RESTORE, WORKTREE_RETRY, WORKTREE_PERMANENT_DELETE, WORKTREE_ARCHIVE,
and WORKTREE_UNARCHIVE handler immediately after assertWorktreeV2Enabled() and
before invoking the lifecycle service. Preserve the existing delegation and
error handling.
In `@packages/server-core/src/sessions/SessionManager.ts`:
- Around line 5741-5750: Update assertSessionCheckoutReady to replace the raw
recovery-fence error text with i18n.t('git.worktree.usableFence', { state }),
matching the existing RPC fence behavior. Ensure the git.worktree.usableFence
translation key exists in all seven locale files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f5cd643-e418-458b-a66b-91f860dec529
⛔ Files ignored due to path filters (6)
apps/electron/resources/release-notes/next.mdis excluded by!**/*.mddocs/adrs/2026-08-05-snapshot-backed-worktree-lifecycle.mdis excluded by!**/*.mddocs/adrs/log.mdis excluded by!**/*.mddocs/index.mdis excluded by!**/*.mddocs/log.mdis excluded by!**/*.mddocs/specs/log.mdis excluded by!**/*.md
📒 Files selected for processing (33)
apps/electron/src/renderer/App.tsxapps/electron/src/renderer/components/app-shell/PanelHeader.tsxapps/electron/src/renderer/components/app-shell/input/__tests__/checkout-controls.test.tsapps/electron/src/renderer/components/app-shell/input/checkout-controls.tsapps/electron/src/renderer/components/right-sidebar/git-changes/GitActionControl.tsxapps/electron/src/renderer/pages/settings/WorktreesSettingsPage.tsxapps/online-docs/core-concepts/git-worktrees.mdxe2e/tests/git/existing-worktree.spec.tse2e/tests/git/github-integration.spec.tse2e/tests/git/managed-worktree.spec.tse2e/tests/git/worktree-v2-manage.spec.tspackages/server-core/src/git/__tests__/remove-worktree-cleanup-failure.test.tspackages/server-core/src/git/__tests__/worktree-lifecycle.test.tspackages/server-core/src/git/__tests__/worktree-registry.test.tspackages/server-core/src/git/__tests__/worktree-settings.test.tspackages/server-core/src/git/__tests__/worktree-snapshot.test.tspackages/server-core/src/git/managed-worktree-service.tspackages/server-core/src/git/worktree-lifecycle-service.tspackages/server-core/src/git/worktree-registry.tspackages/server-core/src/git/worktree-settings-service.tspackages/server-core/src/git/worktree-snapshot-service.tspackages/server-core/src/handlers/rpc/git.tspackages/server-core/src/handlers/rpc/headless-server-flow.test.tspackages/server-core/src/sessions/SessionManager.tspackages/shared/src/i18n/locales/de.jsonpackages/shared/src/i18n/locales/en.jsonpackages/shared/src/i18n/locales/es.jsonpackages/shared/src/i18n/locales/hu.jsonpackages/shared/src/i18n/locales/ja.jsonpackages/shared/src/i18n/locales/pl.jsonpackages/shared/src/i18n/locales/zh-Hans.jsonpackages/shared/src/protocol/dto.tspackages/shared/src/protocol/git.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/electron/src/renderer/components/app-shell/input/tests/checkout-controls.test.ts
- packages/server-core/src/git/tests/worktree-registry.test.ts
- packages/server-core/src/git/tests/worktree-snapshot.test.ts
- apps/electron/src/renderer/components/app-shell/input/checkout-controls.ts
- apps/online-docs/core-concepts/git-worktrees.mdx
- packages/shared/src/i18n/locales/es.json
- packages/shared/src/i18n/locales/pl.json
- packages/server-core/src/git/worktree-registry.ts
- packages/server-core/src/git/worktree-snapshot-service.ts
- packages/server-core/src/git/worktree-settings-service.ts
- packages/server-core/src/git/tests/worktree-lifecycle.test.ts
- packages/server-core/src/git/tests/worktree-settings.test.ts
- packages/server-core/src/git/worktree-lifecycle-service.ts
- packages/shared/src/protocol/git.ts
…reshes - restoreWorktree stamped owner state and rebound leases from the pre-restore owner list, so a session that detached while the restore awaited snapshot I/O was re-associated with the restored checkout. The ready-record commit now captures the CURRENT owner set under the registry lock and the session updates and lease rebinding use it; covered by 'restore never re-associates an owner that detached mid-restore'. - The renderer's session_created/session_updated handler applied an async getSessionMessages fetch even when session_deleted arrived first, resurrecting the deleted session. A per-session event revision is now bumped on every session event and stale refreshes are discarded. Refs #41
…finalization The lease rebinding after a restore used the owner set captured at the ready-commit, so a session that detached while owner state was being stamped could be leased back onto the restored checkout. The owner set is now re-read under the registry lock immediately before stamping and the leases are rebound under the same lock hold as the owner-set observation, so a detachment (which needs that lock to commit) can never be followed by a stale rebind. recordStateForSession also resolved records only through the session's lease path; a restore moves the checkout to a new path before the owner leases are rebound, so a session detaching in that window could not find its record and its ownership was leaked. It now falls back to the owner set, keeping detach and fencing decisions correct during finalization. Covered by 'restore never leases back an owner that detached during session stamping', which detaches inside the stamping hook and asserts the record ends with zero owners and no lease. Refs #41
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/electron/src/renderer/App.tsx (1)
931-961: 🎯 Functional Correctness | 🟠 MajorComplete the revision fence for every stale refresh write.
The revision increments only for lifecycle events. A normal agent event can update a session after Line 936 starts the fetch. The stale fetch can then overwrite that newer atom state at Line 945.
If
getSessionMessages()returnsnull, thegetSessions()fallback has no revision check after it resolves. Asession_deletedevent that occurs during that request can be undone byinitializeSessions.Increment the revision for every session event. Recheck the captured revision immediately before
replaceLoadedSession,addSession, andinitializeSessions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/electron/src/renderer/App.tsx` around lines 931 - 961, Complete the revision fence in the session event handler by incrementing the session revision for every event, not only session_created/session_updated. Before applying refreshed data, recheck the captured revision immediately before replaceLoadedSession and addSession, and capture/revalidate the revision around the getSessions fallback before initializeSessions so session_deleted or newer events cannot be overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/electron/src/renderer/App.tsx`:
- Around line 931-961: Complete the revision fence in the session event handler
by incrementing the session revision for every event, not only
session_created/session_updated. Before applying refreshed data, recheck the
captured revision immediately before replaceLoadedSession and addSession, and
capture/revalidate the revision around the getSessions fallback before
initializeSessions so session_deleted or newer events cannot be overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ceabb79-b483-48c0-9b1e-63e81a66b168
📒 Files selected for processing (3)
apps/electron/src/renderer/App.tsxpackages/server-core/src/git/__tests__/worktree-lifecycle.test.tspackages/server-core/src/git/worktree-lifecycle-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-core/src/git/worktree-lifecycle-service.ts
| const current = tx.get(record.managedWorktreeId) | ||
| if (current) stampOwners = [...current.ownerSessionIds] | ||
| }) | ||
| await this.deps.applyOwnerSessionState?.(stampOwners, { |
There was a problem hiding this comment.
stampOwners is copied while the registry lock is held, but the lock is released before the awaited applyOwnerSessionState call. A session can detach in that interval and still receive restored checkout and recovery state even though it is no longer an owner. Revalidate ownership immediately before applying the state, or make detachment and state application atomic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/server-core/src/git/worktree-lifecycle-service.ts
Line: 598
Comment:
**Detached owner state is restored**
`stampOwners` is copied while the registry lock is held, but the lock is released before the awaited `applyOwnerSessionState` call. A session can detach in that interval and still receive restored checkout and recovery state even though it is no longer an owner. Revalidate ownership immediately before applying the state, or make detachment and state application atomic.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Closes #41
Scope
Worktree V2 Phase 2: snapshot-backed management and automatic cleanup. Every destructive V2 path now enters one lifecycle service; a verified snapshot is mandatory before any materialized checkout is released.
What landed
packages/server-core/src/git/worktree-snapshot-service.ts): streaming binary capture of staged/unstaged projections, untracked regular files with exact permission bits, non-dereferenced symlink nodes, and.worktreeincludefiles; preflight 10k-file/100 MiB bounds; component hashing; CAS-created hiddenrefs/kata/worktree-snapshots/<id>refs; atomic verified publish; exact restore (byte/mode, never force-resets an advanced branch); ownership-proved permanent deletion; no-follow extraction; unresolved merge/rebase/cherry-pick markers and sparse/unmerged/operation state block capture.worktree-lifecycle-service.ts): per-server policy, inventory, fresh preview fingerprints binding owner/path/Git/content/policy, snapshot-first delete with pre-capture and pre-release revalidation (plus stability fingerprints for sweeps/session-delete), restore, retry of safe failed steps, permanent delete with zero-owner + second-confirmation rules, per-owner archive with all-archived cleanup, LRU retention sweeps fenced at the policy-version boundary,lastUsedAthooks, journal-based startup classification, pending-restore cleanup, and orphaned-payload GC.autoDeleteEnabled+retentionLimit(default 15, 1–1000).unowned+ sweep); "Delete session and worktree" is snapshot-first, staged, and rolls back on failure; accepted messages touchlastUsedAt; conversation-branch children lease their checkout; the legacy removal RPC routes through the lifecycle in V2.Verification
packages/shared:bun test src/git+tsc --noEmit✅packages/server-core: fullbun test(468 tests) +typecheck✅; the spec's 11-file target set (161 tests) ✅apps/electron: required renderer dirs (172 tests) +typecheck✅lint:i18n:parity+lint:i18n:sorted✅ (1,703 keys × 7 locales)bun run e2e --list --grep '@worktree-v2.*(cleanup|restore|manage)'✅; dev E2E ✅; release E2E (packaged app) ✅Review gates
Two independent spec-compliance reviews and two independent code-quality review rounds (fresh-context reviewer subagents) were run; all Critical/Important findings were fixed and regression-tested. Residual, consciously accepted: cross-process lease replacement is not serialized (fail-safe direction: a foreign marker blocks removal), and external non-Kata writers remain detected-but-not-serialized (final fingerprint recheck before source release).
Build completion report: #41 (comment)
Summary by CodeRabbit
Greptile Summary
This change adds snapshot-backed worktree lifecycle recovery and related management controls. A restore can still apply checkout and recovery state to a session that detached during the restore operation, leaving stale worktree state associated with a session that no longer owns the worktree.
Confidence Score: 4/5
Not safe to merge until restored state is limited to sessions that still own the worktree.
A deterministic restore flow reproduced a detach interleaving that leaves a non-owner session with restored checkout and recovery state.
Files Needing Attention: packages/server-core/src/git/worktree-lifecycle-service.ts
What T-Rex did
Comments Outside Diff (1)
General comment
session-1afterstampOwnerswas read and beforeapplyOwnerSessionStateran. Restore completed and the registry correctly had no owners, but the hook still receivedsession-1with the restored checkout state.stampOwnersis a stale copy made insideregistry.runExclusiveatworktree-lifecycle-service.ts:594-597; once that lock is released,detachSessioncan remove the owner before the separate awaited hook call at line 598.Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix(worktree): observe owners under the ..." | Re-trigger Greptile