Port validation and lifecycle fixes to Node, Python, and Go#1341
Open
stephentoub wants to merge 2 commits into
Open
Port validation and lifecycle fixes to Node, Python, and Go#1341stephentoub wants to merge 2 commits into
stephentoub wants to merge 2 commits into
Conversation
Port C# argument validation and lifecycle cleanup behavior to Node, Python, and Go. Generated session RPC APIs now validate required params and reject use after disconnect, while clients clean up active session maps consistently and reject duplicate active session IDs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply ruff formatting to the new Python lifecycle regression tests so CI format checks pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ports stricter generated-RPC argument validation and session lifecycle cleanup (previously tightened in C#) to the Node.js, Python, and Go SDKs, so clients fail earlier on invalid usage and don’t retain stale/duplicate active sessions.
Changes:
- Updated TS/Python/Go RPC generators (and regenerated outputs) to validate required request objects before sending and to reject session-scoped RPC calls once a session is disconnected.
- Hardened session/client lifecycle cleanup across Node/Python/Go (disconnect unregisters from the owning client; stop/force-stop/delete ensure sessions are marked unusable; duplicate active session IDs are rejected).
- Added/updated targeted unit + E2E regression coverage to lock in lifecycle/validation behavior, including updated Go resume tests for the new “duplicate active session” contract.
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/typescript.ts | Emit session “active” assertions and required-params checks in generated TS RPC methods. |
| scripts/codegen/python.ts | Emit session “active” assertions and required-params checks in generated Python RPC methods. |
| scripts/codegen/go.ts | Emit session “active” assertions and required-params checks in generated Go RPC methods. |
| python/test_rpc_generated.py | Adds regression tests for required-params validation and active-check behavior. |
| python/test_client.py | Adds/updates lifecycle tests for disconnect/unregister, duplicates, and failure cleanup paths. |
| python/copilot/session.py | Implements disconnect idempotency + active-use rejection; clears handlers/hooks on disconnect and supports unregister callback. |
| python/copilot/generated/rpc.py | Regenerated RPC surface with params validation and assert-active hooks for session APIs. |
| python/copilot/client.py | Adds session registration helpers; enforces duplicate-active rejection and consistent cleanup on stop/force-stop/delete. |
| nodejs/test/client.test.ts | Adds unit coverage for lifecycle cleanup, stop errors, duplicate registration, and params validation. |
| nodejs/src/session.ts | Adds disconnect state, active assertions, idempotent disconnect, forced-disconnect hook, and unregister callback. |
| nodejs/src/generated/rpc.ts | Regenerated RPC surface with params validation and assert-active hooks for session APIs. |
| nodejs/src/client.ts | Enforces duplicate-active session IDs; ensures stop/force-stop/delete mark sessions disconnected and clears internal RPC state. |
| go/session.go | Adds session active-state tracking, idempotent Disconnect, and client unregister on disconnect; rejects usage after disconnect. |
| go/session_test.go | Adds unit tests covering lifecycle cleanup and duplicate-active session registration behavior. |
| go/rpc/zrpc.go | Regenerated Go RPC surface with params validation and assert-active checks for session APIs. |
| go/rpc/generated_rpc_api_shape_test.go | Adds regression tests for required-params validation and active-check ordering. |
| go/internal/e2e/session_e2e_test.go | Updates E2E expectations for post-disconnect behavior and validates duplicate-active resume rejection; adds resume-client helper. |
| go/internal/e2e/session_config_e2e_test.go | Updates resume scenarios to use a separate TCP client consistent with the new “active session” contract. |
| go/internal/e2e/permissions_e2e_test.go | Updates resume permissions scenario to use separate resume client and aligns with new join behavior. |
| go/internal/e2e/mcp_and_agents_e2e_test.go | Updates resume scenarios to use separate resume client (TCP mode). |
| go/internal/e2e/commands_and_elicitation_e2e_test.go | Updates resume scenario to use separate resume client (TCP mode). |
| go/client.go | Enforces duplicate-active session IDs and centralizes session snapshot/clear helpers; ensures stop/force-stop disconnects and marks sessions unusable. |
Copilot's findings
Files not reviewed (1)
- go/rpc/zrpc.go: Language not supported
- Files reviewed: 19/22 changed files
- Comments generated: 2
Comment on lines
1118
to
1124
| def __init__( | ||
| self, session_id: str, client: Any, workspace_path: os.PathLike[str] | str | None = None | ||
| self, | ||
| session_id: str, | ||
| client: Any, | ||
| workspace_path: os.PathLike[str] | str | None = None, | ||
| on_disconnected: Callable[[CopilotSession], None] | None = None, | ||
| ): |
| }) | ||
|
|
||
| t.Run("should deny tool operations when handler explicitly denies after resume", func(t *testing.T) { | ||
| t.Run("should accept explicit deny handler when joining an active session", func(t *testing.T) { |
Contributor
Cross-SDK Consistency ReviewThis PR does an excellent job porting the C# session lifecycle fixes to Node.js, Python, and Go — the implementations are closely parallel:
One gap found: Go's Everything else looks well-aligned. The codegen changes across all three templates (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1341 · ● 2.6M
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
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.
C# recently tightened generated RPC argument validation and session lifecycle cleanup. This ports the commensurate behavior to the SDKs that had matching gaps so clients fail earlier, do not keep stale active sessions around, and reject duplicate active local sessions consistently.
Summary
Rust was reviewed but left unchanged because its existing ownership/lifecycle model already covers the comparable behavior.
Validation
cd nodejs && npm run typecheckcd nodejs && npm test -- client.test.ts --testNamePattern "directly disconnected|reports stop errors|replacement session|duplicate active|validates required"cd python && python -m pytest -q test_client.py test_rpc_generated.pycd python && python -m ruff check test_client.py test_rpc_generated.py copilot\client.py copilot\session.pycd go && go test -timeout 20m ./...git diff --check