feat(session): add FileSessionStorage for persistent sessions#134
feat(session): add FileSessionStorage for persistent sessions#134USEONGEE wants to merge 4 commits into
Conversation
SessionStorage 인터페이스를 구현하는 FileSessionStorage 클래스 추가. 원자적 쓰기, 에러 복구, TTL/LRU 관리 포함. 테스트 22개 통과. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: useongee <118250509+USEONGEE@users.noreply.github.com>
createSessionStorage() 팩토리 함수 도입. CODEX_MCP_MEMORY_ONLY, CODEX_MCP_SESSION_FILE 환경변수 지원. README에 환경변수 설명 및 동시성 경고 추가. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: useongee <118250509+USEONGEE@users.noreply.github.com>
- Remove unused imports (unlink, mkdirSync) - Replace NodeJS.ErrnoException with inline type - Add eslint-disable for intentional control-regex pattern - Apply Prettier formatting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: useongee <118250509+USEONGEE@users.noreply.github.com>
WalkthroughAdds a disk-backed session storage backend and wiring to choose it at runtime, plus docs and extensive tests validating persistence, recovery, concurrency, and TTL/LRU behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handlers
participant SessionStore as FileSessionStorage
participant FS as Filesystem
Client->>Handlers: request (create/ensure/get/addTurn/flush)
Handlers->>SessionStore: call storage API
alt Persist needed
SessionStore->>FS: write `.tmp` JSON file (atomic write)
FS-->>SessionStore: write OK
SessionStore->>FS: rename `.tmp` -> `sessions.json`
FS-->>SessionStore: rename OK
else No persist / in-memory only
SessionStore-->>Handlers: respond from memory
end
SessionStore-->>Handlers: return result
Handlers-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (3)
src/session/file-storage.ts (3)
287-300: Silent failure on save errors loses data on crash.
executeSave()catches all errors silently (line 297-299). While the comment notes data remains in memory, if the process crashes after a failed save, changes are lost. Consider logging save failures to help diagnose issues in production.♻️ Proposed fix to log save errors
private async executeSave(): Promise<void> { try { const dir = dirname(this.filePath); mkdirSync(dir, { recursive: true, mode: 0o700 }); const tmpPath = `${this.filePath}.${process.pid}.tmp`; const content = this.serialize(); await writeFile(tmpPath, content, { mode: 0o600 }); await rename(tmpPath, this.filePath); - } catch { + } catch (err) { // Non-fatal: data is still in memory + console.error('[FileSessionStorage] Save failed:', err); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/file-storage.ts` around lines 287 - 300, The catch block in executeSave() currently silences all errors causing silent save failures; modify the catch to log the failure including this.filePath and the caught error (error.message and/or error.stack) so save problems are visible in production; use the class logger if one exists (e.g., this.logger or a provided logger), otherwise fallback to console.error, and keep the existing behavior of not throwing so in-memory data remains available.
336-345:enforceMaxSessionscallslistSessions()which triggerscleanupExpiredSessions()again.Inside
enforceMaxSessions(), callingthis.listSessions()(line 339) invokescleanupExpiredSessions()a second time. This is redundant sinceenforceMaxSessions()is only called fromcreateSession()andensureSession(), both of which already callcleanupExpiredSessions()first.♻️ Proposed fix to avoid double cleanup
private enforceMaxSessions(): void { if (this.sessions.size <= this.maxSessions) return; - const sessions = this.listSessions(); + // Sort sessions by lastAccessedAt (most recent first) without triggering cleanup again + const sessions = Array.from(this.sessions.values()).sort( + (a, b) => b.lastAccessedAt.getTime() - a.lastAccessedAt.getTime() + ); const sessionsToDelete = sessions.slice(this.maxSessions); for (const session of sessionsToDelete) { this.sessions.delete(session.id); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/file-storage.ts` around lines 336 - 345, enforceMaxSessions currently calls listSessions(), which re-invokes cleanupExpiredSessions and causes redundant cleanup because createSession() and ensureSession() already call cleanupExpiredSessions; change enforceMaxSessions to operate directly on the in-memory map (this.sessions) instead of calling listSessions(): build an array from this.sessions.values(), sort/order it the same way listSessions would (if ordering matters), compute sessionsToDelete = array.slice(this.maxSessions) and delete those by id from this.sessions; keep references to the functions mentioned (enforceMaxSessions, listSessions, cleanupExpiredSessions, createSession, ensureSession) to locate and update the code.
170-173:getCodexConversationIddoes not updatelastAccessedAt.All other read operations (
getSession) updatelastAccessedAtand trigger a save, but this method only reads without touching the session. This inconsistency could cause sessions to expire unexpectedly if they're only accessed viagetCodexConversationId.Consider whether this is intentional. If not:
♻️ Proposed fix to update lastAccessedAt
getCodexConversationId(sessionId: string): string | undefined { const session = this.sessions.get(sessionId); + if (session) { + session.lastAccessedAt = new Date(); + this.queueSave(); + } return session?.codexConversationId; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/file-storage.ts` around lines 170 - 173, getCodexConversationId reads a session but doesn't update lastAccessedAt like getSession does, causing inconsistent expiration behavior; modify getCodexConversationId to mirror getSession by retrieving the session from this.sessions, updating session.lastAccessedAt = Date.now(), invoking the same save/persist method used by getSession (e.g., saveSessions or persistSessions) to persist the timestamp, and then return session.codexConversationId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/handlers.ts`:
- Around line 559-566: The FileSessionStorage uses debounced saves via
queueSave() and pending writes can be lost on exit; modify
createSessionStorage/sessionStorage handling to register a graceful shutdown
handler (e.g., process.on('beforeExit'/'SIGINT'/'SIGTERM')) that calls the
storage.flush() and awaits completion before exiting. To make flush callable,
either add flush(): Promise<void> to the SessionStorage interface and implement
a no-op async flush in InMemorySessionStorage, or call (sessionStorage as
unknown as {flush?: () => Promise<void>}).flush() with a safe existence check
and await it; reference createSessionStorage, SessionStorage,
FileSessionStorage, InMemorySessionStorage, sessionStorage, queueSave(), and
flush() when making the change.
---
Nitpick comments:
In `@src/session/file-storage.ts`:
- Around line 287-300: The catch block in executeSave() currently silences all
errors causing silent save failures; modify the catch to log the failure
including this.filePath and the caught error (error.message and/or error.stack)
so save problems are visible in production; use the class logger if one exists
(e.g., this.logger or a provided logger), otherwise fallback to console.error,
and keep the existing behavior of not throwing so in-memory data remains
available.
- Around line 336-345: enforceMaxSessions currently calls listSessions(), which
re-invokes cleanupExpiredSessions and causes redundant cleanup because
createSession() and ensureSession() already call cleanupExpiredSessions; change
enforceMaxSessions to operate directly on the in-memory map (this.sessions)
instead of calling listSessions(): build an array from this.sessions.values(),
sort/order it the same way listSessions would (if ordering matters), compute
sessionsToDelete = array.slice(this.maxSessions) and delete those by id from
this.sessions; keep references to the functions mentioned (enforceMaxSessions,
listSessions, cleanupExpiredSessions, createSession, ensureSession) to locate
and update the code.
- Around line 170-173: getCodexConversationId reads a session but doesn't update
lastAccessedAt like getSession does, causing inconsistent expiration behavior;
modify getCodexConversationId to mirror getSession by retrieving the session
from this.sessions, updating session.lastAccessedAt = Date.now(), invoking the
same save/persist method used by getSession (e.g., saveSessions or
persistSessions) to persist the timestamp, and then return
session.codexConversationId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6acfa5b1-76af-45a8-9164-4b5b6350563b
📒 Files selected for processing (4)
README.mdsrc/__tests__/file-storage.test.tssrc/session/file-storage.tssrc/tools/handlers.ts
| function createSessionStorage(): SessionStorage { | ||
| if (process.env.CODEX_MCP_MEMORY_ONLY === 'true') { | ||
| return new InMemorySessionStorage(); | ||
| } | ||
| return new FileSessionStorage(process.env.CODEX_MCP_SESSION_FILE || undefined); | ||
| } | ||
|
|
||
| const sessionStorage = createSessionStorage(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if InMemorySessionStorage has a flush method
ast-grep --pattern $'class InMemorySessionStorage {
$$$
flush($$$) {
$$$
}
$$$
}'Repository: tuannvm/codex-mcp-server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find and examine the SessionStorage interface and implementations
fd -e ts -e js | xargs rg -l "interface SessionStorage|class FileSessionStorage|class InMemorySessionStorage" | head -20Repository: tuannvm/codex-mcp-server
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Look for the session storage file
fd "storage\.(ts|js)" | head -10Repository: tuannvm/codex-mcp-server
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for FileSessionStorage class definition
ast-grep --pattern $'class FileSessionStorage {
$$$
flush($$$) {
$$$
}
$$$
}'Repository: tuannvm/codex-mcp-server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Look for SessionStorage interface definition and check its methods
rg "interface SessionStorage" -A 30Repository: tuannvm/codex-mcp-server
Length of output: 1908
🏁 Script executed:
#!/bin/bash
# Read the FileSessionStorage implementation
cat -n src/session/file-storage.tsRepository: tuannvm/codex-mcp-server
Length of output: 11567
🏁 Script executed:
#!/bin/bash
# Check for shutdown handlers in the main handlers file
rg "beforeExit|SIGINT|SIGTERM|process\.on" src/tools/handlers.tsRepository: tuannvm/codex-mcp-server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check if there are any shutdown handlers elsewhere in the codebase
rg "beforeExit|SIGINT|SIGTERM" src/ --type tsRepository: tuannvm/codex-mcp-server
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Verify the createSessionStorage function in handlers.ts at lines 559-566
sed -n '555,570p' src/tools/handlers.tsRepository: tuannvm/codex-mcp-server
Length of output: 509
🏁 Script executed:
#!/bin/bash
# Check context around this function - is it called during initialization?
rg "createSessionStorage|sessionStorage" src/tools/handlers.ts -B 2 -A 2Repository: tuannvm/codex-mcp-server
Length of output: 1859
Missing graceful shutdown to flush pending saves.
FileSessionStorage uses debounced saves via queueSave(). If the process exits before a debounced save completes, data may be lost. The flush() method exists for this purpose but is never called on shutdown.
Add a shutdown handler to ensure pending saves complete before exit. Note: sessionStorage is typed as SessionStorage (the interface), which does not include flush(). To implement this, either add flush() to the SessionStorage interface with a no-op default in InMemorySessionStorage, or use a type assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/handlers.ts` around lines 559 - 566, The FileSessionStorage uses
debounced saves via queueSave() and pending writes can be lost on exit; modify
createSessionStorage/sessionStorage handling to register a graceful shutdown
handler (e.g., process.on('beforeExit'/'SIGINT'/'SIGTERM')) that calls the
storage.flush() and awaits completion before exiting. To make flush callable,
either add flush(): Promise<void> to the SessionStorage interface and implement
a no-op async flush in InMemorySessionStorage, or call (sessionStorage as
unknown as {flush?: () => Promise<void>}).flush() with a safe existence check
and await it; reference createSessionStorage, SessionStorage,
FileSessionStorage, InMemorySessionStorage, sessionStorage, queueSave(), and
flush() when making the change.
|
Nice feature, the atomic write pattern and corruption recovery are well thought out. A few things to address before this is ready: 1. The catch block treats all non-ENOENT/EACCES errors as JSON corruption and renames the file. This means transient I/O errors, // Current: all errors → backup
} catch (err: unknown) {
const error = err as { code?: string };
if (error.code === 'ENOENT') return;
if (error.code === 'EACCES') throw error;
this.backupCorruptedFile(); // too broad
}
// Suggested: separate parse errors from filesystem errors
private loadSync(): void {
let raw: string;
try {
raw = readFileSync(this.filePath, 'utf-8');
} catch (err: unknown) {
const error = err as { code?: string };
if (error.code === 'ENOENT') return;
throw error; // surface all other FS errors
}
try {
const data: StorageFile = JSON.parse(raw);
this.validateAndMigrate(data);
// ... hydrate sessions ...
} catch {
this.backupCorruptedFile(); // only JSON/schema errors
}
}2. Silent error swallowing in The empty - } catch {
+ } catch (err) {
// Non-fatal: data is still in memory
+ console.error('[FileSessionStorage] Save failed:', err);
}3. Session ID validation mismatch (
The CodeRabbit suggestions about The graceful shutdown / |
…rity - Separate loadSync() into two try/catch blocks: filesystem errors (EISDIR, EMFILE, etc.) now propagate instead of being treated as JSON corruption - Remove silent catch in executeSave(): flush() now rejects on write failure, background saves log errors via console.error without auto-retry - Add Date validation on load: sessions with invalid timestamps are skipped with a warning, preventing poisoned serialize() failures - Scope tmp file cleanup to instance-local UUID suffix, preventing cross-instance interference in multi-process environments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: useongee <118250509+USEONGEE@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/session/file-storage.ts (2)
350-359: Minor efficiency note:listSessions()triggers another cleanup pass.
enforceMaxSessionscallslistSessions()which internally callscleanupExpiredSessions(). Since callers likecreateSessionalready callcleanupExpiredSessions()beforeenforceMaxSessions(), this results in redundant cleanup. This is inherited behavior and not blocking.♻️ Optional: Add internal method without cleanup
+ private getSessionsSorted(): SessionData[] { + return Array.from(this.sessions.values()).sort( + (a, b) => b.lastAccessedAt.getTime() - a.lastAccessedAt.getTime() + ); + } + listSessions(): SessionData[] { this.cleanupExpiredSessions(); - return Array.from(this.sessions.values()).sort( - (a, b) => b.lastAccessedAt.getTime() - a.lastAccessedAt.getTime() - ); + return this.getSessionsSorted(); } private enforceMaxSessions(): void { if (this.sessions.size <= this.maxSessions) return; - const sessions = this.listSessions(); + const sessions = this.getSessionsSorted(); const sessionsToDelete = sessions.slice(this.maxSessions);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/file-storage.ts` around lines 350 - 359, enforceMaxSessions currently calls listSessions(), which invokes cleanupExpiredSessions again and causes redundant work when callers (e.g., createSession) already cleaned up; modify enforceMaxSessions to avoid that extra pass by either iterating this.sessions directly (use this.sessions.values()/entries() to build sorted list and then delete extras) or add a new internal helper like listSessionsWithoutCleanup() that mirrors listSessions() logic but does not call cleanupExpiredSessions(); update references in enforceMaxSessions to use the direct iteration or new helper and keep function name enforceMaxSessions unchanged.
165-168: Note:getCodexConversationIddoes not updatelastAccessedAt.Unlike
getSession, this method doesn't update the LRU timestamp. This appears to be intentional to matchInMemorySessionStoragebehavior, but worth documenting if this is the intended design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/session/file-storage.ts` around lines 165 - 168, getCodexConversationId reads a session without updating its LRU timestamp, which diverges from getSession and can break last-accessed semantics; update the session's lastAccessedAt the same way getSession does (set session.lastAccessedAt = Date.now() and persist it back into this.sessions) before returning session.codexConversationId, or if omission is intentional to match InMemorySessionStorage, add a clear JSDoc comment above getCodexConversationId explaining the deliberate behavior and rationale referencing InMemorySessionStorage for future maintainers.src/__tests__/file-storage.test.ts (2)
475-489: Permission-based tests may not work on Windows.Tests using
chmodSyncto simulate permission errors rely on POSIX semantics and will pass differently (or may not simulate failures correctly) on Windows. This is acceptable for CI running on Linux but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/file-storage.test.ts` around lines 475 - 489, The permission-based test 'flush should reject when write fails' uses chmodSync to simulate POSIX permission errors which won't behave reliably on Windows; update the test to skip or short-circuit on Windows (process.platform === 'win32') so it only runs on POSIX CI. Locate the test case for FileSessionStorage (test name "flush should reject when write fails") and add a platform guard at the top (or convert to test.skip when process.platform === 'win32') before calling mkdtempSync/tmpdir/chmodSync to avoid false failures on Windows.
506-506: Timing-based wait may be flaky in slow CI environments.Using
setTimeoutwith a fixed 100ms delay to wait for background save completion could be flaky under load. Consider using a more deterministic approach if this test becomes unstable.♻️ Alternative: Poll for expected state
// Poll until console.error is called or timeout const waitForError = async (spy: jest.SpyInstance, timeoutMs = 500) => { const start = Date.now(); while (Date.now() - start < timeoutMs) { if (spy.mock.calls.length > 0) return; await new Promise(r => setTimeout(r, 10)); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/file-storage.test.ts` at line 506, Replace the flaky fixed 100ms sleep used to wait for background save (the line using await new Promise(r => globalThis.setTimeout(r, 100))) with a deterministic polling helper that checks the test spy (e.g., the console.error or relevant jest.SpyInstance used in this test) until it has been called or a short timeout elapses; implement a helper like waitForError(spy, timeoutMs) that loops checking spy.mock.calls.length and awaits a small delay (e.g., 10ms) between checks, then call that helper in place of the one-shot setTimeout to ensure the test waits for actual completion rather than an arbitrary sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/session/file-storage.ts`:
- Around line 36-37: FileSessionStorage's sessionIdPattern
(/^[^\x00-\x1f\x7f]+$/) is laxer than InMemorySessionStorage's
(/^[a-zA-Z0-9_-]+$/) and can produce session IDs incompatible with the in-memory
backend; update the FileSessionStorage.sessionIdPattern to the stricter
/^[a-zA-Z0-9_-]+$/ (same as InMemorySessionStorage) so both storage
implementations validate session IDs identically, and run/adjust any unit tests
that assert session ID formats for the FileSessionStorage and
InMemorySessionStorage classes.
---
Nitpick comments:
In `@src/__tests__/file-storage.test.ts`:
- Around line 475-489: The permission-based test 'flush should reject when write
fails' uses chmodSync to simulate POSIX permission errors which won't behave
reliably on Windows; update the test to skip or short-circuit on Windows
(process.platform === 'win32') so it only runs on POSIX CI. Locate the test case
for FileSessionStorage (test name "flush should reject when write fails") and
add a platform guard at the top (or convert to test.skip when process.platform
=== 'win32') before calling mkdtempSync/tmpdir/chmodSync to avoid false failures
on Windows.
- Line 506: Replace the flaky fixed 100ms sleep used to wait for background save
(the line using await new Promise(r => globalThis.setTimeout(r, 100))) with a
deterministic polling helper that checks the test spy (e.g., the console.error
or relevant jest.SpyInstance used in this test) until it has been called or a
short timeout elapses; implement a helper like waitForError(spy, timeoutMs) that
loops checking spy.mock.calls.length and awaits a small delay (e.g., 10ms)
between checks, then call that helper in place of the one-shot setTimeout to
ensure the test waits for actual completion rather than an arbitrary sleep.
In `@src/session/file-storage.ts`:
- Around line 350-359: enforceMaxSessions currently calls listSessions(), which
invokes cleanupExpiredSessions again and causes redundant work when callers
(e.g., createSession) already cleaned up; modify enforceMaxSessions to avoid
that extra pass by either iterating this.sessions directly (use
this.sessions.values()/entries() to build sorted list and then delete extras) or
add a new internal helper like listSessionsWithoutCleanup() that mirrors
listSessions() logic but does not call cleanupExpiredSessions(); update
references in enforceMaxSessions to use the direct iteration or new helper and
keep function name enforceMaxSessions unchanged.
- Around line 165-168: getCodexConversationId reads a session without updating
its LRU timestamp, which diverges from getSession and can break last-accessed
semantics; update the session's lastAccessedAt the same way getSession does (set
session.lastAccessedAt = Date.now() and persist it back into this.sessions)
before returning session.codexConversationId, or if omission is intentional to
match InMemorySessionStorage, add a clear JSDoc comment above
getCodexConversationId explaining the deliberate behavior and rationale
referencing InMemorySessionStorage for future maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11793858-23b5-4c73-babd-1b3104afe8a6
📒 Files selected for processing (2)
src/__tests__/file-storage.test.tssrc/session/file-storage.ts
| // eslint-disable-next-line no-control-regex | ||
| private readonly sessionIdPattern = /^[^\x00-\x1f\x7f]+$/; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check InMemorySessionStorage session ID validation pattern
rg -n 'sessionIdPattern|[a-zA-Z0-9_-]' --type ts -C3Repository: tuannvm/codex-mcp-server
Length of output: 50381
🏁 Script executed:
# Get the exact content of file-storage.ts around lines 36-37
head -50 src/session/file-storage.ts | tail -20Repository: tuannvm/codex-mcp-server
Length of output: 726
🏁 Script executed:
# Search for InMemorySessionStorage and its sessionIdPattern
fd -e ts . src/session/ | xargs grep -l "InMemorySessionStorage"Repository: tuannvm/codex-mcp-server
Length of output: 90
🏁 Script executed:
# Get exact pattern from storage.ts
rg -n "sessionIdPattern" src/session/ -A 1 -B 1Repository: tuannvm/codex-mcp-server
Length of output: 841
Unify session ID validation patterns across storage implementations.
The session ID validation differs between FileSessionStorage (line 37: /^[^\x00-\x1f\x7f]+$/) and InMemorySessionStorage (line 37: /^[a-zA-Z0-9_-]+$/). The file-based storage permits any non-control characters while the in-memory storage is restricted to alphanumerics, hyphens, and underscores. Sessions created in FileSessionStorage with characters like spaces or punctuation would fail validation if the backend is switched to InMemorySessionStorage, breaking interoperability.
Consider standardizing both implementations to the stricter pattern (/^[a-zA-Z0-9_-]+$/) to prevent data compatibility issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/session/file-storage.ts` around lines 36 - 37, FileSessionStorage's
sessionIdPattern (/^[^\x00-\x1f\x7f]+$/) is laxer than InMemorySessionStorage's
(/^[a-zA-Z0-9_-]+$/) and can produce session IDs incompatible with the in-memory
backend; update the FileSessionStorage.sessionIdPattern to the stricter
/^[a-zA-Z0-9_-]+$/ (same as InMemorySessionStorage) so both storage
implementations validate session IDs identically, and run/adjust any unit tests
that assert session ID formats for the FileSessionStorage and
InMemorySessionStorage classes.
2 participants
Summary
FileSessionStorageclass that persists sessions to a JSON file (~/.codex-mcp/sessions.json)createSessionStorage()factory with env var controls for backwards compatibilityFeatures
0o600(owner-only read/write).tmpfiles cleaned up on startupEnvironment Variables
CODEX_MCP_MEMORY_ONLY=true: fall back to in-memory storage (existing behavior)CODEX_MCP_SESSION_FILE=/path/to/file.json: custom storage file pathChanges
src/session/file-storage.ts: NewFileSessionStorageclass implementingSessionStorageinterfacesrc/__tests__/file-storage.test.ts: 22 tests covering CRUD, persistence, error recovery, TTL/LRU, atomic writes, schema loadingsrc/tools/handlers.ts:createSessionStorage()factory function replaces hardcodedInMemorySessionStorageREADME.md: Document new environment variables and concurrency noteTest plan
npm run buildpassesnpm run lintpassesnpm run format:checkpassesnpm test— all 93 tests pass (71 existing + 22 new)CODEX_MCP_MEMORY_ONLY=truefalls back to in-memory storage🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests