feat: add durable lifecycle and deterministic rollout selection - #7
Merged
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (100)
📝 WalkthroughWalkthroughAdds archived-state support and lifecycle tooling across memory domain, refactors MemoryStore for managed topic parsing, atomic mutation plans and rollback, threads noop/rejection accounting through sync/audit/recovery, extends CLI with recall/mcp/skills/integrations commands and archive-aware remember/forget, and introduces many integration, retrieval, and atomic-FS utilities and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI/User
participant CLI as cam CLI
participant Store as MemoryStore
participant FS as Filesystem
User->>CLI: run "cam forget <query> [--archive] --cwd <path>"
CLI->>Store: searchEntries(query, scope, state)
Store-->>CLI: matched entries
alt --archive
CLI->>Store: applyMutations(mutations with action="archive")
else
CLI->>Store: applyMutations(mutations with action="delete")
end
Store->>Store: build mutation plan, capture snapshots
Store->>FS: write topic files, indexes, history (atomic via temp + fsync)
FS-->>Store: success / failure
alt failure
Store->>FS: restore snapshots (rollback)
Store-->>CLI: throw error
else success
Store-->>CLI: MemoryApplyRecord[]
CLI-->>User: "Archived X" or "Deleted X" / JSON payload
end
sequenceDiagram
participant Sync as SyncService
participant Store as MemoryStore
participant Audit as MemorySyncAudit
participant Recovery as RecoveryStore
Sync->>Store: applyMutations(mutations[])
Store-->>Sync: MemoryApplyRecord[] (includes lifecycleAction, noop counts)
Sync->>Sync: map apply records → operations (map archive→delete, omit noop)
Sync->>Audit: buildMemorySyncAuditEntry(..., noopOperationCount, rejected counts)
alt write failure
Sync->>Recovery: writeSyncRecoveryRecord(..., noopOperationCount, rejected info)
end
Sync-->>Audit: persisted audit entry
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/util/fs.ts">
<violation number="1" location="src/lib/util/fs.ts:55">
P2: `writeTextFileAtomic` is not crash-durable because it renames without fsyncing the temp file (and directory), so recent writes can be lost after a crash.</violation>
</file>
<file name="src/lib/domain/memory-lifecycle.ts">
<violation number="1" location="src/lib/domain/memory-lifecycle.ts:29">
P2: `parseMemoryRef` accepts refs with an empty `<id>` segment (e.g. trailing `:`), so malformed refs are treated as valid.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/cli/register-commands.ts (1)
142-147:⚠️ Potential issue | 🟡 MinorUpdate the
forgethelp text for archive mode.
forget --helpstill advertises deletion only, so the new--archivepath is easy to miss from the command summary.✏️ Minimal help-text fix
program .command("forget") - .description("Delete matching memory entries") + .description("Delete or archive matching memory entries")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cli/register-commands.ts` around lines 142 - 147, Update the "forget" command help text to mention the new archive mode: modify the chained .description(...) for the .command("forget") (and/or the runForget documentation) to indicate that entries can be either deleted or moved to archive when using the --archive flag, and ensure the .option("--archive", ...) description clearly states "Move matching entries into archive instead of deleting them" so the help output shows both behaviors; reference the .command("forget"), .description(...), .option("--archive", ...) and runForget symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/domain/memory-lifecycle.ts`:
- Around line 23-42: parseMemoryRef is currently accepting refs like
"project:active:workflow:" because idParts.length > 0 allows an empty final
segment; change the guard in parseMemoryRef to reject refs whose assembled id is
empty or whose last idParts element is an empty string so that ids like "" are
not returned (e.g., require idParts.join(":").length > 0 or check
idParts[idParts.length-1] !== ""), ensuring parseMemoryRef returns null for
trailing-colon refs so assertValidMemoryRef only accepts resolvable refs.
In `@src/lib/domain/memory-store.ts`:
- Around line 644-661: The current precheck in assertMutationTargetsAreSafe
reads topic files before buildMutationCommitPlan and before
commitPlannedFileChanges re-snapshots, allowing races; fix by making the safety
check operate against the exact snapshot used to build the commit plan (or by
validating mtime/content drift immediately before writing). Concretely: change
buildMutationCommitPlan to capture topic snapshots (content and mtime) once and
pass that snapshot object into assertMutationTargetsAreSafe (and any calls to
assertTopicFileSafeForRewrite/findEntry) so all checks use the same captured
state, and additionally have commitPlannedFileChanges re-verify that the current
file mtime/content matches the snapshot and abort if it differs. Ensure
references updated: assertMutationTargetsAreSafe, buildMutationCommitPlan,
commitPlannedFileChanges, assertTopicFileSafeForRewrite, and findEntry.
- Around line 187-191: The current parse transforms detailsRaw by dropping any
line not starting with "- ", which silently loses content; instead detect if any
line in detailsRaw does not start with "- " and in that case do not
rewrite/normalize the block — either preserve the original detailsRaw as-is for
storage/round-trip or flip the block's safety flag so it is treated as unsafe
for automated rewrites. Concretely, after splitting detailsRaw (the code around
variables detailsRaw and details), check for lines that fail the /^- / test and,
if found, set the block to “unsafe” (or keep detailsRaw intact) so downstream
code won't overwrite the original formatting; only normalize into the details
array when all lines round-trip cleanly.
- Around line 931-945: The upsert branch silently skips mutations when
MemoryMutation.summary is missing; instead validate and fail fast: in the block
handling mutation.action === "upsert" (the code that builds MemoryEntry), detect
a missing or empty mutation.summary and throw or return an error (or mark the
batch as failed) so the caller is notified and an audit/history entry is
produced rather than silently continuing; update the logic around
MemoryMutation.summary and the code that constructs MemoryEntry to enforce this
validation and propagate an error to callers.
In `@src/lib/domain/recovery-records.ts`:
- Around line 122-123: The current guard on noopOperationCount accepts string
values like "1" as truthy by defaulting non-number to 0 while leaving the
original malformed value on the returned record; change the logic to normalize
the parsed record's noopOperationCount before returning: if
record.noopOperationCount === undefined set noopOperationCount = 0; else if
typeof record.noopOperationCount === "number" use it; else if typeof
record.noopOperationCount === "string" and it matches /^\d+$/ parse it to an
integer (Number.parseInt) and assign that normalized number back onto the record
(so callers see a number); otherwise treat it as invalid and set to 0 (or
explicitly delete/omit the field) — update the code around noopOperationCount
(and the similar occurrence at the later block around line 138) to perform this
normalization on the record prior to returning.
In `@src/lib/domain/rollout.ts`:
- Around line 254-257: The current code re-sorts the mtime-fallback candidates
inside findRelevantRollouts by calling .sort(compareByMtimeThenPath) after
attachRolloutMtime, which overrides the caller's original ranking and can pick
an older rollout based on embedded createdAtMs; remove that sort (and any
equivalent re-ranking in the nearby block at lines 262-280) so
recentMtimeMatches and the other fallback lists preserve the original metas
ordering provided by the caller, or alternatively thread an explicit ranking
strategy into findRelevantRollouts if re-ranking is required (refer to
recentMtimeMatches, attachRolloutMtime, compareByMtimeThenPath,
findRelevantRollouts, createdAtMs, metas).
---
Outside diff comments:
In `@src/lib/cli/register-commands.ts`:
- Around line 142-147: Update the "forget" command help text to mention the new
archive mode: modify the chained .description(...) for the .command("forget")
(and/or the runForget documentation) to indicate that entries can be either
deleted or moved to archive when using the --archive flag, and ensure the
.option("--archive", ...) description clearly states "Move matching entries into
archive instead of deleting them" so the help output shows both behaviors;
reference the .command("forget"), .description(...), .option("--archive", ...)
and runForget symbols when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc82a466-9026-4c9b-8072-0bae2580f939
📒 Files selected for processing (20)
src/lib/cli/register-commands.tssrc/lib/commands/forget.tssrc/lib/commands/memory.tssrc/lib/commands/remember.tssrc/lib/commands/wrapper.tssrc/lib/domain/memory-lifecycle.tssrc/lib/domain/memory-store.tssrc/lib/domain/memory-sync-audit.tssrc/lib/domain/recovery-records.tssrc/lib/domain/rollout.tssrc/lib/domain/sync-service.tssrc/lib/types.tssrc/lib/util/fs.tstest/memory-command.test.tstest/memory-store.test.tstest/memory-sync-audit.test.tstest/rollout.test.tstest/session-command.test.tstest/sync-service.test.tstest/wrapper-session-continuity.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/memory-store.test.ts (1)
978-1051: Drift detection test uses monkey-patching - consider if this is fragile.The test monkey-patches
buildMutationCommitPlanto inject a concurrent file change between planning and commit. While this effectively tests the drift detection, it relies on internal method access via type assertion.This is acceptable for testing internal behavior, but note that refactoring the method name or signature would silently break this test without a compile error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/memory-store.test.ts` around lines 978 - 1051, Test currently monkey-patches buildMutationCommitPlan to inject a concurrent file change, which is fragile; instead add a test-friendly hook and use it in applyMutations so tests can simulate drift without private monkey-patching. Modify MemoryStore to accept an optional onBeforeCommit callback (or expose a protected trigger) and invoke it at the end of buildMutationCommitPlan / immediately before the commit phase inside applyMutations; update the test to register that callback to write driftedContents to the topic file rather than reassigning buildMutationCommitPlan, referencing buildMutationCommitPlan and applyMutations so refactors won't break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/domain/rollout.ts`:
- Around line 127-133: attachRolloutMtime currently uses Promise.all and a
per-meta fs.stat that will reject the whole batch when one rollout file
vanishes; change it to handle missing files per-item (catch ENOENT) and skip
those entries so the function returns only existing RolloutMetaWithMtime
objects. Implement this by mapping metas to per-item promises that try
fs.stat(meta.rolloutPath) and on success return { meta, mtimeMs }, but on ENOENT
return null/undefined (or use Promise.allSettled and filter fulfilled results),
then filter out nulls before resolving; keep the function name
attachRolloutMtime and the RolloutMeta/RolloutMetaWithMtime shapes intact.
- Line 54: The current sorts use left.localeCompare(right) which is
locale-dependent; replace each occurrence (the return using collectRolloutFiles
and the other sorts around lines 148 and 156) with a locale-independent lexical
comparator such as comparing strings with a deterministic ternary (e.g., a === b
? 0 : a < b ? -1 : 1) so the ordering of paths is stable across environments;
update the sort callbacks where localeCompare is used in this file (the return
that sorts collectRolloutFiles results and the two other sort calls) to use that
lexical comparator.
---
Nitpick comments:
In `@test/memory-store.test.ts`:
- Around line 978-1051: Test currently monkey-patches buildMutationCommitPlan to
inject a concurrent file change, which is fragile; instead add a test-friendly
hook and use it in applyMutations so tests can simulate drift without private
monkey-patching. Modify MemoryStore to accept an optional onBeforeCommit
callback (or expose a protected trigger) and invoke it at the end of
buildMutationCommitPlan / immediately before the commit phase inside
applyMutations; update the test to register that callback to write
driftedContents to the topic file rather than reassigning
buildMutationCommitPlan, referencing buildMutationCommitPlan and applyMutations
so refactors won't break the test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0beb40fe-c633-47af-b59f-e44127e5a581
📒 Files selected for processing (12)
src/lib/cli/register-commands.tssrc/lib/domain/memory-lifecycle.tssrc/lib/domain/memory-store.tssrc/lib/domain/recovery-records.tssrc/lib/domain/rollout.tssrc/lib/util/fs.tstest/memory-command.test.tstest/memory-lifecycle.test.tstest/memory-store.test.tstest/recovery-records.test.tstest/rollout.test.tstest/util-fs.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/cli/register-commands.ts
- src/lib/util/fs.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/domain/memory-lifecycle.ts
- test/rollout.test.ts
| ? path.resolve(process.env.CAM_CODEX_SESSIONS_DIR) | ||
| : path.join(os.homedir(), ".codex", "sessions"); | ||
| return collectRolloutFiles(sessionsDir); | ||
| return (await collectRolloutFiles(sessionsDir)).sort((left, right) => left.localeCompare(right)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does String.prototype.localeCompare() without an explicit locale or options depend on the runtime locale/ICU configuration, and can it produce different sort orders across environments?
💡 Result:
Yes.
- Spec behavior:
String.prototype.localeCompare()is defined (when ECMA‑402/Intlis available) to compare strings according to the host environment’s current (default) locale, and it may rely on whatever locale-sensitive collation the host provides. If you don’t passlocales/options, they are treated asundefined, so the runtime’s DefaultLocale is used. [2] [3] - Node.js behavior: Node delegates this to ICU via V8. Node can be built/run with different ICU configurations (
none,small-icu,system-icu,full-icu), and the docs explicitly show thatlocaleCompare()is only partially locale-aware with--with-intl=none, while other modes depend on available ICU/OS data. [1] - Practical consequence: Because the default locale can differ (container/OS/env settings) and because ICU data/version can differ (different Node versions,
system-icuvs bundled ICU,small-iculimitations), sort order can change across environments even with the same input. [1] [2]
If you need consistent ordering, use new Intl.Collator('en-US', { ... }).compare (or another explicit locale/options) rather than relying on the default locale. [3]
Sources: [1] Node.js Intl/ICU build options and localeCompare behavior; [2] ECMA‑262 notes localeCompare follows host environment locale conventions; [3] ECMA‑402 DefaultLocale definition (host environment’s current locale).
🏁 Script executed:
cat -n src/lib/domain/rollout.ts | head -180Repository: Boulea7/Codex-Auto-Memory
Length of output: 6741
🏁 Script executed:
rg "localeCompare" src/lib/domain/rollout.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 308
Use a locale-independent path comparator for deterministic ordering.
These tie-breakers use localeCompare() without an explicit locale or options, which depends on the runtime's default locale and ICU configuration. This can produce different sort orders across environments—especially with mixed-case or non-ASCII characters in paths—weakening the deterministic rollout guarantee.
Replace with a simple lexical comparator:
🔧 Proposed fix
+function comparePathLexically(left: string, right: string): number {
+ return left < right ? -1 : left > right ? 1 : 0;
+}
+
export async function listRolloutFiles(): Promise<string[]> {
const sessionsDir = process.env.CAM_CODEX_SESSIONS_DIR
? path.resolve(process.env.CAM_CODEX_SESSIONS_DIR)
: path.join(os.homedir(), ".codex", "sessions");
- return (await collectRolloutFiles(sessionsDir)).sort((left, right) => left.localeCompare(right));
+ return (await collectRolloutFiles(sessionsDir)).sort(comparePathLexically);
}
@@
- return left.meta.rolloutPath.localeCompare(right.meta.rolloutPath);
+ return comparePathLexically(left.meta.rolloutPath, right.meta.rolloutPath);
@@
- return left.meta.rolloutPath.localeCompare(right.meta.rolloutPath);
+ return comparePathLexically(left.meta.rolloutPath, right.meta.rolloutPath);Applies to lines 54, 148, 156.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/domain/rollout.ts` at line 54, The current sorts use
left.localeCompare(right) which is locale-dependent; replace each occurrence
(the return using collectRolloutFiles and the other sorts around lines 148 and
156) with a locale-independent lexical comparator such as comparing strings with
a deterministic ternary (e.g., a === b ? 0 : a < b ? -1 : 1) so the ordering of
paths is stable across environments; update the sort callbacks where
localeCompare is used in this file (the return that sorts collectRolloutFiles
results and the two other sort calls) to use that lexical comparator.
Comment on lines
+127
to
+133
| async function attachRolloutMtime(metas: RolloutMeta[]): Promise<RolloutMetaWithMtime[]> { | ||
| return Promise.all( | ||
| metas.map(async (meta) => ({ | ||
| meta, | ||
| mtimeMs: (await fs.stat(meta.rolloutPath)).mtimeMs | ||
| })) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does fs.promises.stat(path) reject with ENOENT when the file disappears, and does Promise.all reject the whole await when any mapped promise rejects?
💡 Result:
-
fs.promises.stat(path)/fsPromises.stat(path): if the path no longer exists at the time of the call, the promise rejects (becausethrowIfNoEntrydefaults totrue). (nodejs.org)- On typical OSes, the underlying
stat/fstatatsyscall reports missing paths asENOENT(“a component of pathname does not exist…”), which is what Node surfaces aserr.code === 'ENOENT'. (man7.org) - If you set
{ throwIfNoEntry: false }, Node will returnundefinedinstead of throwing when there’s no entry. (nodejs.org)
- On typical OSes, the underlying
-
Promise.all([...]): yes—the promise returned byPromise.allrejects as soon as any input promise rejects, and thereforeawait Promise.all(...)throws/rejects the whole await with that first rejection reason. (developer.mozilla.org)
Citations:
- 1: https://nodejs.org/api/fs.html
- 2: https://www.man7.org/linux/man-pages/man2/fstatat.2%40%40man-pages.html?utm_source=openai
- 3: https://nodejs.org/api/fs.html
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all?source=post_page-----5cf8307bddac---------------------------------------&utm_source=openai
Skip vanished rollout files during the mtime pass.
A rollout can disappear after readRolloutMeta() succeeds but before Line 131 runs. One ENOENT rejects the whole Promise.all, so findRelevantRollouts() and findLatestProjectRollout() can fail even when other candidates are still valid.
🛠️ Minimal hardening
async function attachRolloutMtime(metas: RolloutMeta[]): Promise<RolloutMetaWithMtime[]> {
- return Promise.all(
- metas.map(async (meta) => ({
- meta,
- mtimeMs: (await fs.stat(meta.rolloutPath)).mtimeMs
- }))
- );
+ const items = await Promise.all(
+ metas.map(async (meta) => {
+ try {
+ return {
+ meta,
+ mtimeMs: (await fs.stat(meta.rolloutPath)).mtimeMs
+ };
+ } catch (error) {
+ if ((error as NodeJS.ErrnoException).code === "ENOENT") {
+ return null;
+ }
+ throw error;
+ }
+ })
+ );
+
+ return items.filter((item): item is RolloutMetaWithMtime => item !== null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function attachRolloutMtime(metas: RolloutMeta[]): Promise<RolloutMetaWithMtime[]> { | |
| return Promise.all( | |
| metas.map(async (meta) => ({ | |
| meta, | |
| mtimeMs: (await fs.stat(meta.rolloutPath)).mtimeMs | |
| })) | |
| ); | |
| async function attachRolloutMtime(metas: RolloutMeta[]): Promise<RolloutMetaWithMtime[]> { | |
| const items = await Promise.all( | |
| metas.map(async (meta) => { | |
| try { | |
| return { | |
| meta, | |
| mtimeMs: (await fs.stat(meta.rolloutPath)).mtimeMs | |
| }; | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| return null; | |
| } | |
| throw error; | |
| } | |
| }) | |
| ); | |
| return items.filter((item): item is RolloutMetaWithMtime => item !== null); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/domain/rollout.ts` around lines 127 - 133, attachRolloutMtime
currently uses Promise.all and a per-meta fs.stat that will reject the whole
batch when one rollout file vanishes; change it to handle missing files per-item
(catch ENOENT) and skip those entries so the function returns only existing
RolloutMetaWithMtime objects. Implement this by mapping metas to per-item
promises that try fs.stat(meta.rolloutPath) and on success return { meta,
mtimeMs }, but on ENOENT return null/undefined (or use Promise.allSettled and
filter fulfilled results), then filter out nulls before resolving; keep the
function name attachRolloutMtime and the RolloutMeta/RolloutMetaWithMtime shapes
intact.
There was a problem hiding this comment.
2 issues found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/domain/memory-store.ts">
<violation number="1" location="src/lib/domain/memory-store.ts:1269">
P2: The snapshot guard only validates topic files. Index/history files that are rebuilt in the same commit are not checked for changes, so concurrent updates to those files can be silently overwritten. Include snapshots for index/history files when they’re part of fileChanges.</violation>
</file>
<file name="src/lib/domain/rollout.ts">
<violation number="1" location="src/lib/domain/rollout.ts:261">
P2: This now picks the last candidate in whatever order the caller provides, so "latest" isn’t guaranteed. Restore the createdAt/mtime sort before selecting the final rollout to avoid choosing an older primary when inputs aren’t already sorted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/domain/rollout.ts (1)
298-301: Add missing semicolon for clarity.Line 298's method chain lacks a terminating semicolon before the
constdeclaration on line 299. While ASI handles this, explicit semicolons after chained expressions improve readability and prevent subtle issues if code is later reorganized.✏️ Suggested fix
- .map((item) => item.meta) - const sorted = await sortRolloutsByCreatedAtThenMtime(metas); + .map((item) => item.meta); + const sorted = await sortRolloutsByCreatedAtThenMtime(metas);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/domain/rollout.ts` around lines 298 - 301, Add a terminating semicolon after the chained expression that ends with .map((item) => item.meta) to avoid relying on ASI; locate the expression that feeds into sortRolloutsByCreatedAtThenMtime (the mapped metas), insert a semicolon at the end of that statement so the subsequent const sorted = await sortRolloutsByCreatedAtThenMtime(metas); and the return sorted.at(-1)?.rolloutPath ?? null; are clearly separate statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/domain/rollout.ts`:
- Around line 258-278: selectLatestPrimaryRolloutFromCandidates currently calls
readRolloutMeta inside Promise.all which will reject the whole batch if a file
vanishes (ENOENT); change the map so each candidate read is wrapped in a
try/catch that returns { candidate, meta: null } when readRolloutMeta throws
ENOENT (and rethrows other errors), preserving the existing filter/typeguard
logic; keep the rest of the flow (passing metas to
sortRolloutsByCreatedAtThenMtime and returning .at(-1)?.rolloutPath ?? null)
intact—this mirrors the defensive pattern used in attachRolloutMtime.
---
Nitpick comments:
In `@src/lib/domain/rollout.ts`:
- Around line 298-301: Add a terminating semicolon after the chained expression
that ends with .map((item) => item.meta) to avoid relying on ASI; locate the
expression that feeds into sortRolloutsByCreatedAtThenMtime (the mapped metas),
insert a semicolon at the end of that statement so the subsequent const sorted =
await sortRolloutsByCreatedAtThenMtime(metas); and the return
sorted.at(-1)?.rolloutPath ?? null; are clearly separate statements.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99d9491d-1aef-4636-8f54-4543decda268
📒 Files selected for processing (4)
src/lib/domain/memory-store.tssrc/lib/domain/rollout.tstest/memory-store.test.tstest/rollout.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/rollout.test.ts
- src/lib/domain/memory-store.ts
Comment on lines
+258
to
278
| export async function selectLatestPrimaryRolloutFromCandidates( | ||
| candidates: string[] | ||
| ): Promise<string | null> { | ||
| const orderedPrimaryCandidates = await sortRolloutsByCreatedAtThenMtime( | ||
| ( | ||
| await Promise.all( | ||
| candidates.map(async (candidate) => ({ | ||
| candidate, | ||
| meta: await readRolloutMeta(candidate) | ||
| })) | ||
| ) | ||
| ) | ||
| .filter( | ||
| (item): item is { candidate: string; meta: RolloutMeta } => | ||
| item.meta !== null && isPrimaryRolloutMeta(item.meta) | ||
| ) | ||
| .map((item) => item.meta) | ||
| ); | ||
|
|
||
| return recentMtimeMatches.sort(); | ||
| return orderedPrimaryCandidates.at(-1)?.rolloutPath ?? null; | ||
| } |
There was a problem hiding this comment.
Handle file-read errors in candidate selection.
readRolloutMeta() at line 266 can throw if a candidate file vanishes between listing and reading. Since this runs in Promise.all, one ENOENT fails the entire selection even when other candidates are valid. This is the same race-condition pattern as attachRolloutMtime.
Consider wrapping individual reads to return null on ENOENT, consistent with the existing filtering pattern:
🛡️ Suggested defensive pattern
const orderedPrimaryCandidates = await sortRolloutsByCreatedAtThenMtime(
(
await Promise.all(
candidates.map(async (candidate) => ({
candidate,
- meta: await readRolloutMeta(candidate)
+ meta: await readRolloutMeta(candidate).catch((err) =>
+ (err as NodeJS.ErrnoException).code === "ENOENT" ? null : Promise.reject(err)
+ )
}))
)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/domain/rollout.ts` around lines 258 - 278,
selectLatestPrimaryRolloutFromCandidates currently calls readRolloutMeta inside
Promise.all which will reject the whole batch if a file vanishes (ENOENT);
change the map so each candidate read is wrapped in a try/catch that returns {
candidate, meta: null } when readRolloutMeta throws ENOENT (and rethrows other
errors), preserving the existing filter/typeguard logic; keep the rest of the
flow (passing metas to sortRolloutsByCreatedAtThenMtime and returning
.at(-1)?.rolloutPath ?? null) intact—this mirrors the defensive pattern used in
attachRolloutMtime.
…2026-04-11 issue5 tail closeout blocker fixes
issue5 tail invalid cwd closure
issue5 close remaining runtime and extractor seams
…on-parity issue5 release gate isolation parity
issue5 init idempotency and force semantics
…sure issue5 keep session inspection read-only
issue5 runtime safety closure
…ediation issue5 runtime contract remediation
…oundaries issue5 claude and gemini host boundaries
…osure issue5 integrations install AGENTS boundary docs
issue5 wave3 contract closure
…sure issue5 release contract closure
issue5 route truth contracts
…06-startup-reviewer-provenance-v3
…sue5-05-continuity-rollout-signals
…ignals issue5 continuity rollout signals
…ewer issue5 manual mutation reviewer
issue5 retrieval sidecar and fallback diagnostics
issue5 Codex integration stack surfaces
Boulea7
added a commit
that referenced
this pull request
Apr 12, 2026
issue5 durable lifecycle and deterministic rollout selection foundation
This was referenced Apr 14, 2026
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
Test plan
pnpm vitest run test/memory-store.test.ts test/memory-command.test.ts test/memory-sync-audit.test.ts test/sync-service.test.ts test/rollout.test.ts test/session-command.test.ts test/wrapper-session-continuity.test.tsNotes
Summary by cubic
Adds a Codex-first hybrid integration stack with read-only memory retrieval and host-aware install/doctor flows, on top of the durable memory lifecycle and deterministic rollout selection. This is the expanded foundation slice for the issue5 stack.
New Features
cam recall {search|timeline|details}and a project-scoped MCP server (cam mcp serve) with helpers (print-config,install,doctor) forcodex/claude/geminiusing@modelcontextprotocol/sdk.cam integrations {install|apply|doctor}pluscam hooks installandcam skills installto drop local bridge/skill assets and retrieval guidance.remember/forget --archivereturn clear refs, follow-ups, and JSON;cam memory reindexand a richercam doctorsurface topic diagnostics and retrieval health.Bug Fixes
Written for commit 670a04e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation