Add safe lane storage lifecycle cleanup - #932
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughAdds guarded lane archive-and-reclaim and restore flows, storage lifecycle scanning, local cleanup policy persistence, runtime/IPC/CLI APIs, and renderer/TUI controls with confirmation and risk previews. ChangesLane storage lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7c60ffab9
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
apps/desktop/src/main/services/ipc/registerIpc.ts (1)
5828-5830: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the overlay-context failure instead of swallowing it.
IPC.lanesDelete(Line 5863) logslane_env_cleanup.pre_delete_context_failedfor the same failure mode; here the reclaim path silently skips env teardown, so leaked lane environment resources leave no trace.♻️ Suggested change
const envContext = ctx.laneEnvironmentService - ? await resolveLaneOverlayContext(ctx, arg.laneId).catch(() => null) + ? await resolveLaneOverlayContext(ctx, arg.laneId).catch((error: unknown) => { + ctx.logger.warn("lane_env_cleanup.pre_reclaim_context_failed", { + laneId: arg.laneId, + error: getErrorMessage(error), + }); + return null; + }) : null;🤖 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/desktop/src/main/services/ipc/registerIpc.ts` around lines 5828 - 5830, Update the envContext resolution in IPC.lanesDelete to log failures from resolveLaneOverlayContext instead of silently converting them to null. Reuse the existing lane_env_cleanup.pre_delete_context_failed logging behavior and context used by the later cleanup path, while preserving the null fallback so deletion continues.apps/ade-cli/src/tuiClient/__tests__/appInput.test.ts (1)
1567-1601: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage of the dirty/confirmation-required path; consider adding cases for
hardBlockedand the "ready to reclaim" (no blockers) branch.The current test only exercises the
dirty_worktree/confirmation_requiredscenario.formatLaneReclaimPreviewalso has distinct output for adisposition: "blocked"reason (different heading andnextCommandtext) and for the no-blockedReasonscase ("Safety check: Ready to reclaim."). A couple more cases would lock in those branches too.🤖 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/ade-cli/src/tuiClient/__tests__/appInput.test.ts` around lines 1567 - 1601, Add tests alongside the existing formatLaneReclaimPreview test for a hard-blocked input with a blockedReasons entry using disposition "blocked", asserting its distinct heading and nextCommand text, and for an input with no blockedReasons, asserting the "Safety check: Ready to reclaim." output. Reuse the existing preview fixture values while changing only the fields needed to exercise each branch.apps/ade-cli/src/tuiClient/app.tsx (1)
9972-10034: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive reclaim gating state from the same source as the preview text.
archiveAndReclaimenforcesrisk.dirtyand/lane archive-and-reclaim ... force-dirty, but the preview hint is driven byrisk.blockedReasons.some((reason) => reason.code === "dirty_worktree"). SincegetReclaimRiskcurrently derives the dirty confirmation block fromdeleteRisk.dirty, deduplicate this into a small shared helper so future changes to “need force-dirty” logic can’t make the preview and gateway disagree.🤖 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/ade-cli/src/tuiClient/app.tsx` around lines 9972 - 10034, Derive the dirty confirmation state used by the reclaim preview and archive flow from the same shared helper that determines the dirty-worktree block. Update getReclaimRisk and the archiveAndReclaim gating path so both use this helper instead of independently checking deleteRisk.dirty or blockedReasons, while preserving force-dirty confirmation behavior.apps/desktop/src/main/services/state/kvDb.ts (1)
3583-3609: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
local_lane_storage_staterows are never deleted when a lane is deleted.The table is keyed by
lane_idwith no FK/cascade, andcleanupLaneDatabaseRowsinapps/desktop/src/main/services/lanes/laneService.ts(which deleteslane_state_snapshots,lane_worktree_locks, etc.) does not touch it. Deleted lanes leave permanent orphan rows carrying an absoluteworktree_pathandlast_errortext.Add
delete from local_lane_storage_state where lane_id = ? and project_id = ?tocleanupLaneDatabaseRows(the restore path already clears it).🤖 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/desktop/src/main/services/state/kvDb.ts` around lines 3583 - 3609, Update cleanupLaneDatabaseRows in laneService.ts to delete the matching local_lane_storage_state row using both lane_id and project_id. Add this cleanup alongside the existing lane-related database row deletions, preserving the restore path’s existing behavior.
🤖 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/desktop/src/main/services/adeActions/registry.ts`:
- Around line 2328-2341: Validate args?.confirmation at the start of
archiveAndReclaim, requiring it to equal exactly "RECLAIM" and rejecting any
missing or different value before resolving teardown state or invoking
runtime.laneService.archiveAndReclaim. Preserve the existing laneId validation
and pass the validated confirmation through to the service operation.
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 5365-5388: Guard the residual lstat in the cleanup block around
runGit and removeWorktreeDirectoryWithRecovery so it runs only when
normalizedWorktree still exists. If git removal fails but the directory is
already absent, skip residual validation and recovery, allowing the existing
worktreeRemoved check to treat cleanup as successful; retain the current
validation and recovery behavior when the directory remains.
- Around line 5546-5559: Wrap the `lanes` update and `local_lane_storage_state`
delete in the same database transaction within the restore flow. Commit only
after both statements succeed, and roll back before the existing
`runGitWorktreeMutation` cleanup when either write fails, so the lane remains
consistent with the storage-state row.
In `@apps/desktop/src/main/services/storage/storageInsightsService.ts`:
- Around line 349-369: Update latestLaneActivityMs to return the current
timestamp when createdAt and all chat, terminal, and operation activity values
are unparseable or absent. Preserve the maximum finite activity timestamp when
at least one valid value exists, ensuring the method never returns -Infinity.
- Around line 1346-1349: Update runMaintenanceNow so runLifecycleScan is
best-effort: catch and suppress its rejection, then always execute and return
runMaintenanceSweep("manual"). Preserve the existing manual maintenance report
and avoid changing the daemon timer paths.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 5056-5062: The getReclaimRisk runtime payload in the preload API
must match the registered object shape. Update the callProjectRuntimeActionOr
invocation for getReclaimRisk to pass the args record under the expected args
property, while preserving the existing IPC fallback using args.
In `@apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx`:
- Around line 495-504: Require an explicit discard-uncommitted-changes
acknowledgement before reclaiming dirty lanes. In ManageLaneDialog’s
archiveAndReclaim flow, add a checkbox shown when reclaimRisk.dirty and derive
forceDirty only from that checkbox; mirror the same acknowledgement in
StorageSection’s ReclaimConfirmDialog and stop passing reclaimRisk.dirty
directly in both affected locations:
apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx#L495-L504 and
apps/desktop/src/renderer/components/settings/StorageSection.tsx#L1068-L1086.
- Around line 666-680: Update the reclaim button in the ManageLaneDialog reclaim
action to remain disabled while reclaimRisk is null, including hosts that cannot
provide a risk snapshot. Preserve the existing laneActionBusy, reclaimBusy,
reclaimBlocked, and confirmation guards, and ensure the action is only enabled
after a risk result is available.
In `@apps/desktop/src/renderer/components/settings/StorageSection.tsx`:
- Around line 980-985: Update the lane cleanup policy editor initialization and
save flow around setPolicy and savePolicy so the editable policy contains only
config.local.laneCleanup values, rather than merging
config.effective.laneCleanup into it. Expose effective values through
placeholders or help text for untouched fields, while preserving the existing
clearing of non-local fields and ensuring Save does not persist inherited
defaults as local overrides.
- Around line 774-787: The archive/reclaim overlay around the “Archive &
reclaim” heading must follow the existing StorageCleanupDialog accessibility
pattern: add dialog semantics with role="dialog" and aria-modal, close it on
Escape, and contain keyboard focus within the dialog while it is open. Reuse the
existing dialog implementation or its established focus-management utilities
rather than introducing a separate hand-rolled approach.
In `@apps/desktop/src/renderer/webclient/adapter/lanes.ts`:
- Around line 102-108: Update the unarchive adapter method to emit the
appropriate lane lifecycle event using the returned result.lane before returning
the restore result. Match the existing event-emission pattern used by archive
and delete, including the expected lane-unarchived/lane-restored event name and
payload.
---
Nitpick comments:
In `@apps/ade-cli/src/tuiClient/__tests__/appInput.test.ts`:
- Around line 1567-1601: Add tests alongside the existing
formatLaneReclaimPreview test for a hard-blocked input with a blockedReasons
entry using disposition "blocked", asserting its distinct heading and
nextCommand text, and for an input with no blockedReasons, asserting the "Safety
check: Ready to reclaim." output. Reuse the existing preview fixture values
while changing only the fields needed to exercise each branch.
In `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 9972-10034: Derive the dirty confirmation state used by the
reclaim preview and archive flow from the same shared helper that determines the
dirty-worktree block. Update getReclaimRisk and the archiveAndReclaim gating
path so both use this helper instead of independently checking deleteRisk.dirty
or blockedReasons, while preserving force-dirty confirmation behavior.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 5828-5830: Update the envContext resolution in IPC.lanesDelete to
log failures from resolveLaneOverlayContext instead of silently converting them
to null. Reuse the existing lane_env_cleanup.pre_delete_context_failed logging
behavior and context used by the later cleanup path, while preserving the null
fallback so deletion continues.
In `@apps/desktop/src/main/services/state/kvDb.ts`:
- Around line 3583-3609: Update cleanupLaneDatabaseRows in laneService.ts to
delete the matching local_lane_storage_state row using both lane_id and
project_id. Add this cleanup alongside the existing lane-related database row
deletions, preserving the restore path’s existing behavior.
🪄 Autofix (Beta)
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: a037591f-8d6b-4d97-8042-4f9d315f33d0
⛔ Files ignored due to path filters (4)
docs/features/lanes/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (40)
apps/ade-cli/README.mdapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/tuiClient/__tests__/appInput.test.tsapps/ade-cli/src/tuiClient/__tests__/commands.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/commands.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/config/projectConfigService.test.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/laneStorageLifecycle.test.tsapps/desktop/src/main/services/lanes/laneWorktreeLockService.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/main/services/storage/storageLedger.tsapps/desktop/src/main/services/usage/usageStatsStore.tsapps/desktop/src/main/services/usage/usageTrackingService.test.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsxapps/desktop/src/renderer/components/settings/StorageSection.test.tsxapps/desktop/src/renderer/components/settings/StorageSection.tsxapps/desktop/src/renderer/webclient/adapter/lanes.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/lanes.tsapps/desktop/src/shared/types/prs.tsapps/desktop/src/shared/types/storage.ts
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a9552f10
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29cdb0d5e1
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5f78bf58a
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsx (1)
98-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape handling picks the earliest-mounted frame, not the topmost.
The listener is registered on
windowin capture phase and callsstopPropagation(), so with twoStorageDialogFrames mounted simultaneously (e.g. a cleanup dialog plus the reclaim confirm dialog) the first-mounted one consumes Escape and closes, while the visually topmost dialog stays open. Scoping the handler to the dialog element (or tracking a mounted-frame stack and only reacting when last) makes Escape always close the front dialog.🤖 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/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsx` around lines 98 - 131, Update the keyboard handler in the StorageDialogFrame effect so Escape is handled only by the visually topmost mounted dialog, rather than every window-level listener competing in capture order. Scope the keydown listener to the dialog element or use an existing mounted-frame ordering mechanism, ensuring the front frame closes while underlying frames ignore Escape; preserve the existing canCloseRef and closeRef behavior.
🤖 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/desktop/src/main/services/lanes/laneRuntimeLifecycle.ts`:
- Around line 77-80: Update the lane cleanup flow around removeRoute and the
allocator lease check so allocator.release(laneId) runs in a finally block even
when removeRoute throws. Preserve the existing active-lease condition and
route-removal behavior.
In `@apps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsx`:
- Around line 207-216: Update the mocked blockedReasons entry in the “requires
an explicit acknowledgement before discarding dirty changes” test to use the IPC
contract’s “dirty_worktree” code instead of “dirty”, while preserving the
existing confirmation_required disposition and message.
In `@apps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.ts`:
- Around line 278-302: The web unarchive adapter’s lifecycle event selection
must reflect worktree recreation. Update the lanes.unarchive handling in
lanes.ts so it emits lane-restored when the result has worktreeRecreated: true,
and lane-unarchived otherwise, while preserving the returned unarchive result
and existing event payload fields.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsx`:
- Around line 98-131: Update the keyboard handler in the StorageDialogFrame
effect so Escape is handled only by the visually topmost mounted dialog, rather
than every window-level listener competing in capture order. Scope the keydown
listener to the dialog element or use an existing mounted-frame ordering
mechanism, ensuring the front frame closes while underlying frames ignore
Escape; preserve the existing canCloseRef and closeRef behavior.
🪄 Autofix (Beta)
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: 88002a5e-893f-4a91-87e3-8175176318a6
📒 Files selected for processing (22)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/lanes/laneRuntimeLifecycle.test.tsapps/desktop/src/main/services/lanes/laneRuntimeLifecycle.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/main/services/lanes/laneStorageLifecycle.test.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsxapps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsxapps/desktop/src/renderer/components/settings/StorageSection.test.tsxapps/desktop/src/renderer/components/settings/StorageSection.tsxapps/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsxapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/lanes.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/ade-cli/src/bootstrap.ts
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/webclient/adapter/lanes.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/main/services/adeActions/registry.ts
- apps/desktop/src/renderer/components/settings/StorageSection.test.tsx
- apps/desktop/src/main/services/storage/storageInsightsService.test.ts
- apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
- apps/desktop/src/main/services/storage/storageInsightsService.ts
- apps/desktop/src/renderer/components/settings/StorageSection.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7426b394f
ℹ️ 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".
| if (!candidate.dueByAge && stillNeedsArchive <= 0) continue; | ||
| try { | ||
| if (hasActiveLaneWorktreeLock(candidate.laneId)) continue; | ||
| laneService.archive({ laneId: candidate.laneId }); |
There was a problem hiding this comment.
Dispatch lane-archived automations during policy scans
When a configured lifecycle scan archives a lane, this calls laneService.archive directly but never invokes automationService.onLaneArchived; the IPC and ADE-action archive paths explicitly invoke that hook, which automationService.ts:3927 maps to the lane.archived trigger. Consequently, automations using that trigger silently do not run when the new policy auto-archives a lane, so this path should route through the same notifier or receive an equivalent callback.
Useful? React with 👍 / 👎.
| reclaimArchivedAfterHours?: number; | ||
| /** @deprecated Read as reclaimArchivedAfterHours for older local.yaml files. */ | ||
| autoDeleteArchivedAfterHours?: number; | ||
| /** Also delete the remote branch when auto-deleting. */ | ||
| /** @deprecated Cleanup never deletes remote branches automatically. */ | ||
| deleteRemoteBranchOnCleanup?: boolean; |
There was a problem hiding this comment.
Update the canonical cleanup configuration schema
The cleanup contract now uses reclaimArchivedAfterHours and treats the former deletion fields as deprecated, but docs/features/onboarding-and-settings/configuration-schema.md:171-184 still presents only autoDeleteArchivedAfterHours and deleteRemoteBranchOnCleanup, and still says the UI is in the now-removed LaneBehaviorSection controls. Users following that schema will configure behavior that no longer deletes anything and will not discover the replacement key or Storage UI; update the canonical schema alongside this type change.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
Summary
Safety
ADE never automatically removes active, attached, running, dirty, unmerged, or unverified work. Filesystem cleanup requires preview and confirmation, rejects symlinks/path traversal, and verifies the exact Git worktree path and branch immediately before removal.
Validation
Summary by CodeRabbit
New Features
/lane reclaim-previewand/lane archive-and-reclaim(and matching CLI commands), including dirty-worktree safeguards.Bug Fixes