Register sessions before RPC to prevent dropped events

[stephentoub]

Summary

Register sessions in the client's sessions map before issuing the \session.create\ and \session.resume\ RPC calls, so that events emitted by the CLI during the RPC (e.g. \session.start, permission requests, tool calls) are not dropped. Previously, sessions were registered only after the RPC completed, creating a window where notifications for the session had no target.

Changes

Across all four SDKs (Node.js, Python, Go, .NET):

• Generate \sessionId\ client-side (via UUID) rather than extracting it from the server response
• Create and register the session in the sessions map before the RPC
• *Set \workspacePath* from the RPC response after it completes
• Clean up the session from the map if the RPC fails

Tests

12 new unit tests across Node.js, Python, and Go:

Test	Node.js	Python	Go
Session in map during \session.create\ RPC	✅	✅	✅
Session in map during \session.resume\ RPC	✅	✅	✅
Cleanup on \session.create\ RPC failure	✅	✅	✅
Cleanup on \session.resume\ RPC failure	✅	✅	✅

Go tests use a fake JSON-RPC server via \io.Pipe()\ to verify map state during the RPC without needing a real CLI process. Node.js and Python tests mock the RPC layer and assert the session is present in the sessions map when the RPC is invoked.

[Copilot]

Pull request overview

Adds cross-SDK unit tests to ensure sessions are inserted into the client’s sessions map before session.create / session.resume JSON-RPC requests are issued, and that failed RPCs clean up sessions correctly.

Changes:

• Python: async unit tests that monkeypatch the RPC request method and assert map state during RPC execution.
• Node.js: Vitest cases that spy on sendRequest and validate session map registration and cleanup behavior.
• Go: unit tests using an io.Pipe()-backed fake JSON-RPC server to observe sessions map state during the request.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
python/test_client.py	Adds async tests asserting session registration timing and cleanup on RPC failures.
nodejs/test/client.test.ts	Adds Vitest tests verifying session map registration before RPC completion and cleanup on failure.
go/client_test.go	Adds a pipe-backed fake JSON-RPC server and unit tests validating registration timing and cleanup in Go client.

[github-actions]

✅ Cross-SDK Consistency Review

This PR successfully adds consistent test coverage across Node.js, Python, and Go SDKs. The tests verify session registration behavior uniformly across all three implementations.

Test Coverage Analysis

All three SDKs now include identical test scenarios:

Test Scenario	Node.js	Python	Go
Session in map during session.create RPC	✅	✅	✅
Session in map during session.resume RPC	✅	✅	✅
Cleanup on session.create RPC failure	✅	✅	✅
Cleanup on session.resume RPC failure	✅	✅	✅

Implementation Consistency

The tests follow language-idiomatic patterns while maintaining semantic equivalence:

• Mocking approach: Each SDK uses its appropriate testing infrastructure:

  • Node.js: Vitest's vi.spyOn() to mock sendRequest
  • Python: Direct replacement of client._client.request method
  • Go: Custom io.Pipe()-based fake JSON-RPC server

• API naming: Consistent across languages (accounting for conventions):

  • Node: createSession() / resumeSession()
  • Python: create_session() / resume_session()
  • Go: CreateSession() / ResumeSessionWithOptions()

• Assertions: All tests verify the same behavior:

  1. Session present in map during RPC call
  2. Session removed from map after RPC failure

.NET Status

The PR description correctly notes that .NET tests are not included due to infrastructure limitations (Connection internals are private, no test injection points). This is a reasonable exception since:

1. Adding equivalent tests would require significant refactoring (e.g., InternalsVisibleTo)
2. Existing .NET E2E tests already validate session events via SessionStartEvent and SessionResumeEvent
3. The underlying behavior is tested in the CLI itself

Recommendation

No consistency issues found. This PR maintains excellent cross-SDK parity for the three SDKs where test infrastructure supports it. The .NET exclusion is well-justified and documented.

---

Generated by SDK Consistency Review Agent

AI generated by SDK Consistency Review Agent

[github-actions]

Cross-SDK Consistency Review

Thank you for this comprehensive fix across all four SDKs! The implementation is remarkably consistent across Node.js, Python, Go, and .NET - all follow the same pattern:

✅ Generate sessionId client-side (UUID)
✅ Create session before RPC
✅ Register in sessions map before RPC
✅ Set workspacePath from response
✅ Clean up on RPC failure with try/catch

However, there's one inconsistency in test coverage:

Missing .NET Tests

The PR includes comprehensive unit tests for Node.js (4 tests), Python (4 tests), and Go (4 tests), but no tests for .NET.

The following test scenarios are covered in the other SDKs but missing for .NET:

1. Session in map during session.create RPC
2. Session in map during session.resume RPC
3. Cleanup on session.create RPC failure
4. Cleanup on session.resume RPC failure

Recommendation: Add equivalent unit tests to the .NET SDK (likely in dotnet/test/ClientTest.cs or a new test file) to match the test coverage in the other SDKs. This ensures the fix is equally verified across all implementations.

AI generated by SDK Consistency Review Agent

[github-actions]

Cross-SDK Consistency Review

Thank you for this comprehensive bug fix! The changes look well-implemented across all four SDKs with consistent patterns. However, I've identified one missing test coverage issue:

✅ Implementation Consistency

The core fix is implemented consistently across all four SDKs:

1. Session ID generation: All SDKs now generate UUIDs client-side when not provided

   • Node.js: randomUUID() from node:crypto
   • Python: str(uuid.uuid4())
   • Go: uuid.New().String() from github.com/google/uuid
   • .NET: Guid.NewGuid() (implied, sessionId parameter handling)

2. Registration before RPC: All SDKs create and register the session in the sessions map before the RPC call

3. Workspace path deferred: All SDKs set workspacePath from the RPC response after it completes

4. Error cleanup: All SDKs clean up the session from the map if the RPC fails

5. Applied to both methods: Changes implemented in both createSession/CreateSession and resumeSession/ResumeSession across all SDKs

❌ Test Coverage Gap

Issue: The .NET SDK is missing the unit tests that were added to Node.js, Python, and Go.

According to the PR description, "12 new unit tests across Node.js, Python, and Go" were added (4 tests × 3 languages). However, .NET has zero new tests added.

Files modified:

• ✅ Node.js: nodejs/test/client.test.ts - 4 new tests
• ✅ Python: python/test_client.py - 4 new tests
• ✅ Go: go/client_test.go - 4 new tests (with a fakeJSONRPCServer helper)
• ❌ .NET: No test file changes

Tests that should be added to .NET:

1. Session in map during session.create RPC
2. Session in map during session.resume RPC
3. Cleanup on session.create RPC failure
4. Cleanup on session.resume RPC failure

While the implementation in dotnet/src/Client.cs appears correct, having equivalent tests in .NET would:

• Ensure the fix works as intended
• Prevent future regressions
• Maintain test parity across all SDKs

Recommendation

Consider adding the missing .NET unit tests to match the coverage provided in the other three SDKs. This would bring all SDKs to the same quality bar and ensure consistent behavior across the entire SDK suite.

AI generated by SDK Consistency Review Agent

[github-actions]

Cross-SDK Consistency Review

Thank you for this comprehensive fix across all four SDK implementations! The core functionality has been consistently implemented across Node.js, Python, Go, and .NET:

✅ Implementation Consistency

• All four SDKs now generate sessionId client-side (UUID/GUID)
• All SDKs register sessions in the map before the RPC call
• All SDKs set workspacePath from the RPC response after completion
• All SDKs clean up sessions from the map on RPC failure
• Both createSession/CreateSession and resumeSession/ResumeSession follow the same pattern consistently

⚠️ Test Coverage Gap

However, I noticed a test coverage inconsistency:

SDK	Create Session Tests	Resume Session Tests	Cleanup Tests
Node.js	✅ registers session in sessions map before session.create RPC completes	✅ registers session in sessions map before session.resume RPC completes	✅ Both create & resume cleanup tests
Python	✅ test_create_session_registers_before_rpc	✅ test_resume_session_registers_before_rpc	✅ Both create & resume cleanup tests
Go	✅ TestClient_CreateSession_RegistersSessionBeforeRPC	✅ TestClient_ResumeSession_RegistersSessionBeforeRPC	✅ Both create & resume cleanup tests
.NET	❌ Missing	❌ Missing	❌ Missing

Recommendation: Add equivalent unit tests to the .NET SDK (dotnet/test/ClientTests.cs) to verify:

1. Session is registered in _sessions dictionary before session.create RPC completes
2. Session is registered in _sessions dictionary before session.resume RPC completes
3. Session is removed from _sessions dictionary when session.create RPC fails
4. Session is removed from _sessions dictionary when session.resume RPC fails

This would bring the .NET SDK test coverage in line with the other three implementations and ensure this critical race condition fix is properly validated across all platforms.

AI generated by SDK Consistency Review Agent

[stephentoub]

@SteveSandersonMS this PR addresses the issue I raised to you yesterday, where session start/resume events weren't being delivered in the SDK because they arrived before the session was added to the sessions dictionary.

[SteveSandersonMS]

@stephentoub Excellent, thanks. Will review shortly. Interested in whether this might also address #490

[stephentoub]

@stephentoub Excellent, thanks. Will review shortly. Interested in whether this might also address #490

Hmm... I don't think it will. We can look at that separately.

[SteveSandersonMS]

@stephentoub The implementation looks great! Thanks very much for this. Only change I'm hoping for is doing the test coverage for all four languages in the existing E2E tests as it seems like it should be trivial to do that there, and will eliminate a lot of mocking and the fake JSON-RPC server.

[stephentoub]

The short answer is, I think, "no", because of the other issue I mentioned to you: the API doesn't let you register an On handler until after you get back the session instance, and by then the start event will likely have already come. For folks to successfully/reliably handle start, there needs to be a way in the API to register a handler before / as part of creating the session, e.g. being able to pass a callback as part of the session config or doing it on the client instance before creating the session.

[SteveSandersonMS]

The short answer is, I think, "no", because of the other issue I mentioned to you: the API doesn't let you register an On handler until after you get back the session instance, and by then the start event will likely have already come. For folks to successfully/reliably handle start, there needs to be a way in the API to register a handler before / as part of creating the session, e.g. being able to pass a callback as part of the session config or doing it on the client instance before creating the session.

To be sure I understand, are you saying the approach in this PR can't be covered by E2E tests because it doesn't behave in the way we want? Or that the implementation is correct and works but E2E tests don't suffice for some reason?

If it's that we can't make it work reliably in E2E tests because it's inherently a race, then I would be very open to adding a SessionConfig property where you can supply an event listener. Seems like a legit usage pattern. Or do you have a different API proposal?

[stephentoub]

I would be very open to adding a SessionConfig property where you can supply an event listener [...] Or do you have a different API proposal?

That is my proposal.

Or that the implementation is correct and works but E2E tests don't suffice for some reason?

The implementation is correct, in that the events are correctly delivered now. But a consumer and thus E2E test can't reliably listen to that event because them signing up their event listener races with it being delivered. Enabling a handler to be registered before session creation addresses that.

[github-actions]

Cross-SDK Consistency Review ✅

I've reviewed PR #664 for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Overall, this PR demonstrates excellent cross-SDK consistency! 🎉

✅ Consistent Implementation Across All SDKs

The core fix has been implemented uniformly across all four languages:

1. Session ID generation: All SDKs now generate UUID client-side before RPC

   • Node.js: randomUUID()
   • Python: str(uuid.uuid4())
   • Go: uuid.New().String() (added github.com/google/uuid dependency)
   • .NET: Guid.NewGuid().ToString()

2. Session registration timing: All SDKs register session in map before RPC call

   • Includes proper session setup (tools, permission handlers, hooks)
   • All SDKs add the new OnEvent/onEvent/on_event handler registration

3. Workspace path handling: All SDKs correctly set workspace path after RPC completes

   • Node.js: session["_workspacePath"] = workspacePath;
   • Python: session._workspace_path = response.get("workspacePath")
   • Go: session.workspacePath = response.WorkspacePath
   • .NET: Property changed from get-only to internal set + session.WorkspacePath = response.WorkspacePath; (inferred from WorkspacePath { get; internal set; })

4. Error cleanup: All SDKs remove session from map if RPC fails

   • Try-catch/try-except/error handling is consistent
   • Proper mutex/lock usage in Go and .NET

5. API additions: The OnEvent/onEvent/on_event field was added to:

   • SessionConfig in all 4 SDKs ✅
   • ResumeSessionConfig in all 4 SDKs ✅
   • Consistent documentation explaining the purpose

6. Test coverage: All four SDKs modified the "should receive session events" test to verify session.start is captured via the onEvent handler

📝 Minor Observation

The PR description mentions:

12 new unit tests across Node.js, Python, and Go

However, I only observe modifications to existing e2e tests (the "should receive session events" test) in all four SDKs, not dedicated new unit tests for:

• "Session in map during session.create RPC"
• "Session in map during session.resume RPC"
• "Cleanup on session.create RPC failure"
• "Cleanup on session.resume RPC failure"

This is not a consistency issue - just noting that if these unit tests exist, they might not be in this diff or the PR description might be referring to test scenarios rather than distinct test functions. Either way, the e2e test modifications provide good coverage of the fix.

🎯 Conclusion

No cross-SDK consistency issues found. This PR maintains excellent feature parity across all four SDK implementations with parallel API design, equivalent behavior, and consistent naming (accounting for language conventions). Great work! 🚀

AI generated by SDK Consistency Review Agent

[patniko]

@SteveSandersonMS would be great to land this. Another developer reported it to me as well.

[stephentoub]

@stephentoub Excellent, thanks. Will review shortly. Interested in whether this might also address #490

Hmm... I don't think it will. We can look at that separately.

#682 addresses shutdown.