C# API review fixes#1343
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| // Protocol v2 backward-compatibility adapters | ||
|
|
||
| public async ValueTask<ToolCallResponseV2> OnToolCallV2(string sessionId, |
There was a problem hiding this comment.
Do we need to maintain the V2 support?
This comment has been minimized.
This comment has been minimized.
![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 #1343 · ● 946.3K
| /// </code> | ||
| /// </example> | ||
| public IDisposable On(string eventType, Action<SessionLifecycleEvent> handler) | ||
| public IDisposable OnLifecycle<T>(Action<T> handler) where T : SessionLifecycleEvent |
There was a problem hiding this comment.
Cross-SDK consistency note: The lifecycle event subscription API has changed from two overloads (On(handler) and On(eventType, handler)) to a single generic method OnLifecycle<T>(handler). The other SDKs use a different pattern:
- Node.js:
client.on(handler)andclient.on(eventType, handler) - Python:
client.on(handler)andclient.on(event_type, handler) - Go:
client.On(handler)andclient.OnEventType(eventType, handler)
The generic type approach is idiomatic .NET and has real advantages (compile-time type safety), but the method name OnLifecycle also diverges from On / on. If this design is retained, it's worth documenting that this is a .NET-specific pattern for filtering by event type, and considering whether the approach (if not the exact API shape) should influence the other SDKs.
| /// When provided, the server will route <c>exitPlanMode.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| public ExitPlanModeHandler? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: The property has been renamed from OnExitPlanMode to OnExitPlanModeRequest, but the equivalent property in the other SDKs still uses the original name (without the Request suffix):
- Node.js:
SessionConfig.onExitPlanMode - Python:
create_session(..., on_exit_plan_mode=...) - Go:
SessionConfig.OnExitPlanMode
If this rename is intentional as an improvement to the API, the same change should be considered for the other SDKs. If the goal is cross-SDK consistency, the other SDKs should eventually align with this new naming (or this property should revert to OnExitPlanMode).
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This has been renamed from CreateSessionFsHandler to CreateSessionFsProvider, but all other SDKs use the ...Handler naming:
- Node.js:
SessionConfig.createSessionFsHandler - Python:
create_session(..., create_session_fs_handler=...) - Go:
SessionConfig.CreateSessionFsHandler
Interestingly, SessionFsProvider is already the name of the returned type in Node.js and Python too, so CreateSessionFsProvider could be seen as more descriptive. If this name is preferred, it may be worth aligning the other SDKs.
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This has been renamed from DisableResume to SuppressResumeEvent, but:
- Node.js:
ResumeSessionConfig.disableResume - Go:
ResumeSessionConfig.DisableResume - Python: Planned as
disable_resume(TODO comment in source)
The new name SuppressResumeEvent is arguably more descriptive, but it diverges from the other SDK naming. If this naming is preferred, the other SDKs should be updated to match.
| /// When provided, the server will route <c>autoModeSwitch.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public AutoModeSwitchHandler? OnAutoModeSwitch { get; set; } | ||
| public AutoModeSwitchHandler? OnAutoModeSwitchRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: Same naming divergence as OnExitPlanModeRequest — the other SDKs use onAutoModeSwitch / on_auto_mode_switch / OnAutoModeSwitch (without the Request suffix). These two renames should be treated together if they're carried through to the other SDKs.
| [JsonConverter(typeof(JsonStringEnumConverter<ConnectionState>))] | ||
| public enum ConnectionState | ||
| [DebuggerDisplay("{Value,nq}")] | ||
| public readonly struct CopilotLogLevel : IEquatable<CopilotLogLevel> |
There was a problem hiding this comment.
This is fine if you think it's useful to maintain a separate type, but we're already using ILogger{Factory}... should we just use LogLevel here? Internally we can translate it to whatever the runtime needs.
There was a problem hiding this comment.
I started with LogLevel but started thinking nothing stops the runtime from coming up with new loglevel values at any time that could have any semantics, without being constrained to things that match a .NET LogLevel. My guess is this won't inconvenience .NET devs too much since they aren't programmatically mapping some other loglevel into this in most cases, so they will still just pick whatever level they think is right. We could add an implicit conversion from LogLevel later if we want.
SteveSandersonMS
force-pushed
the
stevesandersonms/stevesa-api-review-fixes
branch
from
9f16314 to
a64803a
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.3M
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK inconsistency: GetEventsAsync vs getMessages
All other SDK implementations use the "Messages" naming for this method:
- Node.js:
session.getMessages()(nodejs/src/session.ts) - Python:
session.get_messages()(python/copilot/session.py) - Go:
session.GetMessages(ctx)(go/session.go) - Rust:
GetMessagesResponsestruct (rust/src/types.rs)
The .NET rename to GetEventsAsync diverges from the cross-SDK convention. Consider whether the other SDKs should also be updated to use "Events" naming, or whether this should remain GetMessagesAsync for consistency.
| /// When provided, the server will route <c>exitPlanMode.request</c> callbacks to this handler. | ||
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| public ExitPlanModeHandler? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: OnExitPlanModeRequest vs onExitPlanMode
Other SDKs use onExitPlanMode (without the Request suffix):
- Node.js:
onExitPlanMode?: ExitPlanModeHandler(nodejs/src/types.ts) - Python:
on_exit_plan_mode: ExitPlanModeHandler(python/copilot/session.py) - Go:
OnExitPlanMode ExitPlanModeHandler(go/types.go) - Rust:
on_exit_plan_modemethod (rust/src/handler.rs)
The added Request suffix here (and for OnAutoModeSwitchRequest below) makes the semantics more explicit, but creates a naming divergence from all other language SDKs. If the "Request" suffix is the preferred direction, the other SDKs should be updated to match.
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: SuppressResumeEvent vs disableResume
Other SDKs all use disableResume / disable_resume:
- Node.js:
disableResume?: boolean(nodejs/src/types.ts) - Python:
disable_resume: bool(python/copilot/session.py) - Go:
DisableResume bool(go/types.go) - Rust:
disable_resume: Option<bool>(rust/src/types.rs)
SuppressResumeEvent is semantically more descriptive, but it's a significant divergence from the cross-SDK disableResume convention. This is worth aligning across all SDKs if the new name is preferred.
| /// </summary> | ||
| [JsonPropertyName("maxPromptTokens")] | ||
| public int? MaxInputTokens { get; set; } | ||
| public int? MaxPromptTokens { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency: MaxPromptTokens vs maxInputTokens
Other SDKs expose this as maxInputTokens / MaxInputTokens in their public APIs (mapping to maxPromptTokens on the wire):
- Node.js:
maxInputTokens?: numberinProviderConfig(nodejs/src/types.ts:1676), mapped tomaxPromptTokenswire format inclient.ts - Go:
MaxInputTokens intinProviderConfig(go/types.go:991), tagged withjson:"maxPromptTokens,omitempty"
The .NET SDK now exposes MaxPromptTokens directly, which aligns with the wire format but diverges from the other SDKs' public APIs. If MaxPromptTokens is the better name, consider updating Node.js and Go as well.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.2M
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK consistency note: This rename from GetMessagesAsync to GetEventsAsync creates a naming inconsistency with the other SDK implementations, which all still use the getMessages / get_messages / GetMessages name:
- Node.js:
session.getMessages()(nodejs/src/session.ts) - Python:
session.get_messages()(python/copilot/session.py) - Go:
session.GetMessages()(go/session.go)
If GetEventsAsync is the better name (since the method returns all session events, not just messages), it would be worth tracking a follow-up to rename the equivalent methods in the other SDKs for consistency. Alternatively, aligning back to GetMessagesAsync would keep cross-SDK naming in sync today.
| /// Working directory for the runtime process. | ||
| /// </summary> | ||
| public string? Cwd { get; set; } | ||
| public string? WorkingDirectory { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: Three CopilotClientOptions properties have been renamed in this PR relative to the other SDKs:
| .NET (new) | Node.js | Python | Go |
|---|---|---|---|
WorkingDirectory |
cwd |
cwd |
Cwd |
BaseDirectory |
copilotHome |
copilot_home |
CopilotHome |
EnableRemoteSessions (line 331) |
remote |
remote |
Remote |
The new .NET names are arguably more descriptive/idiomatic for C#, but they diverge from the cross-SDK conceptual alignment. If there's intent to keep the API surface consistent in meaning (even if casing differs per language convention), it may be worth tracking follow-ups to update the other SDKs to similarly rename cwd → workingDirectory, copilotHome → baseDirectory, remote → enableRemoteSessions.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.1M
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK consistency note: This renames GetMessagesAsync → GetEventsAsync, but the other three SDKs still expose this method as getMessages / get_messages / GetMessages:
- Node.js (
nodejs/src/session.ts):session.getMessages() - Python (
python/copilot/session.py):session.get_messages() - Go (
go/session.go):session.GetMessages(ctx)
The rename is arguably more accurate (the method returns all session events, not just messages), but it creates a cross-SDK naming divergence. Is there a plan to align the other SDKs to getEvents / get_events / GetEvents in a follow-up PR?
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This renames CreateSessionFsHandler → CreateSessionFsProvider, but other SDKs still use the "Handler" suffix for this config property:
- Node.js (
nodejs/src/client.ts):createSessionFsHandler - Python (
python/copilot/session.py):create_session_fs_handler - Go (
go/types.go):CreateSessionFsHandler
Worth tracking whether the other SDKs should also rename to "Provider" for consistency.
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| /// <summary>Handler for exit-plan-mode requests from the server.</summary> | ||
| public Func<ExitPlanModeRequest, ExitPlanModeInvocation, Task<ExitPlanModeResult>>? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: This adds a Request suffix (OnExitPlanMode → OnExitPlanModeRequest, OnAutoModeSwitch → OnAutoModeSwitchRequest), but other SDKs don't have this suffix on their equivalent config properties:
- Node.js (
nodejs/src/client.ts):onExitPlanMode,onAutoModeSwitch - Python (
python/copilot/session.py):on_exit_plan_mode,on_auto_mode_switch - Go (
go/types.go):OnExitPlanMode,OnAutoModeSwitch
Not a blocker for a .NET API review, but worth noting for eventual cross-SDK alignment.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.4M
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename CreateSessionFsHandler → CreateSessionFsProvider diverges from the other SDKs:
- Node.js (
client.ts):createSessionFsHandler - Python (
session.py):create_session_fs_handler - Go (
types.go):CreateSessionFsHandler
CreateSessionFsProvider is semantically more accurate (a factory that creates a provider, not a handler), so this is a good rename worth carrying over to the other SDKs.
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| /// <summary>Handler for exit-plan-mode requests from the server.</summary> | ||
| public Func<ExitPlanModeRequest, ExitPlanModeInvocation, Task<ExitPlanModeResult>>? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename OnExitPlanMode → OnExitPlanModeRequest diverges from the other SDKs:
- Node.js (
client.ts):onExitPlanMode - Python (
session.py):on_exit_plan_mode - Go (
types.go):OnExitPlanMode
The Request suffix is a nice clarification (it handles a request from the server), so it would be good to track follow-up updates for the other SDKs to match.
| /// Gets the actual TCP port the runtime is listening on, if using TCP transport. | ||
| /// </summary> | ||
| public int? ActualPort => _actualPort; | ||
| public int? RuntimePort => _actualPort; |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename ActualPort → RuntimePort diverges from the other SDKs:
- Python (
client.py):actual_port(public property) - Go (
client.go):ActualPort()(public method) - Node.js: exposes port only internally, but error messages say
actualPort
RuntimePort aligns better with the new RuntimeConnection terminology in this PR, so it makes sense here. Consider whether Go and Python should also rename to runtime_port / RuntimePort for consistency.
| /// </summary> | ||
| public AutoModeSwitchHandler? OnAutoModeSwitch { get; set; } | ||
| /// <summary>Handler for auto-mode-switch requests from the server.</summary> | ||
| public Func<AutoModeSwitchRequest, AutoModeSwitchInvocation, Task<AutoModeSwitchResponse>>? OnAutoModeSwitchRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename OnAutoModeSwitch → OnAutoModeSwitchRequest diverges from the other SDKs:
- Node.js (
client.ts):onAutoModeSwitch - Python (
session.py):on_auto_mode_switch - Go (
types.go):OnAutoModeSwitch
Same as above — the Request suffix is an improvement worth propagating to the other SDKs.
| /// when connecting to an external runtime via <see cref="RuntimeConnection.Uri(string, string?)"/>. | ||
| /// </summary> | ||
| public bool Remote { get; set; } | ||
| public bool EnableRemoteSessions { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename Remote → EnableRemoteSessions diverges from the other SDKs:
- Node.js (
client.ts):remote - Python (
client.py):remote - Go (
types.go):Remote
EnableRemoteSessions is more self-descriptive. Consider a follow-up to rename in Node.js, Python and Go as well.
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| [Obsolete("AutoRestart has no effect and will be removed in a future release.")] | ||
| public bool AutoRestart { get; set; } | ||
| public string? BaseDirectory { get; set; } |
There was a problem hiding this comment.
Cross-SDK inconsistency introduced 🔍
The rename CopilotHome → BaseDirectory diverges from the other SDKs:
- Node.js (
client.ts):copilotHome - Python (
client.py):copilot_home - Go (
types.go):CopilotHome
BaseDirectory is a more neutral term (not coupling to the internal COPILOT_HOME environment variable name), so it's worth tracking as a potential follow-up rename in the other SDKs.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.1M
| /// </summary> | ||
| public ExitPlanModeHandler? OnExitPlanMode { get; set; } | ||
| /// <summary>Handler for exit-plan-mode requests from the server.</summary> | ||
| public Func<ExitPlanModeRequest, ExitPlanModeInvocation, Task<ExitPlanModeResult>>? OnExitPlanModeRequest { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: The .NET rename from OnExitPlanMode → OnExitPlanModeRequest (and OnAutoModeSwitch → OnAutoModeSwitchRequest on the next line) diverges from all other SDKs:
- Node.js:
onExitPlanMode/onAutoModeSwitch - Python:
on_exit_plan_mode/on_auto_mode_switch - Go:
OnExitPlanMode/OnAutoModeSwitch
If the *Request suffix is the correct semantic name (these are requests that the server sends to the client), it would be worth applying the same rename in the other SDKs too. If it's a .NET-only style choice, that's fine but worth documenting the intentional divergence.
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: CreateSessionFsProvider diverges from the name used in all other SDKs:
- Node.js:
createSessionFsHandler - Python:
create_session_fs_handler - Go:
CreateSessionFsHandler
If this rename from Handler → Provider is intentional (the field holds a factory for a SessionFsProvider, so "Provider" is semantically clearer), the same rename could be applied to the other SDKs for consistency.
| /// Gets the actual TCP port the runtime is listening on, if using TCP transport. | ||
| /// </summary> | ||
| public int? ActualPort => _actualPort; | ||
| public int? RuntimePort => _actualPort; |
There was a problem hiding this comment.
Cross-SDK consistency note: RuntimePort diverges from the name used in other SDKs:
- Node.js:
actualPort(private field — not exposed as a public property) - Python:
actual_port(public property) - Go:
ActualPort()(public method)
If RuntimePort is the preferred name going forward, the Python and Go SDKs could be updated. Node.js could also expose this publicly.
| /// </code> | ||
| /// </example> | ||
| public IDisposable On(SessionEventHandler handler) | ||
| public IDisposable On<T>(Action<T> handler) where T : SessionEvent |
There was a problem hiding this comment.
Cross-SDK opportunity: The new On<T>() generic method (where T : SessionEvent) allows callers to filter event subscriptions to a specific event type at compile time — a nice improvement. This capability doesn't currently exist in other SDKs:
- Node.js:
session.on(handler)— all events, no type filtering - Python:
session.on(handler)— all events, no type filtering - Go:
session.On(handler)— all events, no type filtering
Worth considering adding equivalent typed filtering to the other SDKs in a follow-up.
| /// when connecting to an external runtime via <see cref="RuntimeConnection.Uri(string, string?)"/>. | ||
| /// </summary> | ||
| public bool Remote { get; set; } | ||
| public bool EnableRemoteSessions { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: EnableRemoteSessions diverges from the name used in all other SDKs:
- Node.js:
remote - Python:
remote - Go:
Remote
Similarly, BaseDirectory (line ~260) diverges from CopilotHome / copilotHome / copilot_home in the other SDKs, and WorkingDirectory at the client options level (line ~251) diverges from Cwd / cwd in Go, Node.js, and Python.
If these renames are considered improvements (e.g., EnableRemoteSessions is more self-documenting than Remote), it would be worth aligning the other SDKs in follow-up PRs.
| /// <param name="prompt">The user message text.</param> | ||
| /// <param name="cancellationToken">A <see cref="CancellationToken"/> that can be used to cancel the operation.</param> | ||
| /// <returns>A task that resolves with the message ID.</returns> | ||
| public Task<string> SendAsync(string prompt, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK opportunity: The new SendAsync(string prompt) and SendAndWaitAsync(string prompt) string overloads are a convenience improvement. Status across other SDKs:
- Python: Already accepts
prompt: strdirectly — consistent ✅ - Go:
Send(ctx, MessageOptions)requires a struct — no string shorthand - Node.js:
send({ prompt: "..." })requires an object — no string shorthand
Go and Node.js could benefit from adding equivalent string convenience overloads/variants.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1343 · ● 1.9M
| /// Default: false (resume event is emitted). | ||
| /// </summary> | ||
| public bool DisableResume { get; set; } | ||
| public bool SuppressResumeEvent { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: SuppressResumeEvent diverges from the other SDK implementations, which all use DisableResume / disableResume / disable_resume:
- Go:
DisableResume bool(go/types.go:932) - Python:
disable_resume: bool(python/copilot/session.py:1062) - Node.js:
disableResume?: boolean(nodejs/src/types.ts:1593)
Worth aligning on a single canonical name if this is an intentional rename, or tracking a follow-up to update the other SDKs.
| /// </code> | ||
| /// </example> | ||
| public async Task<IReadOnlyList<SessionEvent>> GetMessagesAsync(CancellationToken cancellationToken = default) | ||
| public async Task<IReadOnlyList<SessionEvent>> GetEventsAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Cross-SDK consistency note: GetEventsAsync diverges from the other SDK implementations, which all use GetMessages / get_messages:
- Go:
func (s *Session) GetMessages(ctx context.Context)(go/session.go:1170) - Python:
async def get_messages(self)(python/copilot/session.py:2217) - Node.js:
async getMessages()(nodejs/src/session.ts:994)
If GetEvents is the preferred canonical name going forward, the other three SDKs would need follow-up updates.
| /// </summary> | ||
| [JsonPropertyName("maxPromptTokens")] | ||
| public int? MaxInputTokens { get; set; } | ||
| public int? MaxPromptTokens { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: MaxPromptTokens diverges from the other SDKs' public ProviderConfig API, which all expose this as MaxInputTokens / maxInputTokens / max_input_tokens (each transparently remapping to the wire field maxPromptTokens):
- Go:
MaxInputTokens int→json:"maxPromptTokens"(go/types.go:991) - Python:
max_input_tokens: int(mapped to wiremaxPromptTokensinclient.py:2533) - Node.js:
maxInputTokens?: number(mapped to wiremaxPromptTokensinclient.ts:78)
Renaming only the .NET property creates an inconsistency. If MaxPromptTokens is the preferred name, the other SDKs should follow.
| /// Gets the actual TCP port the runtime is listening on, if using TCP transport. | ||
| /// </summary> | ||
| public int? ActualPort => _actualPort; | ||
| public int? RuntimePort => _actualPort; |
There was a problem hiding this comment.
Cross-SDK consistency note: RuntimePort diverges from the other SDKs, which use ActualPort / actual_port:
- Go:
func (c *Client) ActualPort() int(go/client.go:1300) - Python:
def actual_port(self) -> int | None(python/copilot/client.py:1026)
If RuntimePort is the preferred name, consider updating the Go and Python equivalents in a follow-up.
| /// This is used only when <see cref="CopilotClientOptions.SessionFs"/> is configured. | ||
| /// </summary> | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsHandler { get; set; } | ||
| public Func<CopilotSession, SessionFsProvider>? CreateSessionFsProvider { get; set; } |
There was a problem hiding this comment.
Cross-SDK consistency note: CreateSessionFsProvider diverges from the other SDKs, which use createSessionFsHandler / create_session_fs_handler:
- Node.js:
createSessionFsHandler?inSessionConfig(nodejs/src/types.ts:1545) - Python:
create_session_fs_handler: CreateSessionFsHandler(python/copilot/session.py:994)
This is a minor naming difference, but worth tracking for eventual alignment.
…ForUri Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Missed in 5e41280; this file was excluded from the original add because of an earlier uncommitted debug hunk that has since been committed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The package name is not changing; only the C# namespace lost the .SDK suffix. My earlier bulk rename incorrectly stripped .SDK from these three docs install commands too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…son refs After the namespace rename, GeneratedStringEnumJson sits in the parent namespace of the generated converters (GitHub.Copilot.Rpc), so the unqualified name resolves correctly. Removes leftover qualifier that was needed to disambiguate against the old GitHub.Copilot.SDK namespace. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesandersonms/stevesa-api-review-fixes
branch
from
c1e5d5c to
3e1a3e1
Compare
This comment has been minimized.
This comment has been minimized.
The runtime tracks 'last session id' as a persistent value (it survives session deletion), so this test fails if any other test in the class has run first. Latent on main because xUnit happened to run it first; exposed on this branch by the namespace rename which changes the fully-qualified type name and therefore the discovered test order. Cleaning up any pre-existing sessions before reading the value lets the test pass regardless of order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency ReviewThis PR is correctly scoped to the .NET SDK only, as stated in the description. The changes are high quality and well-considered. Here's a consistency tracking summary for when the improvements are ported to other SDKs. ✅ Already consistent: Typed session event filteringThe Node.js SDK already has an equivalent to the new 🔄 Divergences to track for future porting1. .NET now uses: new CopilotClientOptions { Connection = RuntimeConnection.ForStdio(path: "...") }
new CopilotClientOptions { Connection = RuntimeConnection.ForUri("localhost:3000") }The other SDKs still use flat, mutually-exclusive options:
The discriminated union approach is idiomatic in TypeScript (tagged unions / 2.
When porting, 3.
When porting, consider aligning to 4. .NET extracted a No action needed on this PR — these are all intentional and tracked divergences. Just leaving this note to make the porting work easier. 🚀
|
- Remove autoStart and autoRestart from CopilotClientOptions. The client now always starts on first createSession/resumeSession; users can still call client.start() explicitly for eager startup. - Make MCPServerConfigBase.tools optional (undefined = all, [] = none). - Fix streaming JSDoc block comment that wasn't attached due to single-star. Mirrors C# PR #1343 Phase 4c. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Split SessionLifecycleEvent into a discriminated union of SessionCreatedEvent / SessionDeletedEvent / SessionUpdatedEvent / SessionForegroundEvent / SessionBackgroundEvent. - Promote the metadata payload into a named SessionLifecycleEventMetadata interface; metadata is required on non-delete variants and absent on session.deleted. - Convert metadata.startTime and metadata.modifiedTime from string to Date, matching SessionMetadata. Parse on receipt in client.handleSessionLifecycleNotification. - Export the new variant types from index.ts. Mirrors C# PR #1343 Phase 4f + review §2.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Change BaseHookInput.timestamp from number (Unix ms) to Date. - Parse incoming numeric timestamps into Date in handleHooksInvoke. - Update hooks_extended.e2e.test.ts assertion accordingly. Mirrors C# PR #1343 Phase 4g. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…equest union - Add optional feedback?: string field to PermissionRequestResult so consumers can return free-form text forwarded to the model with the decision. - Delete the hand-written narrow PermissionRequest interface in types.ts and re-export the generated discriminated union from session-events.ts instead. Handlers can now type-safely access per-kind fields (e.g. shell .commands, write .fileName / .diff, mcp .toolName / .args). Mirrors C# PR #1343 Phase 4g + review §2.9. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces the fragile Pick<SessionConfig, '...30+ keys...'> definition for ResumeSessionConfig with a shared SessionConfigBase interface. SessionConfig and ResumeSessionConfig now both extend it: - SessionConfig adds sessionId? and cloud?. - ResumeSessionConfig adds suppressResumeEvent? and continuePendingWork?. SessionConfigBase is exported from index.ts for consumers that want to build shared helpers over both shapes. Mirrors C# PR #1343 Phase 5 + review §2.2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces the flat connection-related fields on CopilotClientOptions
(cliPath, cliArgs, port, useStdio, cliUrl, tcpConnectionToken,
isChildProcess) with a single discriminated 'connection?: RuntimeConnection'
field. Construct values via factory functions:
RuntimeConnection.forStdio({ path?, args? }) // default
RuntimeConnection.forTcp({ port?, connectionToken?, path?, args? })
RuntimeConnection.forUri(url, { connectionToken? })
The mutually-exclusive combinations that used to be runtime errors are
now caught at compile time by the discriminated union. The previous
isChildProcess flag (only ever used by joinSession() in extension.ts) is
dropped from the public API surface; extension.ts now uses an @internal
_internalConnection hook to enter the parent-process stdio mode.
Other renames in this phase:
- CopilotClientOptions.copilotHome -> baseDirectory.
- Internal CopilotClient.actualPort field -> runtimePort.
All TS test files, scenario fixtures, samples, README, and docs updated
to the new shape. Mirrors C# PR #1343 Phase 9.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both methods now accept either a MessageOptions object or just a string
prompt. The string form is a shorthand for { prompt }:
await session.send('Hello');
await session.sendAndWait('Hello');
Mirrors C# PR #1343 Phase 7.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…SDK consistency Matches the C# API review rename in #1343 (EnableRemoteSessions on CopilotClientOptions). The wire-level RPC field stays 'remote' since that is the runtime's contract; only the SDK surface changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Phase A: property/method renames on SessionConfig/ResumeSessionConfig Mirrors C# PR #1343 Phase 4a renames: - onExitPlanMode -> onExitPlanModeRequest - onAutoModeSwitch -> onAutoModeSwitchRequest - createSessionFsHandler -> createSessionFsProvider - ResumeSessionConfig.disableResume -> suppressResumeEvent - ProviderConfig.maxInputTokens -> maxPromptTokens (drops the wire shim) - CopilotSession.getMessages() -> getEvents() - InputOptions -> UiInputOptions Wire RPC name 'session.getMessages' is unchanged (runtime contract). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase C: CopilotClientOptions / MCP / streaming shape changes - Remove autoStart and autoRestart from CopilotClientOptions. The client now always starts on first createSession/resumeSession; users can still call client.start() explicitly for eager startup. - Make MCPServerConfigBase.tools optional (undefined = all, [] = none). - Fix streaming JSDoc block comment that wasn't attached due to single-star. Mirrors C# PR #1343 Phase 4c. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase D: lifecycle event polymorphic union + Date timestamps - Split SessionLifecycleEvent into a discriminated union of SessionCreatedEvent / SessionDeletedEvent / SessionUpdatedEvent / SessionForegroundEvent / SessionBackgroundEvent. - Promote the metadata payload into a named SessionLifecycleEventMetadata interface; metadata is required on non-delete variants and absent on session.deleted. - Convert metadata.startTime and metadata.modifiedTime from string to Date, matching SessionMetadata. Parse on receipt in client.handleSessionLifecycleNotification. - Export the new variant types from index.ts. Mirrors C# PR #1343 Phase 4f + review §2.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase E: hook input timestamps as Date - Change BaseHookInput.timestamp from number (Unix ms) to Date. - Parse incoming numeric timestamps into Date in handleHooksInvoke. - Update hooks_extended.e2e.test.ts assertion accordingly. Mirrors C# PR #1343 Phase 4g. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase F: PermissionRequestResult.feedback + use generated PermissionRequest union - Add optional feedback?: string field to PermissionRequestResult so consumers can return free-form text forwarded to the model with the decision. - Delete the hand-written narrow PermissionRequest interface in types.ts and re-export the generated discriminated union from session-events.ts instead. Handlers can now type-safely access per-kind fields (e.g. shell .commands, write .fileName / .diff, mcp .toolName / .args). Mirrors C# PR #1343 Phase 4g + review §2.9. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase G: extract SessionConfigBase Replaces the fragile Pick<SessionConfig, '...30+ keys...'> definition for ResumeSessionConfig with a shared SessionConfigBase interface. SessionConfig and ResumeSessionConfig now both extend it: - SessionConfig adds sessionId? and cloud?. - ResumeSessionConfig adds suppressResumeEvent? and continuePendingWork?. SessionConfigBase is exported from index.ts for consumers that want to build shared helpers over both shapes. Mirrors C# PR #1343 Phase 5 + review §2.2. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase H: defineTool({ name, ... }) single-arg form Change defineTool from defineTool(name, config) to defineTool({ name, ...config }) so the call shape matches the Tool<T> interface. name remains mandatory and is enforced by the Tool<T> type. Updates all samples, docs, tests, and the CHANGELOG snippet. Review §1.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Revert "Phase H: defineTool({ name, ... }) single-arg form" This reverts commit 49da911. * Phase I: RuntimeConnection discriminated config Replaces the flat connection-related fields on CopilotClientOptions (cliPath, cliArgs, port, useStdio, cliUrl, tcpConnectionToken, isChildProcess) with a single discriminated 'connection?: RuntimeConnection' field. Construct values via factory functions: RuntimeConnection.forStdio({ path?, args? }) // default RuntimeConnection.forTcp({ port?, connectionToken?, path?, args? }) RuntimeConnection.forUri(url, { connectionToken? }) The mutually-exclusive combinations that used to be runtime errors are now caught at compile time by the discriminated union. The previous isChildProcess flag (only ever used by joinSession() in extension.ts) is dropped from the public API surface; extension.ts now uses an @internal _internalConnection hook to enter the parent-process stdio mode. Other renames in this phase: - CopilotClientOptions.copilotHome -> baseDirectory. - Internal CopilotClient.actualPort field -> runtimePort. All TS test files, scenario fixtures, samples, README, and docs updated to the new shape. Mirrors C# PR #1343 Phase 9. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase J: send / sendAndWait string overloads Both methods now accept either a MessageOptions object or just a string prompt. The string form is a shorthand for { prompt }: await session.send('Hello'); await session.sendAndWait('Hello'); Mirrors C# PR #1343 Phase 7. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase K: stripInternal, AsyncDisposable, clean stop(), drop destroy() - Enable stripInternal in tsconfig.json so @internal members no longer appear in the published .d.ts. Verified that CopilotSession constructor, the register*/clientSessionApis hooks, _handle* methods, NO_RESULT_PERMISSION_V2_ERROR, _internalConnection, and ParentProcessRuntimeConnection are all stripped from the public types. The MessageConnection import from vscode-jsonrpc no longer leaks either. - Tag NO_RESULT_PERMISSION_V2_ERROR with @internal explicitly. - Implement Symbol.asyncDispose on CopilotClient so it works inside 'await using' blocks, matching CopilotSession. - Tighten client.stop() so the Node process can exit cleanly without process.exit(): socket.destroy() in addition to socket.end(), explicit destroy() on the child process stdio streams, and cliProcess.unref(). Manually verified by running examples/basic-example.ts against a live runtime: the process exits within a few seconds of the await using block ending. - Remove the deprecated CopilotSession.destroy() alias. - Rewrite examples/basic-example.ts to import from '@github/copilot-sdk' (not '../src/index.js') and demonstrate the await using pattern with the new send/sendAndWait string overloads. Covers review §2.4, §2.5, §2.10, §3.1, §3.2, §4.1, §4.3 and the C# PR's Phase 8 docs/sample updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase L: fix githubToken typos in scenario fixtures Twenty-one test/scenarios/**/typescript/src/index.ts files used 'githubToken: process.env.GITHUB_TOKEN' (lowercase h) instead of the correct 'gitHubToken'. They silently passed an unrecognized property and the runtime ignored the token. Fix in lock-step across all affected scenarios. The existing 'typecheck' npm script already runs 'tsc --noEmit -p tsconfig.test.json' in CI, so no further CI wiring is needed to prevent regressions: this typo would now be a compile error under the SDK's strict CopilotClientOptions shape. Other Phase L items (missing onPermissionRequest, invalid permission kinds, resumeSession scenarios without config) were either already caught by the runtime PR or do not apply to TS — no remaining work. Covers review §1.1, §2.1, §2.8, §2.11. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase L follow-up: run prettier --write over all modified files Prettier check failed on Ubuntu CI because several files modified in earlier phases didn't get re-formatted after the bulk regex rewrites. Running 'npm run format' (prettier --write) normalizes them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix Phase I/E/L test failures surfaced by CI - commands/multi-client/ui_elicitation: shorthand 'copilotClientOptions: { tcpConnectionToken }' wasn't matched by the earlier batch rewrite, so client1 was still spawned in stdio mode while client2 tried to connect by URI. Switch the harness call to TCP + RuntimeConnection.forTcp({ connectionToken: tcpConnectionToken }). - session_fs.e2e.test.ts: add the missing RuntimeConnection import. - hooks_extended.e2e.test.ts: SessionStart and UserPromptSubmitted timestamp assertions still used toBeGreaterThan(0) but BaseHookInput.timestamp is now Date. Switch to toBeInstanceOf(Date). - client.test.ts: delete the two obsolete 'allows *Session without onPermissionRequest' unit tests. They asserted on the 'Client not connected' error that only occurred when autoStart was false; with Phase C removing autoStart, the client now auto-starts on the first session call and those tests would need a real spawned runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add E2E equivalents of removed createSession/resumeSession-without-permission tests The two client.test.ts unit tests deleted in the previous commit only asserted that createSession/resumeSession surface 'Client not connected' when called pre-start. The intent behind them was to confirm that omitting onPermissionRequest doesn't itself throw. With autoStart gone, the only meaningful version of that test is an E2E one that actually spawns a runtime. Port the equivalent C# coverage (ClientE2ETests.Should_Allow_*Session_Called_Without_PermissionHandler) to client.e2e.test.ts so we have parity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add E2E tests for createSession/resumeSession without onPermissionRequest Ports dotnet's Should_Allow_CreateSession_Called_Without_PermissionHandler (Theory: stdio + tcp) and Should_Allow_ResumeSession_Called_Without_PermissionHandler from dotnet/test/E2E/ClientE2ETests.cs into nodejs/test/e2e/session.e2e.test.ts. These exercise the contract that {onPermissionRequest} is optional on both SessionConfig and ResumeSessionConfig: when not provided, the runtime leaves permission prompts pending for the consumer to resolve via the low-level RPC. Without these tests, that contract was unprotected against regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harness: preserve caller-supplied RuntimeConnection while still injecting CLI path Earlier batch rewrite of createSdkTestContext meant that whenever a test passed copilotClientOptions.connection, the spread overrode the harness's own connection variant entirely - losing the COPILOT_CLI_PATH binding. Now merge by variant kind: if the caller asks for tcp/stdio without a path, the harness fills it in from COPILOT_CLI_PATH; explicit values from the caller win. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * session_fs.e2e: fix unconverted tcpConnectionToken flat key on inner client In the 'should reject setProvider when sessions already exist' test, the first client was still using the flat tcpConnectionToken property which is no longer a valid CopilotClientOptions field. Switch to RuntimeConnection.forTcp({ connectionToken }). Verified locally: full session_fs.e2e.test.ts suite (9 tests) now passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move hook input deserialization next to the cast that types it normalizeHookInput lived in client.ts and inspected for a 'timestamp' property by name, which felt magical (brittle against any future hook-shaped wire payload that happens to contain a numeric 'timestamp'). Move the conversion into CopilotSession._handleHooksInvoke, renamed deserializeHookInput, right next to the GenericHandler cast that says 'this unknown is now a HookInput'. That's the only call site that actually knows the payload is a hook input, so it's the correct boundary for the schema transform. This is the TS equivalent of what C# does via UnixMillisecondsDateTimeOffsetConverter (attached per-property on each HookInput.Timestamp); TS just plumbs the same conversion through the hooks dispatcher instead of a per-type JSON converter. Verified 3/3 hooks_extended.e2e tests pass locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Phase K refinement: use unref() instead of destroy() in client.stop() destroy() on the child's stdio pipes and the TCP socket is more aggressive than needed. If the child crashes with a useful message on stderr that our existing data listener hasn't drained yet, stderr.destroy() drops it. If there's an in-flight write to stdin, destroy() raises 'error' on the stream. Same trade-off for socket.destroy() short-circuiting the graceful FIN/ACK. unref() solves the actual problem (event loop staying alive after stop()) without disrupting late output. From the Node docs: 'unref will allow the program to exit if this is the only active socket in the event system. The socket does not lose any functionality' — error events still fire, late data still drains through registered listeners. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * client.stop(): await child exit and socket close Replace the unref()-based fire-and-forget cleanup with deterministic awaiting: - Socket: end() + await 'close'. By the time stop() returns the FIN/ACK exchange has completed. - ChildProcess: kill() + await 'exit'. By the time stop() returns the child has truly exited, its stdio pipes are closed, and there are no lingering handles to keep the event loop alive. No SIGKILL escalation. If the child ignores SIGTERM, stop() blocks; callers that need a guaranteed-bounded shutdown should use forceStop() (which already sends SIGKILL). Replaces the previous unref() approach: that worked for clean exit but allowed late stderr output to surface after stop() resolved, which is exactly the timing window where consumers expect cleanup to be done. Verified locally: full client.test.ts (75 tests) passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Revert incorrect feedback widening of PermissionRequestResult The previous '& { feedback?: string }' incorrectly added feedback to every variant of the union. In the runtime schema, feedback is reject-only — it appears only on PermissionDecisionReject and is already typed by the generated PermissionDecisionRequest['result'] union. No manual augmentation needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Rename client.on -> client.onLifecycle for cross-SDK consistency Matches the C# rename in PR #1357 Phase 4f (client.On -> client.OnLifecycle). client.onLifecycle is clearer than client.on at the call site because the two on() methods on CopilotClient and CopilotSession listen for completely different event families (lifecycle vs per-session). The receiver alone isn't always enough to disambiguate, especially in mixed code that holds both objects. session.on stays unchanged because that's where the bare 'on' verb belongs: session events are the primary stream for that object. Updates README + client_lifecycle.e2e.test.ts to the new name. Tests still pass (validated via tsc -p tsconfig.test.json). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reformat src/types.ts after PermissionRequestResult revert Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address PR review comments from #1357 - Add missing RuntimeConnection imports to 26 test/scenarios TypeScript fixtures. The earlier batch rewrite added the .forStdio/.forUri call sites but missed the corresponding import statement, so each scenario failed to compile until now. - nodejs/test/e2e/harness/sdkTestContext.ts: strip the 'kind' property before spreading a user-supplied RuntimeConnection back through the forStdio/forTcp factory opts. The factory opt types don't accept 'kind' so the spread was producing excess-property type errors. - nodejs/src/client.ts: rewrite the 'Path to Copilot CLI is required' error message to point at the new connection options (RuntimeConnection.forStdio({ path }), forTcp({ path }), forUri(...), or the COPILOT_CLI_PATH environment variable). The old message referenced removed cliPath / cliUrl options. - nodejs/src/client.ts: change the default logLevel from 'debug' to 'info'. 'debug' was a TS-only outlier; Python and Rust default to 'info', and the README has always claimed 'info'. Go and .NET don't pass --log-level at all when omitted (CLI defaults to info anyway), so 'info' is consistent with every other SDK's effective default. - nodejs/src/types.ts: fix MCPServerConfigBase.tools doc comment to spell the all-tools sentinel as ['*'] (the actual type is string[], so a bare '*' string can't be passed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * logLevel: don't impose any default, match C#/Go Instead of defaulting to 'info' on the SDK side, omit the --log-level flag entirely when the caller didn't set one and let the runtime use its own default. Matches dotnet/Client.cs and go/client.go, which both only pass --log-level when explicitly provided. CopilotClientOptions.logLevel JSDoc and README updated to describe this ('When omitted, the runtime uses its own default'). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Rename CopilotClientOptions.remote -> enableRemoteSessions for cross-SDK consistency Matches the C# API review rename in #1343 (EnableRemoteSessions on CopilotClientOptions). The wire-level RPC field stays 'remote' since that is the runtime's contract; only the SDK surface changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
C# codegen now hard-errors instead of falling back to `object` for unmappable schema nodes. Schemas tagged with `x-opaque-json: true` map to `JsonElement`/`JsonElement?`. Hand-written types that round-trip arbitrary JSON (ToolInvocation.Arguments, ToolResultObject.ToolTelemetry, hook *Input* ToolArgs/ToolResult) are retyped to JsonElement to match the generated wire types. Boundary code in Session/Client/SessionFsProvider converts between the hand-written user-facing object?-typed dictionaries (ElicitationSchema.Properties, ElicitationResult.Content) and the wire JsonElement form. This is the SDK side of github/copilot-agent-runtime#8375 — replays the non-Generated/* changes from the reverted PR #1343. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase A - SessionConfig/ResumeSessionConfig renames
- OnExitPlanMode -> OnExitPlanModeRequest
- OnAutoModeSwitch -> OnAutoModeSwitchRequest
- ExitPlanModeHandler -> ExitPlanModeRequestHandler
- AutoModeSwitchHandler -> AutoModeSwitchRequestHandler
- Session.GetMessages -> Session.GetEvents
- ResumeSessionConfig.DisableResume -> SuppressResumeEvent
- Streaming bool -> *bool on SessionConfig and ResumeSessionConfig
Wire-level RPC method (session.getMessages) and internal request types
are unchanged; only the public Go surface is renamed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase B+C - ProviderConfig + MCP Tools tweaks
B. ProviderConfig.MaxInputTokens -> MaxPromptTokens (matches the wire
name 'maxPromptTokens' exactly).
C. MCPStdioServerConfig.Tools / MCPHTTPServerConfig.Tools: change JSON
tag from 'tools' to 'tools,omitempty'. Now nil/omitted slice means
'all tools' (CLI default), matching TS/Rust/C# semantics.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase D - RuntimeConnection refactor
Replaces the flat connection fields on ClientOptions with a single
discriminated Connection RuntimeConnection field constructed from one of:
StdioConnection{Path, Args}
TcpConnection{Port, ConnectionToken, Path, Args}
UriConnection{URL, ConnectionToken}
Removes:
- CLIPath, CLIArgs, CLIUrl, UseStdio, Port, TCPConnectionToken
- AutoStart, AutoRestart
- public ConnectionState type, State* constants, and Client.State() method
Renames:
- CopilotHome -> BaseDirectory
- Remote -> EnableRemoteSessions
- Client.ActualPort() -> Client.RuntimePort()
LogLevel no longer defaults to 'info'. When empty, the SDK does not
pass --log-level to the runtime, matching the TS SDK.
All unit tests, e2e tests, samples, and the Go scenario apps are
migrated. README updated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase E - lifecycle timestamps as time.Time
SessionMetadata and SessionLifecycleEventMetadata: StartTime and
ModifiedTime change from string to time.Time. The runtime emits these
as ISO 8601 strings, which time.Time's default JSON unmarshal handles
natively.
Matches the equivalent C# (#1343) / TS (#1357) changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase F - hook timestamps as time.Time
All six hook input types (PreToolUseHookInput, PostToolUseHookInput,
UserPromptSubmittedHookInput, SessionStartHookInput, SessionEndHookInput,
ErrorOccurredHookInput): Timestamp changes from int64 to time.Time. Each
type gains a MarshalJSON/UnmarshalJSON pair that serializes Timestamp as
Unix milliseconds on the wire, matching the runtime protocol.
Mirrors the equivalent C# (#1343) UnixMillisecondsDateTimeOffsetConverter
and TS (#1357) Date-typed hook timestamps.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phases G-K - misc renames and cleanups
G. InputOptions -> UiInputOptions on the Session.UI().Input convenience.
H. Add Session.SendPrompt(ctx, prompt) and Session.SendPromptAndWait(ctx, prompt)
convenience wrappers around the MessageOptions-based methods (mirrors the
string overloads added in C#/TS).
I. SessionFsProvider interface method renames (Go-specific): Mkdir ->
MakeDirectory, Readdir -> ReadDirectory, ReaddirWithTypes ->
ReadDirectoryWithTypes, Rm -> Remove. The adapter still implements the
wire-protocol method names (rpc.SessionFsHandler) unchanged.
J. SessionConfig/ResumeSessionConfig.CreateSessionFsHandler ->
CreateSessionFsProvider (matches the SessionFsProvider type it returns).
K. Remove deprecated Session.Destroy(); callers must use Session.Disconnect().
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Go SDK API review: Phase L - docs and scenario migrations
Update Go code samples in docs/ to use the new ClientOptions.Connection
shape (StdioConnection / UriConnection). Also migrate the streaming
scenario to copilot.Bool(true) for the new *bool Streaming field.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix Streaming: true in Go doc snippets after *bool change
CI docs-validate extracts Go code blocks and compiles them. Update the
remaining 4 sites (docs/getting-started.md x3, docs/features/streaming-events.md)
to use copilot.Bool(true) now that SessionConfig.Streaming is *bool.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Apply gofmt
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* MCP Tools: change []string to *[]string to preserve nil vs empty
With `json:"tools,omitempty"` on a bare []string, Go collapses both
nil and []string{} to "omitted", losing the documented distinction
between "all tools" (nil) and "no tools" (empty slice). Switch the
field type to *[]string so a non-nil pointer to an empty slice
serializes as `tools: []` on the wire, matching TS `tools?: string[]`
and C# `IList<string>?` with WhenWritingNull.
Callers use the standard Go idiom for pointer-to-slice literals:
Tools: &[]string{"*"} // explicit all tools
Tools: &[]string{} // no tools
Tools: &[]string{"a","b"} // only those tools
Tools: nil // (default) all tools, field omitted
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* docs: show StdioConnection.Args for Go --log-dir snippet
The Go SDK now supports extra runtime args via StdioConnection.Args /
TcpConnection.Args. Replace the outdated 'Go cannot pass args, run the
CLI manually' snippet with the actual idiomatic call.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Drop TS/C# reference from MCPStdioServerConfig doc comment
Go SDK source shouldn't reference other-language SDKs. Rephrase the
Tools field doc to explain the pointer-to-slice form purely in Go
terms.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mirrors PR #1343 Phase 9 (.NET) and PR #1357 Phase I (TypeScript). Removes the flat `SubprocessConfig` / `ExternalServerConfig` split in favour of a `RuntimeConnection` discriminated hierarchy, with process-management options moved to a new `CopilotClientOptions` dataclass. Public API: * `RuntimeConnection` abstract base with static factories `stdio()`, `tcp()`, `uri()`. * `ChildProcessRuntimeConnection` intermediate base carrying `path` and `args` shared by Stdio + Tcp. * `StdioRuntimeConnection`, `TcpRuntimeConnection`, `UriRuntimeConnection` concrete subclasses; pattern-match / `isinstance` on the class to branch on the transport. * `CopilotClientOptions(connection=..., working_directory=..., log_level=..., env=..., github_token=..., base_directory=..., use_logged_in_user=..., telemetry=..., session_fs=..., session_idle_timeout_seconds=..., enable_remote_sessions=...)`. * `CopilotClient(options=CopilotClientOptions(...) | None, *, auto_start=..., on_list_models=...)`. Renames: * `copilot_home` → `base_directory` * `remote` → `enable_remote_sessions` * `CopilotClient.actual_port` → `CopilotClient.runtime_port` * `CopilotClient.on(...)` → `CopilotClient.on_lifecycle(...)` * `tcp_connection_token` moves off `CopilotClientOptions` and onto the variant: `TcpRuntimeConnection.connection_token` / `UriRuntimeConnection.connection_token` * `use_stdio` bool is gone — the variant class IS the discriminator. Migration: * All hand-written code that previously branched on `isinstance(config, ExternalServerConfig)` now branches on the runtime-connection variant via `isinstance(connection, UriRuntimeConnection)` / `StdioRuntimeConnection` etc., giving proper type narrowing in pyright/mypy without relying on Literal-based tagged-union narrowing. * Samples, scenarios, README, and tests updated in lock-step. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
API review fixes for the .NET SDK only. Sources combined into this PR:
api_review_csharp.mdreview doc.Other SDKs are not touched here; once we are happy with the .NET shape we will port what makes sense.
What changed (by phase)
SessionFsProvider.Streamingflag, tools list shape,ToolBinaryResultTypecleanup, misc.CopilotClient.Stateenum; retypedLogLevelto a strongly-typedCopilotLogLevelvalue type.client.OnLifecycle<T>/session.On<T>; lifecycle events are now a typed polymorphic hierarchy (SessionCreatedEvent, etc.) instead of stringly-typed discriminators.DateTimeOffsettimestamps;PermissionRequestResult.Feedbackcleanup.SessionConfigBasebetweenSessionConfig/ResumeSessionConfig; sealed all config classes.Func<...>.SendAsync(string)/SendAndWaitAsync(string)convenience overloads.RuntimeConnectiondiscriminated config: replaces the flatCliPath/CliArgs/Port/UseStdio/CliUrl/TcpConnectionTokenproperties onCopilotClientOptionswith a singleConnectionof typeRuntimeConnection, constructed via factories:RuntimeConnection.Stdio(...),RuntimeConnection.Tcp(...),RuntimeConnection.Uri(...). Also renamesActualPort->RuntimePortandCopilotHome->BaseDirectory.The test harness also got a small UX improvement:
Ctx.CreateClient(useStdio: ..., options: ... { Connection = ... })now throws eagerly if both are supplied. This caught a real bug introduced by Phase 9 bulk rewrite.Notable thing we explicitly did NOT do: Phase 1
Phase 1 was originally going to retype every "opaque JSON" RPC field from
objecttoSystem.Text.Json.JsonElement, on the theory thatJsonElementis more honest about wire shape thanobject(which cannot be safely cast). The change was reverted on this branch (see commits129c281eand074ace71) for two reasons:The underlying problem is in the runtime schema, not the SDK. Several RPC parameters/results are polymorphic
anyOfunions that have no idiomatic mapping in any of our SDK languages. Tracked in copilot-agent-runtime#8375: the schema generator should reject shapes that have no idiomatic mapping in C#/Go/Python/Rust, and require an explicit opt-in for cases that genuinely are meant to be opaque JSON. Once that lands, codegen will produce real typed hierarchies and theJsonElementworkaround stops being needed.Phase 1 asymmetry created a worse consumer experience for inputs. Changing input parameters from
objecttoJsonElementforced consumers to serialize themselves via their ownJsonSerializerOptions, which silently emitted"headers": nulland similar (becauseWhenWritingNullis not a default) and caused the runtime to reject otherwise-valid configs. On the previousobjectpath, the SDK ownJsonSerializerContext(which setsWhenWritingNull) was used, so this never bit consumers. We could have applied per-property[JsonIgnore(WhenWritingNull)]workarounds, but they are band-aids; the right fix is the schema change above. Reverting now keeps the API symmetric and aligned with main.We will revisit
JsonElementonce #8375 lands and codegen can emit real types.Risk / compatibility
This is a breaking-change PR by design. All E2E and unit tests have been updated in lock-step. The
dotnet/samplesanddocs/C# snippets have also been migrated.Two issues filed against the runtime as follow-ups to this work: