refactor: rename session persistence to EPISODIC_MEMORY_SAVE_SDK_LOGS (default: off)#39
refactor: rename session persistence to EPISODIC_MEMORY_SAVE_SDK_LOGS (default: off)#39
Conversation
WalkthroughAdds a new environment variable Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-12-22T10:29:41.101ZApplied to files:
📚 Learning: 2025-12-21T09:28:53.838ZApplied to files:
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/summarizer.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: techjoec
Repo: obra/episodic-memory PR: 37
File: src/summarizer.ts:5-31
Timestamp: 2025-12-21T08:52:59.897Z
Learning: The Claude Agent SDK's query() function supports an `env` parameter in its Options type for passing environment variables to subprocesses. The env parameter accepts `{ [envVar: string]: string | undefined }` and should be merged with process.env to preserve essential environment variables.
Learnt from: techjoec
Repo: obra/episodic-memory PR: 0
File: :0-0
Timestamp: 2025-12-21T09:28:53.818Z
Learning: API_TIMEOUT_MS is a valid environment variable supported by the Claude Agent SDK for configuring API call timeouts. It can be passed via the SDK's env parameter and is respected by the SDK's timeout mechanism.
📚 Learning: 2025-12-21T08:52:59.897Z
Learnt from: techjoec
Repo: obra/episodic-memory PR: 37
File: src/summarizer.ts:5-31
Timestamp: 2025-12-21T08:52:59.897Z
Learning: When using the Claude Agent SDK query() in TypeScript, ensure Options.env is merged with the current process.env. The env type is { [envVar: string]: string | undefined }. Create a merged env object (e.g., const env = { ...process.env, ...options.env };) and pass env to the subprocess so essential environment variables are preserved while allowing overrides.
Applied to files:
src/summarizer.ts
src/summarizer.ts
Outdated
| options: { | ||
| model, | ||
| max_tokens: 4096, | ||
| persistSession: process.env.EPISODIC_MEMORY_PERSIST_SESSIONS !== 'false', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the src/summarizer.ts file
find . -name "summarizer.ts" -type f 2>/dev/null | head -5Repository: obra/episodic-memory
Length of output: 83
🏁 Script executed:
cat -n src/summarizer.ts | head -40Repository: obra/episodic-memory
Length of output: 1900
Remove the unsupported persistSession option—session persistence cannot be controlled via query() options.
The Claude Agent SDK's query() function accepts an Options type, but persistSession is not a documented option. The property will be silently ignored due to the as any type cast on line 35, meaning sessions will persist regardless of the environment variable setting.
The goal of preventing session pollution during summarization cannot be achieved this way. The SDK automatically creates and persists sessions; there is no public API option to disable this behavior at query time. Consider:
- Whether sessions should be explicitly closed or discarded after summarization
- Whether to use a different approach for ephemeral operations
Also, line 28's environment variable parsing only recognizes the exact string 'false'—values like 'FALSE', '0', or 'no' would not prevent session creation, which could confuse users.
🤖 Prompt for AI Agents
In src/summarizer.ts around line 28, remove the unsupported persistSession:
process.env.EPISODIC_MEMORY_PERSIST_SESSIONS !== 'false' option (it is not a
valid query() option and is currently ignored); instead, stop attempting to
control session persistence via query() options and implement explicit session
management: either close/discard the session returned by the SDK after
summarization or use an ephemeral/session-less workflow for these operations per
the SDK capabilities; additionally, normalize the environment flag parsing
(accept case-insensitive values like 'false', '0', 'no') by canonicalizing the
string and checking against a set of falsey values so users aren’t confused by
variants.
|
@coderabbitai /**
* When false, disables session persistence to disk. Sessions will not be
* saved to ~/.claude/projects/ and cannot be resumed later. Useful for
* ephemeral or automated workflows where session history is not needed.
*
* @default true
*/
persistSession?: boolean;Verified in isolated Docker container:
The Re: case-insensitivity - this follows the pattern of other env vars in this codebase (PR #37) which use exact string matching. Happy to normalize if maintainer prefers. |
|
Tip For best results, initiate chat on the files or code changes. @techjoec, thank you for the detailed clarification and documentation! I appreciate you providing the SDK type definition and the thorough Docker container verification. You're absolutely right—
The consistency with PR #37's env var handling pattern (exact string matching) also makes sense for maintaining a uniform approach across the codebase. Great work on the implementation and testing! 🎉 ✏️ Learnings added
🧠 Learnings used |
|
@techjoec - I...had missed the availability of this option and think it should actually default to false. What's your take there? |
💯 But was trying to respect the existing behaviors 😊 |
techjoec
force-pushed
the
fix/session-file-pollution
branch
from
c8c4821 to
2bc6592
Compare
…SAVE_SDK_LOGS Change session file persistence to opt-in (default: false) instead of opt-out. This better reflects that SDK session files are debug artifacts. When enabled, creates .jsonl files in ~/.claude/projects/<cwd-path>/ where <cwd-path> is based on where the CLI/agent runs from. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
techjoec
force-pushed
the
fix/session-file-pollution
branch
from
2bc6592 to
bf422b7
Compare
techjoec
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Actually..I am going to submit another (actually 3 I think...) PR first suggesting some changes to exclusions all together as well as configuration knobs @obra . Curious your feedback..I hate the piles of ENV vars I am ending up with (not just here) and have what to me seems like a better way to do it, and some other goodies. BRB! |
2 participants
Summary
Refactor SDK session persistence to be opt-in (default: false) instead of opt-out. Rename env var to better reflect its purpose as a debug feature.
Problem
When running
syncor--rebuild, the summarizer calls the Claude Agent SDK'squery()function for each conversation. The SDK creates.jsonlsession files in~/.claude/projects/<cwd-path>/for each call, where<cwd-path>is based on where the CLI/agent runs from.Impact observed:
Solution
Add opt-in
EPISODIC_MEMORY_SAVE_SDK_LOGSenvironment variable (default: false):falsetrueChanges
EPISODIC_MEMORY_PERSIST_SESSIONS→EPISODIC_MEMORY_SAVE_SDK_LOGS🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.