fix(security): keep no-session descendants ephemeral - #1251
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6da654721
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jonaowen
left a comment
There was a problem hiding this comment.
The implementation propagates the parent manager’s persistence mode across all three descendant runtime paths and still initializes explicit depth for in-memory lineage. The regression gates child and grandchild managers plus absence of JSONL in both artifact directories; the documentation carefully limits the claim and discloses remaining temp/global state rather than calling --no-session a sandbox. I found no blocker at exact head e6da654721402f88c35506537f1a1d96ab5f649d.
|
@Apocrathia ready for review at b9ab6c0. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a689a75. Configure here.
| activeSessionId: state.activeSessionId, | ||
| sessionId: session.sessionId, | ||
| sessionFile, | ||
| ...(session.sessionManager.allowsPersistence() ? {} : { persistence: "memory" as const }), |
There was a problem hiding this comment.
Restart marker misses memory checkpoints
Medium Severity
Update-restart now snapshots ephemeral sessions with writeCheckpointFile without attaching that path or enabling persistence, then later appends the prime-agent.update_restart marker only to in-memory state. Closing the session never rewrites the checkpoint, so restore loads a transcript without the interruption marker that persisted sessions still get.
Reviewed by Cursor Bugbot for commit a689a75. Configure here.


Summary
--no-sessionFixes #1120.
Risk
Low to medium. The behavioral change is limited to descendants of explicitly ephemeral sessions. Normal persisted sessions continue to create persisted descendants.
Provenance
Extracted directly onto current
mainfrom #1159. Commits retaincherry picked fromtrailers for4c7cdc4b6f6a7ee96e2307e0a42c3db3fe82a68f,6446bafa5fe57421cc1660ce195fd302d9643a89, andd07e5a80db1ee704d67d029c838264699f6b4793. No MCP/provider changes are included.Validation
npm run checknpm run test -- test/agent-session-recursion.test.ts— 97 passednpm run test -- test/suite/agent-session-runtime.test.ts— 24 passednpm run test -- test/daemon-mode.test.ts— 191 passeddocs.jsongit diff --check origin/main...HEADIndependent review found and fixed descendant depth initialization across inline, hosted-runtime, and daemon paths; the added assertions cover child/grandchild depth while confirming no session file is created.
Review focus
Please verify the ephemeral-session inheritance across inline, runtime, and daemon paths, and that the documentation accurately distinguishes application behavior from OS-level isolation.
Note
Keep
--no-sessionRLM descendants ephemeral and clarify security modelSessionManager.inMemoryinstead of writing session files to disk.SessionManager.isPersisted()toallowsPersistence(); addedwriteCheckpointFile()for atomic checkpoint writes without flipping the manager to persisted mode, andopenInMemoryAsync()to load an existing session file into memory without on-disk state.persistence: "memory"in the manifest, restores them as non-persisted, and cleans up the temp files on cancel or after restore.--no-sessionis not a sandbox and that temporary RLM working files may remain.SessionManager.isPersisted()is removed (renamed toallowsPersistence()); any out-of-tree callers ofisPersisted()will break. Daemon update-restart now depends onwriteCheckpointFileand thepersistencemanifest field — reviewers should verify checkpoint cleanup paths incancelPreparedUpdateRestartandrestoreDaemonUpdateRestartSession.Macroscope summarized a689a75.
Note
Medium Risk
Touches session persistence and daemon update-restart recovery so ephemeral sessions can still be snapshotted and restored. A bug here could leak session files or drop in-memory work across restarts.
Overview
--no-sessionnow applies to the whole RLM tree, not just the root. Inline, hosted-runtime, and daemon subagent creation inherit the parent’s persistence policy viaallowsPersistence()and use in-memory managers so descendants no longer write JSONL/artifact trees.Daemon update-restart no longer materializes ephemeral sessions into durable files. It writes a temporary checkpoint (
writeCheckpointFile), markspersistence: "memory", restores withnoSession, then deletes the checkpoint (including on cancel).Docs add a Security and Sandboxing guide and reword
--no-session/ autonomous limits as execution controls, not isolation.Reviewed by Cursor Bugbot for commit a689a75. Bugbot is set up for automated code reviews on this repo. Configure here.