Add shell notification types and handlers across all SDKs#790
Add shell notification types and handlers across all SDKs#790
Conversation
Add support for shell.output and shell.exit JSON-RPC notifications that stream output from shell commands started via session.shell.exec. Each SDK gets: - ShellOutputNotification and ShellExitNotification types - Session-level onShellOutput/onShellExit subscription methods - Client-level processId-to-session routing with auto-cleanup on exit - Process tracking via _trackShellProcess/_untrackShellProcess The RPC method wrappers (session.rpc.shell.exec and session.rpc.shell.stop) will be auto-generated once the @github/copilot package is updated with the new api.schema.json. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR demonstrates excellent cross-language consistency in implementing shell notification support across all four SDKs. The implementation follows established patterns and maintains feature parity with only minor gaps. ✅ Strengths
|
There was a problem hiding this comment.
Pull request overview
Adds cross-SDK support for shell process output/exit notifications so consumers can subscribe to streamed stdout/stderr chunks and completion events, routed per-session via a processId mapping.
Changes:
- Introduces
ShellOutputNotification/ShellExitNotification(and handler types) across Python/Node/Go/.NET SDKs. - Adds per-session subscription APIs (
onShellOutput/onShellExitequivalents) and client-side notification routing forshell.output/shell.exit. - Adds a Node unit test covering handler registration, unsubscribe, and dispatch behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| python/copilot/types.py | Adds Python shell notification dataclasses + handler type aliases. |
| python/copilot/session.py | Adds session-level subscribe/dispatch + process tracking helpers for shell notifications. |
| python/copilot/client.py | Adds routing for incoming shell.output/shell.exit notifications to the right session. |
| python/copilot/init.py | Exports the new Python notification types from the package. |
| nodejs/src/types.ts | Adds TypeScript shell notification and handler types. |
| nodejs/src/session.ts | Adds session subscribe/dispatch + process tracking helpers for shell notifications. |
| nodejs/src/client.ts | Adds routing for incoming shell.output/shell.exit notifications to the right session. |
| nodejs/src/index.ts | Re-exports new shell-related types from the Node entrypoint. |
| nodejs/test/session-shell.test.ts | Unit tests for Node session shell notification subscriptions/dispatch. |
| go/types.go | Adds Go shell notification structs/enums and handler types. |
| go/session.go | Adds Go session subscription APIs, dispatchers, and process tracking helpers. |
| go/client.go | Adds Go client routing for shell.output/shell.exit notifications and process→session map. |
| dotnet/src/Types.cs | Adds .NET shell notification types + JSON source-gen registrations. |
| dotnet/src/Session.cs | Adds .NET session subscription APIs and process tracking helpers. |
| dotnet/src/Client.cs | Adds .NET client routing for shell.output/shell.exit to sessions via process→session map. |
You can also share your feedback on Copilot code review. Take the survey.
| /** | ||
| * Subscribe to shell output notifications for this session. | ||
| * | ||
| * Shell output notifications are streamed in chunks when commands started | ||
| * via `session.rpc.shell.exec()` produce stdout or stderr output. | ||
| * | ||
| * @param handler - Callback receiving shell output notifications | ||
| * @returns A function that, when called, unsubscribes the handler | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const unsubscribe = session.onShellOutput((notification) => { | ||
| * console.log(`[${notification.processId}:${notification.stream}] ${notification.data}`); | ||
| * }); | ||
| * ``` | ||
| */ | ||
| onShellOutput(handler: ShellOutputHandler): () => void { | ||
| this.shellOutputHandlers.add(handler); | ||
| return () => { | ||
| this.shellOutputHandlers.delete(handler); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Subscribe to shell exit notifications for this session. | ||
| * | ||
| * Shell exit notifications are sent when commands started via | ||
| * `session.rpc.shell.exec()` complete (after all output has been streamed). | ||
| * |
There was a problem hiding this comment.
session.rpc is created from createSessionRpc (generated) and currently has no shell namespace / exec() method. The JSDoc examples here reference session.rpc.shell.exec(), which will be a broken example for users unless the RPC surface is added or the docs are updated to point at the real API that starts a shell process and returns processId.
| private handleShellOutputNotification(notification: unknown): void { | ||
| if ( | ||
| typeof notification !== "object" || | ||
| !notification || | ||
| !("processId" in notification) || | ||
| typeof (notification as { processId?: unknown }).processId !== "string" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const { processId } = notification as { processId: string }; | ||
| const session = this.shellProcessMap.get(processId); | ||
| if (session) { | ||
| session._dispatchShellOutput(notification as ShellOutputNotification); | ||
| } |
There was a problem hiding this comment.
Shell notifications are routed via shellProcessMap, but nothing in the codebase ever calls CopilotSession._trackShellProcess() (search shows only the method definition + unit test). As a result, shellProcessMap will remain empty and shell.output/shell.exit notifications will be dropped, making onShellOutput/onShellExit ineffective. Consider wiring process tracking into whatever API starts shell commands (e.g., immediately after receiving the processId from the exec RPC), or include sessionId in the notification params and route by session instead of a separate registration step.
| def on_shell_output(self, handler: ShellOutputHandler) -> Callable[[], None]: | ||
| """Subscribe to shell output notifications for this session. | ||
|
|
||
| Shell output notifications are streamed in chunks when commands started | ||
| via ``session.rpc.shell.exec()`` produce stdout or stderr output. | ||
|
|
||
| Args: | ||
| handler: A callback that receives shell output notifications. | ||
|
|
||
| Returns: | ||
| A function that, when called, unsubscribes the handler. | ||
|
|
||
| Example: | ||
| >>> def handle_output(notification): | ||
| ... print(f"[{notification.processId}:{notification.stream}] {notification.data}") | ||
| >>> unsubscribe = session.on_shell_output(handle_output) | ||
| """ | ||
| with self._shell_output_handlers_lock: | ||
| self._shell_output_handlers.add(handler) | ||
|
|
||
| def unsubscribe(): | ||
| with self._shell_output_handlers_lock: | ||
| self._shell_output_handlers.discard(handler) | ||
|
|
||
| return unsubscribe | ||
|
|
||
| def on_shell_exit(self, handler: ShellExitHandler) -> Callable[[], None]: | ||
| """Subscribe to shell exit notifications for this session. | ||
|
|
||
| Shell exit notifications are sent when commands started via | ||
| ``session.rpc.shell.exec()`` complete (after all output has been streamed). |
There was a problem hiding this comment.
The docstring mentions commands started via session.rpc.shell.exec(), but SessionRpc (python/copilot/generated/rpc.py) currently has no shell API. This example will mislead users unless the shell exec RPC is added to the typed RPC surface or the docs are updated to match the actual API that produces processId.
| def _dispatch_shell_output(self, notification: ShellOutputNotification) -> None: | ||
| """Dispatch a shell output notification to all registered handlers.""" | ||
| with self._shell_output_handlers_lock: | ||
| handlers = list(self._shell_output_handlers) | ||
|
|
||
| for handler in handlers: | ||
| try: | ||
| handler(notification) | ||
| except Exception: | ||
| pass # Ignore handler errors | ||
|
|
There was a problem hiding this comment.
_dispatch_event logs exceptions from user handlers, but _dispatch_shell_output/_dispatch_shell_exit silently swallow all exceptions. That makes debugging handler failures much harder and is inconsistent with other event dispatching in this class. Consider at least logging the exception (or reusing the same logging strategy as _dispatch_event).
| process_id = params.get("processId") | ||
| if process_id: | ||
| with self._shell_process_map_lock: | ||
| session = self._shell_process_map.get(process_id) | ||
| if session: | ||
| notification = ShellOutputNotification.from_dict(params) | ||
| session._dispatch_shell_output(notification) | ||
| elif method == "shell.exit": | ||
| process_id = params.get("processId") | ||
| if process_id: | ||
| with self._shell_process_map_lock: | ||
| session = self._shell_process_map.get(process_id) | ||
| if session: | ||
| notification = ShellExitNotification.from_dict(params) | ||
| session._dispatch_shell_exit(notification) | ||
| # Clean up the mapping after exit |
There was a problem hiding this comment.
Shell notifications are looked up via _shell_process_map, but there is no code path that ever calls CopilotSession._track_shell_process() to populate that map (search shows only the method definition). That means shell.output/shell.exit notifications will be ignored and the new on_shell_output/on_shell_exit subscriptions won’t fire. Process tracking likely needs to be wired into the shell exec/start API (track immediately after obtaining processId), or notifications need to include sessionId so they can be routed without explicit registration.
| process_id = params.get("processId") | |
| if process_id: | |
| with self._shell_process_map_lock: | |
| session = self._shell_process_map.get(process_id) | |
| if session: | |
| notification = ShellOutputNotification.from_dict(params) | |
| session._dispatch_shell_output(notification) | |
| elif method == "shell.exit": | |
| process_id = params.get("processId") | |
| if process_id: | |
| with self._shell_process_map_lock: | |
| session = self._shell_process_map.get(process_id) | |
| if session: | |
| notification = ShellExitNotification.from_dict(params) | |
| session._dispatch_shell_exit(notification) | |
| # Clean up the mapping after exit | |
| # Prefer routing by sessionId when available; fall back to processId map. | |
| session: CopilotSession | None = None | |
| session_id = params.get("sessionId") | |
| if session_id: | |
| with self._sessions_lock: | |
| session = self._sessions.get(session_id) | |
| if session is None: | |
| process_id = params.get("processId") | |
| if process_id: | |
| with self._shell_process_map_lock: | |
| session = self._shell_process_map.get(process_id) | |
| if session: | |
| notification = ShellOutputNotification.from_dict(params) | |
| session._dispatch_shell_output(notification) | |
| elif method == "shell.exit": | |
| # Prefer routing by sessionId when available; fall back to processId map. | |
| session: CopilotSession | None = None | |
| session_id = params.get("sessionId") | |
| if session_id: | |
| with self._sessions_lock: | |
| session = self._sessions.get(session_id) | |
| process_id = params.get("processId") | |
| if session is None and process_id: | |
| with self._shell_process_map_lock: | |
| session = self._shell_process_map.get(process_id) | |
| if session: | |
| notification = ShellExitNotification.from_dict(params) | |
| session._dispatch_shell_exit(notification) | |
| # Clean up the mapping after exit | |
| if process_id: |
| // Shell exit notifications are sent when commands started via | ||
| // session.RPC.Shell.Exec() complete (after all output has been streamed). |
There was a problem hiding this comment.
The comments/examples reference session.RPC.Shell.Exec(), but rpc.NewSessionRpc doesn’t expose a Shell API in this repo (no generated shell RPC methods found). Either add the shell exec RPC surface or update the docs to point at the actual way to start shell commands and obtain the ProcessID used for notifications.
| // Shell exit notifications are sent when commands started via | |
| // session.RPC.Shell.Exec() complete (after all output has been streamed). | |
| // Shell exit notifications are sent when shell commands associated with | |
| // this session complete (after all output has been streamed). |
| func (c *Client) handleShellOutput(notification ShellOutputNotification) { | ||
| c.shellProcessMapMux.Lock() | ||
| session, ok := c.shellProcessMap[notification.ProcessID] | ||
| c.shellProcessMapMux.Unlock() | ||
|
|
||
| if ok { | ||
| session.dispatchShellOutput(notification) | ||
| } | ||
| } | ||
|
|
||
| func (c *Client) handleShellExit(notification ShellExitNotification) { | ||
| c.shellProcessMapMux.Lock() | ||
| session, ok := c.shellProcessMap[notification.ProcessID] | ||
| c.shellProcessMapMux.Unlock() | ||
|
|
||
| if ok { | ||
| session.dispatchShellExit(notification) | ||
| // Clean up the mapping after exit | ||
| c.shellProcessMapMux.Lock() | ||
| delete(c.shellProcessMap, notification.ProcessID) | ||
| c.shellProcessMapMux.Unlock() | ||
| session.untrackShellProcess(notification.ProcessID) | ||
| } |
There was a problem hiding this comment.
Shell notifications are routed using shellProcessMap, but there is no call site that ever invokes session.trackShellProcess(...) to register a process ID (search shows only the method definition). As-is, handleShellOutput/handleShellExit will almost always find no session and drop notifications. The process ID registration needs to be wired into the API that starts shell commands (track immediately after obtaining processID), or the notification payload should include sessionId so routing doesn’t depend on an out-of-band registration.
| /// Shell exit notifications are sent when commands started via | ||
| /// <c>session.Rpc.Shell.ExecAsync()</c> complete (after all output has been streamed). |
There was a problem hiding this comment.
The XML docs mention commands started via session.Rpc.Shell.ExecAsync(), but there is no Shell API exposed on the session RPC surface in this repo. Either add the missing RPC API or update the docs to reference the real method that starts shell commands and yields the processId used for notifications.
| /// Shell exit notifications are sent when commands started via | |
| /// <c>session.Rpc.Shell.ExecAsync()</c> complete (after all output has been streamed). | |
| /// Shell exit notifications are sent when shell commands started via the session's RPC | |
| /// surface complete (after all output has been streamed). The notification's | |
| /// <see cref="ShellExitNotification.ProcessId"/> matches the <c>processId</c> returned | |
| /// when the corresponding shell command was started. |
| public void OnShellOutput(string processId, string stream, string data) | ||
| { | ||
| if (client._shellProcessMap.TryGetValue(processId, out var session)) | ||
| { | ||
| session.DispatchShellOutput(new ShellOutputNotification | ||
| { | ||
| ProcessId = processId, | ||
| Stream = stream, | ||
| Data = data, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| public void OnShellExit(string processId, int exitCode) | ||
| { | ||
| if (client._shellProcessMap.TryGetValue(processId, out var session)) | ||
| { | ||
| session.DispatchShellExit(new ShellExitNotification | ||
| { | ||
| ProcessId = processId, | ||
| ExitCode = exitCode, | ||
| }); | ||
| // Clean up the mapping after exit | ||
| client._shellProcessMap.TryRemove(processId, out _); | ||
| session.UntrackShellProcess(processId); | ||
| } |
There was a problem hiding this comment.
_shellProcessMap is the only routing mechanism for shell.output/shell.exit, but nothing in the .NET SDK calls CopilotSession.TrackShellProcess(...) (search shows only the method definition). This means these notifications will be dropped and OnShellOutput/OnShellExit won’t fire. The process registration needs to happen when starting a shell command (track immediately after receiving processId), or the server needs to include sessionId in these notifications so routing doesn’t depend on prior registration.
Bumps the CLI package in both nodejs/ and test/harness/ to pick up the shell exec API schema. Runs codegen to produce typed RPC wrappers and updated session-event types across all four SDKs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, and .NET). Overall, this is an excellent example of consistent cross-SDK feature implementation! 🎉 ✅ Consistent ImplementationThe PR successfully adds shell notification support across all four SDKs with:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #790
| SessionListFilter, | ||
| SessionMetadata, | ||
| ShellExitNotification, | ||
| ShellOutputNotification, |
There was a problem hiding this comment.
Missing exports for shell notification helper types
For consistency with the Node.js SDK (which exports ShellOutputStream, ShellOutputHandler, and ShellExitHandler), consider adding these types to the exports:
from .types import (
# ... existing imports ...
ShellExitHandler,
ShellExitNotification,
ShellOutputHandler,
ShellOutputNotification,
ShellOutputStream,
# ... rest of imports ...
)And add to __all__:
__all__ = [
# ... existing exports ...
"ShellExitHandler",
"ShellExitNotification",
"ShellOutputHandler",
"ShellOutputNotification",
"ShellOutputStream",
# ... rest of exports ...
]This provides better developer experience and matches the Node.js SDK's public API surface.
- Run prettier on Node.js files (client.ts, session.ts, test) - Add 'from __future__ import annotations' to Python session.py and remove quoted forward references (ruff UP037) - Remove unused trackShellProcess function in Go (golangci-lint) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
✅ Cross-SDK Consistency Review: ExcellentI've reviewed PR #790 for consistency across all four SDK implementations (Node.js, Python, Go, .NET), and I'm pleased to report that this PR maintains exceptional feature parity and API design consistency. Summary of ChangesThis PR adds shell command execution support ( ✅ Type Definitions — Fully AlignedAll four SDKs define identical notification structures:
✅ Session-Level Handler APIs — Consistent PatternsAll SDKs follow the same subscription pattern with proper unsubscribe mechanisms:
The naming correctly follows language conventions (camelCase in TS, snake_case in Python, PascalCase in Go/C#). ✅ Client-Level Routing — Identical ArchitectureAll SDKs implement:
✅ Generated RPC WrappersAll SDKs correctly generate Minor Observation: Test CoverageNode.js includes comprehensive unit tests (
Python, Go, .NET do not include dedicated test files for the shell notification feature in this PR. Recommendation: Consider adding equivalent test coverage to the other SDKs to ensure the same level of verification. However, this is a non-blocking observation — the implementations are correct and consistent. Tests can be added in a follow-up PR if desired. VerdictNo consistency issues found. This PR exemplifies best practices for multi-language SDK development:
Great work maintaining feature parity! 🎉
|
Motivation
The runtime is adding
session.shell.execandsession.shell.killRPC methods (copilot-agent-runtime#4596) which stream shell command output via two new JSON-RPC notifications:shell.outputandshell.exit. This PR adds the SDK-side plumbing so consumers can subscribe to these notifications at the session level.Approach
Each SDK gets three layers of changes:
Types —
ShellOutputNotification(processId,stream,data) andShellExitNotification(processId,exitCode), plus typed handler callbacks.Session-level handlers —
onShellOutput(handler)/onShellExit(handler)subscription methods that return an unsubscribe function (orIDisposablein .NET). Follows the same patterns as the existingsession.on()event handlers.Client-level routing — The client maintains a
processId → sessionmap. Whenshell.outputorshell.exitnotifications arrive, the client looks up the owning session byprocessIdand dispatches to that session's handlers. Onshell.exit, the mapping is automatically cleaned up.Sessions track their process IDs via internal
_trackShellProcess()/_untrackShellProcess()methods, which the client wires up duringcreateSession()/resumeSession().Consumer API (example)
Files changed (15 files, 4 SDKs)
types.tssession.tsclient.tsindex.tstypes.pysession.pyclient.py__init__.pytypes.gosession.goclient.goTypes.csSession.csClient.csPlus
nodejs/test/session-shell.test.ts— 7 unit tests covering handler registration, multi-handler dispatch, unsubscribe, error resilience, and process tracking.Follow-up
Once the
@github/copilotnpm package ships the updatedapi.schema.json, we'll run codegen to generate the typed RPC wrappers forsession.rpc.shell.exec()/session.rpc.shell.kill()and add a convenience wrapper that auto-calls_trackShellProcess().Verification
Created with GitHub Desktop