Conversation
Replace project-scoped legacy JSON cleanup with default-on machine-global JSONL retention. Preserve cold history in verified gzip archives while enforcing aggregate storage bounds and coordinating cleanup through ownership-safe leases and locks.
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a core session-recording janitor with default-on retention, global scanning, bounded JSONL header parsing, archive compression, lease coordination, lock cleanup, and structured metrics. The CLI delegates cleanup to the janitor and exposes retention settings. ChangesSession recording cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
WalkthroughBefore this change, recorded-session maintenance was fragmented and unbounded: cleanup logic lived in ad-hoc CLI utilities, there was no global coordination for concurrent janitor work, and safety around active or in-progress sessions was limited. After this PR, session reclamation is centralized behind a safe global janitor. The core now provides bounded header reading, explicit session locking, lease-based ownership, retention policies, archive compression, and a reclamation engine, while the CLI cleanup path is rewritten to use those bounded primitives and respect the new safety checks. The result is a coordinated, safer cleanup flow that protects current sessions and bounds the work the janitor may perform at once. Release NotesNew Features
Bug Fixes
Tests
Documentation
Refactor
Chore
Changes
Sequence DiagramsequenceDiagram
participant Settings as CLI Settings
participant Cleanup as sessionCleanup
participant SessionUtils as sessionUtils
participant Recording as recording layer
participant LockManager as SessionLockManager
Settings->>Cleanup: load merged sessionRetention config
Cleanup->>Cleanup: validate maxAge, maxCount, minRetention
Cleanup->>SessionUtils: getAllSessionFiles(chatsDir, currentSessionId)
SessionUtils-->>Cleanup: session file entries with parsed or null sessionInfo
Cleanup->>Cleanup: identify corrupted and expired deletable sessions
Cleanup->>SessionUtils: delete session JSONL files by retention policy
Recording->>Recording: bounded header/session reads for discovery and replay
Recording->>LockManager: acquire, stale-check, and cleanup orphaned session locks
Magnitude🎯 4 (XL) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (23)
packages/core/src/recording/SessionLockManager.safety.test.ts (1)
381-430: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport a timeout distinctly in
runBunScript.When the timer fires, the helper sends
SIGTERM. Theclosehandler then receives anullexit code and rejects withProcess exited with code null. The message does not state that the timeout caused the termination. Set atimedOutflag in the timer and include it in the rejection message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.safety.test.ts` around lines 381 - 430, Update runBunScript to track whether the timeout callback terminated the child by setting a timedOut flag before sending SIGTERM, then have the close-handler rejection message explicitly report the timeout when that flag is set while preserving the existing exit-code and stderr details otherwise.packages/core/src/recording/SessionLockManager.internals.ts (1)
199-222: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the temp-write failure path cannot be read as "lock exists".
tryCreateLockreturnsfalsewhen the secondwriteTempLockFilecall also returnsfalse.acquirethen enterstryStaleTakeover, which treats the situation as "lock already exists".writeTempLockFilereturnsfalseonly forEEXISTandENOENT, so the case is narrow, but anENOENTthat persists aftermkdir(for example, a concurrently removedchatsDir) is reported as lock contention rather than an I/O failure. Consider distinguishing the two outcomes so the caller can propagate a real I/O error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.internals.ts` around lines 199 - 222, Update tryCreateLock and its acquire caller so a persistent temp-file write failure caused by ENOENT is propagated as an I/O error rather than returned as lock contention, while preserving false for genuine EEXIST contention. Ensure acquire only invokes tryStaleTakeover when the lock actually exists, and retain cleanup of the temporary path on failure.packages/core/src/recording/SessionLockManager.lazy.test.ts (1)
46-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
runBunScriptduplicates the helper inSessionLockManager.safety.test.ts.Both files define the same spawn, timeout, and settle logic. Extract one helper into a shared test-utility module and import it in both files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.lazy.test.ts` around lines 46 - 89, Extract the shared spawn, timeout, cleanup, and settlement logic from runBunScript into a common test-utility module, then import and reuse that helper in both SessionLockManager.lazy.test.ts and SessionLockManager.safety.test.ts. Preserve the existing timeout, stdout handling, error reporting, and Promise settlement behavior.packages/core/src/recording/SessionLockManager.property.test.ts (1)
827-873: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese
itPropcases use fixed inputs.None of the three cases declares arbitraries, so each case repeats the same fixed scenario for every property run. Either use
itfor these deterministic cases, or generate the age so the property covers both sides of the 48-hour bound. A generated age also proves the boundary rather than one point at 49 hours.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.property.test.ts` around lines 827 - 873, Replace the three deterministic itProp cases around checkStaleWithPidReuse with it, or add an age arbitrary that exercises both sides of the 48-hour threshold. Prefer generated ages if retaining property tests, and assert stale only when the generated age exceeds the bound while recent locks remain non-stale.packages/core/src/recording/SessionLockManager.ts (1)
56-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the safe path-validation helpers.
SessionLockManager.tsandjanitor/sessionSafety.tsduplicateisValidSafeSessionIdandisDirectChildPath, including the 256-character limit. Move them to a dependency-free module, or add a test that compares both implementations to prevent drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.ts` around lines 56 - 67, Centralize SAFE_ID_MAX_LENGTH, SAFE_SESSION_ID_RE, isValidSafeSessionId, and isDirectChildPath in a dependency-free shared module, then update SessionLockManager and janitor/sessionSafety.ts to reuse those helpers instead of duplicating them. Preserve the existing validation behavior and avoid introducing filesystem dependencies into the shared module.packages/core/src/recording/janitor/sessionGrouping.ts (1)
86-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a byte-wise comparison for the deterministic tie-break.
localeComparedepends on the active locale and the ICU build. Two machines can order the same two paths differently, which contradicts the "Deterministic" claim in the doc comment. Ordering drives which sessions are evicted first, so the result must not depend on the environment.♻️ Proposed change
const mtimeDiff = a.mtime.getTime() - b.mtime.getTime(); if (mtimeDiff !== 0) return mtimeDiff; - return normalizedPath(representativePath(a)).localeCompare( - normalizedPath(representativePath(b)), - ); + const pathA = normalizedPath(representativePath(a)); + const pathB = normalizedPath(representativePath(b)); + if (pathA < pathB) return -1; + if (pathA > pathB) return 1; + return 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionGrouping.ts` around lines 86 - 95, Update compareGroupsOldestFirst to replace localeCompare in the normalized representative-path tie-break with a locale-independent byte-wise comparison, while preserving the mtime ordering and deterministic handling of equal paths.packages/core/src/recording/boundedHeaderReader.test.ts (2)
159-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the chunk size instead of duplicating it.
Line 164 hardcodes
64 * 1024. If the reader changes its chunk size, this test no longer places the multi-byte character on a chunk boundary, and the regression it guards stops being covered. Export the chunk size fromboundedHeaderReader.tsand import it here.♻️ Proposed change
- const READ_CHUNK_SIZE = 64 * 1024; - const before = 'a'.repeat(READ_CHUNK_SIZE - 1); + const before = 'a'.repeat(BOUNDED_HEADER_READ_CHUNK_SIZE - 1);Add the export in
packages/core/src/recording/boundedHeaderReader.ts:export const BOUNDED_HEADER_READ_CHUNK_SIZE = 64 * 1024;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/boundedHeaderReader.test.ts` around lines 159 - 173, Export the reader’s chunk-size constant from boundedHeaderReader.ts as BOUNDED_HEADER_READ_CHUNK_SIZE, then import and use it in the test’s boundary setup instead of duplicating 64 * 1024. Update the before-string calculation to reference this shared constant so the multi-byte boundary coverage follows reader changes.
132-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the exclusive byte boundary.
readBoundedFirstLineinspects at mostBOUNDED_HEADER_MAX_BYTESbytes. The newline must occur before byteBOUNDED_HEADER_MAX_BYTES;MAX - 1content bytes plus LF succeeds, whileMAXcontent bytes plus LF returnsnull. State this exact-boundary rule in the reader doc comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/boundedHeaderReader.test.ts` around lines 132 - 157, Update the doc comment for readBoundedFirstLine to explicitly state the exclusive byte-boundary rule: it reads at most BOUNDED_HEADER_MAX_BYTES bytes, and the newline must occur before byte BOUNDED_HEADER_MAX_BYTES. Document that MAX - 1 content bytes plus LF succeeds, while MAX content bytes plus LF returns null.packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts (2)
587-618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe inline comment does not match the implementation.
The comment states "each ~4 KB allocated".
sessionScannerandevictArchivesForBudgetusestat().size, which is the logical byte size, not the allocated block size. Correct the comment so a later reader does not tune the budget against block allocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts` around lines 587 - 618, Update the inline budget comment in the equal-mtime test to describe the archives by their logical byte size, as measured by stat().size, rather than allocated filesystem blocks. Keep the budget explanation consistent with the actual size-based behavior of sessionScanner and evictArchivesForBudget.
38-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared test helpers.
makeTempDir,validHash64,makeConfig,createSession, andfileExistsare duplicated almost verbatim inpackages/core/src/recording/janitor/sessionJanitor.safety.test.tsLines 51-110. The two copies ofcreateSessionalready differ: the safety copy wrapsflushintry/finallyarounddispose, this copy does not. Move the helpers into a shared test-support module so the behavior stays identical.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts` around lines 38 - 118, Extract makeTempDir, validHash64, makeConfig, createSession, and fileExists into a shared test-support module, then import and reuse them from both sessionJanitor.reclamation.test.ts and sessionJanitor.safety.test.ts. Preserve the safety test’s try/finally flush-and-dispose behavior in the shared createSession implementation, and leave reclamation-specific makeArchive and listArchives local.packages/core/src/recording/janitor/reclamationEngine.ts (1)
83-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe unlink fault seam ships in production code.
unlinkFaultFnis module-global mutable state, andsetUnlinkFaultForTestis a public export. Any consumer of@vybestack/llxprt-code-corecan replace the unlink implementation for the janitor. Consider excluding this export from the package barrel (packages/core/src/recording/janitor/index.ts) so it stays reachable only through a direct module import in tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/reclamationEngine.ts` around lines 83 - 105, Exclude setUnlinkFaultForTest from the janitor package barrel in packages/core/src/recording/janitor/index.ts so it is not exposed through the public package API. Keep the function available via direct module imports for tests, while preserving the existing platformUnlink behavior.packages/core/src/recording/janitor/archiveCompressor.ts (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCouple the temp grammar to the generated temp name.
cleanupStaleTempArchivesonly removes files that match^session-.+\.gz\.tmp$. The temp name at Line 224 ispath.basename(sourcePath) + '.' + uuid + TEMP_ARCHIVE_SUFFIX. If a source basename does not start withsession-, the temp artifact never matches the grammar, so a crashed compression leaves an orphan file forever. Add a fixed janitor prefix to the temp name so the grammar always matches, independent of the source basename.♻️ Proposed change
-const TEMP_ARCHIVE_GRAMMAR = /^session-.+\.gz\.tmp$/; +const TEMP_ARCHIVE_PREFIX = 'session-janitor-'; +const TEMP_ARCHIVE_GRAMMAR = /^session-.+\.gz\.tmp$/;- const tempName = sourceBase + '.' + crypto.randomUUID() + TEMP_ARCHIVE_SUFFIX; + const tempName = + TEMP_ARCHIVE_PREFIX + + sourceBase + + '.' + + crypto.randomUUID() + + TEMP_ARCHIVE_SUFFIX;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/archiveCompressor.ts` around lines 52 - 55, Update the temp archive name construction in the compression flow to prepend the fixed `session-` janitor prefix before the source basename, UUID, and `TEMP_ARCHIVE_SUFFIX`. Keep `TEMP_ARCHIVE_GRAMMAR` unchanged so every generated temporary artifact matches `cleanupStaleTempArchives`, regardless of the source filename.packages/core/src/recording/janitor/archiveCompressor.test.ts (1)
243-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate these symlink tests on non-Windows platforms.
Both tests call
fs.symlink, which requires elevated privileges on Windows. Useit.skipIf(process.platform === 'win32'). This is the same root cause as the symlink tests inpackages/core/src/recording/janitor/archiveCompressor.safety.test.ts.Based on learnings: "prefer platform-gating via
it.skipIf(process.platform === 'win32')instead of doing an early return inside the test body".Also applies to: 268-285
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/archiveCompressor.test.ts` around lines 243 - 254, Gate both symlink-related tests in the archive compressor test suite with it.skipIf(process.platform === 'win32'), including the test around compressToArchive and the additional test at the referenced range. Apply the gate directly to each it declaration and remove or avoid any in-test early-return platform checks.Source: Learnings
packages/core/src/recording/janitor/sessionJanitor.safety.test.ts (1)
150-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGate these symlink tests on non-Windows platforms.
Both tests call
fs.symlink, which requires elevated privileges on Windows. Useit.skipIf(process.platform === 'win32').Based on learnings: "prefer platform-gating via
it.skipIf(process.platform === 'win32')instead of doing an early return inside the test body".Also applies to: 196-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.safety.test.ts` around lines 150 - 182, Gate both symlink safety tests, including the test around runSessionCleanup and the additional test at the referenced section, with it.skipIf(process.platform === 'win32') so they are skipped on Windows before execution. Remove any equivalent early-return approach if present and leave the test assertions unchanged.Source: Learnings
packages/core/src/recording/janitor/archiveCompressor.safety.test.ts (1)
61-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSymlink-based tests are not portable to Windows. Every one of these tests calls
fs.symlink, which requires elevated privileges or Developer Mode on Windows, so each fails for a reason unrelated to the code under test. Applyit.skipIf(process.platform === 'win32')at each site.
packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L61-L92: gate the symlinked archive directory and symlinked source tests.packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L159-L180: gate the symlinked temp file lstat test.packages/core/src/recording/janitor/archiveCompressor.test.ts#L243-L254: gate the symlinked sourcesource-invalidtest.packages/core/src/recording/janitor/archiveCompressor.test.ts#L268-L285: gate the symlinked existing archive test.packages/core/src/recording/janitor/sessionJanitor.safety.test.ts#L150-L182: gate the symlinked archive directory sweep test.packages/core/src/recording/janitor/sessionJanitor.safety.test.ts#L196-L229: gate the scan-to-mutation symlink replacement test.Based on learnings: "For TypeScript unit/integration tests that intentionally skip behavior on Windows, prefer platform-gating via
it.skipIf(process.platform === 'win32')".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/archiveCompressor.safety.test.ts` around lines 61 - 92, Symlink-dependent tests must be skipped on Windows. Apply it.skipIf(process.platform === 'win32') to the two tests in packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L61-L92, the lstat symlink test at packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L159-L180, the source-invalid and existing-archive symlink tests in packages/core/src/recording/janitor/archiveCompressor.test.ts#L243-L254 and `#L268-L285`, and both symlink tests in packages/core/src/recording/janitor/sessionJanitor.safety.test.ts#L150-L182 and `#L196-L229`.Source: Learnings
packages/core/src/recording/janitor/janitorLease.ts (3)
136-148: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the file handle before unlinking the temp file.
The
catchblock callssafeUnlink(tempPath)whilefdis still open. Thefinallyblock closesfdafterwards. On Windows,unlinkon an open handle fails, so the temp file remains on disk after a write failure.Move the unlink after the close.
♻️ Proposed reordering
let fd: fsp.FileHandle | undefined; + let createFailed = false; try { fd = await fsp.open(tempPath, 'wx'); await fd.writeFile(JSON.stringify(record), 'utf-8'); await fd.sync(); } catch (error: unknown) { - await safeUnlink(tempPath); - // EEXIST (uuid collision) is the only benign retryable case. - if ((error as NodeJS.ErrnoException).code === 'EEXIST') return false; - throw error; // Propagate genuine I/O failures. + createFailed = true; + await fd?.close().catch(() => {}); + fd = undefined; + await safeUnlink(tempPath); + // EEXIST (uuid collision) is the only benign retryable case. + if ((error as NodeJS.ErrnoException).code === 'EEXIST') return false; + throw error; // Propagate genuine I/O failures. } finally { - await fd?.close().catch(() => {}); + if (!createFailed) await fd?.close().catch(() => {}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.ts` around lines 136 - 148, Reorder the error cleanup in the file-writing flow so the handle opened by fsp.open is closed before safeUnlink(tempPath) runs. Update the try/catch/finally logic around the visible fd variable while preserving EEXIST’s false return and propagation of other errors.
85-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe heartbeat state is per class, not per lease.
heartbeatTimerandinFlightHeartbeatare static.startHeartbeatcallsstopHeartbeatfirst. If one process acquires two leases for two differentglobalTempDirvalues, the second acquisition stops the heartbeat of the first lease. The first lease then ages out and another process can take it over while it is still in use.The janitor currently uses one global temp root, so this is latent. Consider binding the timer to the handle returned by
makeHandle.Also applies to: 251-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.ts` around lines 85 - 86, Move heartbeat state out of the static class fields and bind each timer and in-flight heartbeat to the lease handle created by makeHandle. Update startHeartbeat and stopHeartbeat to operate on that handle’s state so acquiring a second lease cannot stop the first lease’s heartbeat, while preserving cleanup for each individual lease.
401-414: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not release a claim that was never acquired.
acquireTransitionClaimreturnstrueforENOENTwithout creating the claim link. The caller then always callsreleaseTransitionClaim, which unlinks<leasePath>.tclaim. If another process creates the lease and its claim in that window, this unlink removes the other process's claim link. The other process then failsverifyTransitionClaimand skips its heartbeat or release, which can leave a lease file behind.Track whether the claim was actually created and release only in that case.
♻️ Sketch: return an explicit claim state
- private static async acquireTransitionClaim( - leasePath: string, - ): Promise<boolean> { + private static async acquireTransitionClaim( + leasePath: string, + ): Promise<'claimed' | 'no-lease' | 'busy'> { const claimPath = JanitorLease.getClaimPath(leasePath); try { await fsp.link(leasePath, claimPath); - return true; // Claimed the lease inode. + return 'claimed'; } catch (error: unknown) { const code = (error as NodeJS.ErrnoException).code; - if (code === 'ENOENT') return true; // No lease — no claim needed. - if (code !== 'EEXIST') return false; + if (code === 'ENOENT') return 'no-lease'; + if (code !== 'EEXIST') return 'busy'; } - return JanitorLease.tryReclaimClaim(leasePath); + return (await JanitorLease.tryReclaimClaim(leasePath)) ? 'claimed' : 'busy'; }Callers then skip
releaseTransitionClaimwhen the result is'no-lease'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.ts` around lines 401 - 414, Update acquireTransitionClaim to return an explicit state distinguishing an actually created claim from the ENOENT no-lease case, while preserving contention handling through tryReclaimClaim. In the caller of acquireTransitionClaim, invoke releaseTransitionClaim only when the state confirms this process created the claim, and skip release for the no-lease result.schemas/settings.schema.json (1)
254-272: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd numeric bounds so editors reject invalid retention values.
resolveRetentionConfigthrows whenmaxTotalSizeMBis not positive. The schema accepts any number, so editors accept0or a negative value and the failure appears only at runtime.maxCounthas the same gap.♻️ Proposed constraints
"default": 4096, - "type": "number" + "type": "number", + "exclusiveMinimum": 0 }, @@ "markdownDescription": "Maximum number of sessions to keep (most recent). No default count limit.\n\n- Category: `General`\n- Requires restart: `no`", - "type": "number" + "type": "number", + "minimum": 0, + "multipleOf": 1 },Note: this file appears generated from
packages/cli/src/config/settings-schema/schema-core.ts. Apply the change at the source definition.🤖 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 `@schemas/settings.schema.json` around lines 254 - 272, Update the source schema definition for the retention settings, not the generated JSON, to add positive numeric bounds for maxTotalSizeMB and maxCount. Use the existing schema constraints conventions so editors reject zero and negative values while preserving their current defaults and descriptions.packages/core/src/recording/janitor/janitorLease.safety.test.ts (2)
202-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
utimeson the temp directory.Line 204 sets the mtime of
tempDir.checkLeaseStalenessreads the mtime of the lease file, not of the directory. The followingwriteFilealso resets the directory mtime. Line 209 already sets the required file mtime.♻️ Proposed cleanup
// Write a corrupt lease file (old enough to be past the age bound). - const oldTime = new Date(Date.now() - 2 * 60 * 60 * 1000).toISOString(); - await fs.utimes(tempDir, new Date(oldTime), new Date(oldTime)); await fs.writeFile(leasePath, 'this is corrupt garbage!!!');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.safety.test.ts` around lines 202 - 209, Remove the unnecessary fs.utimes call targeting tempDir in the corrupt lease setup, leaving the leasePath mtime update as the sole age-setting operation before checkLeaseStaleness is exercised.
435-494: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTerminate the child process when the test fails.
The test spawns
bunbut never kills the child. If either wait rejects on timeout, the test fails and the child keeps running for the rest of its sleep. TheafterEachthen removestempDirwhile the child still writes to it.janitorLease.test.tsuses arunBunScripthelper that kills the child; reuse that pattern here.♻️ Proposed guard
const child = spawn('bun', ['-e', script], { stdio: ['pipe', 'pipe', 'pipe'], env: { ...process.env, TEST_TEMP_DIR: tempDir }, }); + let childExited = false; + child.on('close', () => (childExited = true)); + try { + // ... existing waits and assertions ... + } finally { + if (!childExited) child.kill('SIGTERM'); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.safety.test.ts` around lines 435 - 494, Ensure the subprocess spawned in the lease test is terminated whenever the test fails or either wait rejects, using the cleanup pattern from runBunScript in janitorLease.test.ts. Wrap the spawn-and-wait flow with guaranteed child cleanup, while preserving normal release handling and preventing afterEach from removing tempDir while the child remains active.packages/core/src/recording/janitor/sessionJanitor.test.ts (1)
43-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
cryptoexplicitly.
validHash64andmakeConfiguse the globalcrypto. This depends on the TypeScriptlibconfiguration exposingCryptoin a non-DOM package. An explicitnode:cryptoimport removes that dependence and matches the other files in this directory.♻️ Proposed import
import * as path from 'node:path'; +import * as crypto from 'node:crypto';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.test.ts` around lines 43 - 50, Import the Node crypto module explicitly in sessionJanitor.test.ts and update validHash64 and makeConfig to use that imported crypto reference instead of the global. Match the existing node:crypto import style used by neighboring files.packages/core/src/recording/janitor/sessionJanitor.ts (1)
341-353: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe final rescan doubles the scan cost of every sweep.
buildFinalResultruns a second fullscanGlobalSessionsover all project-hash directories. The sweep already knowsbytesBeforeand the reclaimed byte counts. On large recording roots this second walk adds significantreaddir/statI/O at startup.If exact post-state bytes are not required, derive
bytesAfterfrombytesBeforeminus reclaimed bytes, and keep the rescan behind an option.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.ts` around lines 341 - 353, Update buildFinalResult to avoid unconditionally calling scanGlobalSessions for the final state. Derive bytesAfter from bytesBefore minus the bytes successfully reclaimed during the sweep, and only perform the final rescan when the existing or added option explicitly requests exact post-state bytes; preserve rescan error accounting when that option is enabled.
🤖 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 `@packages/core/src/recording/boundedHeaderReader.ts`:
- Around line 99-109: Update the decoding flow around StringDecoder.write and
bomStripped so bomStripped is set to true only when decoded text exists. Keep it
false when decoder.write returns an empty string for an incomplete BOM, allowing
the BOM to be removed once the sequence completes before JSON parsing.
In `@packages/core/src/recording/janitor/archiveCompressor.test.ts`:
- Around line 321-344: Make the test’s unreadable-source setup in “returns typed
source-invalid error when source becomes unreadable during reuse check” work
under root by skipping when process.getuid() is 0 or replacing the chmod-based
failure with a privilege-independent unreadable path such as a directory.
Preserve the assertions for the source-invalid result and restore any modified
filesystem state in the existing cleanup.
In `@packages/core/src/recording/janitor/archiveCompressor.ts`:
- Around line 193-215: Move the existing archiveDir identity validation using
isRegularNonSymlinkDir and pathExists above the tryReuseExistingArchive call.
Ensure symlink or non-directory paths return archiveError before any reuse
logic, while preserving the existing reuse and mkdir behavior for valid
directories.
In `@packages/core/src/recording/janitor/janitorLease.test.ts`:
- Around line 118-133: Gate the “tryAcquire propagates I/O errors” test using
it.skipIf so it runs only on non-root POSIX environments, excluding Windows and
processes running with root privileges. Keep the existing chmod, assertion, and
cleanup logic unchanged for supported environments.
- Line 282: Move the 30000ms timeout from the enclosing describe configuration
to the affected it test so bun:test applies it. Preserve the test body and suite
structure, changing only the timeout placement.
In `@packages/core/src/recording/janitor/reclamationEngine.ts`:
- Around line 570-582: Update evictArchivesForBudget after the successful
scanGlobalSessions call to replace state.totalBytes with the fresh scan’s
authoritative aggregate, then re-check the maxTotalSizeBytes budget before
constructing the evictable list. Preserve the existing scan-error accounting and
early return behavior.
- Around line 357-371: Reconcile the running size estimate in
packages/core/src/recording/janitor/reclamationEngine.ts#L357-L371 by avoiding
addition of outcome.archiveBytes when compressToArchive reused an archive
already counted by the scan; expose reuse state through ArchiveResult or
subtract the previously counted archive size. At
packages/core/src/recording/janitor/reclamationEngine.ts#L570-L582, assign
state.totalBytes from the freshScan aggregate after rescanning, then re-check
the budget before constructing the evictable list.
- Around line 492-500: Update the compression-failure branch after
compressToArchive in the reclamation flow to set failed based on
result.errorKind, counting platform failures while keeping protective refusals
uncounted. Preserve the existing archived, rawDeleted, and archiveBytes values.
In `@packages/core/src/recording/janitor/sessionJanitor.safety.test.ts`:
- Around line 13-14: Update the license header in sessionJanitor.safety.test.ts
to use the complete Apache-2.0 wording, including “permissions and” between
“governing” and “limitations.”
In `@packages/core/src/recording/janitor/sessionJanitor.ts`:
- Around line 181-194: Update the CLI startup cleanup flow around
cleanupExpiredSessions so retention configuration resolution is covered by the
existing try/catch boundary. Ensure errors from resolveRetentionConfig,
including through runSessionCleanupWithSettings, are caught and treated as
best-effort cleanup failures rather than propagating from the cli.tsx startup
await.
In `@packages/core/src/recording/janitor/sessionScanner.test.ts`:
- Around line 360-377: Gate the EACCES test and the symlink tests in this
describe block with it.skipIf(process.platform === 'win32') and a root-privilege
check, so they do not run where chmod or symlink creation cannot enforce the
expected failure. Apply the gating to the test declarations, preserving the
existing test bodies and assertions for supported non-root environments.
In `@packages/core/src/recording/janitor/sessionScanner.ts`:
- Around line 207-220: Update the isCurrentSession assignment in the
SessionCandidate construction to return true only when currentSessionId is
defined and equals the parsed header sessionId; preserve false when either value
is undefined.
In `@packages/core/src/recording/SessionLockManager.internals.ts`:
- Around line 391-398: Apply the same managed-target validation used by
temp-artifact cleanup to the orphaned guard sweep around guardFiles: only
process names matching the exact <safeSessionId>.lock.tguard grammar, and
require each path to be a direct child of chatsDir, a regular file, and not a
symlink before calling checkStaleWithPidReuse or safeUnlink.
In `@packages/core/src/recording/SessionLockManager.safety.test.ts`:
- Around line 446-496: Update the stale-takeover race test around the subprocess
script and runBunScript calls to synchronize child startup with an explicit
readiness barrier. Have each of the three children create a unique ready file
and wait until all three ready files exist before calling
SessionLockManager.acquire; apply the same barrier and timing-independent
coordination to the second race test with the 500 ms hold. Preserve the
single-winner assertions and existing lock behavior.
In `@packages/core/src/recording/SessionLockManager.test.ts`:
- Around line 366-386: Gate the read-only-directory test around
SessionLockManager.acquire with it.skipIf so it is skipped on Windows and when
process.getuid() indicates root privileges. Preserve the existing I/O-error
assertions and cleanup, using the repository’s skipIf convention rather than an
early return inside the test.
In `@packages/core/src/recording/SessionLockManager.ts`:
- Around line 78-94: Update isDirectChildPath to remove any trailing path
separator from the normalized parent before constructing parentWithSep, while
preserving the existing direct-child validation and rejection of the parent
itself or nested paths. Ensure assertSafeLockPath accepts valid lock paths when
chatsDir is provided with a trailing separator.
---
Nitpick comments:
In `@packages/core/src/recording/boundedHeaderReader.test.ts`:
- Around line 159-173: Export the reader’s chunk-size constant from
boundedHeaderReader.ts as BOUNDED_HEADER_READ_CHUNK_SIZE, then import and use it
in the test’s boundary setup instead of duplicating 64 * 1024. Update the
before-string calculation to reference this shared constant so the multi-byte
boundary coverage follows reader changes.
- Around line 132-157: Update the doc comment for readBoundedFirstLine to
explicitly state the exclusive byte-boundary rule: it reads at most
BOUNDED_HEADER_MAX_BYTES bytes, and the newline must occur before byte
BOUNDED_HEADER_MAX_BYTES. Document that MAX - 1 content bytes plus LF succeeds,
while MAX content bytes plus LF returns null.
In `@packages/core/src/recording/janitor/archiveCompressor.safety.test.ts`:
- Around line 61-92: Symlink-dependent tests must be skipped on Windows. Apply
it.skipIf(process.platform === 'win32') to the two tests in
packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L61-L92,
the lstat symlink test at
packages/core/src/recording/janitor/archiveCompressor.safety.test.ts#L159-L180,
the source-invalid and existing-archive symlink tests in
packages/core/src/recording/janitor/archiveCompressor.test.ts#L243-L254 and
`#L268-L285`, and both symlink tests in
packages/core/src/recording/janitor/sessionJanitor.safety.test.ts#L150-L182 and
`#L196-L229`.
In `@packages/core/src/recording/janitor/archiveCompressor.test.ts`:
- Around line 243-254: Gate both symlink-related tests in the archive compressor
test suite with it.skipIf(process.platform === 'win32'), including the test
around compressToArchive and the additional test at the referenced range. Apply
the gate directly to each it declaration and remove or avoid any in-test
early-return platform checks.
In `@packages/core/src/recording/janitor/archiveCompressor.ts`:
- Around line 52-55: Update the temp archive name construction in the
compression flow to prepend the fixed `session-` janitor prefix before the
source basename, UUID, and `TEMP_ARCHIVE_SUFFIX`. Keep `TEMP_ARCHIVE_GRAMMAR`
unchanged so every generated temporary artifact matches
`cleanupStaleTempArchives`, regardless of the source filename.
In `@packages/core/src/recording/janitor/janitorLease.safety.test.ts`:
- Around line 202-209: Remove the unnecessary fs.utimes call targeting tempDir
in the corrupt lease setup, leaving the leasePath mtime update as the sole
age-setting operation before checkLeaseStaleness is exercised.
- Around line 435-494: Ensure the subprocess spawned in the lease test is
terminated whenever the test fails or either wait rejects, using the cleanup
pattern from runBunScript in janitorLease.test.ts. Wrap the spawn-and-wait flow
with guaranteed child cleanup, while preserving normal release handling and
preventing afterEach from removing tempDir while the child remains active.
In `@packages/core/src/recording/janitor/janitorLease.ts`:
- Around line 136-148: Reorder the error cleanup in the file-writing flow so the
handle opened by fsp.open is closed before safeUnlink(tempPath) runs. Update the
try/catch/finally logic around the visible fd variable while preserving EEXIST’s
false return and propagation of other errors.
- Around line 85-86: Move heartbeat state out of the static class fields and
bind each timer and in-flight heartbeat to the lease handle created by
makeHandle. Update startHeartbeat and stopHeartbeat to operate on that handle’s
state so acquiring a second lease cannot stop the first lease’s heartbeat, while
preserving cleanup for each individual lease.
- Around line 401-414: Update acquireTransitionClaim to return an explicit state
distinguishing an actually created claim from the ENOENT no-lease case, while
preserving contention handling through tryReclaimClaim. In the caller of
acquireTransitionClaim, invoke releaseTransitionClaim only when the state
confirms this process created the claim, and skip release for the no-lease
result.
In `@packages/core/src/recording/janitor/reclamationEngine.ts`:
- Around line 83-105: Exclude setUnlinkFaultForTest from the janitor package
barrel in packages/core/src/recording/janitor/index.ts so it is not exposed
through the public package API. Keep the function available via direct module
imports for tests, while preserving the existing platformUnlink behavior.
In `@packages/core/src/recording/janitor/sessionGrouping.ts`:
- Around line 86-95: Update compareGroupsOldestFirst to replace localeCompare in
the normalized representative-path tie-break with a locale-independent byte-wise
comparison, while preserving the mtime ordering and deterministic handling of
equal paths.
In `@packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts`:
- Around line 587-618: Update the inline budget comment in the equal-mtime test
to describe the archives by their logical byte size, as measured by stat().size,
rather than allocated filesystem blocks. Keep the budget explanation consistent
with the actual size-based behavior of sessionScanner and
evictArchivesForBudget.
- Around line 38-118: Extract makeTempDir, validHash64, makeConfig,
createSession, and fileExists into a shared test-support module, then import and
reuse them from both sessionJanitor.reclamation.test.ts and
sessionJanitor.safety.test.ts. Preserve the safety test’s try/finally
flush-and-dispose behavior in the shared createSession implementation, and leave
reclamation-specific makeArchive and listArchives local.
In `@packages/core/src/recording/janitor/sessionJanitor.safety.test.ts`:
- Around line 150-182: Gate both symlink safety tests, including the test around
runSessionCleanup and the additional test at the referenced section, with
it.skipIf(process.platform === 'win32') so they are skipped on Windows before
execution. Remove any equivalent early-return approach if present and leave the
test assertions unchanged.
In `@packages/core/src/recording/janitor/sessionJanitor.test.ts`:
- Around line 43-50: Import the Node crypto module explicitly in
sessionJanitor.test.ts and update validHash64 and makeConfig to use that
imported crypto reference instead of the global. Match the existing node:crypto
import style used by neighboring files.
In `@packages/core/src/recording/janitor/sessionJanitor.ts`:
- Around line 341-353: Update buildFinalResult to avoid unconditionally calling
scanGlobalSessions for the final state. Derive bytesAfter from bytesBefore minus
the bytes successfully reclaimed during the sweep, and only perform the final
rescan when the existing or added option explicitly requests exact post-state
bytes; preserve rescan error accounting when that option is enabled.
In `@packages/core/src/recording/SessionLockManager.internals.ts`:
- Around line 199-222: Update tryCreateLock and its acquire caller so a
persistent temp-file write failure caused by ENOENT is propagated as an I/O
error rather than returned as lock contention, while preserving false for
genuine EEXIST contention. Ensure acquire only invokes tryStaleTakeover when the
lock actually exists, and retain cleanup of the temporary path on failure.
In `@packages/core/src/recording/SessionLockManager.lazy.test.ts`:
- Around line 46-89: Extract the shared spawn, timeout, cleanup, and settlement
logic from runBunScript into a common test-utility module, then import and reuse
that helper in both SessionLockManager.lazy.test.ts and
SessionLockManager.safety.test.ts. Preserve the existing timeout, stdout
handling, error reporting, and Promise settlement behavior.
In `@packages/core/src/recording/SessionLockManager.property.test.ts`:
- Around line 827-873: Replace the three deterministic itProp cases around
checkStaleWithPidReuse with it, or add an age arbitrary that exercises both
sides of the 48-hour threshold. Prefer generated ages if retaining property
tests, and assert stale only when the generated age exceeds the bound while
recent locks remain non-stale.
In `@packages/core/src/recording/SessionLockManager.safety.test.ts`:
- Around line 381-430: Update runBunScript to track whether the timeout callback
terminated the child by setting a timedOut flag before sending SIGTERM, then
have the close-handler rejection message explicitly report the timeout when that
flag is set while preserving the existing exit-code and stderr details
otherwise.
In `@packages/core/src/recording/SessionLockManager.ts`:
- Around line 56-67: Centralize SAFE_ID_MAX_LENGTH, SAFE_SESSION_ID_RE,
isValidSafeSessionId, and isDirectChildPath in a dependency-free shared module,
then update SessionLockManager and janitor/sessionSafety.ts to reuse those
helpers instead of duplicating them. Preserve the existing validation behavior
and avoid introducing filesystem dependencies into the shared module.
In `@schemas/settings.schema.json`:
- Around line 254-272: Update the source schema definition for the retention
settings, not the generated JSON, to add positive numeric bounds for
maxTotalSizeMB and maxCount. Use the existing schema constraints conventions so
editors reject zero and negative values while preserving their current defaults
and descriptions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8d4a532-9e83-4fc1-b2f8-96f32b66bd70
⛔ Files ignored due to path filters (1)
project-plans/issue3164/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (45)
docs/cli/configuration.mdpackages/cli/src/config/settings-schema/schema-core.tspackages/cli/src/config/settings.tspackages/cli/src/utils/sessionCleanup-test-helpers.tspackages/cli/src/utils/sessionCleanup.boundary.test.tspackages/cli/src/utils/sessionCleanup.config.test.tspackages/cli/src/utils/sessionCleanup.integration.test.tspackages/cli/src/utils/sessionCleanup.test.tspackages/cli/src/utils/sessionCleanup.tspackages/cli/src/utils/sessionUtils.tspackages/core/package.jsonpackages/core/src/recording/ReplayEngine.tspackages/core/src/recording/SessionDiscovery.tspackages/core/src/recording/SessionLockManager.internals.tspackages/core/src/recording/SessionLockManager.lazy.test.tspackages/core/src/recording/SessionLockManager.property.test.tspackages/core/src/recording/SessionLockManager.safety.test.tspackages/core/src/recording/SessionLockManager.test.tspackages/core/src/recording/SessionLockManager.tspackages/core/src/recording/boundedHeaderReader.test.tspackages/core/src/recording/boundedHeaderReader.tspackages/core/src/recording/janitor/archiveCompressor.safety.test.tspackages/core/src/recording/janitor/archiveCompressor.test.tspackages/core/src/recording/janitor/archiveCompressor.tspackages/core/src/recording/janitor/cleanupTypes.tspackages/core/src/recording/janitor/index.tspackages/core/src/recording/janitor/janitorLease.safety.test.tspackages/core/src/recording/janitor/janitorLease.test.tspackages/core/src/recording/janitor/janitorLease.tspackages/core/src/recording/janitor/reclamationEngine.tspackages/core/src/recording/janitor/retentionPolicy.test.tspackages/core/src/recording/janitor/retentionPolicy.tspackages/core/src/recording/janitor/sessionGrouping.tspackages/core/src/recording/janitor/sessionHeaderReader.test.tspackages/core/src/recording/janitor/sessionHeaderReader.tspackages/core/src/recording/janitor/sessionJanitor.reclamation.test.tspackages/core/src/recording/janitor/sessionJanitor.safety.test.tspackages/core/src/recording/janitor/sessionJanitor.test.tspackages/core/src/recording/janitor/sessionJanitor.tspackages/core/src/recording/janitor/sessionSafety.test.tspackages/core/src/recording/janitor/sessionSafety.tspackages/core/src/recording/janitor/sessionScanner.test.tspackages/core/src/recording/janitor/sessionScanner.tspackages/test-utils/src/interactive-run.test.tsschemas/settings.schema.json
💤 Files with no reviewable changes (5)
- packages/cli/src/utils/sessionCleanup.config.test.ts
- packages/cli/src/utils/sessionCleanup-test-helpers.ts
- packages/cli/src/utils/sessionCleanup.test.ts
- packages/cli/src/utils/sessionCleanup.integration.test.ts
- packages/cli/src/utils/sessionUtils.ts
Contributor
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/recording/SessionLockManager.safety.test.ts`:
- Around line 471-485: Make the readiness loop fail if any ready file is still
absent after its deadline: track whether fs.access succeeds for each readyFile,
and throw a timeout error immediately when it does not. Ensure goFile is written
only after every readyFile has been confirmed present.
In `@packages/core/src/recording/SessionLockManager.test.ts`:
- Around line 385-388: Strengthen the error assertion in the test around the
SessionLockManager failure path by verifying the propagated filesystem error has
code ENOTDIR, while retaining the existing SessionLockedError exclusion. Use the
captured error variable and assert its error code directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: faf3685d-22ab-4c9a-9058-dc96dd3db605
📒 Files selected for processing (23)
packages/cli/src/utils/sessionCleanup.tspackages/core/src/recording/SessionJanitor.tspackages/core/src/recording/SessionLockManager.internals.tspackages/core/src/recording/SessionLockManager.lazy.test.tspackages/core/src/recording/SessionLockManager.safety.test.tspackages/core/src/recording/SessionLockManager.test.tspackages/core/src/recording/SessionLockManager.tspackages/core/src/recording/boundedHeaderReader.test.tspackages/core/src/recording/boundedHeaderReader.tspackages/core/src/recording/index.tspackages/core/src/recording/janitor/archiveCompressor.safety.test.tspackages/core/src/recording/janitor/archiveCompressor.test.tspackages/core/src/recording/janitor/archiveCompressor.tspackages/core/src/recording/janitor/cleanupTypes.tspackages/core/src/recording/janitor/index.tspackages/core/src/recording/janitor/janitorLease.test.tspackages/core/src/recording/janitor/reclamationEngine.tspackages/core/src/recording/janitor/sessionJanitor.reclamation.test.tspackages/core/src/recording/janitor/sessionJanitor.safety.test.tspackages/core/src/recording/janitor/sessionJanitor.tspackages/core/src/recording/janitor/sessionSafety.tspackages/core/src/recording/janitor/sessionScanner.test.tspackages/core/src/recording/janitor/sessionScanner.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core/src/recording/janitor/index.ts
- packages/core/src/recording/janitor/janitorLease.test.ts
- packages/core/src/recording/boundedHeaderReader.ts
- packages/core/src/recording/janitor/sessionSafety.ts
- packages/core/src/recording/janitor/archiveCompressor.ts
- packages/cli/src/utils/sessionCleanup.ts
- packages/core/src/recording/janitor/sessionScanner.ts
- packages/core/src/recording/SessionLockManager.internals.ts
- packages/core/src/recording/SessionLockManager.ts
- packages/core/src/recording/janitor/reclamationEngine.ts
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/core/src/recording/SessionLockManager.safety.test.ts (1)
391-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe timeout path does not report a timeout.
At Line 404, the timer sends
SIGTERMand does not settle the promise. Theclosehandler then rejects with the exit code, so a hung child produces a message without the timeout cause. Set a flag in the timer and include it in the rejection message to make failures diagnosable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/SessionLockManager.safety.test.ts` around lines 391 - 440, Update runBunScript so the timeout callback records that the child exceeded the 20-second limit before sending SIGTERM. In the close handler, use that flag to reject with a timeout-specific message while preserving the existing exit-code and stderr details for non-timeout failures.packages/core/src/recording/janitor/janitorLease.ts (1)
84-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProcess-global heartbeat state assumes one lease per process.
heartbeatTimerandinFlightHeartbeatare static. If two lease acquisitions overlap in one process,startHeartbeatclears the first timer, and the first lease then stops heartbeating and can be classified as stale afterSTALE_LEASE_AGE_MS.releaseLeasealso callsstopHeartbeat, which stops a newer lease's timer. The current consumer inpackages/core/src/recording/janitor/sessionJanitor.tsacquires one lease per sweep, so this is not reachable today. Consider binding the timer state to the handle to keep the invariant local.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/janitorLease.ts` around lines 84 - 120, The heartbeat state in JanitorLease is process-global, so overlapping leases can stop one another’s heartbeats. Move heartbeat timer and in-flight heartbeat tracking from the static JanitorLease fields into each JanitorLeaseHandle, and update startHeartbeat, stopHeartbeat, and releaseLease to operate only on the owning handle’s state.packages/cli/src/utils/sessionCleanup.boundary.test.ts (1)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
randomUUIDfromnode:crypto.Replace both
crypto.randomUUID()calls withrandomUUID()to avoid relying on the global Web Crypto object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/utils/sessionCleanup.boundary.test.ts` around lines 43 - 45, Import randomUUID from node:crypto in the test module and update validHash64 to call randomUUID() directly, replacing the existing crypto.randomUUID() usage while preserving the hash-generation logic.packages/core/src/recording/janitor/sessionSafety.test.ts (1)
162-190: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSkip both symlink tests on Windows.
The nightly
windows-latestjob runsnpm run test, and unprivileged Windows runners can rejectfs.symlink. Useit.skipIf(process.platform === 'win32'), matching existing symlink tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionSafety.test.ts` around lines 162 - 190, Update the symlink-specific tests for isRegularNonSymlinkFile and isRegularNonSymlinkDir to use it.skipIf(process.platform === 'win32'), while leaving the regular-file, directory, and non-existent-path tests unchanged.Source: Learnings
packages/core/src/recording/janitor/sessionJanitor.test.ts (1)
92-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
makeArchivewrites uncompressed bytes under a.gzname.The sibling helper in
packages/core/src/recording/janitor/sessionJanitor.reclamation.test.tsgzips the content. Here the file content is plain text. The current assertions only measure sizes, so the tests pass. If a future change validates gzip magic bytes during scanning or eviction, these fixtures become invalid. Consider gzipping the content for realism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.test.ts` around lines 92 - 106, Update the makeArchive test helper to gzip the provided content before writing it to the .gz archive path, matching the behavior of the sibling reclamation test helper while preserving the existing age and return-path handling.packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts (1)
131-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fault-seam reset to this suite's
afterEach.The
Item 1suite installs the process-global unlink fault seam at Line 189. Thetry/finallyblock clears it for that test, but a failure before thetryblock leaves the seam installed for later suites. TheItem 4suite already resets the seam inafterEach. Apply the same safety net here.♻️ Proposed fix
afterEach(async () => { + setUnlinkFaultForTest(null); await fs.rm(tempDir, { recursive: true, force: true }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts` around lines 131 - 133, Update this suite’s afterEach cleanup to reset the process-global unlink fault seam, matching the existing reset used by the Item 4 suite. Ensure the reset runs alongside the tempDir removal so failures before Item 1’s try/finally cannot leak the seam into later suites.packages/core/src/recording/janitor/sessionHeaderReader.test.ts (1)
37-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
projectHashis 32 characters, not 64.
crypto.randomUUID().replace(/-/g, '')produces 32 characters, so.slice(0, 64)leaves 32. Sibling test files use.repeat(2)before slicing to build a 64-hex value. If any consumer validates a 64-hex project hash, this helper does not represent real recorder output. Align the helper with the sibling files.♻️ Proposed fix
- projectHash: crypto.randomUUID().replace(/-/g, '').slice(0, 64), + projectHash: crypto.randomUUID().replace(/-/g, '').repeat(2).slice(0, 64),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/recording/janitor/sessionHeaderReader.test.ts` around lines 37 - 46, Update the projectHash construction in makeConfig to repeat the hyphen-free UUID before slicing, producing a 64-character hexadecimal value consistent with sibling test helpers and recorder output.
🤖 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 `@packages/core/src/recording/janitor/janitorLease.safety.test.ts`:
- Around line 435-494: Update the subprocess lifecycle in the test around child,
childStdout, and both wait promises: continuously consume child.stderr, and wrap
the acquisition/assertion/release flow in a finally block that kills the child
if it is still running. Preserve the existing success-path release and lease
assertions while ensuring timeout or assertion failures cannot leave the
subprocess alive.
In `@packages/core/src/recording/janitor/janitorLease.test.ts`:
- Around line 254-284: Harden the concurrency test around the child-process
script and its lease-holding delay so the first winner keeps the lease until
both subprocesses have had time to call JanitorLease.tryAcquire. Increase the
contention window or coordinate release with an external signal, while
preserving the expectation that exactly one result is WON.
In `@packages/core/src/recording/janitor/janitorLease.ts`:
- Around line 401-414: Update acquireTransitionClaim and tryReclaimClaim to
return whether a real claim was created, rather than treating ENOENT as an owned
claim; preserve the no-lease outcome without claiming ownership. Propagate this
ownership result through tryStaleTakeover, updateHeartbeat, and releaseLease,
and call releaseTransitionClaim only when the caller actually holds the claim.
- Around line 130-161: Update tryCreateLease so the file descriptor is closed
before safeUnlink(tempPath) runs when creating or writing the temporary lease
file fails. Preserve the existing EEXIST retry behavior and propagation of other
I/O errors, ensuring cleanup occurs after closure on all platforms.
In `@packages/core/src/recording/janitor/sessionScanner.test.ts`:
- Around line 281-353: Gate every test that directly creates or inspects
symlinks with it.skipIf(process.platform === 'win32') instead of returning
early: the four symlink tests in
packages/core/src/recording/janitor/sessionScanner.test.ts (lines 281-353), the
three symlink tests and lstat symlink test in
packages/core/src/recording/janitor/archiveCompressor.safety.test.ts (lines
61-115 and 182), and the symlinked-archive and symlink-replacement tests in
packages/core/src/recording/janitor/sessionJanitor.safety.test.ts (lines 150-182
and 196). Use the existing platform-gate pattern near sessionScanner.test.ts
line 379.
In `@packages/core/src/recording/SessionLockManager.test.ts`:
- Line 386: Update the error assertion in the Windows path test to accept either
ENOTDIR or ENOENT, while preserving validation that the error code matches one
of these expected values.
---
Nitpick comments:
In `@packages/cli/src/utils/sessionCleanup.boundary.test.ts`:
- Around line 43-45: Import randomUUID from node:crypto in the test module and
update validHash64 to call randomUUID() directly, replacing the existing
crypto.randomUUID() usage while preserving the hash-generation logic.
In `@packages/core/src/recording/janitor/janitorLease.ts`:
- Around line 84-120: The heartbeat state in JanitorLease is process-global, so
overlapping leases can stop one another’s heartbeats. Move heartbeat timer and
in-flight heartbeat tracking from the static JanitorLease fields into each
JanitorLeaseHandle, and update startHeartbeat, stopHeartbeat, and releaseLease
to operate only on the owning handle’s state.
In `@packages/core/src/recording/janitor/sessionHeaderReader.test.ts`:
- Around line 37-46: Update the projectHash construction in makeConfig to repeat
the hyphen-free UUID before slicing, producing a 64-character hexadecimal value
consistent with sibling test helpers and recorder output.
In `@packages/core/src/recording/janitor/sessionJanitor.reclamation.test.ts`:
- Around line 131-133: Update this suite’s afterEach cleanup to reset the
process-global unlink fault seam, matching the existing reset used by the Item 4
suite. Ensure the reset runs alongside the tempDir removal so failures before
Item 1’s try/finally cannot leak the seam into later suites.
In `@packages/core/src/recording/janitor/sessionJanitor.test.ts`:
- Around line 92-106: Update the makeArchive test helper to gzip the provided
content before writing it to the .gz archive path, matching the behavior of the
sibling reclamation test helper while preserving the existing age and
return-path handling.
In `@packages/core/src/recording/janitor/sessionSafety.test.ts`:
- Around line 162-190: Update the symlink-specific tests for
isRegularNonSymlinkFile and isRegularNonSymlinkDir to use
it.skipIf(process.platform === 'win32'), while leaving the regular-file,
directory, and non-existent-path tests unchanged.
In `@packages/core/src/recording/SessionLockManager.safety.test.ts`:
- Around line 391-440: Update runBunScript so the timeout callback records that
the child exceeded the 20-second limit before sending SIGTERM. In the close
handler, use that flag to reject with a timeout-specific message while
preserving the existing exit-code and stderr details for non-timeout failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b547cf4b-23cb-4313-90fc-bf393d8e3417
⛔ Files ignored due to path filters (1)
project-plans/issue3164/plan.mdis excluded by!project-plans/**
📒 Files selected for processing (48)
docs/cli/configuration.mdpackages/cli/src/config/settings-schema/schema-core.tspackages/cli/src/config/settings.tspackages/cli/src/ui/components/AuthDialog.test.tsxpackages/cli/src/utils/sessionCleanup-test-helpers.tspackages/cli/src/utils/sessionCleanup.boundary.test.tspackages/cli/src/utils/sessionCleanup.config.test.tspackages/cli/src/utils/sessionCleanup.integration.test.tspackages/cli/src/utils/sessionCleanup.test.tspackages/cli/src/utils/sessionCleanup.tspackages/cli/src/utils/sessionUtils.tspackages/core/package.jsonpackages/core/src/recording/ReplayEngine.tspackages/core/src/recording/SessionDiscovery.tspackages/core/src/recording/SessionJanitor.tspackages/core/src/recording/SessionLockManager.internals.tspackages/core/src/recording/SessionLockManager.lazy.test.tspackages/core/src/recording/SessionLockManager.property.test.tspackages/core/src/recording/SessionLockManager.safety.test.tspackages/core/src/recording/SessionLockManager.test.tspackages/core/src/recording/SessionLockManager.tspackages/core/src/recording/boundedHeaderReader.test.tspackages/core/src/recording/boundedHeaderReader.tspackages/core/src/recording/index.tspackages/core/src/recording/janitor/archiveCompressor.safety.test.tspackages/core/src/recording/janitor/archiveCompressor.test.tspackages/core/src/recording/janitor/archiveCompressor.tspackages/core/src/recording/janitor/cleanupTypes.tspackages/core/src/recording/janitor/index.tspackages/core/src/recording/janitor/janitorLease.safety.test.tspackages/core/src/recording/janitor/janitorLease.test.tspackages/core/src/recording/janitor/janitorLease.tspackages/core/src/recording/janitor/reclamationEngine.tspackages/core/src/recording/janitor/retentionPolicy.test.tspackages/core/src/recording/janitor/retentionPolicy.tspackages/core/src/recording/janitor/sessionGrouping.tspackages/core/src/recording/janitor/sessionHeaderReader.test.tspackages/core/src/recording/janitor/sessionHeaderReader.tspackages/core/src/recording/janitor/sessionJanitor.reclamation.test.tspackages/core/src/recording/janitor/sessionJanitor.safety.test.tspackages/core/src/recording/janitor/sessionJanitor.test.tspackages/core/src/recording/janitor/sessionJanitor.tspackages/core/src/recording/janitor/sessionSafety.test.tspackages/core/src/recording/janitor/sessionSafety.tspackages/core/src/recording/janitor/sessionScanner.test.tspackages/core/src/recording/janitor/sessionScanner.tspackages/test-utils/src/interactive-run.test.tsschemas/settings.schema.json
💤 Files with no reviewable changes (5)
- packages/cli/src/utils/sessionCleanup.config.test.ts
- packages/cli/src/utils/sessionCleanup.integration.test.ts
- packages/cli/src/utils/sessionCleanup.test.ts
- packages/cli/src/utils/sessionCleanup-test-helpers.ts
- packages/cli/src/utils/sessionUtils.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/core/package.json
- docs/cli/configuration.md
- packages/core/src/recording/janitor/index.ts
- packages/core/src/recording/index.ts
- packages/cli/src/config/settings-schema/schema-core.ts
- packages/core/src/recording/ReplayEngine.ts
- packages/core/src/recording/SessionJanitor.ts
- packages/core/src/recording/janitor/sessionHeaderReader.ts
- packages/core/src/recording/janitor/cleanupTypes.ts
- packages/core/src/recording/boundedHeaderReader.ts
- packages/core/src/recording/SessionDiscovery.ts
- packages/core/src/recording/janitor/retentionPolicy.ts
- packages/core/src/recording/SessionLockManager.ts
- packages/test-utils/src/interactive-run.test.ts
- packages/core/src/recording/janitor/sessionGrouping.ts
- packages/core/src/recording/janitor/reclamationEngine.ts
- packages/cli/src/utils/sessionCleanup.ts
- packages/cli/src/config/settings.ts
- packages/core/src/recording/janitor/retentionPolicy.test.ts
- schemas/settings.schema.json
- packages/core/src/recording/janitor/sessionJanitor.ts
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.
TLDR
Makes recording-session cleanup functional and safe for the JSONL format that the recorder actually writes. Cleanup now runs by default across the machine-global session corpus, bounds aggregate raw and compressed storage, preserves cold history in verified gzip archives, protects active or ambiguous sessions, and cleans stale locks through ownership-safe filesystem coordination.
Dive Deeper
Reviewer Test Plan
Final local focused verification passed for the recording area, janitor, locking, CLI boundary, lint, typecheck, format, build, and smoke test. The final aggregate npm run test completed with status 1 because four unchanged agents files timed out at 180 seconds: agent.approvalMode.behavior.test.ts, createAgent.harness.behavior.test.ts, mutationCoverage.auth.behavior.test.ts, and mutationCoverage.tokens.behavior.test.ts. All remaining workspaces completed. A previous local aggregate run timed out in a different shifting set of unchanged agents files. On the final pushed commit, the CI agents shard and every other required CI job passed: 40 passed, 0 failed, and 3 intentionally skipped.
Testing Matrix
Linked issues / bugs
Fixes #3164
Summary by CodeRabbit
New Features
Documentation