feat: Implement Redis session store for transport recreation and enha… #108
Conversation
…nce session management
WalkthroughAdds Redis-backed transport session persistence and recreation, on-disk dev JWKS key persistence with validation and atomic I/O, converts JWKS accessors to async with init guards, validates mcp-session-id headers in transport flows, and updates CLI scaffolding (.gitignore, test script, tsconfig types). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Flow as Transport Flow
participant Registry as Transport Registry
participant Memory as In-Memory Cache
participant Redis as Redis Session Store
participant Transporter
Client->>Flow: Request + mcp-session-id
Flow->>Flow: validateMcpSessionHeader()
alt invalid header provided
Flow-->>Client: 404 sessionNotFound("invalid session id")
else
Flow->>Registry: getTransporter(sessionId)
Registry->>Memory: lookup
alt found
Memory-->>Registry: Transporter
else not found
Registry->>Registry: check local history
alt history indicates created
Registry->>Redis: getStoredSession(type, token, sessionId)
Redis-->>Registry: StoredSession
Registry->>Registry: recreateTransporter(storedSession) [mutex]
Registry->>Transporter: instantiate from StoredSession
Transporter-->>Registry: ready
Registry->>Memory: cache Transporter
else not created
Registry-->>Flow: no transporter
end
end
Registry-->>Flow: Transporter or error
Flow->>Transporter: handle request
Transporter-->>Client: response
end
sequenceDiagram
autonumber
participant Caller
participant Jwks as JwksService
participant DevFS as Dev Key Persistence
participant FS as File System
participant Crypto as Crypto Lib
Caller->>Jwks: request public JWKS
Jwks->>Jwks: initializeOrchestratorKey() (guarded)
Jwks->>DevFS: isDevKeyPersistenceEnabled()
alt enabled
DevFS-->>Jwks: true
Jwks->>DevFS: loadDevKey()
DevFS->>FS: read file
FS-->>DevFS: JSON
DevFS->>DevFS: validate (Zod)
alt valid
DevFS-->>Jwks: DevKeyData
else invalid/missing
DevFS-->>Jwks: null
Jwks->>Crypto: generate key
Crypto-->>Jwks: key + jwk
Jwks->>DevFS: saveDevKey(keyData)
DevFS->>FS: atomic write (temp -> rename, 0o600)
FS-->>DevFS: saved
end
else disabled
DevFS-->>Jwks: false
Jwks->>Crypto: generate ephemeral key
end
Jwks-->>Caller: public JWKS (awaited)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
libs/{sdk,adapters,plugins,cli}/src/**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
libs/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-11-05T15:00:47.800ZApplied to files:
🧬 Code graph analysis (1)libs/sdk/src/transport/transport.registry.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/common/types/options/auth.options.ts (1)
269-315: Add documentation for the newtransportRecreationConfigfeature indocs/draft/docs/servers/server.mdx.The
transportRecreationConfigSchemais exported as a public API (with typesTransportRecreationConfigandTransportRecreationConfigInput), but the feature is not documented. Add a section under "Transport controls" describing how to enable Redis-backed session persistence and transport recreation after server restart, including therecreationobject structure (enabled flag, Redis configuration) and default behavior.
♻️ Duplicate comments (1)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
20-54: Duplicate validation code - see previous comment.Same schema and helper duplicated from the SSE flow. Extract to a shared module as noted in the SSE flow review.
🧹 Nitpick comments (13)
libs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts (1)
429-448: Permission tests may fail on Windows.The file permission tests (0o600, 0o700) are POSIX-specific and will likely fail on Windows systems where the permission model differs.
Consider conditionally skipping these tests on Windows:
const isWindows = process.platform === 'win32'; it.skipIf(isWindows)('should set file permissions to 0o600', async () => { // ... });Or use
describe.skipIf(isWindows)if the entire test suite is POSIX-only.libs/sdk/src/auth/jwks/dev-key-persistence.ts (1)
121-125: Missingkidconsistency validation between DevKeyData and public JWKS.The function validates that key types match between public and private keys, but doesn't verify that
parsed.kidmatchespublicKey.kid. Inconsistentkidvalues could cause token verification failures.// Verify public key matches private key algorithm const publicKey = parsed.publicJwk.keys[0]; if (publicKey.kty !== parsed.privateKey.kty) { return { valid: false, error: 'Public and private key types do not match' }; } + + // Verify kid consistency + if (publicKey.kid !== parsed.kid) { + return { valid: false, error: 'Key ID (kid) does not match between DevKeyData and public JWKS' }; + }libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
20-54: Consider extracting shared header validation to a common module.The
mcpSessionHeaderSchemaandvalidateMcpSessionHeaderare duplicated verbatim inhandle.streamable-http.flow.ts. Extract these to a shared utility (e.g.,libs/sdk/src/transport/utils/session-header.utils.ts) to reduce duplication and ensure consistent validation across transport flows.-/** - * Zod schema for validating mcp-session-id header format. - * Uses Zod's built-in validators to prevent ReDoS attacks and ensure safe validation. - * - * - Max length: 256 characters (session IDs are typically UUIDs or short tokens) - * - Only allows printable ASCII characters (0x20-0x7E) - * - Rejects control characters and null bytes - */ -const mcpSessionHeaderSchema = z - .string() - .min(1) - .max(256) - .refine( - (value) => { - // Check each character is printable ASCII (0x20-0x7E) - for (let i = 0; i < value.length; i++) { - const code = value.charCodeAt(i); - if (code < 0x20 || code > 0x7e) { - return false; - } - } - return true; - }, - { message: 'Session ID must contain only printable ASCII characters' }, - ); - -/** - * Validate mcp-session-id header using Zod schema. - * Returns undefined for invalid or missing values. - */ -function validateMcpSessionHeader(value: string | undefined): string | undefined { - if (!value) return undefined; - const result = mcpSessionHeaderSchema.safeParse(value); - return result.success ? result.data : undefined; -} +import { validateMcpSessionHeader } from '../utils/session-header.utils';libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (2)
238-253: Consider adding error handling around transport recreation.If
recreateTransporterthrows (e.g., Redis connection issues, corrupt session data), the error propagates without context. Wrapping in try-catch would allow logging the recreation failure while still falling through to the "session not found" path gracefully.if (!transport) { const storedSession = await transportService.getStoredSession('streamable-http', token, session.id); if (storedSession) { - logger.info('Recreating transport from Redis session', { - sessionId: session.id?.slice(0, 20), - createdAt: storedSession.createdAt, - }); - transport = await transportService.recreateTransporter( - 'streamable-http', - token, - session.id, - storedSession, - response, - ); + try { + logger.info('Recreating transport from Redis session', { + sessionId: session.id?.slice(0, 20), + createdAt: storedSession.createdAt, + }); + transport = await transportService.recreateTransporter( + 'streamable-http', + token, + session.id, + storedSession, + response, + ); + } catch (error) { + logger.error('Failed to recreate transport from Redis session', { + sessionId: session.id?.slice(0, 20), + error: error instanceof Error ? error.message : String(error), + }); + // Fall through to session-not-found handling + } } }
317-332: Same error handling concern applies here.The recreation logic in
onSseListenermirrorsonMessageand would benefit from the same try-catch pattern for consistent error handling and logging.libs/sdk/src/auth/session/transport-session.types.ts (2)
227-236: ValidatedefaultTtlMsas a sane positive TTL (and align type-vs-schema expectations).
defaultTtlMsis documented as TTL, butz.number()will accept negatives/decimals; considerz.number().int().positive()(and maybe an upper bound). Also note: the interface hasdefaultTtlMs?: numberwhileredisConfigSchemawill always output a defined number due to.default(...); that mismatch can confuse consumers of parsed config.
335-343: Consider tighteningredisConfigSchema.defaultTtlMsconstraints (int/positive) to prevent bad runtime TTLs.libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
9-27: Jest mock shape is fine, but consider typing the mock Redis instance (avoidas never).libs/sdk/src/transport/__tests__/transport.registry.test.ts (2)
88-141: Add an assertion that a failed Redis ping triggersdisconnect()(and store is disabled).The implementation logs and disconnects on ping failure; the test checks the log but not the disconnect side-effect.
214-229: If possible, assert Redis persistence details (key + TTL) not just “set called”.libs/sdk/src/auth/session/redis-session.store.ts (1)
117-137: TTL handling logic is clear; consider validatingttlMsis finite int (defensive).libs/sdk/src/transport/transport.registry.ts (2)
229-240: Avoid mutating thekeyparameter.Line 231 mutates
key.sessionId = sessionIdinside the dispose callback. WhilekeyOf()creates new objects, this mutation pattern is fragile and could cause issues if the key reference is used elsewhere after transporter creation.The mutation appears intentional for eviction, but consider creating a local copy to make the intent clearer:
// Create new transport - const transporter = new LocalTransporter(this.scope, key, res, () => { - key.sessionId = sessionId; - this.evictLocal(key); + const evictionKey = { ...key, sessionId }; + const transporter = new LocalTransporter(this.scope, key, res, () => { + this.evictLocal(evictionKey);
548-579: Eviction may not free memory if all sessions are active.The eviction logic correctly skips sessions with active transports, but if
sessionHistory.size > MAX_SESSION_HISTORYand most/all entries have active transports, the cache will continue growing unbounded. This is a rare edge case but could occur under sustained high concurrency.Consider adding a fallback or logging when eviction fails to meet target:
if (!hasActiveTransport) { this.sessionHistory.delete(histKey); evicted++; } } + + if (evicted < targetEvictions && this.sessionHistory.size > this.MAX_SESSION_HISTORY) { + this.scope.logger.warn('[TransportService] Session history eviction limited by active transports', { + historySize: this.sessionHistory.size, + targetEvictions, + actualEvictions: evicted, + }); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignore(1 hunks)libs/cli/src/commands/create.ts(5 hunks)libs/cli/src/tsconfig.ts(1 hunks)libs/sdk/src/auth/flows/well-known.jwks.flow.ts(2 hunks)libs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts(1 hunks)libs/sdk/src/auth/jwks/dev-key-persistence.ts(1 hunks)libs/sdk/src/auth/jwks/index.ts(1 hunks)libs/sdk/src/auth/jwks/jwks.service.ts(11 hunks)libs/sdk/src/auth/jwks/jwks.types.ts(2 hunks)libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts(1 hunks)libs/sdk/src/auth/session/index.ts(1 hunks)libs/sdk/src/auth/session/redis-session.store.ts(1 hunks)libs/sdk/src/auth/session/transport-session.manager.ts(2 hunks)libs/sdk/src/auth/session/transport-session.types.ts(2 hunks)libs/sdk/src/common/types/options/auth.options.ts(2 hunks)libs/sdk/src/scope/scope.instance.ts(1 hunks)libs/sdk/src/transport/__tests__/transport.registry.test.ts(1 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(2 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(4 hunks)libs/sdk/src/transport/transport.registry.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/auth/jwks/index.tslibs/cli/src/tsconfig.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/jwks/jwks.types.tslibs/sdk/src/common/types/options/auth.options.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/transport-session.manager.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/index.tslibs/cli/src/commands/create.tslibs/sdk/src/auth/flows/well-known.jwks.flow.tslibs/sdk/src/auth/jwks/jwks.service.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/auth/jwks/index.tslibs/cli/src/tsconfig.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/jwks/jwks.types.tslibs/sdk/src/common/types/options/auth.options.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/transport-session.manager.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/index.tslibs/cli/src/commands/create.tslibs/sdk/src/auth/flows/well-known.jwks.flow.tslibs/sdk/src/auth/jwks/jwks.service.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/auth/jwks/index.tslibs/cli/src/tsconfig.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/jwks/jwks.types.tslibs/sdk/src/common/types/options/auth.options.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/transport-session.manager.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/index.tslibs/cli/src/commands/create.tslibs/sdk/src/auth/flows/well-known.jwks.flow.tslibs/sdk/src/auth/jwks/jwks.service.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts
🧠 Learnings (13)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/index.ts : Use barrel exports (index.ts) to export all public API surface without legacy exports or aliases
Applied to files:
libs/sdk/src/auth/jwks/index.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/auth/jwks/index.tslibs/sdk/src/auth/jwks/jwks.types.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/auth/flows/well-known.jwks.flow.tslibs/sdk/src/auth/jwks/jwks.service.ts
📚 Learning: 2025-11-16T22:47:05.139Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 50
File: docs/docs/v/0.1/getting-started/welcome.mdx:2-3
Timestamp: 2025-11-16T22:47:05.139Z
Learning: In the agentfront/frontmcp repository, the docs/docs/v/** directory governance rule prohibiting manual edits applies to already-frozen releases, but does not preclude initial setup and bootstrap commits that establish the versioned docs structure.
Applied to files:
.gitignore
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Enable strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaults
Applied to files:
libs/cli/src/tsconfig.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Follow the preset pattern for hierarchical configurations across the codebase
Applied to files:
libs/cli/src/tsconfig.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use FrontMCP's TypeScript-first schema validation framework philosophy - all types should align with MCP protocol definitions
Applied to files:
libs/cli/src/tsconfig.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/common/records/**/*.ts : Centralize record types in common/records and import from there instead of from module-specific files
Applied to files:
libs/cli/src/tsconfig.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/cli/src/tsconfig.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.tslibs/cli/src/commands/create.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Use type parameters with constraints instead of unconstrained generics, and prefer `unknown` over `any` for generic type defaults
Applied to files:
libs/cli/src/tsconfig.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead of `unknown` for `execute()` and `read()` methods
Applied to files:
libs/cli/src/tsconfig.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Use `changeScope` instead of `scope` for change event properties to avoid confusion with the Scope class
Applied to files:
libs/sdk/src/scope/scope.instance.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.ts
🧬 Code graph analysis (7)
libs/sdk/src/scope/scope.instance.ts (1)
libs/sdk/src/transport/transport.registry.ts (1)
TransportService(22-591)
libs/sdk/src/transport/__tests__/transport.registry.test.ts (2)
libs/sdk/src/auth/session/redis-session.store.ts (1)
key(49-51)libs/sdk/src/transport/transport.registry.ts (1)
TransportService(22-591)
libs/sdk/src/auth/jwks/jwks.types.ts (1)
libs/sdk/src/auth/jwks/dev-key-persistence.ts (1)
DevKeyPersistenceOptions(27-38)
libs/sdk/src/common/types/options/auth.options.ts (1)
libs/sdk/src/auth/session/transport-session.types.ts (1)
redisConfigSchema(335-343)
libs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts (2)
libs/sdk/src/auth/jwks/dev-key-persistence.ts (4)
isDevKeyPersistenceEnabled(143-153)resolveKeyPath(158-168)DevKeyPersistenceOptions(27-38)DevKeyData(11-22)libs/sdk/src/auth/jwks/jwks.types.ts (1)
DevKeyPersistenceOptions(18-18)
libs/sdk/src/transport/transport.registry.ts (5)
libs/sdk/src/auth/session/redis-session.store.ts (2)
RedisSessionStore(12-190)key(49-51)libs/sdk/src/common/types/options/auth.options.ts (1)
TransportRecreationConfigInput(686-686)libs/sdk/src/transport/transport.types.ts (3)
Transporter(51-63)TransportType(7-7)TransportKey(9-15)libs/sdk/src/auth/session/transport-session.types.ts (1)
StoredSession(153-164)libs/sdk/src/transport/transport.local.ts (1)
LocalTransporter(12-78)
libs/sdk/src/auth/session/redis-session.store.ts (1)
libs/sdk/src/auth/session/transport-session.types.ts (4)
SessionStore(189-214)RedisConfig(227-236)StoredSession(153-164)storedSessionSchema(327-333)
🔇 Additional comments (40)
.gitignore (1)
54-58: LGTM! Security-conscious .gitignore additions.The new entries appropriately exclude FrontMCP development keys, caches, and coverage directories. The explanatory comment on line 55 is helpful and aligns with the TEMPLATE_GITIGNORE being added to CLI scaffolding.
libs/cli/src/tsconfig.ts (1)
29-29: LGTM! Correctly adds test-related types.The addition of '@types/jest' and '@frontmcp/testing' to the recommended TypeScript configuration properly supports the new test scaffolding infrastructure added in create.ts.
libs/cli/src/commands/create.ts (3)
56-56: LGTM! Test infrastructure additions are well-aligned.The test script and @types/jest dependency correctly support the new test scaffolding. The merge logic on line 102 follows the established pattern of preferring existing values while providing sensible defaults.
Also applies to: 76-76, 102-102
213-250: LGTM! Comprehensive and well-structured .gitignore template.The template covers all essential exclusions (dependencies, build artifacts, IDE/OS files, secrets, FrontMCP keys, coverage, logs) with clear organization. The security comment on line 242 correctly emphasizes that FrontMCP keys should never be committed.
345-346: LGTM! Good addition to project scaffolding.Scaffolding a .gitignore file ensures new projects start with proper Git exclusions out of the box, reducing the risk of accidentally committing sensitive data or build artifacts.
libs/sdk/src/auth/jwks/index.ts (1)
3-3: LGTM!The barrel export follows the established pattern and correctly exposes the new dev-key-persistence module as part of the public API surface. Based on learnings, this aligns with the guidance to use barrel exports for public API without legacy aliases.
libs/sdk/src/auth/jwks/jwks.types.ts (1)
11-18: LGTM!The new
devKeyPersistenceoption is well-documented with clear JSDoc comments explaining its purpose and scope (development mode only). The type re-export pattern is appropriate for making the type available to consumers.libs/sdk/src/auth/flows/well-known.jwks.flow.ts (2)
78-85: LGTM!The orchestrated branch correctly awaits the now-async
getPublicJwks()and maintains proper error handling for unavailable JWKS.
96-104: LGTM!The non-orchestrated branch also correctly awaits the async call and preserves the existing behavior of returning a 404 when JWKS is invalid.
libs/sdk/src/auth/jwks/jwks.service.ts (5)
240-263: Race condition handling looks correct.The promise guard pattern effectively prevents concurrent key generation. The
finallyblock correctly clears the promise after initialization completes, allowing future rotation cycles.
265-297: Dev key persistence integration is well-implemented.The flow correctly:
- Checks if persistence is enabled before attempting to load
- Validates loaded key's algorithm matches configuration
- Handles key reconstruction failures gracefully with fallback to generation
- Logs appropriate warnings for mismatches
Based on learnings, the development-only key persistence is an intentional design for local development convenience.
299-316: Consider handlingsaveDevKeyfailure more explicitly.When
saveDevKeyreturnsfalse(line 313), the code logs a warning but continues. This is acceptable for development mode, but consider whether this should be more visible or configurable.The current behavior silently degrades - keys work but won't persist. Verify this aligns with the intended UX for development mode.
188-197: LGTM!The orchestrator key methods correctly await
ensureOrchestratorKey()before accessing the key material, maintaining consistency with the new async initialization flow.
44-46: All callers ofgetPublicJwks()have been properly updated.The two production usages in
libs/sdk/src/auth/flows/well-known.jwks.flow.ts(lines 79 and 98) are correctly awaited. The test fixture inlibs/testing/src/fixtures/test-fixture.ts(line 190) is also correctly implemented withgetJwks: () => factory.getPublicJwks(), which properly wraps the async call.libs/sdk/src/auth/jwks/dev-key-persistence.ts (7)
1-40: Well-structured interfaces and imports.The interface definitions are clear with good JSDoc documentation. The security warning on
forceEnableis appropriate. Using the nativeJsonWebKeytype for portability andJSONWebKeySetfrom jose for the public JWKS is a sound design choice.
42-100: Zod schemas are correctly structured for JWK validation.The RSA schema correctly requires the essential private key components (n, e, d) while making CRT parameters optional. The EC schema correctly requires all necessary components. Using
.passthrough()allows for additional standard JWK fields. The validation is appropriate for a dev-only persistence layer.
140-153: Good security pattern for environment-aware enablement.Disabling persistence by default in production and requiring explicit opt-in (
forceEnable) is the correct approach. This aligns with the intended dev-only use case. Based on learnings, development-only convenience features are acceptable in this codebase.
155-168: LGTM!Path resolution logic is correct - absolute paths are used as-is, and relative paths are resolved from the current working directory.
170-205: Well-implemented async load with proper error handling.The function correctly handles all edge cases: disabled persistence, missing file (first run), and invalid format. The ENOENT check for graceful first-run handling and the validation before returning data are good practices.
207-249: Excellent atomic write implementation with proper security.The atomic write pattern (temp file + rename) prevents corruption during writes. Setting restrictive permissions (0o700 for directory, 0o600 for file) is the correct approach for private key storage. The random suffix on temp files prevents collisions in concurrent scenarios.
256-267: Verify: intentionally skippingisDevKeyPersistenceEnabledcheck?Unlike
loadDevKeyandsaveDevKey, this function doesn't checkisDevKeyPersistenceEnabled()before attempting deletion. This allows key deletion regardless of environment, which could be intentional for cleanup purposes but creates an API inconsistency. Consider either:
- Adding the check for consistency, or
- Documenting that deletion is always permitted (e.g., for cleanup scripts in production)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
124-125: LGTM!The header validation integration is clean. Invalid headers are safely treated as absent, preserving the existing fallback logic without breaking changes.
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
255-283: LGTM!The change to
wasSessionCreatedAsynccorrectly supports async Redis-backed session existence checks. The differentiated error handling (expired vs. never-created) with appropriate logging is well-implemented.libs/sdk/src/auth/session/index.ts (1)
4-4: LGTM!Clean addition of
RedisSessionStoreto the public API exports, following the existing pattern forInMemorySessionStore.libs/sdk/src/scope/scope.instance.ts (1)
87-89: LGTM!Clean integration of transport recreation configuration. The optional chaining (
transportConfig?.recreation) safely handles cases where auth/transport config is not provided, and the extracted config is correctly passed toTransportService.libs/sdk/src/auth/session/transport-session.manager.ts (1)
114-116: Correct integration ofRedisSessionStorewith proper type safety. Theconfig.configparameter (typed asRedisConfigfrom the discriminatedSessionStorageConfigunion) matches theRedisSessionStoreconstructor's first union variant exactly, ensuring no type compatibility issues. The conditional instantiation based on thestorediscriminant is clean and type-safe.libs/sdk/src/common/types/options/auth.options.ts (1)
682-687: Nice: public types exported for the new schema.libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
139-151: Good coverage of GETEX path, TTL usage, and owned-vs-external disconnect semantics.Also applies to: 227-264, 387-404
libs/sdk/src/transport/__tests__/transport.registry.test.ts (1)
219-229: Mutex-based concurrency tests are a solid regression net.Also applies to: 381-415
libs/sdk/src/auth/session/redis-session.store.ts (2)
160-167: Owned-vs-external connection lifecycle is handled correctly.
18-44: GETEX support is correctly implemented with proper version compatibility.The
getex(key, 'PX', this.defaultTtlMs)signature at line 68 matches ioredis 5.8.0's API exactly. GETEX requires Redis 6.2.0+, and the code gracefully handles older versions with a fallback to.get()at line 71. TLS configuration at lines 37–39 is valid—passing an emptytlsobject enables default TLS behavior in ioredis. No runtime dependency issues exist.libs/sdk/src/transport/transport.registry.ts (9)
57-81: LGTM!Constructor properly initializes Redis session store conditionally and sets appropriate defaults. The distinct key prefix
mcp:transport:differentiates from the general session store prefix.
83-99: LGTM!Good graceful degradation pattern - validates Redis connection on startup and falls back to in-memory only if Redis is unavailable, preventing silent failures on subsequent operations.
140-158: LGTM!Good security practice: validates token hash before returning stored session and logs only truncated identifiers. Returning
undefinedon mismatch is the correct safe behavior.
171-201: LGTM!Mutex implementation correctly prevents concurrent recreation races. The
finallyblock ensures mutex cleanup even on failure, and waiting callers properly receive the result or error from the pending creation.
329-349: LGTM!Session persistence correctly constructs the
StoredSessionwith required fields. Fire-and-forget with error logging is appropriate here since session persistence is not critical to the request flow.
459-471: Consider token validation in async session check.
wasSessionCreatedAsyncchecks only ifsessionIdexists in Redis without validating the token hash. WhilegetStoredSessiondoes validate, this method could returntruefor sessions created by different tokens with the same session ID.For the HTTP 400 vs 404 differentiation use case, this might be acceptable, but consider documenting this behavior or adding token validation for consistency.
Is the intent that
wasSessionCreatedAsynconly answers "does this session exist?" without validating ownership? If so, consider adding a doc comment clarifying this distinction fromgetStoredSession.
479-501: LGTM!JSON encoding for history keys properly handles session IDs containing special characters. Parse validation is robust with graceful failure handling.
266-295: LGTM!Mutex protection pattern is consistent with
recreateTransporter. Proper cleanup infinallyblock ensures no mutex leaks.
101-107: LGTM!Proper cleanup of Redis connection in
destroy()method.
…lidation for sessionId
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
89-105: Return HTTP 404 whenmcp-session-idheader is present but fails validation, per user requirement.The current code silently treats an invalid header as absent, falling back to
authorization.session. However, the user requirement (documented inhttp-output.schema.ts) explicitly states: "Invalid or missing session should return HTTP 404". When a client sends a session ID that fails validation, this should fail fast withhttpRespond.sessionNotFound()rather than falling back, since the client explicitly referenced a session (even if malformed).Additionally,
request.headers?.['mcp-session-id']could theoretically be a non-string value; narrow it withtypeof raw === 'string'before passing to the validator for safety.- const rawMcpSessionHeader = request.headers?.['mcp-session-id'] as string | undefined; + const raw = request.headers?.['mcp-session-id']; + const rawMcpSessionHeader = typeof raw === 'string' ? raw : undefined; const mcpSessionHeader = validateMcpSessionHeader(rawMcpSessionHeader); + // If client sent a header but validation failed, return 404 per user requirement + if (raw !== undefined && !mcpSessionHeader) { + this.respond(httpRespond.sessionNotFound('invalid session id')); + return; + }libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
91-115: Use type-safe header extraction instead of type assertions.The code uses
as string | undefinedto extract request headers, which hides potential non-string values (HTTP headers can bestring[],string, orundefined). Instead, checktypeofbefore treating the value as a string. Additionally, invalid session IDs fromvalidateMcpSessionHeadercurrently fall back silently—present-but-invalid headers are treated identically to missing headers, with no explicit error handling.- const rawMcpSessionHeader = request.headers?.['mcp-session-id'] as string | undefined; - const mcpSessionHeader = validateMcpSessionHeader(rawMcpSessionHeader); + const raw = request.headers?.['mcp-session-id']; + const rawMcpSessionHeader = typeof raw === 'string' ? raw : undefined; + const mcpSessionHeader = validateMcpSessionHeader(rawMcpSessionHeader); + + // Optional: if (rawMcpSessionHeader && !mcpSessionHeader) { ...handle invalid ID... }Same pattern also appears at line 111 for the
user-agentheader.
♻️ Duplicate comments (1)
libs/sdk/src/auth/session/redis-session.store.ts (1)
69-82: GETEX may extend Redis TTL beyond session'sexpiresAt– consider bounding.The current implementation uses
defaultTtlMsunconditionally for GETEX, which could extend the Redis key's TTL past the session's logicalexpiresAt. While the application-level check at lines 100-106 catches this, the key remains in Redis longer than necessary.To bound the TTL extension, compute the TTL after parsing:
try { - // GETEX with EXAT/PXAT is atomic - no race condition possible - raw = await this.redis.getex(key, 'PX', this.defaultTtlMs); + // First get the value + raw = await this.redis.get(key); } catch { - // Fallback for older Redis versions that don't support GETEX raw = await this.redis.get(key); } if (!raw) return null; try { const parsed = JSON.parse(raw); const result = storedSessionSchema.safeParse(parsed); + // ... validation ... + + // Extend TTL (bounded by expiresAt if set) + const ttlMs = session.session.expiresAt + ? Math.min(this.defaultTtlMs, session.session.expiresAt - Date.now()) + : this.defaultTtlMs; + if (ttlMs > 0) { + this.redis.pexpire(key, ttlMs).catch(() => void 0); + }This trades atomicity for correctness. Alternatively, use a Lua script for atomic bounded TTL extension.
🧹 Nitpick comments (9)
libs/sdk/src/auth/jwks/dev-key-persistence.ts (1)
102-143: Consider marking internal exports explicitly.The validation logic is thorough and correct, with appropriate checks for algorithm/key-type consistency and timestamp bounds. However, since the AI summary indicates this is an "internal export, not public," consider making this explicit using JSDoc
@internalor a naming convention (e.g.,_validateJwkStructure).Add JSDoc annotation to clarify internal usage:
/** * Validate JWK structure based on algorithm + * @internal */ function validateJwkStructure(data: unknown): { valid: boolean; error?: string } {libs/sdk/src/auth/session/transport-session.types.ts (2)
227-236: Avoid RedisConfig “optional + default” mismatch; introduce an inferred “resolved” type (and consider trimming host).
Right nowRedisConfigmarks several fields optional, butredisConfigSchemawill always produce them after parse (defaults). That tends to leakRedisConfiginto places that actually require resolved config.export interface RedisConfig { host: string; port?: number; password?: string; db?: number; tls?: boolean; keyPrefix?: string; /** Default TTL in milliseconds for session extension on access (sliding expiration) */ defaultTtlMs?: number; } + +// Use this for post-parse / internal code paths that rely on defaults being present. +export type ResolvedRedisConfig = z.infer<typeof redisConfigSchema>;And if you want to reject whitespace-only hosts (currently allowed by
min(1)):-export const redisConfigSchema = z.object({ - host: z.string().min(1), +export const redisConfigSchema = z.object({ + host: z.string().trim().min(1), port: z.number().int().positive().optional().default(6379), password: z.string().optional(), db: z.number().int().nonnegative().optional().default(0), tls: z.boolean().optional().default(false), keyPrefix: z.string().optional().default('mcp:session:'), defaultTtlMs: z.number().int().positive().optional().default(3600000), // 1 hour default });Also applies to: 335-343
234-235: Clarify whether sliding TTL can be disabled (e.g., allowdefaultTtlMs = 0).
If “always extend on access” is intentional, consider documenting thatdefaultTtlMsmust be > 0; otherwise, consider allowing0as “no sliding expiration”.- defaultTtlMs: z.number().int().positive().optional().default(3600000), // 1 hour default + // Consider allowing 0 to disable sliding expiration if needed by deployments. + defaultTtlMs: z.number().int().nonnegative().optional().default(3600000), // 1 hour defaultAlso applies to: 342-343
libs/sdk/src/common/schemas/session-header.schema.ts (1)
11-27: Consider rejecting leading/trailing whitespace inmcp-session-id.
Since space is allowed by the printable-ASCII check," abc"and"abc "would currently validate but likely won’t match stored IDs.export const mcpSessionHeaderSchema = z .string() .min(1) .max(256) + .refine((v) => v === v.trim(), { message: 'Session ID must not have leading/trailing whitespace' }) .refine( (value) => { // Check each character is printable ASCII (0x20-0x7E) for (let i = 0; i < value.length; i++) { const code = value.charCodeAt(i); if (code < 0x20 || code > 0x7e) { return false; } } return true; }, { message: 'Session ID must contain only printable ASCII characters' }, );libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (2)
108-114: Type assertionas neveris a workaround – consider typing the mock properly.Using
as neverto pass the mock Redis instance bypasses type checking. While this works for tests, it would be cleaner to type the mock to match the expected interface.Consider creating a properly typed mock:
const mockRedisInstance: Partial<Redis> = { get: jest.fn(), getex: jest.fn(), // ... other methods };Then use
as Redisinstead ofas neverfor better type safety in tests.
479-499: Concurrent operation tests use single mock response – consider varying data.Both concurrent get and set tests use the same mock session data for all concurrent calls. This validates parallelism but doesn't detect potential issues where concurrent operations might interfere with each other's data.
For more robust concurrency testing, consider varying the session data:
it('should handle concurrent get requests', async () => { const sessions = ['session1', 'session2', 'session3'].map(id => { const s = createValidStoredSession(); s.session.id = id; return s; }); mockRedisInstance.getex .mockResolvedValueOnce(JSON.stringify(sessions[0])) .mockResolvedValueOnce(JSON.stringify(sessions[1])) .mockResolvedValueOnce(JSON.stringify(sessions[2])); // ... });libs/sdk/src/auth/session/redis-session.store.ts (1)
90-95: Schema validation error handling is correct but consider logging the validation error.When schema validation fails, the session is deleted. Consider logging the actual validation errors for debugging:
if (!result.success) { - this.logger?.warn('[RedisSessionStore] Invalid session format', { sessionId: sessionId.slice(0, 20) }); + this.logger?.warn('[RedisSessionStore] Invalid session format', { + sessionId: sessionId.slice(0, 20), + errors: result.error.issues.slice(0, 3), // First few issues for debugging + });libs/sdk/src/transport/transport.registry.ts (2)
341-361: Session persistence uses fire-and-forget – silent failures possible.The
sessionStore.set()call uses.catch()to log errors but doesn't propagate them. While this prevents blocking the transport creation, it means sessions may silently fail to persist to Redis.Consider emitting a metric or event on persistence failure to enable monitoring:
sessionStore.set(sessionId, storedSession, recreationConfig?.defaultTtlMs).catch((err) => { this.scope.logger.warn('[TransportService] Failed to persist session to Redis', { sessionId: sessionId.slice(0, 20), error: err instanceof Error ? err.message : String(err), }); + // Emit metric for monitoring + this.scope.metrics?.increment('transport.session.persist_failure'); });
503-513:parseHistoryKey()defined but only used in eviction – verify necessity.The helper is well-implemented but only used in the eviction loop at line 572. If this is the only usage, consider inlining it or documenting its intended broader use.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
libs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts(1 hunks)libs/sdk/src/auth/jwks/dev-key-persistence.ts(1 hunks)libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts(1 hunks)libs/sdk/src/auth/session/redis-session.store.ts(1 hunks)libs/sdk/src/auth/session/transport-session.types.ts(2 hunks)libs/sdk/src/common/schemas/index.ts(1 hunks)libs/sdk/src/common/schemas/session-header.schema.ts(1 hunks)libs/sdk/src/common/types/options/auth.options.ts(2 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(2 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(4 hunks)libs/sdk/src/transport/transport.registry.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/auth/jwks/tests/dev-key-persistence.test.ts
- libs/sdk/src/common/types/options/auth.options.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/common/schemas/index.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/common/schemas/session-header.schema.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/common/schemas/index.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/common/schemas/session-header.schema.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/schemas/index.tslibs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/auth/session/transport-session.types.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/common/schemas/session-header.schema.tslibs/sdk/src/auth/jwks/dev-key-persistence.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts
🧠 Learnings (11)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/*/src/index.ts : Use barrel exports (index.ts) to export all public API surface without legacy exports or aliases
Applied to files:
libs/sdk/src/common/schemas/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/** : Organize components into schema.ts (definitions) and implementation .ts files with matching names
Applied to files:
libs/sdk/src/common/schemas/index.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Use HTMX schema with strict mode including get, post, put, delete, target, swap, and trigger properties
Applied to files:
libs/sdk/src/common/schemas/index.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use FrontMCP's TypeScript-first schema validation framework philosophy - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/common/schemas/session-header.schema.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/auth/session/redis-session.store.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
Applied to files:
libs/sdk/src/common/schemas/session-header.schema.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead of `unknown` for `execute()` and `read()` methods
Applied to files:
libs/sdk/src/common/schemas/session-header.schema.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/components/**/*.schema.ts : Every component must have a Zod schema with `.strict()` mode to reject unknown properties
Applied to files:
libs/sdk/src/common/schemas/session-header.schema.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/auth/jwks/dev-key-persistence.ts
🧬 Code graph analysis (3)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
libs/sdk/src/common/schemas/session-header.schema.ts (1)
validateMcpSessionHeader(33-37)
libs/sdk/src/auth/session/redis-session.store.ts (1)
libs/sdk/src/auth/session/transport-session.types.ts (4)
SessionStore(189-214)RedisConfig(227-236)StoredSession(153-164)storedSessionSchema(327-333)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
libs/sdk/src/common/schemas/session-header.schema.ts (1)
validateMcpSessionHeader(33-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (19)
libs/sdk/src/auth/jwks/dev-key-persistence.ts (6)
1-38: Well-structured type definitions and imports.The type definitions are clear, properly documented, and follow TypeScript best practices. The restriction to specific algorithms (RS256/ES256) and the warning about
forceEnablein production are good defensive measures.
40-100: Comprehensive Zod validation schemas.The validation schemas correctly model JWK structures per RFC 7517. The use of
.passthrough()is appropriate for JWK objects, and the union type forprivateKeyproperly handles both RSA and EC keys.
145-173: Environment-aware enablement and correct path resolution.The environment-based enablement correctly restricts persistence to development by default, with an explicit opt-in for production. The path resolution properly handles both absolute and relative paths. Based on learnings, dev-only behavior is intentional in this codebase.
175-210: Solid implementation with appropriate error handling.The function correctly handles missing files, invalid formats, and read errors by returning
null. The type assertion after Zod validation is safe, and the ENOENT handling recognizes that a missing file is normal on first run.
212-254: Excellent atomic write implementation with strong security measures.The atomic write pattern (temp file + rename) prevents corruption, and the restrictive permissions (0o600 for files, 0o700 for directories) properly protect sensitive key material. The cleanup logic gracefully handles temp file deletion errors.
256-272: Clean deletion logic with appropriate error handling.The function correctly treats ENOENT as a non-error case and logs other failures without throwing. Note that it doesn't check
isDevKeyPersistenceEnabled, which means it can delete keys regardless of current persistence settings—this appears intentional for cleanup purposes.libs/sdk/src/common/schemas/index.ts (1)
1-4: Good: session-header schema is exported via the schemas barrel.
This keeps the public surface discoverable/consistent. Based on learnings, this aligns with the repo’s barrel-export convention.libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
220-247: Nice: asyncwasSessionCreatedAsyncaligns with Redis-backed existence checks.
This looks like the right direction for correctness once sessions can outlive in-memory transports.libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (3)
178-187: Good coverage for logical expiration handling.The test correctly validates that logically expired sessions (where
expiresAtis in the past) are deleted and returnnull. This properly tests the application-level expiration separate from Redis TTL.
431-438: Empty sessionId rejection test properly addresses prior review concern.This test validates that all CRUD operations reject empty sessionIds with a specific error message, preventing the prefix-only key issue flagged in prior reviews.
565-582: Test helper creates valid sessions aligned with schema.The
createValidStoredSession()helper correctly constructs sessions matching theStoredSessioninterface andstoredSessionSchema, including all required fields.libs/sdk/src/auth/session/redis-session.store.ts (4)
56-61: Empty sessionId validation properly implemented.The
key()method now validates that sessionId is not empty or whitespace-only, throwing a descriptive error. This addresses the prior review concern about operating on prefix-only keys.
134-149: TTL logic inset()is well-structured.The method correctly handles three cases: explicit TTL, TTL derived from
expiresAt, and no TTL. The comment at line 143 clarifies that already-expired sessions are stored for cleanup on next access.
176-180: Disconnect correctly handles owned vs external Redis instances.The
externalInstanceflag prevents closing connections that were passed in externally, which is the correct behavior for dependency injection patterns.
155-164:delete()andexists()correctly delegate sessionId validation to thekey()guard, which throws on empty or whitespace-only values.Both methods call
this.key(sessionId), which validates that sessionId is non-empty (lines 57–59). Tests confirm this behavior: empty sessionId properly throws[RedisSessionStore] sessionId cannot be empty(line 431–438). The design is sound—validation at the guard level prevents invalid keys from reaching Redis operations.libs/sdk/src/transport/transport.registry.ts (4)
86-99: Good graceful degradation on Redis connection failure.The initialization correctly validates the Redis connection via
ping()and nullifiessessionStoreon failure, allowing the service to continue with in-memory-only operation. The error is properly logged.
143-161: Token hash verification ingetStoredSession()prevents session hijacking.The method correctly verifies that the stored
authorizationIdmatches the request's token hash before returning the session. This prevents a malicious actor from guessing session IDs to hijack sessions.
187-211: Mutex implementation correctly handles concurrent recreation requests.The mutex pattern using a
Map<string, Promise<Transporter>>correctly serializes concurrent recreation requests for the same session. The JSON-encoded key handles colons in sessionIds as noted in prior reviews.
560-600: Improved eviction logic prevents removing entries with active transports.The eviction now correctly skips entries that have active transports, preventing inconsistent state where history says a session existed but it's actually still active. The warning log at lines 593-599 helps identify memory pressure issues.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
206-233: Recreation error handling implemented correctly.The try-catch wrapper ensures failed recreations fall through to the existing 404 logic. The mutex protection against concurrent recreation is handled at the
transportService.recreateTransporter()level (viacreationMutexin transport.registry.ts lines 187-211), so no additional coordination is needed here.libs/sdk/src/transport/transport.registry.ts (2)
471-485: Token validation correctly implemented via getStoredSession.The method now uses
getStoredSession()(line 480) which validates the token hash before confirming session existence (lines 150-158 in getStoredSession), addressing the security concern from previous reviews.
51-55: Update comment to reflect JSON-encoded mutex keys.The comment at line 53 states the mutex key format is
${type}:${tokenHash}:${sessionId}, but the actual implementation uses JSON encoding (lines 189, 291). This was correctly updated to match the history key format.Apply this diff to fix the comment:
/** * Mutex map for preventing concurrent transport creation for the same key. - * Key: `${type}:${tokenHash}:${sessionId}`, Value: Promise that resolves when creation completes + * Key: JSON-encoded {t: type, h: tokenHash, s: sessionId}, Value: Promise that resolves when creation completes */ private readonly creationMutex: Map<string, Promise<Transporter>> = new Map();
🧹 Nitpick comments (3)
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
165-170: Consider asserting cleanup on corrupted JSON (and align with store behavior).
Right now this only assertsnull; if you decide corrupted JSON should be deleted (recommended), add an assertion thatdelis called (or keep as-is but then expect repeated warnings in prod).libs/sdk/src/transport/__tests__/transport.registry.test.ts (1)
272-286: Make mocked “stored session” objects consistently schema-shaped to avoid brittle tests.
E.g., includesession.authorizationId,session.createdAt,session.nodeId, etc., wherever you’re simulating a Redis StoredSession.libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
293-330: Consider consistent error differentiation in SSE listener.The SSE listener flow uses
httpRespond.notFoundwhen no transport is available (line 323), whereasonMessagedistinguishes between "session expired" and "session not initialized" usingwasSessionCreatedAsync(lines 237-261). This inconsistency could provide less useful feedback to SSE clients.Consider aligning the error handling:
if (!transport) { - this.respond(httpRespond.notFound('Session not found')); + const wasCreated = await transportService.wasSessionCreatedAsync('streamable-http', token, session.id); + if (wasCreated) { + this.respond(httpRespond.sessionExpired('session expired')); + } else { + this.respond(httpRespond.sessionNotFound('session not initialized')); + } return; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts(1 hunks)libs/sdk/src/auth/session/redis-session.store.ts(1 hunks)libs/sdk/src/common/schemas/session-header.schema.ts(1 hunks)libs/sdk/src/transport/README.md(1 hunks)libs/sdk/src/transport/__tests__/transport.registry.test.ts(1 hunks)libs/sdk/src/transport/flows/handle.sse.flow.ts(2 hunks)libs/sdk/src/transport/flows/handle.streamable-http.flow.ts(4 hunks)libs/sdk/src/transport/transport.registry.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/common/schemas/session-header.schema.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/transport/transport.registry.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/transport/transport.registry.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.tslibs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/README.mdlibs/sdk/src/transport/__tests__/transport.registry.test.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/transport/transport.registry.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/__tests__/transport.registry.test.ts
🧠 Learnings (5)
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to libs/{sdk,adapters,plugins,cli}/src/**/*.ts : Don't mutate rawInput in flows - use state.set() for managing flow state instead
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Use FrontMCP's TypeScript-first schema validation framework philosophy - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/transport/flows/handle.sse.flow.tslibs/sdk/src/transport/flows/handle.streamable-http.flow.ts
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/__tests__/transport.registry.test.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/auth/session/redis-session.store.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/transport/transport.registry.ts
🧬 Code graph analysis (4)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
libs/sdk/src/common/schemas/session-header.schema.ts (1)
validateMcpSessionHeader(36-40)
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (1)
libs/sdk/src/common/schemas/session-header.schema.ts (1)
validateMcpSessionHeader(36-40)
libs/sdk/src/transport/__tests__/transport.registry.test.ts (2)
libs/sdk/src/auth/session/redis-session.store.ts (1)
key(56-61)libs/sdk/src/transport/transport.registry.ts (1)
TransportService(22-615)
libs/sdk/src/transport/transport.registry.ts (3)
libs/sdk/src/auth/session/redis-session.store.ts (2)
RedisSessionStore(13-216)key(56-61)libs/sdk/src/transport/transport.types.ts (3)
Transporter(51-63)TransportType(7-7)TransportKey(9-15)libs/sdk/src/auth/session/transport-session.types.ts (1)
StoredSession(153-164)
🪛 markdownlint-cli2 (0.18.1)
libs/sdk/src/transport/README.md
215-215: Reference links and images should use a label that is defined
Missing link or image reference definition: "1"
(MD052, reference-links-images)
288-288: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (16)
libs/sdk/src/transport/flows/handle.sse.flow.ts (1)
89-122: Header validation + precedence logic look solid.
Rejecting malformedmcp-session-idearly and using the validated value for lookup reduces ambiguity and prevents weird cache keys.libs/sdk/src/auth/session/redis-session.store.ts (1)
69-83: No action required. ioredis 5.8.0 includes full TypeScript support forgetex()with proper overloads covering the'PX'token pattern used here. The callthis.redis.getex(key, 'PX', this.defaultTtlMs)is properly typed and poses no risk to TS consumers. The fallback toget()correctly handles Redis versions prior to 6.2.0 which don't support GETEX.Likely an incorrect or invalid review comment.
libs/sdk/src/transport/flows/handle.streamable-http.flow.ts (2)
91-99: LGTM! Header validation is secure and correct.The validation correctly distinguishes between missing headers and invalid headers, responding with 404 only when an invalid header is provided. This prevents processing requests with malformed session identifiers.
237-237: LGTM! Correct usage of async session check.Using
wasSessionCreatedAsynchere allows checking session existence across server restarts via Redis, which aligns with the restoration flow logic.libs/sdk/src/transport/transport.registry.ts (12)
14-14: LGTM! Imports are appropriate for Redis session functionality.Also applies to: 19-20
57-82: LGTM! Constructor properly initializes Redis session store.The conditional initialization ensures Redis is only set up when explicitly configured, with sensible defaults and proper logging.
86-100: Excellent graceful degradation for Redis failures.The ping validation during initialization ensures Redis connectivity, and the nullification of
sessionStoreon failure enables clean degradation to in-memory-only mode. This prevents silent failures throughout the service lifecycle.
104-110: LGTM! Proper Redis cleanup in destroy.
112-132: LGTM! Clear documentation of Redis restoration flow.The comment correctly indicates that Redis-stored sessions require explicit recreation via
recreateTransporter(), maintaining clear separation between lookup and recreation operations.
134-161: LGTM! Token hash validation prevents unauthorized session access.The method correctly validates the token hash before returning stored session data, preventing information leakage to unauthorized callers.
163-212: LGTM! Mutex-based concurrency control prevents recreation races.The mutex ensures only one recreation occurs per transport key, with proper cleanup in finally block and informative error logging.
214-275: LGTM! Recreation flow maintains session consistency.The double-check pattern, history preservation with original creation time, and asynchronous Redis updates ensure proper recreation without blocking the response path.
277-307: LGTM! Consistent mutex pattern for transport creation.The mutex protection aligns with the
recreateTransporterimplementation, preventing concurrent creation races.
309-368: LGTM! Redis persistence is non-blocking and properly scoped.The Redis persistence is correctly scoped to streamable-http transports, with async error handling that doesn't block transport creation. The dispose callback ensures cleanup on transport destruction.
493-515: LGTM! Robust history key utilities handle edge cases.The JSON encoding/decoding with explicit type validation and error handling correctly handles sessionIds containing special characters.
553-604: Improved eviction logic prevents inconsistent state.The enhanced eviction ensures history entries are only removed when no active transport exists, preventing race conditions. The warning log (lines 596-602) alerts operators if the eviction target cannot be met due to all sessions having active transports.
Note: If most sessions remain active, the history could grow beyond MAX_SESSION_HISTORY. The warning log provides visibility into this condition, allowing operators to adjust MAX_SESSION_HISTORY or investigate long-lived sessions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
435-442: Empty sessionId validation now properly tested.The test suite now correctly verifies that empty
sessionIdis rejected across all operations (get, set, delete, exists), throwing a specific error message. This addresses the previous concern about prefix-only Redis keys and aligns with the guard implemented inredis-session.store.ts.libs/sdk/src/transport/transport.registry.ts (2)
143-161: Token hash verification correctly prevents unauthorized access.The
getStoredSession()method properly validates the token hash against the storedauthorizationId(line 151) before returning session data. This prevents unauthorized callers from checking session existence and addresses the previous security concern.
471-485: Async session check correctly validates token.The
wasSessionCreatedAsync()method properly usesgetStoredSession()(line 480) instead of rawsessionStore.exists(), ensuring token validation before confirming session existence. This prevents information leakage to unauthorized callers and addresses the previous security concern.libs/sdk/src/transport/README.md (1)
288-320: Add language identifier to code fence for markdownlint compliance.The flow diagram code fence lacks a language identifier, which will cause markdownlint MD040 to fail. Additionally, if reference links like
[1]are used elsewhere in the document without definitions, MD052 will fail.Apply this diff to add the language identifier:
-``` +```text Client Request with mcp-session-id: "abc123" │ ▼
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts(1 hunks)libs/sdk/src/auth/session/redis-session.store.ts(1 hunks)libs/sdk/src/transport/README.md(1 hunks)libs/sdk/src/transport/transport.registry.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Enable strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use specific error classes with MCP error codes instead of generic errors
Use type parameters with constraints instead of unconstrained generics, and preferunknownoveranyfor generic type defaults
Follow the preset pattern for hierarchical configurations across the codebase
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/redis-session.store.ts
**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts: Achieve and maintain 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including errors, constructor validation, and error classinstanceofchecks
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts
libs/{sdk,adapters,plugins,cli}/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/{sdk,adapters,plugins,cli}/src/**/*.ts: Return strictly typed MCP protocol responses (GetPromptResult, ReadResourceResult, etc.) instead ofunknownforexecute()andread()methods
Validate URIs per RFC 3986 at metadata level using Zod validation with custom refinements
UsegetCapabilities()for dynamic capability exposure instead of hardcoding capabilities in adapters
UsechangeScopeinstead ofscopefor change event properties to avoid confusion with the Scope class
Validate hooks match their entry type and fail fast with InvalidHookFlowError for unsupported flows
Don't mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/redis-session.store.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.tslibs/sdk/src/transport/transport.registry.tslibs/sdk/src/auth/session/redis-session.store.tslibs/sdk/src/transport/README.md
🧠 Learnings (3)
📚 Learning: 2025-11-26T15:23:04.965Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-11-26T15:23:04.965Z
Learning: Applies to libs/ui/src/**/*.test.ts : Test behavior across platform configurations (OpenAI, Claude, Gemini, ngrok) where applicable
Applied to files:
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts
📚 Learning: 2025-11-05T15:00:47.800Z
Learnt from: frontegg-david
Repo: agentfront/frontmcp PR: 11
File: libs/core/src/auth/jwks/jwks.service.ts:62-0
Timestamp: 2025-11-05T15:00:47.800Z
Learning: In the FrontMCP codebase (libs/core/src/auth/jwks/jwks.service.ts), the verifyGatewayToken method intentionally skips signature verification when running in development-only no-auth mode, using decodeJwtPayloadSafe instead of jwtVerify. This is a deliberate design for local development convenience and should not be flagged as a security issue when the PR or code context indicates development/no-auth mode.
Applied to files:
libs/sdk/src/transport/transport.registry.ts
📚 Learning: 2025-12-01T00:33:33.644Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-01T00:33:33.644Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/auth/session/redis-session.store.ts
🧬 Code graph analysis (1)
libs/sdk/src/auth/session/redis-session.store.ts (1)
libs/sdk/src/auth/session/transport-session.types.ts (4)
SessionStore(189-214)RedisConfig(227-236)StoredSession(153-164)storedSessionSchema(327-333)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
libs/sdk/src/transport/README.md (2)
343-352: Mutex scope now clarified as per-process.The "Mutex protection" row now explicitly states "(per-process)", which addresses the previous concern about scope ambiguity. This correctly indicates that the mutex is process-local and does not provide cross-node mutual exclusion.
219-375: Well-documented Redis recreation section.The new "Transport Recreation with Redis" section provides comprehensive documentation covering:
- Clear motivation and problem statement
- Detailed data structure documentation
- Security justification with specific measures (SHA-256 hashing, AES-256-GCM encryption)
- Visual flow diagram
- Configuration examples
- Clear limitations and error handling
The documentation aligns well with the implementation in
transport.registry.tsandredis-session.store.ts.libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
1-586: Comprehensive test coverage for RedisSessionStore.The test suite provides excellent coverage of the RedisSessionStore implementation:
- Constructor variants and configuration options
- All CRUD operations (get, set, delete, exists)
- Atomic TTL extension via GETEX
- Schema validation and error handling
- Edge cases (empty IDs, colons, large payloads, concurrency)
- Utility methods (allocId, ping, disconnect, getRedisClient)
The test structure follows best practices with clear describe blocks, proper mocking, and comprehensive assertions. This aligns with the 95%+ coverage goal specified in coding guidelines.
libs/sdk/src/auth/session/redis-session.store.ts (2)
111-119: TTL bounding logic correctly prevents stale keys.The implementation correctly bounds the Redis TTL by
session.expiresAtto prevent keeping expired sessions in Redis beyond their application-level expiration. The fire-and-forgetpexpirecall (line 117) is appropriate here since this is an optimization—GETEX already extended the TTL atomically, and we're just shortening it if needed.
1-218: Well-implemented Redis session store with proper safeguards.The
RedisSessionStoreimplementation demonstrates solid engineering:Strengths:
- Empty
sessionIdvalidation prevents prefix-only key collisions- Atomic GETEX with TTL extension prevents race conditions
- TTL bounding by
expiresAtprevents stale keys (lines 111-119)- Corrupted data deletion prevents poison pills (lines 96, 137)
- External vs owned Redis instance handling for proper lifecycle
- Comprehensive error handling with structured logging
- Schema validation using Zod
Addressed past concerns:
- Empty sessionId rejection (line 57-59)
- TTL extension bounded by expiresAt (lines 111-119)
- Logger injection instead of console.warn (lines 91, 132)
- Corrupted JSON deletion (line 137)
The implementation aligns well with coding guidelines (no
anytypes, proper error handling, no non-null assertions) and integrates cleanly with the transport recreation flow.libs/sdk/src/transport/transport.registry.ts (4)
187-211: Mutex protection correctly prevents recreation races.The mutex implementation (lines 187-211) properly prevents concurrent recreation of the same transport. The JSON-encoded mutex key handles
sessionIdvalues containing colons, addressing the previous concern about key collisions.The double-check pattern at lines 224-225 ensures no duplicate transport is created even if multiple requests acquire the mutex sequentially.
341-361: Session persistence properly integrated into creation flow.The session persistence logic (lines 341-361) is well-designed:
- Only persists for
streamable-httptype (line 342)- Includes
nodeIdfromgetMachineId()for multi-node tracking (line 349)- Uses fire-and-forget with error logging to avoid blocking the request (lines 355-360)
- Applies configurable TTL from
recreationConfig.defaultTtlMs(line 355)This allows graceful degradation if Redis operations fail while maintaining session creation flow.
564-603: Conservative eviction logic correctly prevents state corruption.The updated eviction logic (lines 564-603) properly protects active sessions:
- Only evicts history entries without active transports (lines 587-591)
- Safely handles malformed history keys (lines 575-580)
- Logs warning when eviction target cannot be met (lines 594-602)
This conservative approach correctly prioritizes consistency over memory efficiency—it's better to exceed
MAX_SESSION_HISTORYthan to corrupt the session state by evicting active transport history.
1-615: Redis-backed transport recreation well-integrated.The
TransportServicechanges successfully integrate Redis-backed session persistence for transport recreation:Key improvements:
- Mutex-protected creation/recreation prevents races (JSON-encoded keys handle special characters)
- Token hash validation in
getStoredSession()andwasSessionCreatedAsync()prevents unauthorized access- Graceful degradation on Redis connection failure (lines 90-95)
- Conservative eviction logic protects active sessions (lines 564-603)
- Fire-and-forget Redis updates with error logging avoid blocking requests
Security measures:
- Token hash verification before returning stored sessions (line 151)
- Session-specific cleanup on dispose (lines 248-250, 332-334)
- Proper separation of concerns (getStoredSession vs recreateTransporter)
The implementation aligns with coding guidelines and properly addresses past review concerns about key format and token validation.
…nce session management
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.