Skip to content

Add safe lane storage lifecycle cleanup - #932

Merged
arul28 merged 6 commits into
mainfrom
ade/start-skill-discuss-all-befor-06fb0d77
Jul 28, 2026
Merged

Add safe lane storage lifecycle cleanup#932
arul28 merged 6 commits into
mainfrom
ade/start-skill-discuss-all-befor-06fb0d77

Conversation

@arul28

@arul28 arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

  • enforce lane cleanup settings in the backend with conservative safety gates
  • add Archive & Reclaim with safe restore, retry state, ownership checks, and cross-process locking
  • add a plain-language Settings > Storage review screen with sizes, ages, ownership, blocked reasons, and reclaim estimates
  • add desktop, CLI, TUI, docs, and lifecycle regression coverage

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

  • /quality: CLEAN
  • focused desktop tests: 306 passed
  • affected desktop CI shards 1, 2, 4, 6, 8: 6,608 passed; 2 existing e2e skips
  • ADE CLI: 2,428 passed
  • ade code TUI: 1,021 passed
  • docs validator: 203 files passed
  • local typecheck reaches only the known agentChatService SDK-definition mismatch already present on origin/main; no storage/CLI/TUI type errors

Summary by CodeRabbit

  • New Features

    • Added lane reclaim previews with detailed risk/block reasons, plus confirmation-based Archive & Reclaim and lane restore (Unarchive).
    • Added /lane reclaim-preview and /lane archive-and-reclaim (and matching CLI commands), including dirty-worktree safeguards.
    • Expanded Storage settings/UI with reclaim and review workflows for archived/reclaimable lanes, and new lifecycle scan behavior.
  • Bug Fixes

    • Improved restore messaging and setup-warning reporting after worktree recreation.
    • Tightened safety gating and serialized cleanup so reclaim/archive/restore flows don’t conflict and only execute when fully valid.

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview Jul 28, 2026 11:03pm

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Lane storage lifecycle

Layer / File(s) Summary
Contracts, configuration, and local persistence
apps/desktop/src/shared/types/*, apps/desktop/src/main/services/config/*, apps/desktop/src/main/services/state/kvDb.ts
Adds reclaim and restore types, lifecycle snapshot metadata, cleanup-policy migration and validation, and local-only lifecycle state tables.
Lane reclaim and restore service
apps/desktop/src/main/services/lanes/*
Adds risk assessment, guarded worktree ownership checks, archive-and-reclaim execution, asynchronous unarchive, retry state, lifecycle events, and concurrency locking.
Storage lifecycle scan and review cleanup
apps/desktop/src/main/services/storage/*, apps/desktop/src/main/main.ts, apps/ade-cli/src/bootstrap.ts
Scans inactive lanes, records reclaim review state, enriches storage snapshots, serializes cleanup, schedules lifecycle scans, and changes filesystem maintenance to review-only behavior.
Runtime actions, IPC, and CLI commands
apps/desktop/src/main/services/adeActions/*, apps/desktop/src/main/services/ipc/*, apps/desktop/src/preload/*, apps/desktop/src/shared/ipc.ts, apps/desktop/src/renderer/webclient/*, apps/ade-cli/src/cli.ts, apps/ade-cli/src/services/sync/*
Routes reclaim-risk, archive-and-reclaim, and unarchive operations through desktop runtime actions, IPC, preload APIs, web adapters, synchronized commands, and CLI planning.
Renderer and TUI reclaim experience
apps/desktop/src/renderer/components/lanes/*, apps/desktop/src/renderer/components/settings/*, apps/ade-cli/src/tuiClient/*, apps/desktop/src/renderer/browserMock.ts
Adds reclaim confirmation dialogs, storage policy and review panels, restore/reclaim actions, lifecycle metadata, slash-command previews, and related test coverage.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • arul28/ADE#380: Both changes modify ManageLaneDialog lane-tab initialization and archive-related UI behavior.
  • arul28/ADE#508: Both modify lane lifecycle and worktree deletion behavior.
  • arul28/ADE#711: Both modify lane unarchive lifecycle handling and event plumbing.

Suggested labels: desktop, docs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: safer lane storage lifecycle cleanup with reclaim and restore support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade/start-skill-discuss-all-befor-06fb0d77

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@arul28 arul28 changed the title lane-storage-cleanup-lifecycle -> Primary Add safe lane storage lifecycle cleanup Jul 28, 2026
@arul28

arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/desktop/src/main/services/lanes/laneService.ts
Comment thread apps/desktop/src/main/services/lanes/laneService.ts Outdated
Comment thread apps/desktop/src/main/services/storage/storageInsightsService.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (4)
apps/desktop/src/main/services/ipc/registerIpc.ts (1)

5828-5830: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log the overlay-context failure instead of swallowing it. IPC.lanesDelete (Line 5863) logs lane_env_cleanup.pre_delete_context_failed for 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 win

Good coverage of the dirty/confirmation-required path; consider adding cases for hardBlocked and the "ready to reclaim" (no blockers) branch.

The current test only exercises the dirty_worktree / confirmation_required scenario. formatLaneReclaimPreview also has distinct output for a disposition: "blocked" reason (different heading and nextCommand text) and for the no-blockedReasons case ("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 win

Derive reclaim gating state from the same source as the preview text.

archiveAndReclaim enforces risk.dirty and /lane archive-and-reclaim ... force-dirty, but the preview hint is driven by risk.blockedReasons.some((reason) => reason.code === "dirty_worktree"). Since getReclaimRisk currently derives the dirty confirmation block from deleteRisk.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_state rows are never deleted when a lane is deleted.

The table is keyed by lane_id with no FK/cascade, and cleanupLaneDatabaseRows in apps/desktop/src/main/services/lanes/laneService.ts (which deletes lane_state_snapshots, lane_worktree_locks, etc.) does not touch it. Deleted lanes leave permanent orphan rows carrying an absolute worktree_path and last_error text.

Add delete from local_lane_storage_state where lane_id = ? and project_id = ? to cleanupLaneDatabaseRows (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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a97221 and a7c60ff.

⛔ Files ignored due to path filters (4)
  • docs/features/lanes/README.md is excluded by !docs/**
  • docs/features/onboarding-and-settings/README.md is excluded by !docs/**
  • docs/features/storage-and-recovery/README.md is excluded by !docs/**
  • docs/logging.md is excluded by !docs/**
📒 Files selected for processing (40)
  • apps/ade-cli/README.md
  • apps/ade-cli/src/bootstrap.ts
  • apps/ade-cli/src/cli.test.ts
  • apps/ade-cli/src/cli.ts
  • apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts
  • apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
  • apps/ade-cli/src/tuiClient/__tests__/appInput.test.ts
  • apps/ade-cli/src/tuiClient/__tests__/commands.test.ts
  • apps/ade-cli/src/tuiClient/app.tsx
  • apps/ade-cli/src/tuiClient/commands.ts
  • apps/desktop/src/main/main.ts
  • apps/desktop/src/main/services/adeActions/registry.test.ts
  • apps/desktop/src/main/services/adeActions/registry.ts
  • apps/desktop/src/main/services/config/projectConfigService.test.ts
  • apps/desktop/src/main/services/config/projectConfigService.ts
  • apps/desktop/src/main/services/ipc/registerIpc.ts
  • apps/desktop/src/main/services/lanes/laneService.test.ts
  • apps/desktop/src/main/services/lanes/laneService.ts
  • apps/desktop/src/main/services/lanes/laneStorageLifecycle.test.ts
  • apps/desktop/src/main/services/lanes/laneWorktreeLockService.ts
  • apps/desktop/src/main/services/state/kvDb.ts
  • apps/desktop/src/main/services/storage/storageInsightsService.test.ts
  • apps/desktop/src/main/services/storage/storageInsightsService.ts
  • apps/desktop/src/main/services/storage/storageLedger.ts
  • apps/desktop/src/main/services/usage/usageStatsStore.ts
  • apps/desktop/src/main/services/usage/usageTrackingService.test.ts
  • apps/desktop/src/preload/global.d.ts
  • apps/desktop/src/preload/preload.ts
  • apps/desktop/src/renderer/browserMock.ts
  • apps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsx
  • apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
  • apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx
  • apps/desktop/src/renderer/components/settings/StorageSection.test.tsx
  • apps/desktop/src/renderer/components/settings/StorageSection.tsx
  • apps/desktop/src/renderer/webclient/adapter/lanes.ts
  • apps/desktop/src/shared/ipc.ts
  • apps/desktop/src/shared/types/config.ts
  • apps/desktop/src/shared/types/lanes.ts
  • apps/desktop/src/shared/types/prs.ts
  • apps/desktop/src/shared/types/storage.ts

Comment thread apps/desktop/src/main/services/adeActions/registry.ts Outdated
Comment thread apps/desktop/src/main/services/lanes/laneService.ts
Comment thread apps/desktop/src/main/services/lanes/laneService.ts
Comment thread apps/desktop/src/main/services/storage/storageInsightsService.ts
Comment thread apps/desktop/src/main/services/storage/storageInsightsService.ts
Comment thread apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
Comment thread apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
Comment thread apps/desktop/src/renderer/components/settings/StorageSection.tsx Outdated
Comment thread apps/desktop/src/renderer/components/settings/StorageSection.tsx
Comment thread apps/desktop/src/renderer/webclient/adapter/lanes.ts Outdated
@arul28

arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/desktop/src/main/services/adeActions/registry.ts Outdated
Comment thread apps/desktop/src/main/services/lanes/laneService.ts
Comment thread apps/desktop/src/main/services/storage/storageInsightsService.ts
@arul28

arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/desktop/src/main/services/lanes/laneService.ts
Comment thread apps/ade-cli/src/services/sync/syncRemoteCommandService.ts Outdated
@arul28

arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread apps/desktop/src/main/services/lanes/laneService.ts Outdated
Comment thread apps/desktop/src/main/services/lanes/laneService.ts Outdated
Comment thread apps/desktop/src/main/services/storage/storageInsightsService.ts
Comment thread apps/desktop/src/main/services/ipc/registerIpc.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsx (1)

98-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Escape handling picks the earliest-mounted frame, not the topmost.

The listener is registered on window in capture phase and calls stopPropagation(), so with two StorageDialogFrames 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

📥 Commits

Reviewing files that changed from the base of the PR and between a7c60ff and b5f78bf.

📒 Files selected for processing (22)
  • apps/ade-cli/src/bootstrap.ts
  • apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts
  • apps/ade-cli/src/services/sync/syncRemoteCommandService.ts
  • apps/desktop/src/main/main.ts
  • apps/desktop/src/main/services/adeActions/registry.test.ts
  • apps/desktop/src/main/services/adeActions/registry.ts
  • apps/desktop/src/main/services/ipc/registerIpc.ts
  • apps/desktop/src/main/services/lanes/laneRuntimeLifecycle.test.ts
  • apps/desktop/src/main/services/lanes/laneRuntimeLifecycle.ts
  • apps/desktop/src/main/services/lanes/laneService.ts
  • apps/desktop/src/main/services/lanes/laneStorageLifecycle.test.ts
  • apps/desktop/src/main/services/storage/storageInsightsService.test.ts
  • apps/desktop/src/main/services/storage/storageInsightsService.ts
  • apps/desktop/src/preload/preload.test.ts
  • apps/desktop/src/preload/preload.ts
  • apps/desktop/src/renderer/components/lanes/ManageLaneDialog.test.tsx
  • apps/desktop/src/renderer/components/lanes/ManageLaneDialog.tsx
  • apps/desktop/src/renderer/components/settings/StorageSection.test.tsx
  • apps/desktop/src/renderer/components/settings/StorageSection.tsx
  • apps/desktop/src/renderer/components/settings/storage/StorageCleanupDialog.tsx
  • apps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.ts
  • apps/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

Comment thread apps/desktop/src/main/services/lanes/laneRuntimeLifecycle.ts Outdated
@arul28

arul28 commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment on lines +186 to 190
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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@arul28
arul28 merged commit 092a689 into main Jul 28, 2026
32 checks passed
@arul28
arul28 deleted the ade/start-skill-discuss-all-befor-06fb0d77 branch July 28, 2026 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant