fix: improve string escaping in toTsLiteral function for better compatibility#298
Merged
frontegg-david merged 6 commits intorelease/1.0.xfrom Mar 19, 2026
Merged
Conversation
…session state persistence
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes MCP initialization payload persistence via a new helper, updates unit test expectations in session utilities, and tightens string escaping in the schema type generator to preserve backslashes in TypeScript literals. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InitHandler as InitializeRequestHandler
participant PersistHelper as persistInitPayload
participant SessionStore as SessionPayloadStore
participant Transport as TransportAdapter
Client->>InitHandler: send initialize request (clientInfo / sessionIdPayload)
InitHandler->>PersistHelper: call persistInitPayload(data)
PersistHelper->>SessionStore: updateSessionPayload(data)
PersistHelper->>Transport: setInitSessionPayload(data)
PersistHelper-->>InitHandler: ack
InitHandler-->>Client: respond (sessionId not propagated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
frontegg-david
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts (3)
106-142: Minor duplication in transport persistence logic.Both branches (with and without
clientInfo) follow the same pattern:
- Build
initPayload- Call
updateSessionPayload- Persist on transport via
setInitSessionPayloadConsider extracting the common persistence logic into a helper to reduce duplication.
💡 Potential refactor
// Helper function for persistence function persistInitPayload( sessionId: string, initPayload: Record<string, unknown>, transport?: { setInitSessionPayload: (p: Record<string, unknown>) => void } ): void { updateSessionPayload(sessionId, initPayload); transport?.setInitSessionPayload(initPayload); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts` around lines 106 - 142, Extract the duplicated persistence steps into a small helper (e.g., persistInitPayload) that takes sessionId, initPayload, and an optional transport and calls updateSessionPayload(sessionId, initPayload) and transport?.setInitSessionPayload(initPayload); then replace the duplicated blocks in initialize-request.handler.ts (both branches that currently build initPayload and call updateSessionPayload / transport?.setInitSessionPayload) with a single call to that helper to remove duplication while keeping existing behavior.
115-115: Consider using the new session ID returned byupdateSessionPayload.
updateSessionPayloadnow returns the new encrypted session ID containing the updated payload, but this return value is ignored. While both old and new session IDs are cached, consider whether the new session ID should be propagated to ensure clients use the updated session ID in subsequent requests.If ignoring the return value is intentional (relying on dual caching), a comment explaining this would improve clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts` at line 115, The call to updateSessionPayload(sessionId, initPayload) currently ignores its returned new encrypted session ID; either capture that return value and propagate it (e.g., replace cached sessionId and ensure it is included in subsequent responses to clients) so clients use the updated session ID, or if the dual-caching approach is intentional, add an explicit comment next to the call explaining why the returned new session ID is safely ignored; reference updateSessionPayload, sessionId, and initPayload when making the change.
121-122: Type cast to access transport suggests a type definition gap.The cast
(ctx.authInfo as SdkAuthInfo)?.transportis used twice to access the transport property. Ifctx.authInfoshould include thetransportproperty, consider updating the type definition forauthInfoin the context type to include it, avoiding runtime type assertions.Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts` around lines 121 - 122, Update the context type so ctx.authInfo is typed to include the transport property (i.e., extend the Context/AuthInfo interface to reference SdkAuthInfo or a transport-bearing type) instead of using ad-hoc casts; then remove the inline casts `(ctx.authInfo as SdkAuthInfo)?.transport` in the places where you call transport?.setInitSessionPayload(initPayload) and the later usage (lines showing setInitSessionPayload and the second occurrence), ensuring calls reference ctx.authInfo.transport directly and that the transport type exposes setInitSessionPayload.libs/sdk/src/auth/session/__tests__/session-id.utils.spec.ts (2)
240-241: Consider asserting the returned session ID type.The assertion
expect(updated).not.toBeNull()verifies the update succeeded, but doesn't verify the return value is actually a string (the new session ID). Consider addingexpect(typeof updated).toBe('string')to fully validate the new return type contract.💡 Suggested enhancement
- expect(updated).not.toBeNull(); + expect(updated).not.toBeNull(); + expect(typeof updated).toBe('string');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/auth/session/__tests__/session-id.utils.spec.ts` around lines 240 - 241, The test currently only asserts that `updated` is not null; add an assertion that the returned session ID is a string by checking `typeof updated === 'string'` (i.e., add `expect(typeof updated).toBe('string')`) immediately after the existing `expect(updated).not.toBeNull()` in the test that sets `updated` to validate the return type contract for the session ID.
281-289: Test name is now inconsistent with actual behavior.The test is named "should return false for non-existent session" but the function now returns
null, notfalse. Consider updating the test name for clarity.📝 Suggested name fix
- it('should return false for non-existent session', () => { + it('should return null for non-existent session', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/auth/session/__tests__/session-id.utils.spec.ts` around lines 281 - 289, The test title is inconsistent with the asserted behavior: update the test description for the spec that calls updateSessionPayload (the one using mockSafeDecrypt) from "should return false for non-existent session" to something like "should return null for non-existent session" so it matches the actual return value (the assertion expect(result).toBeNull()); ensure the string passed to the it(...) call is updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/sdk/src/auth/session/__tests__/session-id.utils.spec.ts`:
- Around line 240-241: The test currently only asserts that `updated` is not
null; add an assertion that the returned session ID is a string by checking
`typeof updated === 'string'` (i.e., add `expect(typeof
updated).toBe('string')`) immediately after the existing
`expect(updated).not.toBeNull()` in the test that sets `updated` to validate the
return type contract for the session ID.
- Around line 281-289: The test title is inconsistent with the asserted
behavior: update the test description for the spec that calls
updateSessionPayload (the one using mockSafeDecrypt) from "should return false
for non-existent session" to something like "should return null for non-existent
session" so it matches the actual return value (the assertion
expect(result).toBeNull()); ensure the string passed to the it(...) call is
updated accordingly.
In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts`:
- Around line 106-142: Extract the duplicated persistence steps into a small
helper (e.g., persistInitPayload) that takes sessionId, initPayload, and an
optional transport and calls updateSessionPayload(sessionId, initPayload) and
transport?.setInitSessionPayload(initPayload); then replace the duplicated
blocks in initialize-request.handler.ts (both branches that currently build
initPayload and call updateSessionPayload / transport?.setInitSessionPayload)
with a single call to that helper to remove duplication while keeping existing
behavior.
- Line 115: The call to updateSessionPayload(sessionId, initPayload) currently
ignores its returned new encrypted session ID; either capture that return value
and propagate it (e.g., replace cached sessionId and ensure it is included in
subsequent responses to clients) so clients use the updated session ID, or if
the dual-caching approach is intentional, add an explicit comment next to the
call explaining why the returned new session ID is safely ignored; reference
updateSessionPayload, sessionId, and initPayload when making the change.
- Around line 121-122: Update the context type so ctx.authInfo is typed to
include the transport property (i.e., extend the Context/AuthInfo interface to
reference SdkAuthInfo or a transport-bearing type) instead of using ad-hoc
casts; then remove the inline casts `(ctx.authInfo as SdkAuthInfo)?.transport`
in the places where you call transport?.setInitSessionPayload(initPayload) and
the later usage (lines showing setInitSessionPayload and the second occurrence),
ensuring calls reference ctx.authInfo.transport directly and that the transport
type exposes setInitSessionPayload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9103250-e17f-4b27-89a3-c8c0becbfe5b
📒 Files selected for processing (7)
apps/e2e/demo-e2e-elicitation/e2e/elicitation.e2e.spec.tslibs/sdk/package.jsonlibs/sdk/src/auth/session/__tests__/session-id.utils.spec.tslibs/sdk/src/auth/session/utils/session-id.utils.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.tsscripts/generate-schema-types.mjs
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 100 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-03-19T01:08:42.234Z |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts (1)
19-28: Consider stronger typing forinitPayloadparameter.The
initPayloadparameter is typed asPartial<Record<string, unknown>>, which is very loose. SinceupdateSessionPayloadexpectsPartial<SessionIdPayload>, using a more specific type would provide better compile-time safety and prevent invalid properties from being passed.♻️ Suggested type improvement
+import type { SessionIdPayload } from '../../auth/session/utils/session-id.utils'; + function persistInitPayload( sessionId: string, - initPayload: Partial<Record<string, unknown>>, + initPayload: Partial<SessionIdPayload>, ctx: { authInfo?: unknown }, ): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts` around lines 19 - 28, The initPayload param of persistInitPayload is too loose (Partial<Record<string, unknown>>) and should be tightened to match the shape expected by updateSessionPayload; change the type to Partial<SessionIdPayload> (or a union/partial of the concrete payload interface used by updateSessionPayload) so TypeScript enforces valid keys, and update any call sites accordingly; ensure imports/types for SessionIdPayload are available in the file and keep ctx handling (SdkAuthInfo?.transport.setInitSessionPayload) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts`:
- Around line 19-28: The initPayload param of persistInitPayload is too loose
(Partial<Record<string, unknown>>) and should be tightened to match the shape
expected by updateSessionPayload; change the type to Partial<SessionIdPayload>
(or a union/partial of the concrete payload interface used by
updateSessionPayload) so TypeScript enforces valid keys, and update any call
sites accordingly; ensure imports/types for SessionIdPayload are available in
the file and keep ctx handling (SdkAuthInfo?.transport.setInitSessionPayload)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ff3372b-978b-48f2-a8fd-5443b8105632
📒 Files selected for processing (2)
libs/sdk/src/auth/session/__tests__/session-id.utils.spec.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.ts
Contributor
Cherry-pick CreatedA cherry-pick PR to Please review and merge if this change should also be in If the cherry-pick is not needed, close the PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests