Harden ADE against disk-full and interrupted-write incidents#797
Merged
Conversation
A disk-full during the legacy schema retrofit's create/copy/drop/rename
of automation_ingress_events stranded its staging table and bricked
every later launch ("table already exists"). Root causes fixed:
- Shared rebuildTableInTransaction helper (BEGIN IMMEDIATE, row-count
validation, index recreation, rollback on failure) now backs both
schema retrofits and the runtime_processes rebuild.
- retrofitLegacyPrimaryKeyNotNullSchema skips CRR-excluded tables: they
never become CRRs and keep their unique indexes, which also stops the
unprotected full rebuild it performed on every startup.
- recoverInterruptedTableRebuilds runs before migrate() and classifies
stranded __ade_crr_repair_/__ade_fk_repair_ staging tables (drop empty
or partial staging, complete interrupted renames, never drop ambiguous
nonempty staging; ambiguous tables are skipped by the retrofits so
startup stays alive).
- writeMigrationBackupIfNeeded checks free-space headroom, copies to a
temp file, verifies size, and renames — no partial backups.
- classifySqliteOpenError exposes typed codes (disk_full, db_integrity,
insufficient_headroom, migration_*) for the recovery flow.
Covers the exact incident state in tests, plus a real SQLITE_FULL via
PRAGMA max_page_count, per-statement fault injection, and interrupted
drop-then-rename on a migrate()-owned table.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
persistChatState wrote the only copy of every provider thread pointer with an in-place writeFileSync; a torn write during the disk-full incident destroyed chat continuity. Changes: - New durableFile module: writeFileAtomic (same-dir temp + rename + optional fsync), writeJsonWithPrevious (exactly one .lkg previous generation), readJsonWithRecovery, and safe abandoned-temp cleanup. - Chat metadata now writes atomically with shape validation; reads fall back to the .lkg copy and log the recovery. Session-creation transcript headers and Linear context files write atomically too. - Bounded per-project continuity ledger (thread-pointers.jsonl, 64 KiB newest-wins compaction) records every provider pointer change with the previous value; fingerprints are seeded on session load so restarts do not spam entries. Pointer changes fsync metadata and flush transcripts. - Transcript appends heal a truncated tail line (missing trailing newline) before the first append per path, so an interrupted write can no longer corrupt the next event; user messages flush immediately instead of waiting out the 100ms debounce. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bounding
During the disk-full incident the brain crash-looped under launchd with
no backoff, unbounded logs, and no record Desktop could read; project
open surfaced a raw internal socket error with only a Dismiss button.
- Shared AdeRecoveryErrorCode + AdeLastFailureReport types; kvDb open
errors map onto them.
- lastFailureStore writes an atomic, bounded (current + one previous)
failure report to the machine and project .ade/runtime dirs; the brain
records classified startup/db failures, applies a short backoff after
repeated identical failures, and clears the report on healthy startup.
- launchd err/out logs are copytruncate-rotated at 10 MiB and repeated
identical startup-loop failures are deduped to one line plus a
periodic count, so a crash loop can no longer grow logs unboundedly.
- The local-runtime pool attaches typed codes (disk_full,
brain_crash_looping, socket_stale_no_owner, socket_owned_by_other,
brain_not_installed) to its refusals — guard conditions unchanged —
and project-open IPC surfaces them; projectTransitionError is now
{ message, code?, detail? } with a shared coded-error parser and a
plain-language hint plus technical-details fold in the alert.
Sync-host/socket-publication decoupling was evaluated and deferred: DB
failures already retry without exiting, but the socket publishes after
project scope init; restructuring that ownership is follow-up work.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity recovery After the disk-full crash, Codex resume failures silently called startFreshCodexThread inside existing chats, overwriting the thread pointer while the original rollouts were still valid on disk. - providerResumeClassifier classifies resume failures (thread_missing / provider_environment / transient / unknown) with a bounded rollout- file existence probe; optional-MCP failures are provider-health notices, not continuity loss. - All three silent fallbacks removed: resume failures fail the turn with a classified error, keep the pointer, and thread_missing/unknown mark the chat continuityRecovery=required and emit a plain-language system_notice (detail.kind "continuity_recovery"). New turns fail fast while recovery is required. - startFreshCodexThread now requires an explicit replace token when a pointer exists (provider_thread_replace_blocked otherwise), making silent replacement structurally impossible. - agentChat.recoverContinuity action (IPC + brain RPC allowlist): retry_original, recover_from_history (bounded ≤24KiB capsule from the durable transcript, thread marked reconstructed and linked to the original), start_new_chat (separate linked session; old chat kept). - Startup reconciliation restores a missing/corrupt pointer from the continuity ledger, SQLite resume_command, or transcript thread-id events, verifying rollout existence for Codex; Claude stale-session handling preserves the pointer in continuityRecovery instead of nulling it. Also carries the typed project-open error wrapper in registerIpc from the recovery-model change (shared file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Desktop surface for the continuity-recovery contract: a warm amber in-transcript card when a chat's original provider thread could not be resumed, offering Retry original thread, Recover from ADE history (with an inline confirm and a post-recovery capsule disclosure), and Start a new chat (deep-links to the new session). Reconstructed threads render a calm "Rebuilt from ADE history" pill; superseded chats link to their continuation. Plain-language copy throughout; identifiers live behind a Details fold. Optional-MCP/transient failures keep their provider-health treatment and never render as continuity loss. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
projectRecoveryService diagnoses a failed project open without needing the brain (free space, last-failure reports, socket liveness, service state, read-only database quick_check) and maps it to typed, plain-language recovery states. repair() mirrors the incident's manual recovery as ordered, reported steps: check space -> stop the service safely -> validate the database -> finish interrupted saves (the self-healing kvDb open) -> restart -> verify the endpoint and a real project RPC -> count chats needing continuity attention. It never deletes any file. ade.recovery.diagnose/repair IPC exposes it; the project-transition alert gains a Repair ADE action that retries the open on success. The shared wiring files in this commit (ipc.ts, registerIpc.ts, preload.ts, global.d.ts, main.ts) also carry the storage-pressure IPC additions from the parallel change landing next. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
diskPressure monitor (desktop main + brain) samples free space on the project and ADE-home volumes: normal / warning / critical / exhausted with rise-fast fall-slow hysteresis, subscriber-only sampling, and fail-open on statfs errors. Enforcement protects data without killing work: at exhausted, new structured chat turns fail fast with a plain-language notice, new ADE-launched CLI sessions are refused, and new managed process starts are refused; running turns, PTYs, and processes are never touched, and warning level never blocks anything. The TopBar gains a storage indicator (sibling of the CPU pressure pill, 30s poll, amber warning / red critical) that deep-links to Settings storage. No reserve files, no cleanup of non-ADE data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…leanup storageInsightsService walks ADE-owned roots (lstat only, never following symlinks, clamped to allowed bases, capped with an explicit truncated flag) into plain-language categories: chats & terminal history, lanes & worktrees, build & release staging (including os.tmpdir ade-* dirs and simulator DerivedData), caches, proof & attachments, recovery backups, and the project database. Worktree entries join live lane rows so archived lanes and leftovers from deleted lanes are itemized with per-lane sizes. Cleanup is preview -> confirm with kind-scoped path validation; protected data (active worktrees, chat session records, database, terminal history, secrets) is refused server-side even when requested explicitly. Registered as the storage action domain on both desktop and the brain, extending window.ade.storage alongside getPressure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Typed project-open failures now mount ProjectRecoveryScreen in the content area instead of a dismissible banner: plain-language diagnosis (via ade.recovery.diagnose with a code-keyed fallback), a one-click Repair ADE flow that renders the ordered repair steps and the post-repair report (database health, chats resumed, chats needing attention, no files removed), Review storage, Back, and a Show technical details fold that is the only place raw internals appear. Successful repairs automatically retry the project open through the normal transition veil. The old banner remains only for un-coded string errors; its now-unreachable repair machinery is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Settings gains a Storage section built on the storage insights domain: a disk gauge with ADE usage vs free space (pressure-tinted), a stacked category breakdown, and plain-language category cards with safety badges (Safe to remove / Can be compressed / Review first / Protected). The lanes card itemizes active, archived, and deleted-lane leftovers with per-lane sizes and per-row removal; every destructive action runs preview -> confirm -> itemized result with blocked items explained calmly, and protected data never gets a destructive control. Archived- lane removals resolve the real lane id from the lanes list, and cleanup kinds are inferred from paths with the backend as final gatekeeper. The compress action lights up when the compression API is present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation historyCompression gzips inactive transcripts (30+ days, never a file with pending appends): stream-compress to a .partial, fsync, verify by streamed gunzip comparing length and content hash plus a source unchanged check, rename to .gz, re-check the session has not resumed, and only then remove the original — any failure keeps the original and removes the partial. Chat history reads, pagination, PTY scrollback, and universal search indexing all read .gz transparently (plain file wins in the crash window), and a resumed session reinflates atomically before appending. Sweeps run idle-scheduled (10 min after startup, then daily, capped per run) on desktop and brain, and skip entirely under disk pressure; storage.compressNow exposes the on-demand action and snapshots report compressedBytes. Recovery backups flip to safe-to-remove only after seven days with a healthy database check and no fresh open failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The compression API landed after the dashboard was written; drop the structural cast now that preload typing exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…racts - Shared faultInjection test utility: Nth-call fs fault injection with an ENOSPC default and the canonical PRAGMA max_page_count SQLITE_FULL recipe; existing ad-hoc injections in the kvDb, durableFile, and chat persistence suites now use it. - diskFullIncident.integration.test.ts replays the July 2026 incident end to end: disk fills mid-rebuild and mid-metadata-write, nothing is damaged, the next open self-heals, a lost pointer reconciles from redundant state, crash-loop accounting and log bounding behave, the repair flow recovers a stranded staging database, pressure enforcement gates without jargon, and a symlink farm cannot leak scanning or cleanup outside ADE roots. The vitest workspace gains an integration-main project since unit projects exclude *.integration. - New docs/features/storage-and-recovery/README.md (source map, the staging-classification table, continuity redundancy, typed failure flow, pressure states, storage categories/safety, compression rules, and every backup/retention bound in one table) plus updates to the chat and remote-runtime feature docs, docs/README.md, and ARCHITECTURE.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verified findings from the dual-track review, all behavior-preserving except three specified hardenings: - The recovery screen mounts only when a typed error carries a project rootPath (repair-less takeovers fall back to the banner); the remote binding-switch failure now threads its rootPath. - recoverContinuity gains a per-session in-flight guard; the storage indicator poll catches rejections; durableFile's directory fsync follows the caller's fsync intent. - Single sources of truth: the recovery-code union derives from one const array with a shared narrower; volume.ts owns free-space reads and no-space error detection for the new call sites; the compression age and repair/advice disk floors are derived so they cannot contradict; shared codedError owns the create/encode/parse wire contract; DiskPressure DTO types moved to shared/types; formatBytes canonicalized in renderer lib; the thread-pointer ledger extracted to its own module; kvDb exports its quick_check and readonly-open helpers; the jargon-guard test util owns the banned-term list (bare SQL now subsumes SQLite). Review outcome: no Blocker or High findings; data-safety paths (transactional rebuilds, verified compression, identity-gated cleanup, pressure fail-open) independently confirmed sound. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- ade storage snapshot/compress typed subcommands (cleanup stays behind the explicit actions escape hatch because it is destructive and preview-gated); ade brain status prints the machine last-failure line when a fresh report exists; ade doctor reports disk headroom with a next action at warning/critical. - Internal docs verified against the final code and completed: storage-and-recovery source map reflects the extracted modules (threadPointerLedger, volume, shared codedError, shared storage types); terminals-and-sessions covers .gz transcript reads, reinflate on append, and the pressure gates; search documents transparent compressed-history indexing; onboarding-and-settings adds the Storage tab; ARCHITECTURE lists the ade.storage.*/ade.recovery.* channel families and new runtime services. Known follow-up (pre-existing, unrelated to this branch): headless one-shot `ade actions list` shows the reduced chat facade rather than the brain's full chat action surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The broad src/main/**/*.integration.test glob resurrected the dormant remoteRuntime.offlineRpc.integration.test.ts, which predates this branch, was excluded from every vitest project, and no longer passes. Scope the project to services/__tests__ and leave the dormant file exactly as it was before this branch; fixing or deleting it is a separate decision noted in the config comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
July 12, 2026 21:23
📝 WalkthroughWalkthroughThe PR adds disk-pressure monitoring, storage inspection and cleanup, durable project recovery, compressed transcript support, chat continuity recovery, and corresponding CLI, IPC, preload, runtime, and desktop UI integrations. It also adds unit and integration coverage for persistence, storage safety, disk-full recovery, and recovery workflows. ChangesStorage, recovery, and continuity platform
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 |
Semantic resolutions: kept both the pointer-reconciliation block and main's new seedForkedProviderPointer, converting the latter's raw metadata write to writeJsonWithPrevious (this branch's atomic-write contract); combined the disk-pressure send gate with main's preparedMessage option in the send path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/processes/processService.ts (1)
875-994: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDisk-pressure gate only covers
start(); other process-launching entry points bypass it.Updates
start()to gate process startup ondiskPressureMonitor?.canPerform("process_start"); when disallowed, it throws an error augmented with a recoverycode(typed asAdeRecoveryErrorCode) and the monitor’s message. Howeverrestart()(889-895),startStack()/restartStack()(901-921, viarunStartSet),startGroup()/restartGroup()(923-949, viarunStartGroupParallel), andstartAll()(951-954, viarunStartSet) all callstartById/startByDefinitiondirectly and never consultdiskPressureMonitor. Given the entire point of this hardening effort is to stop ADE from spawning new writer processes while the disk is critically full, any of these alternate paths (e.g. a "restart stack" or "start all" action, which is exactly what would run on lane/project open) can still launch processes during disk exhaustion.Recommend moving the check into the single choke point
startByDefinition(orstartById) so every caller is protected, rather than duplicating it per public method.🛡️ Proposed fix: enforce at the shared choke point
+ const assertDiskPressureAllows = (kind: "process_start"): void => { + const decision = diskPressureMonitor?.canPerform(kind); + if (decision && !decision.allowed) { + throw Object.assign(new Error(decision.message), { + code: decision.code satisfies AdeRecoveryErrorCode, + }); + } + }; + const startByDefinition = async ( laneId: string, definition: ProcessDefinition, opts: { skipTrust?: boolean; overlay?: LaneOverlayOverrides } = {}, ): Promise<ProcessRuntime> => { + assertDiskPressureAllows("process_start"); if (!opts.skipTrust) projectConfigService.getExecutableConfig();async start(arg: ProcessActionArgs): Promise<ProcessRuntime> { - const decision = diskPressureMonitor?.canPerform("process_start"); - if (decision && !decision.allowed) { - throw Object.assign(new Error(decision.message), { - code: decision.code satisfies AdeRecoveryErrorCode, - }); - } return await startById(arg.laneId, arg.processId); },🤖 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/processes/processService.ts` around lines 875 - 994, Move the disk-pressure validation from the public start method into the shared process-launch choke point, preferably startByDefinition (or startById if that is the common path), so every startup route is gated. Preserve the existing disallowed error behavior, including the monitor message and AdeRecoveryErrorCode recovery code, and remove the redundant check from start once the shared guard applies to start, restart, stack, group, and all-start flows.
🧹 Nitpick comments (9)
apps/desktop/src/test/jargonGuard.ts (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd word boundaries to avoid false positives on unrelated words.
WALwithout\bboundaries will match inside legitimate words like "firewall", "wallet", or "walnut" that could plausibly appear in future desktop copy (e.g. networking/security text), causing spurious test failures unrelated to actual jargon leakage.🔧 Proposed fix
-export const JARGON_PATTERN = /socket|SQL|launchd|CRR|JSONL|WAL|ENOSPC/i; +export const JARGON_PATTERN = /\b(socket|SQL|launchd|CRR|JSONL|WAL|ENOSPC)\b/i;🤖 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/test/jargonGuard.ts` at line 5, Add word boundaries around the WAL alternative in JARGON_PATTERN so it only matches the standalone jargon term and not substrings within words such as “firewall” or “wallet”; preserve the existing matching behavior for the other pattern alternatives.apps/desktop/src/renderer/components/app/ProjectTransitionErrorAlert.tsx (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "deferred to recovery screen" predicate.
This same
code && rootPathcondition is re-implemented inApp.tsx'sProjectTabHost(deciding whether to renderProjectRecoveryScreen). Keeping the ownership boundary in one place (e.g. a shared selector/helper likehasRecoverableProjectError(error)) avoids the two call sites silently drifting out of sync as new codes are added.♻️ Suggested shared helper
+// apps/desktop/src/renderer/state/appStore.ts +export function isRecoverableProjectTransitionError( + error: ProjectTransitionError | null, +): error is ProjectTransitionError & { rootPath: string } { + return Boolean(error?.code && error.rootPath); +}- if (projectTransitionError.code && projectTransitionError.rootPath) return null; + if (isRecoverableProjectTransitionError(projectTransitionError)) return 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/renderer/components/app/ProjectTransitionErrorAlert.tsx` around lines 17 - 19, Extract the `projectTransitionError.code && projectTransitionError.rootPath` predicate into a shared helper or selector such as `hasRecoverableProjectError(error)`, then reuse it in both `ProjectTransitionErrorAlert` and `ProjectTabHost` when deciding whether recovery handles the error. Preserve the current null-rendering behavior while ensuring both call sites use the same ownership rule.apps/desktop/src/main/services/pty/ptyService.ts (1)
3600-3605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM on the gating placement (runs before any spawn/transcript work, only for fresh tracked CLI launches).
Same duplicated
Object.assign(new Error(...), { code })pattern as flagged inprocessService.ts— worth consolidating into a shared coded-error helper if one exists inapps/desktop/src/shared/codedError.ts.🤖 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/pty/ptyService.ts` around lines 3600 - 3605, Consolidate the coded error construction in the fresh tracked CLI launch gate within the PTY service by reusing the shared helper from codedError.ts, if available, instead of duplicating Object.assign(new Error(decision.message), { code: decision.code }). Preserve the existing decision message, error code, and gating behavior.apps/desktop/src/main/services/processes/processService.ts (1)
876-881: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared codedError helper here
apps/desktop/src/shared/codedError.tsalready exportscodedError, so this can replace the inlineObject.assign(new Error(...), { code })in bothprocessService.tsandptyService.tsfor consistency and a stable error shape.🤖 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/processes/processService.ts` around lines 876 - 881, Replace the inline Object.assign error construction in the process-start disk-pressure check with the shared codedError helper, passing decision.message and decision.code. Apply the same change in the corresponding disk-pressure handling in ptyService.ts, preserving the existing throw behavior and AdeRecoveryErrorCode typing.apps/desktop/src/renderer/components/app/StoragePressureIndicator.tsx (1)
63-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
role="status"masks button semantics for assistive tech.The clickable element uses
role="status", which overrides the native<button>role. Screen readers will likely announce this as a passive live-region rather than an actionable control, even thougharia-label/titledescribe state — the click affordance ("opens storage settings") is not conveyed to AT users the same way it is to sighted/mouse users.Consider keeping the native button role and using a visually-hidden
aria-liveregion (oraria-live="polite"wrapper) solely for the state-change announcement, separate from the interactive element's exposed role.🤖 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/app/StoragePressureIndicator.tsx` around lines 63 - 87, The StoragePressureIndicator button should retain its native button semantics instead of overriding them with role="status". Remove the role override from the button and add a separate visually hidden aria-live="polite" element or wrapper that announces snapshot.state changes, while keeping the button’s accessible label and storage-settings click behavior unchanged.apps/desktop/src/main/services/runtime/lastFailureStore.ts (1)
140-148: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTOCTOU between
existsSyncand rename inclearLastFailure.The existence check and the rename are separate syscalls; a concurrent writer (desktop + ade-cli both touch the machine-scoped file) could remove/replace the file in between, though the resulting
ENOENTis caught and logged rather than crashing. Low risk given the graceful fallback, but worth noting alongside the atomicity gap above.🤖 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/runtime/lastFailureStore.ts` around lines 140 - 148, Update clearLastFailure to remove the separate existsSync precheck and attempt moveCurrentToPrevious directly within its existing try/catch. Preserve graceful handling by treating a missing file (ENOENT) as a no-op rather than reporting it, while continuing to report other write failures through reportWriteFailure.apps/ade-cli/src/bootstrap.ts (1)
465-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass
loggerto the CLI disk-pressure monitor. Both desktop call sites constructcreateDiskPressureMonitor({ roots, logger }), but the CLI runtime omitsloggereven though it's already available (Line 464). This drops disk-pressure state-transition/warning logs in exactly the headless disk-full scenario this PR hardens.🔧 Proposed change
const diskPressureMonitor = createDiskPressureMonitor({ roots: [projectRoot, resolveMachineAdeLayout().adeDir], + logger, });Please confirm
createDiskPressureMonitoraccepts an optionallogger(the desktop usage inmain.tsindicates it does).🤖 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/bootstrap.ts` around lines 465 - 467, Update the createDiskPressureMonitor call in bootstrap.ts to include the already available logger from the surrounding scope alongside roots. Preserve the existing projectRoot and ADE directory configuration, and rely on the monitor’s optional logger parameter as used by the desktop call sites.apps/desktop/src/main/services/sessions/sessionService.ts (1)
1144-1189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting shared tail-slice/align/strip logic.
The
.gzbranch and the existing file-handle branch both implement the same "slice tail → optionally align to newline → stringify → strip ANSI" sequence on aBuffer. Functionally correct as written, but a shared helper (Buffer -> string) taking the decompressed/read buffer would avoid future divergence between the two paths.🤖 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/sessions/sessionService.ts` around lines 1144 - 1189, Extract the duplicated Buffer-to-string processing from the gzip branch and file-handle branch into a shared helper near the surrounding session service logic. Have the helper perform tail slicing, optional alignToLineBoundary newline adjustment, UTF-8 conversion, and conditional stripAnsi handling, then pass each branch’s Buffer and existing options to it without changing behavior.apps/desktop/src/main/services/chat/agentChatService.ts (1)
2324-2324: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
checkedTranscriptTailsgrows unbounded — clear it on session disposal/deletion.Entries are added in
flushQueuedTranscriptWrite(Line 2347) but never removed.finishSession/deleteSession/forceDisposeManagedSessionclean uplastPersistedPointerFingerprints,eventHistoryBySession,transcriptHistoryCacheBySession, etc., but leave this Set. On a long-lived desktop process cycling through many chat sessions, the resolved-path strings accumulate indefinitely. Since this healing check only needs to run once per path per process, prune the transcript-path entries when a session's runtime/state is torn down (e.g. alongside the existing candidate-path cleanup in the disposal paths).Also applies to: 36147-36157
🤖 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/chat/agentChatService.ts` at line 2324, Clear the path entries tracked by checkedTranscriptTails during session teardown to prevent unbounded growth. Update the cleanup logic in finishSession, deleteSession, and forceDisposeManagedSession, alongside the existing lastPersistedPointerFingerprints, eventHistoryBySession, transcriptHistoryCacheBySession, and candidate-path cleanup, removing entries associated with the disposed session’s transcript paths while preserving process-wide deduplication for active sessions.
🤖 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/ade-cli/src/services/runtime/failureLogDeduper.ts`:
- Around line 12-40: Update createFailureLogDeduper so failure entries are keyed
by a stable error code or name rather than dynamically formatted messages,
preventing runSyncHostStartupLoop from creating unbounded signatures in
failures; preserve the existing occurrence counting, summary logging, and clear
behavior.
In `@apps/ade-cli/src/services/runtime/runtimeLogMaintenance.ts`:
- Around line 23-31: Update the rotation logic around the Buffer allocated in
runtime log maintenance to capture the byte count returned by fs.readSync and
write only that read range to ${filePath}.1, rather than the entire allocUnsafe
buffer; preserve the existing keep calculation and file descriptor cleanup.
In `@apps/desktop/src/main/services/chat/chatTranscriptHistoryPager.ts`:
- Around line 71-91: The .gz paging branch repeatedly decompresses the entire
transcript for each request. Update the gzip snapshot handling around
readHistoryFileSync to cache decompressed buffers by transcript path plus
current mtime/size, reuse valid cached snapshots, and evict entries with a
bounded cache policy; preserve the existing paging and parsing behavior for
cached and newly read snapshots.
In `@apps/desktop/src/main/services/runtime/lastFailureStore.ts`:
- Around line 100-134: Preserve the current failure report across interrupted
rotations by updating moveCurrentToPrevious and/or readLastFailure: avoid
leaving a gap while replacing last-failure.json, or make readLastFailure fall
back to previousPath(targetPath) when the primary report is missing. Ensure a
failed replacement still exposes the previous report on the next launch without
changing normal rotation behavior.
In `@apps/desktop/src/main/services/runtime/projectRecoveryService.ts`:
- Around line 510-528: Update the stop_service branch in the project recovery
flow to invoke the dependency’s actual service stop/uninstall operation instead
of installServiceBestEffort(). Preserve the existing waitForSocketState
verification, failure handling, and step reporting so validation proceeds only
after the daemon has been stopped.
In `@apps/desktop/src/main/services/search/searchService.ts`:
- Around line 583-623: Bound the compressed read window in both
session-processing paths, including processTerminalSession, by
MAX_READ_BYTES_PER_PASS instead of setting it to the full fileSize. Keep
compressed data access as a subarray view, preserve plain-file behavior, and
retain the existing self-requeue flow so remaining content is processed in
subsequent passes.
In `@apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsx`:
- Around line 25-35: Update AUTO_REPAIR_CODES to include
provider_thread_missing, provider_resume_failed, and
continuity_reconstruction_required so canAutoRepair’s fallback eligibility
matches the automatic-repair promises in FALLBACK_COPY and displays the “Repair
ADE” action for these recovery states.
In `@apps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.tsx`:
- Around line 197-198: Update the catch branch in the recovery action to use the
plain-language copy returned by failureCopy("recovery_failed") instead of
exposing error.message or String(error). Preserve the existing error capture
flow while ensuring unexpected transport failures display the same sanitized
message as other recovery failures.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 1000-1035: Update formatProjectTransitionError so every recognized
recovery code, including provider_thread_missing, provider_resume_failed,
continuity_reconstruction_required, and optional_mcp_failed, receives calm
user-facing recovery text instead of falling back to the raw error. Preserve the
raw fallback for unrecognized or absent codes, and retain the existing detail
behavior where applicable.
- Around line 1913-1919: Update the closeProject error handling around
formatProjectTransitionError so coded close failures include closingProjectRoot
as the rootPath when setting projectTransitionError. Preserve the existing error
object for uncoded failures and use the closingProjectRoot already captured
before the try block.
---
Outside diff comments:
In `@apps/desktop/src/main/services/processes/processService.ts`:
- Around line 875-994: Move the disk-pressure validation from the public start
method into the shared process-launch choke point, preferably startByDefinition
(or startById if that is the common path), so every startup route is gated.
Preserve the existing disallowed error behavior, including the monitor message
and AdeRecoveryErrorCode recovery code, and remove the redundant check from
start once the shared guard applies to start, restart, stack, group, and
all-start flows.
---
Nitpick comments:
In `@apps/ade-cli/src/bootstrap.ts`:
- Around line 465-467: Update the createDiskPressureMonitor call in bootstrap.ts
to include the already available logger from the surrounding scope alongside
roots. Preserve the existing projectRoot and ADE directory configuration, and
rely on the monitor’s optional logger parameter as used by the desktop call
sites.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Line 2324: Clear the path entries tracked by checkedTranscriptTails during
session teardown to prevent unbounded growth. Update the cleanup logic in
finishSession, deleteSession, and forceDisposeManagedSession, alongside the
existing lastPersistedPointerFingerprints, eventHistoryBySession,
transcriptHistoryCacheBySession, and candidate-path cleanup, removing entries
associated with the disposed session’s transcript paths while preserving
process-wide deduplication for active sessions.
In `@apps/desktop/src/main/services/processes/processService.ts`:
- Around line 876-881: Replace the inline Object.assign error construction in
the process-start disk-pressure check with the shared codedError helper, passing
decision.message and decision.code. Apply the same change in the corresponding
disk-pressure handling in ptyService.ts, preserving the existing throw behavior
and AdeRecoveryErrorCode typing.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 3600-3605: Consolidate the coded error construction in the fresh
tracked CLI launch gate within the PTY service by reusing the shared helper from
codedError.ts, if available, instead of duplicating Object.assign(new
Error(decision.message), { code: decision.code }). Preserve the existing
decision message, error code, and gating behavior.
In `@apps/desktop/src/main/services/runtime/lastFailureStore.ts`:
- Around line 140-148: Update clearLastFailure to remove the separate existsSync
precheck and attempt moveCurrentToPrevious directly within its existing
try/catch. Preserve graceful handling by treating a missing file (ENOENT) as a
no-op rather than reporting it, while continuing to report other write failures
through reportWriteFailure.
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 1144-1189: Extract the duplicated Buffer-to-string processing from
the gzip branch and file-handle branch into a shared helper near the surrounding
session service logic. Have the helper perform tail slicing, optional
alignToLineBoundary newline adjustment, UTF-8 conversion, and conditional
stripAnsi handling, then pass each branch’s Buffer and existing options to it
without changing behavior.
In `@apps/desktop/src/renderer/components/app/ProjectTransitionErrorAlert.tsx`:
- Around line 17-19: Extract the `projectTransitionError.code &&
projectTransitionError.rootPath` predicate into a shared helper or selector such
as `hasRecoverableProjectError(error)`, then reuse it in both
`ProjectTransitionErrorAlert` and `ProjectTabHost` when deciding whether
recovery handles the error. Preserve the current null-rendering behavior while
ensuring both call sites use the same ownership rule.
In `@apps/desktop/src/renderer/components/app/StoragePressureIndicator.tsx`:
- Around line 63-87: The StoragePressureIndicator button should retain its
native button semantics instead of overriding them with role="status". Remove
the role override from the button and add a separate visually hidden
aria-live="polite" element or wrapper that announces snapshot.state changes,
while keeping the button’s accessible label and storage-settings click behavior
unchanged.
In `@apps/desktop/src/test/jargonGuard.ts`:
- Line 5: Add word boundaries around the WAL alternative in JARGON_PATTERN so it
only matches the standalone jargon term and not substrings within words such as
“firewall” or “wallet”; preserve the existing matching behavior for the other
pattern alternatives.
🪄 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
Run ID: f0399e41-45a6-447b-8358-6f7ca149d22b
⛔ Files ignored due to path filters (8)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/remote-runtime/README.mdis excluded by!docs/**docs/features/search/README.mdis excluded by!docs/**docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (86)
apps/ade-cli/README.mdapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/runtime/failureLogDeduper.test.tsapps/ade-cli/src/services/runtime/failureLogDeduper.tsapps/ade-cli/src/services/runtime/lastFailureStore.test.tsapps/ade-cli/src/services/runtime/runtimeLogMaintenance.test.tsapps/ade-cli/src/services/runtime/runtimeLogMaintenance.tsapps/ade-cli/src/services/sync/syncHostStartupLoop.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/__tests__/diskFullIncident.integration.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatTranscriptHistoryPager.test.tsapps/desktop/src/main/services/chat/chatTranscriptHistoryPager.tsapps/desktop/src/main/services/chat/providerResumeClassifier.test.tsapps/desktop/src/main/services/chat/providerResumeClassifier.tsapps/desktop/src/main/services/chat/threadPointerLedger.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/processes/processService.test.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/runtime/lastFailureStore.tsapps/desktop/src/main/services/runtime/projectRecoveryService.test.tsapps/desktop/src/main/services/runtime/projectRecoveryService.tsapps/desktop/src/main/services/search/searchService.test.tsapps/desktop/src/main/services/search/searchService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/durableFile.test.tsapps/desktop/src/main/services/state/durableFile.tsapps/desktop/src/main/services/state/kvDb.rebuildRecovery.test.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/storage/diskPressure.test.tsapps/desktop/src/main/services/storage/diskPressure.tsapps/desktop/src/main/services/storage/historyCompression.test.tsapps/desktop/src/main/services/storage/historyCompression.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/main/services/storage/volume.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/App.tsxapps/desktop/src/renderer/components/app/App.workKeepAlive.test.tsxapps/desktop/src/renderer/components/app/AppShell.aiStatus.test.tsxapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.test.tsxapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsxapps/desktop/src/renderer/components/app/ProjectTransitionErrorAlert.test.tsxapps/desktop/src/renderer/components/app/ProjectTransitionErrorAlert.tsxapps/desktop/src/renderer/components/app/SettingsPage.tsxapps/desktop/src/renderer/components/app/StoragePressureIndicator.test.tsxapps/desktop/src/renderer/components/app/StoragePressureIndicator.tsxapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/app/settingsSections.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.test.tsxapps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.tsxapps/desktop/src/renderer/components/projects/PublishToGitHubDialog.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/components/settings/storage/storageView.tsapps/desktop/src/renderer/lib/codedError.tsapps/desktop/src/renderer/lib/format.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/codedError.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/recovery.tsapps/desktop/src/shared/types/storage.tsapps/desktop/src/test/faultInjection.tsapps/desktop/src/test/jargonGuard.tsapps/desktop/vitest.workspace.ts
Preview identities lived in service state keyed by path, so a second preview of the same path replaced the identity an earlier confirmation depended on — a stale confirm could then delete a generation the user never previewed. Identities now travel inside the returned preview and cleanup validates each target against the submitted preview's identity (missing identity blocks with the re-preview message). Regression test reproduces the drift: same-size content change between two previews, stale confirm blocked, fresh confirm succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
![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: 8892e9227b
ℹ️ 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".
Verified each bot finding against the code; ten confirmed and fixed, one refuted with the intent made explicit: - Repair flow now genuinely stops the service before exclusive database work (new pool uninstallServiceBestEffort spawning serve --uninstall-service — desktop keeps the child-process boundary and never imports ade-cli service code) and reinstalls afterwards. - Compressed transcript reads are bounded per pass in search indexing and history paging; the failure-log deduper uses stable signatures; log rotation writes only initialized bytes; interrupted last-failure rotation preserves the primary report. - Renderer: every recognized recovery code gets calm copy (no raw fallback text), coded close failures retain the project root, the recovery screen's repair eligibility matches its own copy, and unexpected continuity-recovery errors are sanitized behind Details. - Daemon-backed storage fallback now declares isPathActive explicitly with the ownership rationale (brain runs the real compression sweep). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d1adf7981
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4430859d61
ℹ️ 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".
…tic in tests - The disk-pressure gate is now the first act of every process launch path (startById, runStartSet, startByDefinition), so refusal happens before any config or trust access — which is also what the incident integration test asserts. - Continuity-recovery tests own a fresh CODEX_HOME per test instead of inheriting the machine's real ~/.codex, whose size previously decided whether a rollout probe completed (CI: definitive absence; a dev machine: inconclusive) and flipped classification. Tests that need "local history exists" write an explicit rollout fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
2367-2370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear
checkedTranscriptTailsduring session cleanup.checkedTranscriptTailsonly grows: once a transcript path is flushed,isTranscriptPathActivewill treat it as active forever, even afterdeleteSessionremoves the file. That breaks the new storage-cleanup guard and leaks memory in a long-running desktop process. Remove the resolved paths from this set alongsidelastPersistedPointerFingerprintsindeleteSession.🤖 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/chat/agentChatService.ts` around lines 2367 - 2370, Update deleteSession to remove the session’s resolved transcript paths from checkedTranscriptTails during cleanup, alongside the existing lastPersistedPointerFingerprints removal. Ensure deleted transcript paths are no longer treated as active and the set does not grow indefinitely.
🤖 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/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 791-833: Update uninstallServiceBestEffort() to enforce a timeout
for the spawned uninstall child process so its Promise always settles if the
process hangs. On timeout, terminate the child, log
local_runtime.service_uninstall_failed with timeout context, and reject; clear
the timer when the child emits error or close, preserving existing success and
failure handling.
- Around line 791-833: Update uninstallServiceBestEffort() to cache and reuse an
in-flight uninstall promise, mirroring the existing serviceInstallPromise guard
in installServiceBestEffort(). Ensure concurrent callers share one spawned
--uninstall-service process, and clear the cached promise after completion so
later calls can run normally.
In `@apps/desktop/src/main/services/processes/processService.test.ts`:
- Around line 1298-1305: Update service.restart to check disk pressure before
invoking stopEntries, ensuring a disk_full failure from startById does not leave
the existing run exited without a replacement. Preserve the current run when the
replacement cannot start, while retaining normal stop-and-restart behavior for
successful starts; adjust the restart test expectations accordingly.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 2367-2370: Update deleteSession to remove the session’s resolved
transcript paths from checkedTranscriptTails during cleanup, alongside the
existing lastPersistedPointerFingerprints removal. Ensure deleted transcript
paths are no longer treated as active and the set does not grow indefinitely.
🪄 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
Run ID: d698a639-fc44-47e2-ad21-b70d89802b1f
⛔ Files ignored due to path filters (2)
docs/features/chat/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**
📒 Files selected for processing (36)
apps/ade-cli/README.mdapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/runtime/runtimeLogMaintenance.test.tsapps/ade-cli/src/services/runtime/runtimeLogMaintenance.tsapps/ade-cli/src/services/sync/syncHostStartupLoop.test.tsapps/ade-cli/src/services/sync/syncHostStartupLoop.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/__tests__/diskFullIncident.integration.test.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/chatTranscriptHistoryPager.test.tsapps/desktop/src/main/services/chat/chatTranscriptHistoryPager.tsapps/desktop/src/main/services/chat/providerResumeClassifier.test.tsapps/desktop/src/main/services/chat/providerResumeClassifier.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/processes/processService.test.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/runtime/lastFailureStore.test.tsapps/desktop/src/main/services/runtime/lastFailureStore.tsapps/desktop/src/main/services/runtime/projectRecoveryService.test.tsapps/desktop/src/main/services/runtime/projectRecoveryService.tsapps/desktop/src/main/services/search/searchService.test.tsapps/desktop/src/main/services/search/searchService.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.test.tsxapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.test.tsxapps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.tsxapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/storage.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- apps/ade-cli/README.md
- apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx
- apps/desktop/src/shared/types/storage.ts
- apps/desktop/src/main/services/chat/providerResumeClassifier.ts
- apps/desktop/src/main/services/processes/processService.ts
- apps/ade-cli/src/services/sync/syncHostStartupLoop.ts
- apps/desktop/src/renderer/state/appStore.test.ts
- apps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.tsx
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/main/services/runtime/projectRecoveryService.test.ts
- apps/desktop/src/main/services/tests/diskFullIncident.integration.test.ts
- apps/desktop/src/main/services/externalSessions/discoverCodex.ts
- apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.test.tsx
- apps/desktop/src/main/services/chat/providerResumeClassifier.test.ts
- apps/desktop/src/main/services/storage/storageInsightsService.test.ts
- apps/desktop/src/main/services/chat/chatTranscriptHistoryPager.ts
- apps/desktop/src/main/services/runtime/lastFailureStore.ts
- apps/desktop/src/renderer/components/chat/ChatContinuityRecoveryCard.test.tsx
- apps/desktop/src/main/services/search/searchService.ts
- apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsx
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/storage/storageInsightsService.ts
- apps/ade-cli/src/cli.ts
- apps/desktop/src/main/services/runtime/projectRecoveryService.ts
There was a problem hiding this comment.
💡 Codex Review
ADE/apps/desktop/src/renderer/state/appStore.ts
Line 1738 in 7a309f8
Preserve selected root for recovery after picker opens
When project.openRepo() fails after the user chooses a folder, typed recovery errors like disk_full or migration_incomplete are stored here without a rootPath; App only mounts the full recovery flow when both code and rootPath are present. This affects the first-open/file-picker path (unlike switchProjectToPath, which adds the root below), so users get only a dismissible banner and cannot run diagnose/repair for the project they just selected.
ℹ️ 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".
…ll timeout - retry_original for Codex now runs runtime setup (ensure/resolve) inside the catchable block, so a setup failure re-marks continuity recovery instead of leaving the chat with the recovery card gone. - Open-repository dialog failures encode the selected root (chosen in the main process, never seen by the renderer) into the coded error so the recovery screen can offer Repair rather than the generic banner; the shared codedError wire format carries an optional NUL-delimited rootPath, covered by a new round-trip test. - uninstallServiceBestEffort now has a 20s timeout that kills a hung child and rejects, so a stuck login-item removal can't leave the repair flow waiting forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
A send to an already-active chat was converted to steer() before the chat_turn disk-pressure check ran, and steer() has no gate of its own — so at exhausted capacity active sessions kept appending user input and driving provider work while only idle sends paused. The gate now runs before the steer route, covering both paths; regression test drives an active session at exhausted pressure and asserts no steer/turn starts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef06362d7
ℹ️ 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".
- ProjectRecoveryScreen's Review storage now clears the transition error before navigating; while it was set, ProjectTabHost kept rendering the takeover and the route change never revealed Settings. - The fresh-thread replace guard authorizes a recover-from-history retry against the recovery's recorded original thread, not only the current pointer, so a crash that left threadId pointing at a half-built reconstruction thread (state still "required") no longer wedges the chat behind provider_thread_replace_blocked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aafa8869ef
ℹ️ 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".
…tree storage cleanup matched lane worktrees by basename as a fallback, so an archived lane at /tmp/feature could authorize removing an active lane's .ade/worktrees/feature and delete live work. laneForPath now matches only the exact resolved worktree_path, for both snapshot classification and cleanup authorization. Regression test: a same-named archived lane cannot classify or delete an active worktree, even with a forged preview. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1ca795567
ℹ️ 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".
- The transcript tail-check marked a path verified before the append landed, so an ENOSPC-torn append left the file without a trailing newline and every later flush skipped healing forever, corrupting the next event. The path is now marked checked only after a successful append, so a failed append re-checks and heals on the next flush. - Resuming a tracked CLI session whose PTY is gone spawns a new process; the disk-pressure cli_launch gate now covers it (the live-attach case still returns earlier), so exhausted storage no longer lets a dead session respawn and append to its transcript. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b780f0007
ℹ️ 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".
restart/restartStack/restartGroup reached the process-start gate only after stopEntries had already killed the running process, so exhausted storage would stop a healthy process and then refuse to launch its replacement — leaving the user worse off than doing nothing. The gate now runs at the top of each restart path, before any stop, so a refused restart leaves the existing process running. Updated the test that previously encoded the killed-then-unrestartable behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/main/services/chat/agentChatService.ts (2)
2370-2370: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
checkedTranscriptTailsis never cleared, permanently poisoningisTranscriptPathActive.
checkedTranscriptTails(module-levelSet<string>) gets an entry added on every successful transcript flush (line 2422) but is never removed — not indeleteSession,dispose,finishSession, orforceDisposeAll. UnlikependingTranscriptWrites, which self-cleans via.delete(normalizedPath)influshQueuedTranscriptWrite, this Set only grows.
isTranscriptPathActive(line 37140) treats membership in this Set as proof the path is "active" (pendingTranscriptWrites.has(normalized) || checkedTranscriptTails.has(normalized)). Since every transcript path that was ever flushed stays in the Set forever, once a chat session ends and its transcript files are deleted,isTranscriptPathActivewill still reporttruefor that path indefinitely. If this function gates the storage cleanup / compressed-inactive-history sweep this PR introduces, it means transcripts for ended sessions can never be recognized as inactive and swept — directly undermining the "compressed inactive history" feature, in addition to an unbounded memory leak over the life of the process.🐛 Proposed fix — clear the tail-checked marker when a transcript path is torn down
flushQueuedTranscriptWrite(managed.transcriptPath); flushQueuedTranscriptWrite(path.join(chatTranscriptsDir, `${trimmedSessionId}.jsonl`)); + checkedTranscriptTails.delete(path.resolve(managed.transcriptPath)); + checkedTranscriptTails.delete(path.resolve(path.join(chatTranscriptsDir, `${trimmedSessionId}.jsonl`))); teardownRuntime(managed, "ended_session"); managedSessions.delete(trimmedSessionId);Also applies to: 2392-2422, 37138-37149
🤖 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/chat/agentChatService.ts` at line 2370, Clear the checkedTranscriptTails marker whenever its transcript path is torn down, including deleteSession, dispose, finishSession, and forceDisposeAll, using the same normalized path key added during transcript flushing. Update the lifecycle cleanup around checkedTranscriptTails and isTranscriptPathActive so ended or deleted sessions no longer remain marked active, while preserving pendingTranscriptWrites behavior.
30762-30792: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftExtend the disk-pressure gate beyond
sendMessage().
messageSession()still routes queued/auto/wake sends straight intosteer(), andrunSessionTurn()dispatchesexecutePreparedSendMessage()directly, so critical pressure can still drive provider work through those paths.checkedTranscriptTailsis only ever added to, andisTranscriptPathActive()treats it as permanently active; clear that state on teardown or old transcript paths will stay protected from compression forever.🤖 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/chat/agentChatService.ts` around lines 30762 - 30792, Extend the disk-pressure “chat_turn” gate used by sendMessage() to messageSession() before queued, auto, or wake sends can reach steer(), and to runSessionTurn() before executePreparedSendMessage() dispatches provider work. Preserve existing failure emission and return behavior for denied decisions. Clear checkedTranscriptTails during session/transcript teardown so isTranscriptPathActive() does not permanently protect obsolete transcript paths.apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts (2)
1565-1567: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSanitize
lastFailure.detailbefore exposing it.lastFailureStore.tsalready caps the byte length, butdetailis still appended verbatim to the thrown error, so control characters and raw diagnostics can cross the IPC boundary. Strip or normalize it here before appending.🤖 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/localRuntime/localRuntimeConnectionPool.ts` around lines 1565 - 1567, Sanitize lastFailure.detail in the reportDetail construction within the local runtime connection pool before appending it to the thrown error. Normalize or strip control characters and unsafe raw diagnostic content while preserving the existing capped-length behavior from lastFailureStore.ts, and keep the fallback unchanged when no detail exists.
1558-1561: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDifferentiate probe failures from a stale socket
probeSocketHasOwner()collapses timeouts anderrorevents intofalse, so transient or permission failures get reported assocket_stale_no_owner. Return a tri-state, or only map definitive missing/refused-socket cases to that recovery code.🤖 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/localRuntime/localRuntimeConnectionPool.ts` around lines 1558 - 1561, Update the recovery classification around probeSocketHasOwner so probe timeouts and error events are not converted into socket_stale_no_owner. Preserve socket_owned_by_other for confirmed ownership, map only definitive missing or refused-socket results to socket_stale_no_owner, and propagate or classify indeterminate probe failures separately.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
30369-30409: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the fault-injection restore with try/finally.
renameFault.restore()runs as a bare statement after severalexpect()calls. If any assertion between injecting the fault (30385) and restoring it (30405) throws,fs.renameSyncstays monkey-patched globally for the rest of the file's test run, risking cascading, hard-to-diagnose failures in unrelated tests.🔧 Proposed fix
- const renameFault = injectFsFault({ - op: "renameSync", - matchPath: (candidate) => path.resolve(candidate) === path.resolve(metadataPath(session.id)), - }); - - mockState.emitCodexPayload({ - jsonrpc: "2.0", - method: "thread/deleted", - params: { threadId: "thread-resumed" }, - }); - await vi.waitFor(() => { - expect(logger.warn).toHaveBeenCalledWith( - "agent_chat.persist_failed", - expect.objectContaining({ sessionId: session.id, lkgUpdated: true }), - ); - }); - expect(fs.readFileSync(metadataPath(session.id))).toEqual(before); - expect(readPersistedChatState(session.id).threadId).toBe("thread-resumed"); - expect(fs.readdirSync(chatSessionsDir()).filter((name) => name.includes(".tmp-"))).toEqual([]); - - renameFault.restore(); - await service.updateSession({ sessionId: session.id, title: "Persist after ENOSPC" }); - expect(readPersistedChatState(session.id).threadId).toBeUndefined(); - expect(JSON.parse(fs.readFileSync(`${metadataPath(session.id)}.lkg`, "utf8")).threadId).toBe("thread-resumed"); + const renameFault = injectFsFault({ + op: "renameSync", + matchPath: (candidate) => path.resolve(candidate) === path.resolve(metadataPath(session.id)), + }); + try { + mockState.emitCodexPayload({ + jsonrpc: "2.0", + method: "thread/deleted", + params: { threadId: "thread-resumed" }, + }); + await vi.waitFor(() => { + expect(logger.warn).toHaveBeenCalledWith( + "agent_chat.persist_failed", + expect.objectContaining({ sessionId: session.id, lkgUpdated: true }), + ); + }); + expect(fs.readFileSync(metadataPath(session.id))).toEqual(before); + expect(readPersistedChatState(session.id).threadId).toBe("thread-resumed"); + expect(fs.readdirSync(chatSessionsDir()).filter((name) => name.includes(".tmp-"))).toEqual([]); + } finally { + renameFault.restore(); + } + await service.updateSession({ sessionId: session.id, title: "Persist after ENOSPC" }); + expect(readPersistedChatState(session.id).threadId).toBeUndefined(); + expect(JSON.parse(fs.readFileSync(`${metadataPath(session.id)}.lkg`, "utf8")).threadId).toBe("thread-resumed");🤖 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/chat/agentChatService.test.ts` around lines 30369 - 30409, Wrap the assertions and persistence attempt after injecting renameFault in a try/finally block, and move renameFault.restore() into the finally clause. Keep the existing checks and follow-up updateSession behavior unchanged while guaranteeing the fs.renameSync fault is restored even when an assertion fails.apps/desktop/src/main/services/chat/agentChatService.ts (1)
7856-7876: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid inflating gzip transcripts twice
readTranscriptTailForHistoryinflates.gzfiles into a full buffer up toMAX_TRANSPARENT_HISTORY_BYTESbefore slicing, so gzip tail reads still pay the full decompression cost.resolveBestTranscriptPathForSessionIdinflates the same gzip candidate again just to computesize; thread the decompressed length throughparseTranscriptHistoryTailor cache it so each candidate is read once.🤖 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/chat/agentChatService.ts` around lines 7856 - 7876, The gzip transcript path in readTranscriptTailForHistory currently inflates the full file, while resolveBestTranscriptPathForSessionId inflates the same candidate again to determine size. Reuse the decompressed buffer or thread its length through parseTranscriptHistoryTail and the path-resolution flow so each gzip candidate is read and decompressed only once, while preserving tail slicing and offset 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/chat/agentChatService.test.ts`:
- Around line 30533-30535: Update the test setup around the per-test CODEX_HOME
assignment to retain the created temporary directory and remove it in that
describe block’s afterEach cleanup. Preserve the existing tmpHomeRoot cleanup
and ensure the CODEX_HOME directory is deleted recursively even when a test
fails.
In `@apps/desktop/src/shared/codedError.ts`:
- Around line 31-34: Update the decoding logic around withRoot and rootPath so
the filesystem path is taken directly from the payload after the delimiter
without trimming; trim only the user-facing raw message. Preserve rootPath bytes
across the IPC boundary and add a regression test covering a path with trailing
whitespace.
- Around line 17-19: Update encodeCodedErrorMessage and the surrounding
surfaceCodedError flow so errors with an existing `${code}:` prefix are
re-encoded whenever meta.rootPath is present, preserving the root path for IPC
repair handling. Update parseCodedErrorMessage to retain the decoded rootPath
exactly without trimming it; continue existing behavior when no rootPath is
supplied.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Line 2370: Clear the checkedTranscriptTails marker whenever its transcript
path is torn down, including deleteSession, dispose, finishSession, and
forceDisposeAll, using the same normalized path key added during transcript
flushing. Update the lifecycle cleanup around checkedTranscriptTails and
isTranscriptPathActive so ended or deleted sessions no longer remain marked
active, while preserving pendingTranscriptWrites behavior.
- Around line 30762-30792: Extend the disk-pressure “chat_turn” gate used by
sendMessage() to messageSession() before queued, auto, or wake sends can reach
steer(), and to runSessionTurn() before executePreparedSendMessage() dispatches
provider work. Preserve existing failure emission and return behavior for denied
decisions. Clear checkedTranscriptTails during session/transcript teardown so
isTranscriptPathActive() does not permanently protect obsolete transcript paths.
In `@apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts`:
- Around line 1565-1567: Sanitize lastFailure.detail in the reportDetail
construction within the local runtime connection pool before appending it to the
thrown error. Normalize or strip control characters and unsafe raw diagnostic
content while preserving the existing capped-length behavior from
lastFailureStore.ts, and keep the fallback unchanged when no detail exists.
- Around line 1558-1561: Update the recovery classification around
probeSocketHasOwner so probe timeouts and error events are not converted into
socket_stale_no_owner. Preserve socket_owned_by_other for confirmed ownership,
map only definitive missing or refused-socket results to socket_stale_no_owner,
and propagate or classify indeterminate probe failures separately.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 30369-30409: Wrap the assertions and persistence attempt after
injecting renameFault in a try/finally block, and move renameFault.restore()
into the finally clause. Keep the existing checks and follow-up updateSession
behavior unchanged while guaranteeing the fs.renameSync fault is restored even
when an assertion fails.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 7856-7876: The gzip transcript path in
readTranscriptTailForHistory currently inflates the full file, while
resolveBestTranscriptPathForSessionId inflates the same candidate again to
determine size. Reuse the decompressed buffer or thread its length through
parseTranscriptHistoryTail and the path-resolution flow so each gzip candidate
is read and decompressed only once, while preserving tail slicing and offset
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
Run ID: bdbbdf39-2b3f-42ad-afd2-bc893fb92602
📒 Files selected for processing (13)
apps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/storage/storageInsightsService.test.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.test.tsxapps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsxapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/codedError.test.tsapps/desktop/src/shared/codedError.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/main/services/processes/processService.ts
- apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.test.tsx
- apps/desktop/src/renderer/state/appStore.ts
- apps/desktop/src/renderer/components/app/ProjectRecoveryScreen.tsx
- apps/desktop/src/main/services/storage/storageInsightsService.test.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/main/services/storage/storageInsightsService.ts
- apps/desktop/src/main/services/pty/ptyService.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf6c6f4ee6
ℹ️ 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".
- surfaceCodedError re-encodes an already-code-prefixed error when a rootPath needs attaching, so an open-repo failure that rethrows a pre-prefixed coded error no longer drops the path the recovery screen needs. - parseCodedErrorMessage splits the rootPath off before the wrapper strip trims the string, so a valid filesystem path with leading or trailing whitespace survives byte-for-byte instead of pointing the repair at a nonexistent directory. Regression test covers a spaced path. - The continuity-recovery tests clean up their per-test CODEX_HOME temp directory instead of leaking one per run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
…sules
- The idle-compression sweep gated on "not normal", so warning-level
pressure returned 0 files — refusing to compress exactly when the user
most wants to reclaim space. It now gates on canPerform("compression")
(allowed at warning, refused only at critical/exhausted), matching
compressOne. Test covers a warning-level sweep compressing a file.
- buildContinuityCapsule read transcript bytes by offset and fed them to
the parser, so a gzipped transcript produced no envelopes and the
Recover-from-history capsule was empty for exactly the old chats most
likely to be compressed. It now decompresses .gz transcripts
transparently before parsing. Test proves the capsule carries the
original task from a compressed transcript.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: defe235b9a
ℹ️ 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 on lines
+79
to
+82
| if (fs.existsSync(filePath)) { | ||
| try { | ||
| fs.copyFileSync(filePath, `${filePath}.lkg`); | ||
| lkgUpdated = true; |
There was a problem hiding this comment.
When the primary JSON is already torn/corrupt but the .lkg is still valid, this copies the bad primary over the only good backup before attempting the new atomic write. If that new write then fails under the disk-full/interrupted-write conditions this code is meant to survive, readJsonWithRecovery() can no longer recover the previous chat/session state because both files are now invalid. Only promote the current primary to .lkg after validating it, or update the backup atomically from the last valid payload.
Useful? React with 👍 / 👎.
Comment on lines
+207
to
+208
| const timer = window.setTimeout(() => { | ||
| void switchProjectToPath(rootPath); |
There was a problem hiding this comment.
If switchProjectToPath(rootPath) rejects after repair() reported success (for example the service still cannot open the project), the rejection is ignored and this component stays in the success phase with reopenStartedRef set. The store may receive a new transition error, but the recovery screen remains mounted for the same root and keeps telling the user the project was repaired and reopened, with no retry/failure state. Catch the reopen failure and move the screen back to a failure/idle state or reset the reopen guard.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Incident response + product hardening for the July 12 disk-full outage, where a full disk during a kvDb table rebuild bricked every subsequent launch and two chats silently lost their Codex threads. This PR makes a full disk an ordinary operating condition: canonical data is never silently lost, ADE recovers automatically when safe, and a nontechnical user gets a guided, jargon-free recovery path.
Durability
migrate(). Also stops the previously unprotected rebuild ofautomation_ingress_eventsthat ran on every startup..lkgprevious generation; provider-pointer changes are journaled to a bounded (64 KiB) ledger; truncated transcript tails are healed instead of corrupting the next append; pre-migration DB backups are headroom-checked, size-verified, and singular.Continuity
Recovery UX
AdeRecoveryErrorCode) survive IPC; the brain writes bounded machine/project last-failure reports, backs off on crash loops, dedupes repeated failure logs, and copytruncates launchd logs at 10 MiB.Storage
ade-*staging.ade storage snapshot/compress, last-failure line inade brain status, storage line inade doctor.Tests & docs
PRAGMA max_page_count.docs/features/storage-and-recovery/README.md(contracts + all retention bounds) plus updates to chat, terminals, search, settings, remote-runtime docs and ARCHITECTURE.Validation
syncHostService.test.ts"streams terminal snapshots…" — reproduces identically at the branch basef9344115on this machine; unrelated to this diff).remoteRuntime.offlineRpc.integration.test.tsis a dormant pre-existing test that no vitest project ran before this branch; the newintegration-mainproject is deliberately scoped so it stays dormant (see comment invitest.workspace.ts).🤖 Generated with Claude Code
Summary by CodeRabbit
ade storage/ade diskCLI commands for snapshot, compress, and maintenance actions (including text output).Greptile Summary
This PR hardens ADE storage, recovery, and continuity paths against disk-full and interrupted-write failures. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
No blocking issues found in the changed code.
What T-Rex did
Important Files Changed
Reviews (8): Last reviewed commit: "Compress at warning level and decompress..." | Re-trigger Greptile