Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Boulea7, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughRefactors CLI registration into a dedicated module, centralizes runtime config patch-and-reload, introduces a session-continuity persistence layer and presenters, adds release workflow and tarball/dist smoke tests, expands multilingual docs and contribution guidance, and adds/reshapes extensive tests and test helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Entry (src/cli.ts)
participant Reg as registerCommands (registration)
participant Cmd as Command Handler (e.g., runSession / runWrappedCodex)
participant Persist as persistSessionContinuity
participant Store as Session Continuity Store / Audit
participant Presenter as Session Presenter
participant Out as stdout/JSON
CLI->>Reg: createProgram() / registerCommands(program)
CLI->>CLI: parseAsync(args)
CLI->>Cmd: invoke command (session save / wrapper run)
Cmd->>Persist: persistSessionContinuity(rolloutPath, scope, trigger, writeMode)
Persist->>Store: parse evidence, read existing state
Persist->>Store: save summary (replace/save) and append audit
alt audit append fails
Persist->>Store: write recovery record
Persist-->>Cmd: throw error
else
Persist->>Store: cleanup recovery record
Persist-->>Cmd: return PersistSessionContinuityResult
end
Cmd->>Presenter: build/format JSON or text output
Presenter->>Out: write stdout / JSON
sequenceDiagram
participant Wrapper as runWrappedCodex
participant Runtime as RuntimeContext
participant Persist as persistSessionContinuity
participant Codex as External Codex Binary
participant Audit as Audit Log / Recovery
Wrapper->>Runtime: buildRuntimeContext / check autoLoad
alt autoLoad enabled
Wrapper->>Wrapper: inject startup continuity into base_instructions
end
Wrapper->>Codex: spawn codex process (with injected args)
Codex-->>Wrapper: completes
Wrapper->>Runtime: check autoSave
alt autoSave enabled
Wrapper->>Persist: persistSessionContinuity(...)
Persist->>Audit: append audit entry
alt audit append fails
Persist->>Audit: write recovery record
Persist-->>Wrapper: throw
else
Persist->>Audit: attempt recovery cleanup
Persist-->>Wrapper: success result (written, diagnostics)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 33 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="docs/release-checklist.md">
<violation number="1" location="docs/release-checklist.md:27">
P3: Duplicate checklist entry: `pnpm test:tarball-install-smoke` is listed twice — once as a bare command and again later with detailed verification criteria. Remove the earlier bare entry to avoid running the same step twice during a release.</violation>
</file>
<file name="test/wrapper-session-continuity.test.ts">
<violation number="1" location="test/wrapper-session-continuity.test.ts:30">
P2: Add `vi.restoreAllMocks()` to `afterEach` to prevent spy leaks when a test fails before reaching its manual `mockRestore()` call. The two `vi.spyOn` usages (lines 478 and 543) rely on control reaching `mockRestore()`, which is skipped if the preceding `expect(…).rejects.toThrow(…)` assertion fails.</violation>
</file>
<file name="README.en.md">
<violation number="1" location="README.en.md:8">
P2: Missing `|` separator between the "English" and "日本語" language links. The other README translations (README.ja.md, README.md) correctly include `|` after "English" since it is no longer the last item. The fix requires adding ` |` to the end of the unchanged `English</a>` line above.</violation>
</file>
<file name=".github/workflows/release.yml">
<violation number="1" location=".github/workflows/release.yml:43">
P2: `npm pack` triggers the `prepack` lifecycle hook (`pnpm build`), which silently rebuilds the project *after* `verify:release` already built and tested it. The tarball therefore ships artifacts that were never the ones the test suite validated.
Use `--ignore-scripts` to pack the already-verified build output instead of rebuilding.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:8">
P2: Missing ` |` separator between the English and Japanese navigation links. The English link line needs a trailing ` |` to match the pattern used by the other links (and the correct version in `README.ja.md`).</violation>
</file>
<file name="src/lib/runtime/runtime-context.ts">
<violation number="1" location="src/lib/runtime/runtime-context.ts:48">
P2: The first `buildRuntimeContext` call constructs a full runtime (config loading, SyncService, SessionContinuityStore, filesystem layout) only to extract `project.projectRoot`. Use `detectProjectContext(cwd)` directly to avoid the redundant initialization and I/O.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const configJson = makeAppConfig; | ||
| const writeProjectConfig = writeCamConfig; | ||
|
|
||
| afterEach(async () => { |
There was a problem hiding this comment.
P2: Add vi.restoreAllMocks() to afterEach to prevent spy leaks when a test fails before reaching its manual mockRestore() call. The two vi.spyOn usages (lines 478 and 543) rely on control reaching mockRestore(), which is skipped if the preceding expect(…).rejects.toThrow(…) assertion fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/wrapper-session-continuity.test.ts, line 30:
<comment>Add `vi.restoreAllMocks()` to `afterEach` to prevent spy leaks when a test fails before reaching its manual `mockRestore()` call. The two `vi.spyOn` usages (lines 478 and 543) rely on control reaching `mockRestore()`, which is skipped if the preceding `expect(…).rejects.toThrow(…)` assertion fails.</comment>
<file context>
@@ -0,0 +1,563 @@
+const configJson = makeAppConfig;
+const writeProjectConfig = writeCamConfig;
+
+afterEach(async () => {
+ process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir;
+ await cleanupTempDirs(tempDirs);
</file context>
| <a href="./README.md">简体中文</a> | | ||
| <a href="./README.zh-TW.md">繁體中文</a> | | ||
| <a href="./README.en.md">English</a> | ||
| <a href="./README.ja.md">日本語</a> |
There was a problem hiding this comment.
P2: Missing | separator between the "English" and "日本語" language links. The other README translations (README.ja.md, README.md) correctly include | after "English" since it is no longer the last item. The fix requires adding | to the end of the unchanged English</a> line above.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.en.md, line 8:
<comment>Missing `|` separator between the "English" and "日本語" language links. The other README translations (README.ja.md, README.md) correctly include `|` after "English" since it is no longer the last item. The fix requires adding ` |` to the end of the unchanged `English</a>` line above.</comment>
<file context>
@@ -3,7 +3,9 @@
<a href="./README.md">简体中文</a> |
+ <a href="./README.zh-TW.md">繁體中文</a> |
<a href="./README.en.md">English</a>
+ <a href="./README.ja.md">日本語</a>
</p>
<p>
</file context>
| - name: Pack release tarball | ||
| id: pack | ||
| run: | | ||
| TARBALL=$(npm pack | tail -n 1) |
There was a problem hiding this comment.
P2: npm pack triggers the prepack lifecycle hook (pnpm build), which silently rebuilds the project after verify:release already built and tested it. The tarball therefore ships artifacts that were never the ones the test suite validated.
Use --ignore-scripts to pack the already-verified build output instead of rebuilding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/release.yml, line 43:
<comment>`npm pack` triggers the `prepack` lifecycle hook (`pnpm build`), which silently rebuilds the project *after* `verify:release` already built and tested it. The tarball therefore ships artifacts that were never the ones the test suite validated.
Use `--ignore-scripts` to pack the already-verified build output instead of rebuilding.</comment>
<file context>
@@ -0,0 +1,49 @@
+ - name: Pack release tarball
+ id: pack
+ run: |
+ TARBALL=$(npm pack | tail -n 1)
+ echo "tarball=${TARBALL}" >> "$GITHUB_OUTPUT"
+
</file context>
| <a href="./README.md">简体中文</a> | | ||
| <a href="./README.zh-TW.md">繁體中文</a> | | ||
| <a href="./README.en.md">English</a> | ||
| <a href="./README.ja.md">日本語</a> |
There was a problem hiding this comment.
P2: Missing | separator between the English and Japanese navigation links. The English link line needs a trailing | to match the pattern used by the other links (and the correct version in README.ja.md).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 8:
<comment>Missing ` |` separator between the English and Japanese navigation links. The English link line needs a trailing ` |` to match the pattern used by the other links (and the correct version in `README.ja.md`).</comment>
<file context>
@@ -3,7 +3,9 @@
<a href="./README.md">简体中文</a> |
+ <a href="./README.zh-TW.md">繁體中文</a> |
<a href="./README.en.md">English</a>
+ <a href="./README.ja.md">日本語</a>
</p>
<p>
</file context>
| configScope: ConfigScope, | ||
| updates: Partial<AppConfig> | ||
| ): Promise<ReloadedRuntimeContext> { | ||
| const initialRuntime = await buildRuntimeContext(cwd); |
There was a problem hiding this comment.
P2: The first buildRuntimeContext call constructs a full runtime (config loading, SyncService, SessionContinuityStore, filesystem layout) only to extract project.projectRoot. Use detectProjectContext(cwd) directly to avoid the redundant initialization and I/O.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/runtime/runtime-context.ts, line 48:
<comment>The first `buildRuntimeContext` call constructs a full runtime (config loading, SyncService, SessionContinuityStore, filesystem layout) only to extract `project.projectRoot`. Use `detectProjectContext(cwd)` directly to avoid the redundant initialization and I/O.</comment>
<file context>
@@ -29,6 +40,20 @@ export async function buildRuntimeContext(
+ configScope: ConfigScope,
+ updates: Partial<AppConfig>
+): Promise<ReloadedRuntimeContext> {
+ const initialRuntime = await buildRuntimeContext(cwd);
+ const configUpdatePath = await patchConfigFile(
+ initialRuntime.project.projectRoot,
</file context>
| - Run `pnpm test:reviewer-smoke` | ||
| - Run `pnpm test:cli-smoke` | ||
| - Run `pnpm test:dist-cli-smoke` | ||
| - Run `pnpm test:tarball-install-smoke` |
There was a problem hiding this comment.
P3: Duplicate checklist entry: pnpm test:tarball-install-smoke is listed twice — once as a bare command and again later with detailed verification criteria. Remove the earlier bare entry to avoid running the same step twice during a release.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/release-checklist.md, line 27:
<comment>Duplicate checklist entry: `pnpm test:tarball-install-smoke` is listed twice — once as a bare command and again later with detailed verification criteria. Remove the earlier bare entry to avoid running the same step twice during a release.</comment>
<file context>
@@ -18,30 +18,39 @@ Use this checklist before cutting any alpha or beta release of `codex-auto-memor
- Run `pnpm test:reviewer-smoke`
- Run `pnpm test:cli-smoke`
+- Run `pnpm test:dist-cli-smoke`
+- Run `pnpm test:tarball-install-smoke`
- Run `pnpm test`
- Run `pnpm build`
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
test/helpers/cli-runner.ts (1)
7-11: Consider making paths relative to the repository root explicitly.The
path.resolve()calls resolve relative to the current working directory at module load time. If tests are run from a different working directory, these paths may not resolve correctly.💡 Optional: Use `import.meta.url` for more robust path resolution
+import { fileURLToPath } from "node:url"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(__dirname, "../.."); + -const sourceCliPath = path.resolve("src/cli.ts"); -const distCliPath = path.resolve("dist/cli.js"); +const sourceCliPath = path.resolve(repoRoot, "src/cli.ts"); +const distCliPath = path.resolve(repoRoot, "dist/cli.js"); const tsxBinaryPath = path.resolve( - process.platform === "win32" ? "node_modules/.bin/tsx.cmd" : "node_modules/.bin/tsx" + repoRoot, + process.platform === "win32" ? "node_modules/.bin/tsx.cmd" : "node_modules/.bin/tsx" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/cli-runner.ts` around lines 7 - 11, The path.resolve calls for sourceCliPath, distCliPath, and tsxBinaryPath are relative to process.cwd() and can break when tests run from other directories; update them to compute paths from the repository file location using import.meta.url (convert to a filesystem path with fileURLToPath and path.dirname) and then join to "src/cli.ts", "dist/cli.js", and the node_modules/.bin/tsx[.cmd] locations so the resolved paths are stable regardless of the current working directory.src/lib/cli/register-commands.ts (1)
50-70: Consider improving readability of nested option calls.The nested
addSessionScopeOption(addSessionRolloutOption(addJsonOption(...)))pattern, while functional, can be harder to follow. A fluent chain would be clearer.♻️ Optional: Fluent option chaining
- addSessionScopeOption( - addSessionRolloutOption( - addJsonOption( - sessionCommand - .command("save") - .description("Save temporary session continuity from a rollout") - ) - ) - ) - .action(withStdout(async (options) => runSession("save", options))); + sessionCommand + .command("save") + .description("Save temporary session continuity from a rollout") + .option("--json", "Print JSON output") + .option("--rollout <path>", "Specific rollout JSONL file to summarize") + .option("--scope <scope>", "Target continuity scope: project, project-local, or both", "both") + .action(withStdout(async (options) => runSession("save", options)));Alternatively, keep the helpers but apply them in sequence rather than nested.
🤖 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 50 - 70, The nested calls to addSessionScopeOption(addSessionRolloutOption(addJsonOption(...))) reduce readability; refactor by creating a local command variable for each subcommand (e.g., start with sessionCommand.command("save")/command("refresh") and assign to a variable like cmd), then apply the option helpers in sequence (cmd = addJsonOption(cmd); cmd = addSessionRolloutOption(cmd); cmd = addSessionScopeOption(cmd)), and finally attach .action(withStdout(async (options) => runSession("save" or "refresh", options))); this preserves behavior (functions: addJsonOption, addSessionRolloutOption, addSessionScopeOption, sessionCommand, runSession, withStdout) but makes the flow easier to read.test/wrapper-session-continuity.test.ts (1)
275-303: These test mock-codex scripts use POSIX shebangs and won't execute on Windows.The four mock binaries (lines 275–303, 352–372, 445–463, 510–529) rely on
#!/usr/bin/env nodeandfs.chmod(..., 0o755). On Windows,spawn()cannot execute these files. However, this only becomes an issue if Windows test coverage is added—CI currently runs only on Linux. If Windows test support is planned, route thewriteWrapperMockCodexhelper and all call sites through a platform-aware approach (e.g., generating.cmdshims on win32).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/wrapper-session-continuity.test.ts` around lines 275 - 303, The test mocks use POSIX shebangs (creating mockCodexPath and calling fs.chmod(..., 0o755)) which fail on Windows; update the helper (writeWrapperMockCodex) and its call sites to be platform-aware: detect process.platform === "win32" and either emit a .cmd shim that runs "node <script>.js" or write a .js file and ensure tests spawn it with the Node executable (instead of relying on shebang+chmod); replace direct fs.chmod(..., 0o755) usage and adjust places that call spawn() to use the shim or explicit "node" invocation so mock binaries run cross-platform (refer to symbols mockCodexPath, writeWrapperMockCodex, fs.chmod, and any spawn/child_process.spawn usages).src/lib/commands/session-presenters.ts (1)
142-166: Make the layer formatter complete, or the load view will keep drifting.
formatLayerSectiononly renders part ofSessionContinuityState, soformatSessionLoadTextstill has to hand-build the rest of the local block and reimplement the shared/merged blocks from scratch. The three sections already diverge in ordering/copy, which makes future state-shape changes easy to miss in one path.Also applies to: 327-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/session-presenters.ts` around lines 142 - 166, The formatLayerSection function is incomplete as it only formats part of the SessionContinuityState, causing code duplication and divergence in formatSessionLoadText. To fix this, extend formatLayerSection to cover all fields of SessionContinuityState completely and consistently, and refactor formatSessionLoadText to reuse formatLayerSection for all local and shared/merged blocks. This will ensure single-point updates and consistency across rendering logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.en.md`:
- Around line 6-8: The language selector in README.en.md is missing a pipe
separator between the "English" and "日本語" links; update the HTML fragment
containing the anchor tags for "English" and "日本語" so there is a " | " separator
inserted between the <a href="./README.en.md">English</a> and <a
href="./README.ja.md">日本語</a> links to match the other separators and restore
visual consistency.
In `@src/lib/commands/session-presenters.ts`:
- Around line 291-310: The persisted JSON currently only sets "action" inside
the conditional for refresh+rolloutSelection, so responses like "save" or
"refresh" without a selection drop the action; always include an "action"
top-level property using the action variable, and keep the existing conditional
to add writeMode: "replace" and rolloutSelection when action === "refresh" &&
rolloutSelection; update the object built in session-presenters.ts (the block
referencing action, rolloutSelection, persisted and SessionContinuityWriteMode)
so that action: action appears unconditionally while preserving the conditional
addition of writeMode and rolloutSelection.
In `@src/lib/commands/session.ts`:
- Around line 175-177: The JSON path currently ignores the --print-startup flag
because buildSessionLoadJson always includes startup; update
buildSessionLoadJson (in src/lib/commands/session-presenters.ts) to accept a
boolean (e.g., includeStartup) or read the flag you pass from the caller and
only include the startup field when that boolean is true, then change the call
site in session.ts (the branch where action === "load" and options.json) to pass
the options.printStartup value into buildSessionLoadJson so session load --json
and session load --json --print-startup produce different payload shapes.
In `@src/lib/domain/session-continuity-persistence.ts`:
- Around line 170-188: The code reads only recentAuditPreviewReadLimit entries
into recentContinuityAuditPreviewEntries then slices to recentAuditLimit, so if
recentAuditLimit > recentAuditPreviewReadLimit callers get fewer items than
requested; fix by computing a fetchLimit = Math.max(recentAuditPreviewReadLimit,
recentAuditLimit) (or similar) and pass that to
options.runtime.sessionContinuityStore.readRecentAuditEntries, then use
recentContinuityAuditPreviewEntries.slice(0, recentAuditLimit) to return the
requested number; update references to
recentAuditPreviewReadLimit/recentAuditLimit and the call to
readRecentAuditEntries accordingly.
In `@test/dist-cli-smoke.test.ts`:
- Around line 147-186: The test assumes the dist entrypoint successfully
launches the mock codex and then reads captured-args.json, but when the dist
build/CLI fails the file is missing; update the test around runCli/capturedArgs
to defensively check the CLI result before reading captured-args.json: after
calling runCli(repoDir, ["exec", "continue"], { entrypoint: "dist" }) inspect
result.exitCode and any stderr/stdout (result.exitCode !== 0) and fail the test
with a clear diagnostic (e.g., include result.stdout/result.stderr) instead of
immediately reading capturedArgsPath, and only read/parse captured-args.json if
result.exitCode === 0; reference runCli, capturedArgsPath, and the expect checks
for exec/continue.
- Around line 30-41: The dist smoke test (test/dist-cli-smoke.test.ts) runs
before build in CI and fails because dist/cli.js is missing; fix by removing it
from the default test run and running it only after build: add a dedicated
script for unit tests (e.g., "test:unit") that runs vitest with a pattern or
folder excluding dist-cli-smoke.test.ts and create a separate "test:dist" (or
include it in verify:release) that runs after "build"; update package.json
scripts so "test" calls "test:unit" (keeping verify:release to run build then
"test:dist"), and/or add a CI change to run the new "test:dist" step after pnpm
build. Reference test/dist-cli-smoke.test.ts, the "test" script and
verify:release script to locate where to modify.
In `@test/helpers/session-test-support.ts`:
- Around line 45-104: writeWrapperMockCodex and writeMockCodexBinary create
POSIX-only executable scripts which fail on Windows; detect process.platform ===
"win32" and alongside the generated mock-codex script write a .cmd wrapper that
invokes node with the script path (forwarding all args) and return the .cmd path
to callers on Windows (or alternatively, when creating the mock binary for
Windows, return process.execPath + " " + scriptPath so callers use node
directly). Update both functions (writeWrapperMockCodex and
writeMockCodexBinary) to create the .cmd file on Windows, ensure it forwards
arguments and points to the generated script, and have the function return the
platform-appropriate executable path so spawn() works cross-platform.
In `@test/session-command.test.ts`:
- Around line 47-57: The writeNoopCodexBinary helper is POSIX-only (writes a
shebang script and chmods), causing Windows spawns to fail; make it
platform-aware by detecting process.platform === "win32" and on Windows either
emit a .cmd wrapper alongside the script (or write a .bat/.cmd that calls node
%~dp0\mock-codex) or return a path that will be launched via process.execPath
(i.e., write a .js file and invoke it with process.execPath in tests), and avoid
relying solely on chmod; update any tests that spawn the returned path (or their
spawn options) to use shell:true or to use process.execPath when on Windows so
the noop codex executes reliably across platforms (refer to function name
writeNoopCodexBinary and any tests that call it).
- Around line 36-39: Ensure any spy on
SessionContinuityStore.prototype.appendAuditLog is restored in the test suite
teardown: in the afterEach hook, call mockRestore() (or vi.restoreAllMocks())
for the spy used in tests so the mocked appendAuditLog cannot leak between
tests; locate the afterEach block and add restoration of the spy or a global
restore to guarantee cleanup even if a test fails before its explicit
mockRestore.
In `@test/tarball-install-smoke.test.ts`:
- Around line 50-83: The tests can hang because runCommandCapture (which calls
spawnSync in src/lib/util/process.ts) doesn't set a timeout; update the
spawnSync call inside runCommandCapture (and any helpers in that module) to pass
a reasonable timeout option (e.g. configurable DEFAULT_SUBPROCESS_TIMEOUT) so
synchronous child processes like npm pack/install are killed if they exceed the
limit, expose/configure that timeout constant so tests can override it if
needed, and ensure the runCommandCapture return object still reports
exitCode/stdout/stderr when a timeout occurs.
In `@test/wrapper-session-continuity.test.ts`:
- Around line 30-33: Add a global mock restore to the test teardown: modify the
afterEach block that currently resets process.env and calls
cleanupTempDirs(tempDirs) to also call vi.restoreAllMocks() so any spies/mocks
created with vi.spyOn(...).mockRejectedValueOnce(...) are cleared even if
individual tests fail before calling mockRestore(); update the afterEach
containing process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir and await
cleanupTempDirs(tempDirs) to include vi.restoreAllMocks() as the first or last
cleanup step.
---
Nitpick comments:
In `@src/lib/cli/register-commands.ts`:
- Around line 50-70: The nested calls to
addSessionScopeOption(addSessionRolloutOption(addJsonOption(...))) reduce
readability; refactor by creating a local command variable for each subcommand
(e.g., start with sessionCommand.command("save")/command("refresh") and assign
to a variable like cmd), then apply the option helpers in sequence (cmd =
addJsonOption(cmd); cmd = addSessionRolloutOption(cmd); cmd =
addSessionScopeOption(cmd)), and finally attach .action(withStdout(async
(options) => runSession("save" or "refresh", options))); this preserves behavior
(functions: addJsonOption, addSessionRolloutOption, addSessionScopeOption,
sessionCommand, runSession, withStdout) but makes the flow easier to read.
In `@src/lib/commands/session-presenters.ts`:
- Around line 142-166: The formatLayerSection function is incomplete as it only
formats part of the SessionContinuityState, causing code duplication and
divergence in formatSessionLoadText. To fix this, extend formatLayerSection to
cover all fields of SessionContinuityState completely and consistently, and
refactor formatSessionLoadText to reuse formatLayerSection for all local and
shared/merged blocks. This will ensure single-point updates and consistency
across rendering logic.
In `@test/helpers/cli-runner.ts`:
- Around line 7-11: The path.resolve calls for sourceCliPath, distCliPath, and
tsxBinaryPath are relative to process.cwd() and can break when tests run from
other directories; update them to compute paths from the repository file
location using import.meta.url (convert to a filesystem path with fileURLToPath
and path.dirname) and then join to "src/cli.ts", "dist/cli.js", and the
node_modules/.bin/tsx[.cmd] locations so the resolved paths are stable
regardless of the current working directory.
In `@test/wrapper-session-continuity.test.ts`:
- Around line 275-303: The test mocks use POSIX shebangs (creating mockCodexPath
and calling fs.chmod(..., 0o755)) which fail on Windows; update the helper
(writeWrapperMockCodex) and its call sites to be platform-aware: detect
process.platform === "win32" and either emit a .cmd shim that runs "node
<script>.js" or write a .js file and ensure tests spawn it with the Node
executable (instead of relying on shebang+chmod); replace direct fs.chmod(...,
0o755) usage and adjust places that call spawn() to use the shim or explicit
"node" invocation so mock binaries run cross-platform (refer to symbols
mockCodexPath, writeWrapperMockCodex, fs.chmod, and any
spawn/child_process.spawn usages).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 258987eb-4a54-47a1-bf45-87e3a099bd5e
📒 Files selected for processing (33)
.github/workflows/ci.yml.github/workflows/release.ymlCONTRIBUTING.mdREADME.en.mdREADME.ja.mdREADME.mdREADME.zh-TW.mddocs/architecture.en.mddocs/architecture.mddocs/release-checklist.mdpackage.jsonsrc/cli.tssrc/lib/cli/register-commands.tssrc/lib/commands/doctor.tssrc/lib/commands/forget.tssrc/lib/commands/memory.tssrc/lib/commands/remember.tssrc/lib/commands/session-presenters.tssrc/lib/commands/session.tssrc/lib/commands/sync.tssrc/lib/commands/wrapper.tssrc/lib/domain/session-continuity-persistence.tssrc/lib/runtime/runtime-context.tstest/audit.test.tstest/dist-cli-smoke.test.tstest/docs-contract.test.tstest/fixtures/rollouts/session-continuity-conflict-heavy.jsonltest/helpers/cli-runner.tstest/helpers/session-test-support.tstest/memory-command.test.tstest/session-command.test.tstest/tarball-install-smoke.test.tstest/wrapper-session-continuity.test.ts
| <a href="./README.zh-TW.md">繁體中文</a> | | ||
| <a href="./README.en.md">English</a> | ||
| <a href="./README.ja.md">日本語</a> |
There was a problem hiding this comment.
Missing pipe separator between language links.
The language selector is missing a | separator between "English" and "日本語" links, which breaks the visual consistency with the other separators.
🔧 Proposed fix
<a href="./README.zh-TW.md">繁體中文</a> |
- <a href="./README.en.md">English</a>
- <a href="./README.ja.md">日本語</a>
+ <a href="./README.en.md">English</a> |
+ <a href="./README.ja.md">日本語</a>📝 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.
| <a href="./README.zh-TW.md">繁體中文</a> | | |
| <a href="./README.en.md">English</a> | |
| <a href="./README.ja.md">日本語</a> | |
| <a href="./README.zh-TW.md">繁體中文</a> | | |
| <a href="./README.en.md">English</a> | | |
| <a href="./README.ja.md">日本語</a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.en.md` around lines 6 - 8, The language selector in README.en.md is
missing a pipe separator between the "English" and "日本語" links; update the HTML
fragment containing the anchor tags for "English" and "日本語" so there is a " | "
separator inserted between the <a href="./README.en.md">English</a> and <a
href="./README.ja.md">日本語</a> links to match the other separators and restore
visual consistency.
| return JSON.stringify( | ||
| { | ||
| ...(action === "refresh" && rolloutSelection | ||
| ? { | ||
| action: "refresh", | ||
| writeMode: "replace" satisfies SessionContinuityWriteMode, | ||
| rolloutSelection | ||
| } | ||
| : {}), | ||
| rolloutPath: persisted.rolloutPath, | ||
| written: persisted.written, | ||
| excludePath: persisted.excludePath, | ||
| summary: persisted.summary, | ||
| diagnostics: persisted.diagnostics, | ||
| latestContinuityAuditEntry: persisted.latestContinuityAuditEntry, | ||
| recentContinuityAuditEntries: persisted.recentContinuityAuditEntries, | ||
| continuityAuditPath: persisted.continuityAuditPath, | ||
| pendingContinuityRecovery: persisted.pendingContinuityRecovery, | ||
| continuityRecoveryPath: persisted.continuityRecoveryPath | ||
| }, |
There was a problem hiding this comment.
Always include action in the persisted JSON payload.
Line 293 currently hides action inside the refresh && rolloutSelection branch, so every "save" response — and any "refresh" call that omits the optional selection object — loses the operation type entirely. That makes the --json contract ambiguous for consumers.
Suggested fix
export function buildPersistedSessionJson(
action: "save" | "refresh",
persisted: PersistSessionContinuityResult,
rolloutSelection?: RolloutSelectionSummary & { rolloutPath: string }
): string {
return JSON.stringify(
{
- ...(action === "refresh" && rolloutSelection
- ? {
- action: "refresh",
- writeMode: "replace" satisfies SessionContinuityWriteMode,
- rolloutSelection
- }
- : {}),
+ action,
+ ...(action === "refresh" && rolloutSelection
+ ? {
+ writeMode: "replace" satisfies SessionContinuityWriteMode,
+ rolloutSelection
+ }
+ : {}),
rolloutPath: persisted.rolloutPath,
written: persisted.written,
excludePath: persisted.excludePath,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/commands/session-presenters.ts` around lines 291 - 310, The persisted
JSON currently only sets "action" inside the conditional for
refresh+rolloutSelection, so responses like "save" or "refresh" without a
selection drop the action; always include an "action" top-level property using
the action variable, and keep the existing conditional to add writeMode:
"replace" and rolloutSelection when action === "refresh" && rolloutSelection;
update the object built in session-presenters.ts (the block referencing action,
rolloutSelection, persisted and SessionContinuityWriteMode) so that action:
action appears unconditionally while preserving the conditional addition of
writeMode and rolloutSelection.
| if (action === "load") { | ||
| if (options.json) { | ||
| return JSON.stringify( | ||
| { | ||
| projectLocation, | ||
| localLocation, | ||
| projectState, | ||
| localState, | ||
| mergedState, | ||
| startup, | ||
| latestContinuityAuditEntry, | ||
| latestContinuityDiagnostics, | ||
| recentContinuityAuditEntries, | ||
| continuityAuditPath: runtime.sessionContinuityStore.paths.auditFile, | ||
| pendingContinuityRecovery, | ||
| continuityRecoveryPath: runtime.sessionContinuityStore.getRecoveryPath() | ||
| }, | ||
| null, | ||
| 2 | ||
| ); | ||
| } | ||
|
|
||
| const lines = [ | ||
| "Session Continuity", | ||
| `Project continuity: ${projectLocation.exists ? "active" : "missing"} (${projectLocation.path})`, | ||
| `Project-local continuity: ${localLocation.exists ? "active" : "missing"} (${localLocation.path})`, | ||
| `Latest generation: ${latestContinuityDiagnostics ? formatSessionContinuityDiagnostics(latestContinuityDiagnostics) : "none recorded yet"}`, | ||
| ...(latestContinuityAuditEntry ? [`Latest rollout: ${latestContinuityAuditEntry.rolloutPath}`] : []), | ||
| `Continuity audit: ${runtime.sessionContinuityStore.paths.auditFile}`, | ||
| "Merged resume brief combines shared continuity with any project-local overrides.", | ||
| "Recent prior generations below are compact audit previews, not startup-injected history.", | ||
| ...(latestContinuityAuditEntry | ||
| ? formatSessionContinuityAuditDrillDown(latestContinuityAuditEntry) | ||
| : []), | ||
| ...(pendingContinuityRecovery | ||
| ? formatPendingContinuityRecovery( | ||
| pendingContinuityRecovery, | ||
| runtime.sessionContinuityStore.getRecoveryPath() | ||
| ) | ||
| : []), | ||
| "Recent prior generations:", | ||
| ...formatRecentGenerationLines(recentContinuityAuditPreviewEntries), | ||
| "", | ||
| "Shared project continuity:", | ||
| `Goal: ${projectState?.goal || "No active goal recorded."}`, | ||
| "", | ||
| "Confirmed working:", | ||
| ...(projectState?.confirmedWorking.length | ||
| ? projectState.confirmedWorking.map((item) => `- ${item}`) | ||
| : ["- Nothing confirmed yet."]), | ||
| "", | ||
| "Tried and failed:", | ||
| ...(projectState?.triedAndFailed.length | ||
| ? projectState.triedAndFailed.map((item) => `- ${item}`) | ||
| : ["- No failed approaches recorded."]), | ||
| "", | ||
| "Not yet tried:", | ||
| ...(projectState?.notYetTried.length | ||
| ? projectState.notYetTried.map((item) => `- ${item}`) | ||
| : ["- No untried approaches recorded."]), | ||
| "", | ||
| "Files / decisions / environment:", | ||
| ...(projectState?.filesDecisionsEnvironment.length | ||
| ? projectState.filesDecisionsEnvironment.map((item) => `- ${item}`) | ||
| : ["- No additional file, decision, or environment notes."]), | ||
| "", | ||
| "Project-local continuity:", | ||
| `Goal: ${localState?.goal || "No active goal recorded."}`, | ||
| "", | ||
| "Confirmed working:", | ||
| ...(localState?.confirmedWorking.length | ||
| ? localState.confirmedWorking.map((item) => `- ${item}`) | ||
| : ["- Nothing confirmed yet."]), | ||
| "", | ||
| "Tried and failed:", | ||
| ...(localState?.triedAndFailed.length | ||
| ? localState.triedAndFailed.map((item) => `- ${item}`) | ||
| : ["- No failed approaches recorded."]), | ||
| "", | ||
| "Incomplete / next:", | ||
| ...(localState?.incompleteNext.length | ||
| ? localState.incompleteNext.map((item) => `- ${item}`) | ||
| : ["- No next step recorded."]) | ||
| ]; | ||
|
|
||
| lines.push( | ||
| "", | ||
| "Project-local not yet tried:", | ||
| ...(localState?.notYetTried.length | ||
| ? localState.notYetTried.map((item) => `- ${item}`) | ||
| : ["- No untried local approaches recorded."]), | ||
| "", | ||
| "Project-local files / decisions / environment:", | ||
| ...(localState?.filesDecisionsEnvironment.length | ||
| ? localState.filesDecisionsEnvironment.map((item) => `- ${item}`) | ||
| : ["- No additional local file, decision, or environment notes."]), | ||
| "", | ||
| "Effective merged resume brief:", | ||
| `Goal: ${mergedState.goal || "No active goal recorded."}`, | ||
| "Confirmed working:", | ||
| ...(mergedState.confirmedWorking.length > 0 | ||
| ? mergedState.confirmedWorking.map((item) => `- ${item}`) | ||
| : ["- Nothing confirmed yet."]), | ||
| "Tried and failed:", | ||
| ...(mergedState.triedAndFailed.length > 0 | ||
| ? mergedState.triedAndFailed.map((item) => `- ${item}`) | ||
| : ["- No failed approaches recorded."]), | ||
| "Not yet tried:", | ||
| ...(mergedState.notYetTried.length > 0 | ||
| ? mergedState.notYetTried.map((item) => `- ${item}`) | ||
| : ["- No untried approaches recorded."]), | ||
| "Incomplete / next:", | ||
| ...(mergedState.incompleteNext.length > 0 | ||
| ? mergedState.incompleteNext.map((item) => `- ${item}`) | ||
| : ["- No next step recorded."]), | ||
| "Files / decisions / environment:", | ||
| ...(mergedState.filesDecisionsEnvironment.length > 0 | ||
| ? mergedState.filesDecisionsEnvironment.map((item) => `- ${item}`) | ||
| : ["- No additional file, decision, or environment notes."]) | ||
| ); | ||
|
|
||
| if (options.printStartup) { | ||
| lines.push("", "Startup continuity:", startup.text.trimEnd()); | ||
| return buildSessionLoadJson(view); |
There was a problem hiding this comment.
--print-startup is ignored for JSON output.
buildSessionLoadJson() always includes startup, so session load --json and session load --json --print-startup currently return the same payload. That makes the flag a no-op on the JSON path and changes the output shape even when the caller did not opt in.
🔧 Suggested fix
- return buildSessionLoadJson(view);
+ return buildSessionLoadJson(view, options.printStartup);Update src/lib/commands/session-presenters.ts so it omits startup unless that flag is true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/commands/session.ts` around lines 175 - 177, The JSON path currently
ignores the --print-startup flag because buildSessionLoadJson always includes
startup; update buildSessionLoadJson (in src/lib/commands/session-presenters.ts)
to accept a boolean (e.g., includeStartup) or read the flag you pass from the
caller and only include the startup field when that boolean is true, then change
the call site in session.ts (the branch where action === "load" and
options.json) to pass the options.printStartup value into buildSessionLoadJson
so session load --json and session load --json --print-startup produce different
payload shapes.
| const recentAuditPreviewReadLimit = | ||
| options.recentAuditPreviewReadLimit ?? defaultRecentContinuityPreviewReadLimit; | ||
| const recentAuditLimit = options.recentAuditLimit ?? defaultRecentContinuityAuditLimit; | ||
| const recentContinuityAuditPreviewEntries = | ||
| await options.runtime.sessionContinuityStore.readRecentAuditEntries(recentAuditPreviewReadLimit); | ||
| const pendingContinuityRecoveryRecord = | ||
| await options.runtime.sessionContinuityStore.readRecoveryRecord(); | ||
|
|
||
| return { | ||
| rolloutPath: options.rolloutPath, | ||
| written, | ||
| excludePath: | ||
| options.scope === "project" | ||
| ? null | ||
| : options.runtime.sessionContinuityStore.getLocalIgnorePath(), | ||
| summary: generation.summary, | ||
| diagnostics: generation.diagnostics, | ||
| latestContinuityAuditEntry: recentContinuityAuditPreviewEntries[0] ?? null, | ||
| recentContinuityAuditEntries: recentContinuityAuditPreviewEntries.slice(0, recentAuditLimit), |
There was a problem hiding this comment.
Honor recentAuditLimit when it exceeds the preview size.
Right now you read recentAuditPreviewReadLimit entries and only then slice to recentAuditLimit. If a caller sets recentAuditLimit higher than the preview read limit, this API silently returns fewer rows than requested.
🔧 Suggested fix
- const recentAuditPreviewReadLimit =
- options.recentAuditPreviewReadLimit ?? defaultRecentContinuityPreviewReadLimit;
const recentAuditLimit = options.recentAuditLimit ?? defaultRecentContinuityAuditLimit;
+ const recentAuditPreviewReadLimit = Math.max(
+ options.recentAuditPreviewReadLimit ?? defaultRecentContinuityPreviewReadLimit,
+ recentAuditLimit
+ );
const recentContinuityAuditPreviewEntries =
await options.runtime.sessionContinuityStore.readRecentAuditEntries(recentAuditPreviewReadLimit);📝 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.
| const recentAuditPreviewReadLimit = | |
| options.recentAuditPreviewReadLimit ?? defaultRecentContinuityPreviewReadLimit; | |
| const recentAuditLimit = options.recentAuditLimit ?? defaultRecentContinuityAuditLimit; | |
| const recentContinuityAuditPreviewEntries = | |
| await options.runtime.sessionContinuityStore.readRecentAuditEntries(recentAuditPreviewReadLimit); | |
| const pendingContinuityRecoveryRecord = | |
| await options.runtime.sessionContinuityStore.readRecoveryRecord(); | |
| return { | |
| rolloutPath: options.rolloutPath, | |
| written, | |
| excludePath: | |
| options.scope === "project" | |
| ? null | |
| : options.runtime.sessionContinuityStore.getLocalIgnorePath(), | |
| summary: generation.summary, | |
| diagnostics: generation.diagnostics, | |
| latestContinuityAuditEntry: recentContinuityAuditPreviewEntries[0] ?? null, | |
| recentContinuityAuditEntries: recentContinuityAuditPreviewEntries.slice(0, recentAuditLimit), | |
| const recentAuditLimit = options.recentAuditLimit ?? defaultRecentContinuityAuditLimit; | |
| const recentAuditPreviewReadLimit = Math.max( | |
| options.recentAuditPreviewReadLimit ?? defaultRecentContinuityPreviewReadLimit, | |
| recentAuditLimit | |
| ); | |
| const recentContinuityAuditPreviewEntries = | |
| await options.runtime.sessionContinuityStore.readRecentAuditEntries(recentAuditPreviewReadLimit); | |
| const pendingContinuityRecoveryRecord = | |
| await options.runtime.sessionContinuityStore.readRecoveryRecord(); | |
| return { | |
| rolloutPath: options.rolloutPath, | |
| written, | |
| excludePath: | |
| options.scope === "project" | |
| ? null | |
| : options.runtime.sessionContinuityStore.getLocalIgnorePath(), | |
| summary: generation.summary, | |
| diagnostics: generation.diagnostics, | |
| latestContinuityAuditEntry: recentContinuityAuditPreviewEntries[0] ?? null, | |
| recentContinuityAuditEntries: recentContinuityAuditPreviewEntries.slice(0, recentAuditLimit), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/domain/session-continuity-persistence.ts` around lines 170 - 188, The
code reads only recentAuditPreviewReadLimit entries into
recentContinuityAuditPreviewEntries then slices to recentAuditLimit, so if
recentAuditLimit > recentAuditPreviewReadLimit callers get fewer items than
requested; fix by computing a fetchLimit = Math.max(recentAuditPreviewReadLimit,
recentAuditLimit) (or similar) and pass that to
options.runtime.sessionContinuityStore.readRecentAuditEntries, then use
recentContinuityAuditPreviewEntries.slice(0, recentAuditLimit) to return the
requested number; update references to
recentAuditPreviewReadLimit/recentAuditLimit and the call to
readRecentAuditEntries accordingly.
| export async function writeWrapperMockCodex( | ||
| repoDir: string, | ||
| sessionsDir: string, | ||
| options: { | ||
| sessionId: string; | ||
| message: string; | ||
| callOutput?: string; | ||
| } | ||
| ): Promise<{ capturedArgsPath: string; mockCodexPath: string }> { | ||
| const capturedArgsPath = path.join(repoDir, "captured-args.json"); | ||
| const mockCodexPath = path.join(repoDir, "mock-codex"); | ||
| const todayDir = path.join(sessionsDir, "2026", "03", "15"); | ||
| await fs.mkdir(todayDir, { recursive: true }); | ||
| await fs.writeFile( | ||
| mockCodexPath, | ||
| `#!/usr/bin/env node | ||
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
| const cwd = process.cwd(); | ||
| const sessionsDir = process.env.CAM_CODEX_SESSIONS_DIR; | ||
| fs.writeFileSync(path.join(cwd, "captured-args.json"), JSON.stringify(process.argv.slice(2), null, 2)); | ||
| const rolloutDir = path.join(sessionsDir, "2026", "03", "15"); | ||
| fs.mkdirSync(rolloutDir, { recursive: true }); | ||
| const rolloutPath = path.join(rolloutDir, "rollout-2026-03-15T00-00-00-000Z-session.jsonl"); | ||
| fs.writeFileSync(rolloutPath, [ | ||
| JSON.stringify({ type: "session_meta", payload: { id: ${JSON.stringify(options.sessionId)}, timestamp: "2026-03-15T00:00:00.000Z", cwd } }), | ||
| JSON.stringify({ type: "event_msg", payload: { type: "user_message", message: ${JSON.stringify(options.message)} } }), | ||
| JSON.stringify({ type: "response_item", payload: { type: "function_call", name: "exec_command", call_id: "call-1", arguments: "{\\"cmd\\":\\"pnpm test\\"}" } }), | ||
| JSON.stringify({ type: "response_item", payload: { type: "function_call_output", call_id: "call-1", output: ${JSON.stringify(options.callOutput ?? "Process exited with code 0")} } }) | ||
| ].join("\\n")); | ||
| `, | ||
| "utf8" | ||
| ); | ||
| await fs.chmod(mockCodexPath, 0o755); | ||
|
|
||
| return { | ||
| capturedArgsPath, | ||
| mockCodexPath | ||
| }; | ||
| } | ||
|
|
||
| export async function writeMockCodexBinary( | ||
| tempRoot: string, | ||
| body: string | ||
| ): Promise<string> { | ||
| const mockBinary = path.join(tempRoot, "mock-codex"); | ||
| await fs.writeFile( | ||
| mockBinary, | ||
| `#!/usr/bin/env node | ||
| const fs = require("node:fs"); | ||
| const args = process.argv.slice(2); | ||
| const outputIndex = args.indexOf("-o"); | ||
| const outputPath = args[outputIndex + 1]; | ||
| ${body} | ||
| `, | ||
| "utf8" | ||
| ); | ||
| await fs.chmod(mockBinary, 0o755); | ||
| return mockBinary; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "session-test-support.ts" -type fRepository: Boulea7/Codex-Auto-Memory
Length of output: 107
🏁 Script executed:
git ls-files | grep -E "(test|spec)" | head -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 1013
🏁 Script executed:
rg "writeWrapperMockCodex|writeMockCodexBinary" --type tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 1581
🏁 Script executed:
cat -n test/helpers/session-test-support.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 4065
🏁 Script executed:
head -80 test/wrapper-session-continuity.test.ts | tail -40Repository: Boulea7/Codex-Auto-Memory
Length of output: 1561
🏁 Script executed:
rg "codexBinary" --type ts -B 3 -A 3Repository: Boulea7/Codex-Auto-Memory
Length of output: 27436
🏁 Script executed:
rg "export.*function runCommand" --type ts -A 8Repository: Boulea7/Codex-Auto-Memory
Length of output: 955
🏁 Script executed:
rg "function runCommandCapture" --type ts -A 8Repository: Boulea7/Codex-Auto-Memory
Length of output: 500
🏁 Script executed:
fd -e ts | xargs grep -l "runCommand\|runCommandCapture" | grep -v test | head -5Repository: Boulea7/Codex-Auto-Memory
Length of output: 245
🏁 Script executed:
cat -n src/lib/util/process.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 1290
🏁 Script executed:
web_search "Node.js spawn Windows shebang file execution"Repository: Boulea7/Codex-Auto-Memory
Length of output: 119
🏁 Script executed:
rg "win32|Windows|process.platform" --type ts src/Repository: Boulea7/Codex-Auto-Memory
Length of output: 376
🏁 Script executed:
rg "\.cmd|\.bat" --type tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 448
🏁 Script executed:
cat -n test/wrapper-session-continuity.test.ts | head -100Repository: Boulea7/Codex-Auto-Memory
Length of output: 4556
🏁 Script executed:
rg "process.platform.*win32" --type ts -B 2 -A 5 test/helpers/Repository: Boulea7/Codex-Auto-Memory
Length of output: 521
🏁 Script executed:
grep -r "skip.*win32\|skip.*Windows" test/ || echo "No platform-specific test skips found"Repository: Boulea7/Codex-Auto-Memory
Length of output: 106
🏁 Script executed:
cat -n test/helpers/cli-runner.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 1126
🏁 Script executed:
rg "test.*platform|platform.*test" --type ts -i | head -10Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
python3 << 'EOFPYTHON'
import json
import subprocess
# Verify Node.js spawn behavior on Windows shebang files
# Research what Node.js spawn() does with shebang-only files on Windows
info = """
Node.js child_process.spawn() behavior on Windows:
- On Windows, spawn() cannot directly execute files with only shebangs
- Shebang (#!) is a POSIX convention; Windows doesn't natively interpret it
- spawn() with shell:false (default) will fail on Windows for shebang-only files
- Common solutions:
1. Create .cmd wrapper that calls node explicitly
2. Use shell:true (but requires different code paths for different platforms)
3. Invoke via 'node' directly with the script as argument
4. Use execFile with shell: true on Windows only
The codebase shows awareness of this:
- cli-runner.ts uses: process.platform === "win32" ? ".cmd" : ""
- tarball-install-smoke.test.ts uses similar pattern
The helper functions in session-test-support.ts don't have this pattern.
"""
print(info)
EOFPYTHONRepository: Boulea7/Codex-Auto-Memory
Length of output: 843
🏁 Script executed:
cat -n src/lib/util/open.ts | head -40Repository: Boulea7/Codex-Auto-Memory
Length of output: 1019
Both writeWrapperMockCodex() and writeMockCodexBinary() fail on Windows without .cmd wrappers.
The helpers create bare shebang files that work on POSIX but cannot be executed by spawn() on Windows. Codebase patterns elsewhere (cli-runner.ts, tarball-install-smoke.test.ts) show the correct approach: generate platform-specific wrappers. On Windows, create a .cmd file that invokes node with the script path, or use node directly as the command instead of the script path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/session-test-support.ts` around lines 45 - 104,
writeWrapperMockCodex and writeMockCodexBinary create POSIX-only executable
scripts which fail on Windows; detect process.platform === "win32" and alongside
the generated mock-codex script write a .cmd wrapper that invokes node with the
script path (forwarding all args) and return the .cmd path to callers on Windows
(or alternatively, when creating the mock binary for Windows, return
process.execPath + " " + scriptPath so callers use node directly). Update both
functions (writeWrapperMockCodex and writeMockCodexBinary) to create the .cmd
file on Windows, ensure it forwards arguments and points to the generated
script, and have the function return the platform-appropriate executable path so
spawn() works cross-platform.
| afterEach(async () => { | ||
| process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir; | ||
| await cleanupTempDirs(tempDirs); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the test file
fd test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Read the file to understand its structure and the afterEach hook
cat -n test/session-command.test.ts | head -100Repository: Boulea7/Codex-Auto-Memory
Length of output: 4208
🏁 Script executed:
# Search for vi.spyOn and mock usage patterns
rg "vi\.spyOn|mockRejected|restoreAllMocks" test/session-command.test.ts -A 2 -B 2Repository: Boulea7/Codex-Auto-Memory
Length of output: 256
🏁 Script executed:
# Look for the afterEach implementation
rg "afterEach" test/session-command.test.ts -A 5Repository: Boulea7/Codex-Auto-Memory
Length of output: 700
🏁 Script executed:
# Search for all vi.spyOn calls in the file to see the full scope
rg "vi\.spyOn" test/session-command.test.ts -B 2 -A 3Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Check if vi.restoreAllMocks is called anywhere in the file
rg "restoreAllMocks|restore\(\)" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Get the full file length to understand its size
wc -l test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 102
🏁 Script executed:
# Check for beforeEach or other test lifecycle hooks
rg "beforeEach|describe|afterEach" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 185
🏁 Script executed:
# Search more carefully for all spy/mock patterns
rg "mockRejectedValueOnce|spyOn" test/session-command.test.ts -cRepository: Boulea7/Codex-Auto-Memory
Length of output: 70
🏁 Script executed:
# Let's see the actual context around the spy usage
rg "spyOn.*appendAuditLog" test/session-command.test.ts -B 5 -A 10Repository: Boulea7/Codex-Auto-Memory
Length of output: 558
🏁 Script executed:
# Check if there are any manual mock restore calls for specific spies
rg "\.restore\(\)|mockRestore" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 102
🏁 Script executed:
# Get context around lines where spies might be restored after their use
rg "mockRejectedValueOnce" test/session-command.test.ts -A 20Repository: Boulea7/Codex-Auto-Memory
Length of output: 706
🏁 Script executed:
# Find all vi.spyOn calls and their context to see if they're all restored
rg "vi\.spyOn" test/session-command.test.ts -A 15 | head -60Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Search for all spy variable declarations to track them
rg "const \w+Spy = vi\.spyOn" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Check if there's a second mockRejectedValueOnce that might not be restored
rg "mockRejectedValueOnce" test/session-command.test.ts -B 5 -A 20 | tail -40Repository: Boulea7/Codex-Auto-Memory
Length of output: 822
🏁 Script executed:
# Get more context around the first spyOn to see the full scope
sed -n '200,400p' test/session-command.test.ts | rg "spyOn|mockRejected|mockRestore" -B 3 -A 5Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Search the entire file more carefully for all spy-related calls
rg "spyOn|mockRestore|mockRejected" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 241
🏁 Script executed:
# Get the context of both mockRejectedValueOnce occurrences
rg "mockRejectedValueOnce" test/session-command.test.ts -B 10 -A 15Repository: Boulea7/Codex-Auto-Memory
Length of output: 872
🏁 Script executed:
# Let's verify there are no other vi.spyOn calls by searching more broadly
rg "vi\." test/session-command.test.ts | grep -E "spyOn|mock" | head -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Check the test structure to understand if there are nested describes
rg "describe\(" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 99
🏁 Script executed:
# Let's look at the line range around 36-39 to see full context including surrounding tests
sed -n '30,60p' test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 936
🏁 Script executed:
# Let's check what the actual full test looks like with the spy and mockRestore
sed -n '150,200p' test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 2372
🏁 Script executed:
# Confirm there's only one vi.spyOn in the entire file
rg -c "\.spyOn" test/session-command.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 70
🏁 Script executed:
# Let's view the complete test that contains the spyOn to understand the risk
rg "const appendAuditSpy" test/session-command.test.ts -B 30 -A 30 | head -70Repository: Boulea7/Codex-Auto-Memory
Length of output: 2142
Restore spies in afterEach as a safety net for test isolation.
This suite uses vi.spyOn(...).mockRejectedValueOnce(...) on SessionContinuityStore.prototype.appendAuditLog. While the test explicitly calls mockRestore(), if an assertion fails before that line executes, the patched prototype leaks into subsequent tests, turning one failure into a cascade.
🔧 Suggested fix
afterEach(async () => {
+ vi.restoreAllMocks();
process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir;
await cleanupTempDirs(tempDirs);
});📝 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.
| afterEach(async () => { | |
| process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir; | |
| await cleanupTempDirs(tempDirs); | |
| }); | |
| afterEach(async () => { | |
| vi.restoreAllMocks(); | |
| process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir; | |
| await cleanupTempDirs(tempDirs); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/session-command.test.ts` around lines 36 - 39, Ensure any spy on
SessionContinuityStore.prototype.appendAuditLog is restored in the test suite
teardown: in the afterEach hook, call mockRestore() (or vi.restoreAllMocks())
for the spy used in tests so the mocked appendAuditLog cannot leak between
tests; locate the afterEach block and add restoration of the spy or a global
restore to guarantee cleanup even if a test fails before its explicit
mockRestore.
| async function writeNoopCodexBinary(repoDir: string): Promise<string> { | ||
| const mockCodexPath = path.join(repoDir, "mock-codex"); | ||
| const todayDir = path.join(sessionsDir, "2026", "03", "15"); | ||
| await fs.mkdir(todayDir, { recursive: true }); | ||
| await fs.writeFile( | ||
| mockCodexPath, | ||
| `#!/usr/bin/env node | ||
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
| const cwd = process.cwd(); | ||
| const sessionsDir = process.env.CAM_CODEX_SESSIONS_DIR; | ||
| fs.writeFileSync(path.join(cwd, "captured-args.json"), JSON.stringify(process.argv.slice(2), null, 2)); | ||
| const rolloutDir = path.join(sessionsDir, "2026", "03", "15"); | ||
| fs.mkdirSync(rolloutDir, { recursive: true }); | ||
| const rolloutPath = path.join(rolloutDir, "rollout-2026-03-15T00-00-00-000Z-session.jsonl"); | ||
| fs.writeFileSync(rolloutPath, [ | ||
| JSON.stringify({ type: "session_meta", payload: { id: ${JSON.stringify(options.sessionId)}, timestamp: "2026-03-15T00:00:00.000Z", cwd } }), | ||
| JSON.stringify({ type: "event_msg", payload: { type: "user_message", message: ${JSON.stringify(options.message)} } }), | ||
| JSON.stringify({ type: "response_item", payload: { type: "function_call", name: "exec_command", call_id: "call-1", arguments: "{\\"cmd\\":\\"pnpm test\\"}" } }), | ||
| JSON.stringify({ type: "response_item", payload: { type: "function_call_output", call_id: "call-1", output: ${JSON.stringify(options.callOutput ?? "Process exited with code 0")} } }) | ||
| ].join("\\n")); | ||
| process.exit(0); | ||
| `, | ||
| "utf8" | ||
| ); | ||
| await fs.chmod(mockCodexPath, 0o755); | ||
|
|
||
| return { | ||
| capturedArgsPath, | ||
| mockCodexPath | ||
| }; | ||
| return mockCodexPath; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the specific lines
head -60 test/session-command.test.ts | tail -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 660
🏁 Script executed:
# Get more context around the function and see full content
cat -n test/session-command.test.ts | sed -n '40,70p'Repository: Boulea7/Codex-Auto-Memory
Length of output: 1266
🏁 Script executed:
# Search for usage of writeNoopCodexBinary and codexBinary to understand the execution context
rg -n "writeNoopCodexBinary|codexBinary" test/session-command.test.ts -A 3 -B 3Repository: Boulea7/Codex-Auto-Memory
Length of output: 1032
🏁 Script executed:
# Search for where codexBinary is executed in the source code
rg -n "codexBinary" --type ts --type js -B 2 -A 2 | head -100Repository: Boulea7/Codex-Auto-Memory
Length of output: 6617
🏁 Script executed:
# Look for spawn, exec, or similar calls that might use the binary
rg -n "spawn|execFile|exec" --type ts --type js | grep -i "binary\|codex" | head -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 1480
🏁 Script executed:
# Check the main source structure
find . -type f -name "*.ts" -not -path "./node_modules/*" -not -path "./test/*" | head -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 667
🏁 Script executed:
# Find where runWrappedCodex is defined
rg -n "runWrappedCodex" -B 2 -A 10 | head -80Repository: Boulea7/Codex-Auto-Memory
Length of output: 6846
🏁 Script executed:
# Check wrapper.ts to see how codexBinary is executed
cat -n src/lib/commands/wrapper.ts | head -100Repository: Boulea7/Codex-Auto-Memory
Length of output: 4013
🏁 Script executed:
# Search for spawn or execFile calls that might use codexBinary
rg -n "spawn|execFile" src/lib -B 3 -A 5Repository: Boulea7/Codex-Auto-Memory
Length of output: 1948
🏁 Script executed:
# Look for how runWrappedCodex uses codexBinary - check entire wrapper.ts
wc -l src/lib/commands/wrapper.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 100
🏁 Script executed:
# Read the wrapper.ts file to find the runWrappedCodex function implementation
cat -n src/lib/commands/wrapper.ts | sed -n '100,200p'Repository: Boulea7/Codex-Auto-Memory
Length of output: 3027
🏁 Script executed:
# Search for runWrappedCodex function definition and how it uses codexBinary
rg -n "function runWrappedCodex|export.*runWrappedCodex" src/lib/commands/wrapper.ts -A 30Repository: Boulea7/Codex-Auto-Memory
Length of output: 1040
🏁 Script executed:
# Check the full runCommand function to see if there's any platform handling
cat -n src/lib/util/process.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 1290
🏁 Script executed:
# Check if there's any Windows-specific handling in the codebase
rg -n "win32|windows|platform" src/lib/util/process.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 51
🏁 Script executed:
# Search for any mock implementations or test helpers that might have Windows handling
rg -n "writeWrapperMockCodex" test/ -A 20 | head -60Repository: Boulea7/Codex-Auto-Memory
Length of output: 4432
🏁 Script executed:
# Find the writeWrapperMockCodex implementation
rg -n "writeWrapperMockCodex" test/helpers/ -lRepository: Boulea7/Codex-Auto-Memory
Length of output: 105
🏁 Script executed:
# Read the implementation of writeWrapperMockCodex
rg -n "writeWrapperMockCodex" test/helpers/ -A 30Repository: Boulea7/Codex-Auto-Memory
Length of output: 2972
🏁 Script executed:
# Look for platform-aware patterns in the test helper files
rg -n "win32|windows|process.platform|\.cmd" test/helpers/ -B 2 -A 2Repository: Boulea7/Codex-Auto-Memory
Length of output: 392
🏁 Script executed:
# Check if there are any platform-specific test skips or conditions
rg -n "skip|only|platform|win32" test/session-command.test.ts test/wrapper-session-continuity.test.ts -B 2 -A 2 | head -40Repository: Boulea7/Codex-Auto-Memory
Length of output: 3341
🏁 Script executed:
# Check the full writeWrapperMockCodex to see if it's handled differently
cat -n test/helpers/session-test-support.ts | sed -n '45,85p'Repository: Boulea7/Codex-Auto-Memory
Length of output: 2135
🏁 Script executed:
# Look at spawn options in Node.js docs context - check if shell option is used anywhere
rg -n "shell:" src/lib -B 2 -A 2Repository: Boulea7/Codex-Auto-Memory
Length of output: 51
Make writeNoopCodexBinary and helper functions platform-aware.
The function writes a bare shebang script with chmod 0o755, which works on POSIX but fails on Windows. When spawn() executes the binary without shell: true, Windows cannot execute a shebang-based text file. Either emit a .cmd wrapper on Windows, launch the file through process.execPath, or use shell: true in spawn options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/session-command.test.ts` around lines 47 - 57, The writeNoopCodexBinary
helper is POSIX-only (writes a shebang script and chmods), causing Windows
spawns to fail; make it platform-aware by detecting process.platform === "win32"
and on Windows either emit a .cmd wrapper alongside the script (or write a
.bat/.cmd that calls node %~dp0\mock-codex) or return a path that will be
launched via process.execPath (i.e., write a .js file and invoke it with
process.execPath in tests), and avoid relying solely on chmod; update any tests
that spawn the returned path (or their spawn options) to use shell:true or to
use process.execPath when on Windows so the noop codex executes reliably across
platforms (refer to function name writeNoopCodexBinary and any tests that call
it).
| const packResult = runCommandCapture( | ||
| npmCommand(), | ||
| ["pack", "--pack-destination", packDir], | ||
| process.cwd(), | ||
| env | ||
| ); | ||
| expect(packResult.exitCode).toBe(0); | ||
|
|
||
| const tarballName = packResult.stdout.trim().split(/\r?\n/).at(-1); | ||
| expect(tarballName).toBeTruthy(); | ||
| const tarballPath = path.join(packDir, tarballName!); | ||
|
|
||
| const initResult = runCommandCapture(npmCommand(), ["init", "-y"], installDir, env); | ||
| expect(initResult.exitCode).toBe(0); | ||
|
|
||
| const installResult = runCommandCapture( | ||
| npmCommand(), | ||
| ["install", "--no-package-lock", tarballPath], | ||
| installDir, | ||
| env | ||
| ); | ||
| expect(installResult.exitCode).toBe(0); | ||
|
|
||
| const versionResult = runCommandCapture(camBinaryPath(installDir), ["--version"], installDir, env); | ||
| expect(versionResult.exitCode).toBe(0); | ||
| expect(versionResult.stdout.trim()).toBe(packageJson.version); | ||
|
|
||
| const sessionStatusResult = runCommandCapture( | ||
| camBinaryPath(installDir), | ||
| ["session", "status", "--json"], | ||
| installDir, | ||
| env | ||
| ); | ||
| expect(sessionStatusResult.exitCode).toBe(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the implementation of runCommandCapture
fd -t f "process.ts" --path "*lib/util*" | head -20Repository: Boulea7/Codex-Auto-Memory
Length of output: 302
🏁 Script executed:
# Search for runCommandCapture definition
rg "runCommandCapture" -t ts --no-heading -B 2 -A 10 | head -100Repository: Boulea7/Codex-Auto-Memory
Length of output: 5702
🏁 Script executed:
# Look at test file around lines 50-83
cat -n test/tarball-install-smoke.test.ts | sed -n '40,90p'Repository: Boulea7/Codex-Auto-Memory
Length of output: 2274
🏁 Script executed:
# Find the process.ts file in src
fd -t f "process.ts" srcRepository: Boulea7/Codex-Auto-Memory
Length of output: 92
🏁 Script executed:
# Get the full runCommandCapture function implementation
rg "function runCommandCapture|export.*runCommandCapture" -A 30 --no-headingRepository: Boulea7/Codex-Auto-Memory
Length of output: 929
Add timeout protection to npm subprocess calls.
The runCommandCapture() helper uses spawnSync() without a timeout parameter. If npm pack or npm install stalls, the test will hang indefinitely and the Vitest test timeout won't interrupt a blocked synchronous spawn. Add a timeout option to spawnSync() in src/lib/util/process.ts to prevent these subprocess calls from blocking the entire worker.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tarball-install-smoke.test.ts` around lines 50 - 83, The tests can hang
because runCommandCapture (which calls spawnSync in src/lib/util/process.ts)
doesn't set a timeout; update the spawnSync call inside runCommandCapture (and
any helpers in that module) to pass a reasonable timeout option (e.g.
configurable DEFAULT_SUBPROCESS_TIMEOUT) so synchronous child processes like npm
pack/install are killed if they exceed the limit, expose/configure that timeout
constant so tests can override it if needed, and ensure the runCommandCapture
return object still reports exitCode/stdout/stderr when a timeout occurs.
| afterEach(async () => { | ||
| process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir; | ||
| await cleanupTempDirs(tempDirs); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -type f -name "wrapper-session-continuity.test.ts" | head -1Repository: Boulea7/Codex-Auto-Memory
Length of output: 124
🏁 Script executed:
cd /repo && cat -n test/wrapper-session-continuity.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 124
🏁 Script executed:
pwd && ls -laRepository: Boulea7/Codex-Auto-Memory
Length of output: 1603
🏁 Script executed:
find . -type f -name "wrapper-session-continuity.test.ts" 2>/dev/null | head -5Repository: Boulea7/Codex-Auto-Memory
Length of output: 110
🏁 Script executed:
cat -n ./test/wrapper-session-continuity.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 26909
🏁 Script executed:
rg "vi\.spyOn|mockRestore|restoreAllMocks" ./test/wrapper-session-continuity.test.ts -A 2 -B 2Repository: Boulea7/Codex-Auto-Memory
Length of output: 382
🏁 Script executed:
sed -n '477,484p' ./test/wrapper-session-continuity.test.ts && echo "---" && sed -n '542,549p' ./test/wrapper-session-continuity.test.tsRepository: Boulea7/Codex-Auto-Memory
Length of output: 710
Add vi.restoreAllMocks() to afterEach.
This suite uses vi.spyOn(...).mockRejectedValueOnce(...) with local mockRestore() calls inside test bodies (lines 484, 549). If a test fails before its mockRestore() executes, the mock persists into later tests and obscures failures. Global cleanup in afterEach prevents this leak.
Suggested fix
afterEach(async () => {
+ vi.restoreAllMocks();
process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir;
await cleanupTempDirs(tempDirs);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/wrapper-session-continuity.test.ts` around lines 30 - 33, Add a global
mock restore to the test teardown: modify the afterEach block that currently
resets process.env and calls cleanupTempDirs(tempDirs) to also call
vi.restoreAllMocks() so any spies/mocks created with
vi.spyOn(...).mockRejectedValueOnce(...) are cleared even if individual tests
fail before calling mockRestore(); update the afterEach containing
process.env.CAM_CODEX_SESSIONS_DIR = originalSessionsDir and await
cleanupTempDirs(tempDirs) to include vi.restoreAllMocks() as the first or last
cleanup step.
|
Follow-up update for the PR-review CI failure in Fixed in What changed:
Validation:
The PR head is now |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/dist-cli-smoke.test.ts (1)
48-59:⚠️ Potential issue | 🔴 CriticalDist smoke tests still depend on build artifacts being present before execution.
At Line 55, Line 143, and Line 198, this suite invokes the
distentrypoint and asserts success (Line 57, Line 152, Line 203). CI failures showdist/cli.jsis missing, so this is still the same pre-build ordering problem rather than a test-body regression.Also applies to: 143-154, 198-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dist-cli-smoke.test.ts` around lines 48 - 59, The test fails because it relies on compiled artifacts (entrypoint "dist") that may not exist; update the test to ensure the cli is built before invoking runCli or switch to the source entrypoint: either (A) run the build step (e.g., invoke the repo's build command or call the build helper) and await completion before calling runCli with entrypoint "dist", or (B) change the test to call runCli(repoDir, ["--version"], { entrypoint: "src" }) so it uses the uncompiled entrypoint; update the three locations referencing runCli(..., { entrypoint: "dist" }) (in test/dist-cli-smoke.test.ts and the duplicates) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/dist-cli-smoke.test.ts`:
- Around line 48-59: The test fails because it relies on compiled artifacts
(entrypoint "dist") that may not exist; update the test to ensure the cli is
built before invoking runCli or switch to the source entrypoint: either (A) run
the build step (e.g., invoke the repo's build command or call the build helper)
and await completion before calling runCli with entrypoint "dist", or (B) change
the test to call runCli(repoDir, ["--version"], { entrypoint: "src" }) so it
uses the uncompiled entrypoint; update the three locations referencing
runCli(..., { entrypoint: "dist" }) (in test/dist-cli-smoke.test.ts and the
duplicates) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac6b982c-a50f-4c18-8ffb-85b8373b62f7
📒 Files selected for processing (2)
test/dist-cli-smoke.test.tstest/helpers/cli-runner.ts
✅ Files skipped from review due to trivial changes (1)
- test/helpers/cli-runner.ts
|
Second follow-up for the failing PR CI. Root cause was not the PR itself and not a flaky rerun issue: the default Fixed in What changed:
Validation:
The PR branch is updated in place; no PR close/reopen is needed. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
CONTRIBUTING.md (3)
70-70: Document the rationale behind this architectural constraint.The prohibition "Do not hard-merge the rollout selection rules for
cam session refreshand wrapper auto-save" is notably defensive and suggests underlying design complexity or code that appears to warrant consolidation but shouldn't be consolidated.When architectural guidelines prohibit seemingly logical refactoring, documenting why the separation must be maintained helps future maintainers understand the design trade-offs. Consider adding a brief explanation in
docs/architecture.mdordocs/session-continuity.mdthat explains the different concerns these two features address and why their provenance selection must remain independent despite sharing persistence mechanics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 70, The CONTRIBUTING.md note forbids hard-merging rollout selection rules for "cam session refresh" and "wrapper auto-save" but lacks rationale; add a brief explanation in docs/architecture.md (or create docs/session-continuity.md) that explains why these two features must keep independent provenance selection despite sharing persistence semantics — describe the distinct concerns (e.g., session continuity/provenance vs. UX autosave behavior), give a short example of a problematic hard-merge scenario, and state the intended invariant to preserve so future maintainers understand the constraint; reference the terms "cam session refresh" and "wrapper auto-save" in that document so it’s discoverable from CONTRIBUTING.md.
66-70: Consider relocating implementation contract details to architecture documentation.These lines document specific persistence semantics (merge vs. replace) and a prohibition about rollout selection rules. While valuable information, this level of implementation detail is typically better suited for
docs/architecture.mdordocs/session-continuity.md, where other architectural contracts and design decisions are documented.CONTRIBUTING.md works best for contribution workflow and high-level coding principles, while detailed contract constraints and their rationale belong in architecture documentation where they can be properly contextualized with the overall design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 66 - 70, The CONTRIBUTING.md lines that specify persistence semantics for `cam session save` (merge), `cam session refresh` (replace), and wrapper auto-save (merge), and the note about not hard-merging rollout selection rules, should be moved into a design/architecture doc (e.g., docs/architecture.md or docs/session-continuity.md); update CONTRIBUTING.md to reference that architecture doc for implementation contracts and keep only high-level contribution workflow guidance, while copying the full details and rationale for `cam session save`, `cam session refresh`, wrapper auto-save, and the rollout selection rules into the chosen architecture document so they remain discoverable and contextually documented.
62-65: Consider varying the sentence structure for readability.Four consecutive sentences begin with "Keep," which creates noticeable repetition. While parallel structure can aid clarity in technical documentation, varying the phrasing slightly would improve readability. As per coding guidelines (LanguageTool hint), three or more successive sentences starting with the same word should be reconsidered.
For example:
- "New commands should be registered through..."
- "Runtime composition belongs in..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` around lines 62 - 65, Rewrite the four consecutive lines that begin with "Keep" to reduce repetition while preserving meaning: change the first line to start with "Register new commands through src/lib/cli/register-commands.ts instead of expanding src/cli.ts," change the second to "Perform runtime composition in src/lib/runtime/runtime-context.ts so command files reuse that runtime surface," change the third to "Limit src/lib/commands/session.ts to provenance selection and action dispatch, and move reviewer-facing presentation to src/lib/commands/session-presenters.ts," and change the fourth to "Centralize shared continuity persistence in src/lib/domain/session-continuity-persistence.ts so session commands and auto-save use the same persistence path." Ensure the phrasing is parallel but varied (avoid starting three or more consecutive sentences with the same word).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CONTRIBUTING.md`:
- Line 70: The CONTRIBUTING.md note forbids hard-merging rollout selection rules
for "cam session refresh" and "wrapper auto-save" but lacks rationale; add a
brief explanation in docs/architecture.md (or create docs/session-continuity.md)
that explains why these two features must keep independent provenance selection
despite sharing persistence semantics — describe the distinct concerns (e.g.,
session continuity/provenance vs. UX autosave behavior), give a short example of
a problematic hard-merge scenario, and state the intended invariant to preserve
so future maintainers understand the constraint; reference the terms "cam
session refresh" and "wrapper auto-save" in that document so it’s discoverable
from CONTRIBUTING.md.
- Around line 66-70: The CONTRIBUTING.md lines that specify persistence
semantics for `cam session save` (merge), `cam session refresh` (replace), and
wrapper auto-save (merge), and the note about not hard-merging rollout selection
rules, should be moved into a design/architecture doc (e.g.,
docs/architecture.md or docs/session-continuity.md); update CONTRIBUTING.md to
reference that architecture doc for implementation contracts and keep only
high-level contribution workflow guidance, while copying the full details and
rationale for `cam session save`, `cam session refresh`, wrapper auto-save, and
the rollout selection rules into the chosen architecture document so they remain
discoverable and contextually documented.
- Around line 62-65: Rewrite the four consecutive lines that begin with "Keep"
to reduce repetition while preserving meaning: change the first line to start
with "Register new commands through src/lib/cli/register-commands.ts instead of
expanding src/cli.ts," change the second to "Perform runtime composition in
src/lib/runtime/runtime-context.ts so command files reuse that runtime surface,"
change the third to "Limit src/lib/commands/session.ts to provenance selection
and action dispatch, and move reviewer-facing presentation to
src/lib/commands/session-presenters.ts," and change the fourth to "Centralize
shared continuity persistence in
src/lib/domain/session-continuity-persistence.ts so session commands and
auto-save use the same persistence path." Ensure the phrasing is parallel but
varied (avoid starting three or more consecutive sentences with the same word).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6edcc22a-4ef1-4ea5-9153-cdbc6b3e56db
📒 Files selected for processing (2)
CONTRIBUTING.mdpackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Created backup tags before pruning merged local and remote branches after PR #6 landed in main.
1 participant
Summary
Describe the behavior change in user-facing terms.
What changed
Claude parity
Explain whether this change improves Claude Code auto memory parity, preserves it, or intentionally diverges.
Validation
pnpm lintpnpm testpnpm buildDocs
docs/claude-reference.mdupdated if parity behavior changeddocs/architecture.mdupdated if internals changeddocs/native-migration.mdupdated if migration assumptions changedRisks
List any risks, edge cases, or follow-up work.
Summary by cubic
Cleans up the CLI and session continuity internals for reliability, and adds tagged release automation with hardened smoke tests for the dist CLI and tarball installs.
Refactors
src/cli.ts; centralized command wiring insrc/lib/cli/register-commands.ts.src/lib/domain/session-continuity-persistence.tsas a shared persistence spine forcam sessionand the wrapper.src/lib/commands/session-presenters.tsfor consistent text/json surfaces.src/lib/runtime/runtime-context.ts(viapatchConfigAndReloadRuntime).test/helpers/cli-runner.tsto run source or dist.New Features
package.json, runspnpm verify:release, and packs a tarball artifact.pnpm testnow excludes them for speed.package.jsonaddsprepack, script cleanups, and isolates dist/tarball smoke tests; docs updated withREADME.ja.md,README.zh-TW.md, and architecture/release checklist tweaks.Written for commit bbc7d55. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation