Update to match runtime changes#737
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the multi-language Copilot SDKs to align with recent runtime/protocol changes, including new session-scoped RPCs, new event shapes, and an initial Node.js extension-joining API with accompanying documentation.
Changes:
- Add
session.logRPC support across Node.js/Python/Go/.NET SDKs, plus conveniencesession.log(...)/Log(...)APIs and E2E coverage. - Extend protocol models/events for
reasoningEfforton model switch,system.notificationevent, andalreadyInUseon session start/resume. - Introduce Node.js extension infrastructure via
@github/copilot-sdk/extensionjoinSession()and publish extension docs in the npm package.
Reviewed changes
Copilot reviewed 17 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/csharp.ts | Refactors C# RPC emission to support top-level session RPC methods on SessionRpc. |
| python/copilot/session.py | Adds Session.log() wrapper calling the new session.log RPC. |
| python/copilot/generated/rpc.py | Adds SessionLog* RPC types, Level enum, and ReasoningEffort support. |
| python/copilot/generated/session_events.py | Adds system.notification + alreadyInUse and notification-kind metadata types. |
| python/e2e/test_session.py | Adds E2E coverage for session.log event emission. |
| nodejs/src/generated/rpc.ts | Adds session.log RPC and reasoningEffort param on model switch. |
| nodejs/src/generated/session-events.ts | Adds alreadyInUse and system.notification event typing. |
| nodejs/src/session.ts | Adds CopilotSession.log() convenience method. |
| nodejs/src/extension.ts | Replaces prior extension entry behavior with joinSession() for CLI child-process extensions. |
| nodejs/src/client.ts | Moves bundled-CLI helpers (no functional change). |
| nodejs/test/e2e/session.test.ts | Adds E2E test for session logging events (uses vi.waitFor). |
| nodejs/package.json | Ships docs/**/* in the published package. |
| nodejs/docs/extensions.md | New extension architecture/usage documentation. |
| nodejs/docs/examples.md | New, detailed extension examples (tools, hooks, events). |
| nodejs/docs/agent-author.md | New agent-oriented extension authoring guide. |
| go/rpc/generated_rpc.go | Adds SessionLog* RPC types, Level, and ReasoningEffort; wires request building. |
| go/session.go | Adds Session.Log() convenience API and LogOptions. |
| go/generated_session_events.go | Adds alreadyInUse, system.notification, and kind metadata structures. |
| go/internal/e2e/session_test.go | Adds TestSessionLog E2E coverage with event polling helper. |
| dotnet/src/Generated/Rpc.cs | Adds session.log RPC types + updates model switch signature for reasoningEffort. |
| dotnet/src/Session.cs | Adds Session.LogAsync(...) convenience method. |
| dotnet/src/Generated/SessionEvents.cs | Adds system.notification and alreadyInUse fields/types. |
| dotnet/test/SessionTests.cs | Adds E2E coverage for LogAsync event emission. |
Comments suppressed due to low confidence (2)
nodejs/docs/examples.md:293
- The call to
execwithfilePathembedded inside a shell command string enables command injection iffilePathcontains characters like quotes and shell operators. If an attacker can controlinput.toolArgs.pathfor theedittool (for example, via malicious tool arguments), they can cause this extension to run arbitrary shell commands when invoking the linter. Use a process-spawning API that does not invoke a shell and passes the path as a separate argument, or rigorously validate/escape the path before including it in any shell command string.
exec(`npx eslint "${filePath}"`, (err, stdout) => {
nodejs/docs/examples.md:615
- In
openInEditor, interpolatingfilePathdirectly into a shell command passed toexeccreates a command injection risk iffilePathcontains quotes or shell metacharacters. An attacker controlling thepathargument of thecreate/edittools can supply a value that breaks out of the quoted path and appends arbitrary commands, which the extension will execute on Windows whenever it opens the editor. Prefer a non-shell API (or pass the path as a separate argument) and/or strictly validate and escapefilePathbefore using it in any shell command.
if (isWindows) exec(`code "${filePath}"`, () => {});
| var events = new List<SessionEvent>(); | ||
| session.On(evt => events.Add(evt)); | ||
|
|
||
| await session.LogAsync("Info message"); | ||
| await session.LogAsync("Warning message", level: SessionLogRequestLevel.Warning); | ||
| await session.LogAsync("Error message", level: SessionLogRequestLevel.Error); | ||
| await session.LogAsync("Ephemeral message", ephemeral: true); | ||
|
|
||
| // Poll until all 4 notification events arrive | ||
| await WaitForAsync(() => | ||
| { | ||
| var notifications = events.Where(e => | ||
| e is SessionInfoEvent info && info.Data.InfoType == "notification" || | ||
| e is SessionWarningEvent warn && warn.Data.WarningType == "notification" || | ||
| e is SessionErrorEvent err && err.Data.ErrorType == "notification" | ||
| ).ToList(); | ||
| return notifications.Count >= 4; | ||
| }, timeout: TimeSpan.FromSeconds(10)); |
There was a problem hiding this comment.
session.On(evt => events.Add(evt)) appends to a List<SessionEvent> from the JSON-RPC event dispatch path while WaitForAsync concurrently enumerates events via LINQ. This can race and throw InvalidOperationException (collection modified) or produce flaky counts. Consider using a thread-safe collection (e.g., ConcurrentQueue<SessionEvent> / ConcurrentBag<SessionEvent>) or guarding both writes and reads with a lock (snapshot to array before filtering).
| import { CopilotClient } from "./client.js"; | ||
| import type { CopilotSession } from "./session.js"; | ||
| import type { ResumeSessionConfig } from "./types.js"; | ||
|
|
||
| export const extension = new CopilotClient({ isChildProcess: true }); | ||
| /** | ||
| * Joins the current foreground session. | ||
| * | ||
| * @param config - Configuration to add to the session | ||
| * @returns A promise that resolves with the joined session | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * import { approveAll } from "@github/copilot-sdk"; | ||
| * import { joinSession } from "@github/copilot-sdk/extension"; | ||
| * | ||
| * const session = await joinSession({ | ||
| * onPermissionRequest: approveAll, | ||
| * tools: [myTool], | ||
| * }); | ||
| * ``` | ||
| */ | ||
| export async function joinSession(config: ResumeSessionConfig): Promise<CopilotSession> { | ||
| const sessionId = process.env.SESSION_ID; | ||
| if (!sessionId) { | ||
| throw new Error( | ||
| "joinSession() is intended for extensions running as child processes of the Copilot CLI." | ||
| ); | ||
| } | ||
|
|
||
| const client = new CopilotClient({ isChildProcess: true }); | ||
| return client.resumeSession(sessionId, { | ||
| ...config, | ||
| disableResume: config.disableResume ?? true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This changes the public API of the @github/copilot-sdk/extension entrypoint: the previous extension export is removed and replaced with joinSession(). If any consumers rely on import { extension } from "@github/copilot-sdk/extension", this is a breaking change. Consider keeping extension as a deprecated export (or providing a backwards-compatible alias) while introducing joinSession, or ensure the package versioning/release notes reflect the breaking change.
| const filePath = input.toolArgs?.path; | ||
| if (filePath) { | ||
| // Open the file in VS Code | ||
| exec(`code "${filePath}"`, () => {}); |
There was a problem hiding this comment.
Using exec with filePath interpolated into a shell command allows command injection if filePath contains shell metacharacters or quotes. An attacker who can influence input.toolArgs.path (for example via the create/edit tools) can craft a value that closes the quoted path and appends arbitrary commands, which will then execute with the user's privileges whenever the extension opens the file in VS Code. To mitigate, avoid spawning a shell for this operation (use a non-shell API that takes the path as a separate argument) or strictly validate/escape the path before use.
This issue also appears in the following locations of the same file:
- line 293
- line 615
✅ Cross-SDK Consistency Review - PR #737I've reviewed this PR for consistency across all four SDK implementations (Node.js, Python, Go, .NET). Here are my findings: ✅ Consistently Implemented Features (All SDKs)1. New RPC API:
|
| SDK | Method Signature | Parameters |
|---|---|---|
| Node.js | async log(message, options?) |
✅ message, level, ephemeral |
| Python | async def log(message, *, level, ephemeral) |
✅ message, level, ephemeral |
| Go | func Log(ctx, message, opts) |
✅ message, level, ephemeral (via LogOptions) |
| .NET | async Task LogAsync(message, level, ephemeral, cancellationToken) |
✅ message, level, ephemeral |
Assessment: ✅ Fully consistent - Minor API differences follow language conventions (e.g., Go context parameter, .NET cancellation token, Go's LogOptions struct).
2. RPC Update: reasoningEffort parameter on session.model.switchTo
All SDKs add the new optional parameter:
| SDK | Parameter Name | Optional? | Allowed Values |
|---|---|---|---|
| Node.js | reasoningEffort? |
✅ Yes | "low" | "medium" | "high" | "xhigh" |
| Python | reasoning_effort |
✅ Yes (None default) | ReasoningEffort enum |
| Go | ReasoningEffort |
✅ Yes (pointer) | Low, Medium, High, Xhigh |
| .NET | reasoningEffort |
✅ Yes (nullable) | SessionModelSwitchToRequestReasoningEffort enum |
Assessment: ✅ Fully consistent - All SDKs properly expose this as optional with the same four allowed values.
3. New Session Event: system.notification
All SDKs include the new event type in generated session events:
| SDK | Event Type Constant | Data Structure |
|---|---|---|
| Node.js | "system.notification" |
✅ Proper TypeScript types |
| Python | SYSTEM_NOTIFICATION = "system.notification" |
✅ Proper dataclasses |
| Go | SystemNotification SessionEventType |
✅ Proper struct definitions |
| .NET | SystemNotificationEvent : SessionEvent |
✅ Proper class hierarchy with JSON attributes |
Assessment: ✅ Fully consistent - All SDKs support receiving and handling system notification events.
4. New Session Flag: alreadyInUse
All SDKs add this flag to session.created and session.resumed events:
| SDK | Property Name | Type |
|---|---|---|
| Node.js | alreadyInUse? |
boolean | undefined |
| Python | already_in_use |
bool | None |
| Go | AlreadyInUse |
*bool (pointer) |
| .NET | AlreadyInUse |
bool? (nullable) |
Assessment: ✅ Fully consistent - All SDKs use optional/nullable boolean typing.
⚠️ Potential Inconsistency: Extension Infrastructure
Issue: Extension support (extension.ts + documentation) appears to be Node.js-only.
Node.js:
- ✅
nodejs/src/extension.ts- ProvidesjoinSession()for extension processes - ✅
nodejs/docs/extensions.md- Complete extension authoring guide - ✅ Exported as
@github/copilot-sdk/extension
Python, Go, .NET:
- ❌ No extension modules or APIs found
- ❌ No extension documentation
Question: Is extension support intentionally Node.js-only, or should Python/Go/.NET SDKs also provide extension APIs?
Recommendation:
- If extensions are Node.js-only (e.g., because extensions run as Node.js processes), consider adding a note to the main README explaining this
- If extensions should eventually support other languages, consider filing follow-up issues to track parity
📋 Summary
Overall Assessment: ✅ Excellent cross-SDK consistency
This PR maintains strong feature parity across all SDKs for the core runtime changes (session.log, reasoningEffort, system notifications, alreadyInUse flag). The only potential gap is extension infrastructure, which appears to be a deliberate Node.js-specific feature.
All generated types and RPC methods are properly synchronized across language implementations. Great work maintaining consistency! 🎉
Generated by SDK Consistency Review Agent for issue #737 · ◷
SDK Consistency ReviewI've reviewed this PR for cross-language SDK consistency. Here's my analysis: ✅ Consistent Features (Implemented in All SDKs)The following features are consistently implemented across all four SDKs (Node.js, Python, Go, .NET):
All of these features include comprehensive E2E tests across all languages. Great work maintaining parity! 👍
|
✅ Cross-SDK Consistency Review: EXCELLENTI've completed a comprehensive review of PR #737 for cross-language SDK consistency. This PR demonstrates exemplary multi-SDK development - all four language implementations have been updated in parallel with complete feature parity. Summary of ChangesThis PR adds four new runtime-related features across all SDKs:
✅ Consistency Validation1. session.log() MethodAll four SDKs implement this with equivalent signatures:
✅ Consistent parameters: message, level (info/warning/error), ephemeral 2. system.notification Event✅ All SDKs have the 3. alreadyInUse Flag✅ All SDKs have added the optional 4. reasoningEffort Parameter✅ All SDKs support the new parameter in 📚 DocumentationThe PR includes new extension documentation (3 new/updated files in 🎯 ConclusionNo consistency issues found. This PR is a model example of how to maintain feature parity across a multi-language SDK. The changes are well-structured, thoroughly tested, and maintain consistent API semantics while respecting each language's idioms. Great work! 🚀
|
Includes:
Note: E2E tests will fail until we get a corresponding runtime update. They pass locally.