Skip to content

Go SDK API review fixes#1360

Merged
SteveSandersonMS merged 12 commits into
mainfrom
stevesandersonms/go-sdk-api-review
May 21, 2026
Merged

Go SDK API review fixes#1360
SteveSandersonMS merged 12 commits into
mainfrom
stevesandersonms/go-sdk-api-review

Conversation

@SteveSandersonMS
Copy link
Copy Markdown
Contributor

Applies the equivalent of the C# (#1343) and TypeScript (#1357) API review fixes to the Go SDK, plus the Go-specific items from api_review_go.md.

This is a breaking-change PR by design, intentionally landed in one pass before the SDK is declared stable. All unit tests, e2e tests, samples, docs, and the Go scenario apps under test/scenarios/**/go were migrated in lockstep with the source changes. (E2E suites still need the local nodejs CLI install to actually run — they migrate cleanly but I haven't executed them locally.)

Cross-language parity note for reviewers

Please assess consistency against C# and TypeScript only at this point. Python and Rust haven't been touched yet; once we're happy with the Go shape we'll port the equivalent changes there.

Phases

Each phase is a self-contained commit. After every phase: go build ./... && go vet ./... && go test . ./rpc (and go build ./... in go/samples/ and every scenario go/).

  • Phase ASessionConfig / ResumeSessionConfig renames
    • OnExitPlanModeOnExitPlanModeRequest, OnAutoModeSwitchOnAutoModeSwitchRequest
    • ExitPlanModeHandlerExitPlanModeRequestHandler, AutoModeSwitchHandlerAutoModeSwitchRequestHandler
    • Session.GetMessagesSession.GetEvents
    • ResumeSessionConfig.DisableResumeSuppressResumeEvent
    • Streaming bool*bool on both configs
  • Phase B + C — small wire/shape tweaks
    • ProviderConfig.MaxInputTokensMaxPromptTokens (matches the wire name maxPromptTokens)
    • MCPStdioServerConfig.Tools / MCPHTTPServerConfig.Tools: JSON tag → tools,omitempty. nil slice now means "all tools" (matches TS/C#).
  • Phase DRuntimeConnection discriminated config (the largest change)
    • Replaces CLIPath / CLIArgs / CLIUrl / UseStdio / Port / TCPConnectionToken / AutoStart / AutoRestart on ClientOptions with a single Connection RuntimeConnection field, constructed from one of:
      • StdioConnection{Path, Args}
      • TcpConnection{Port, ConnectionToken, Path, Args}
      • UriConnection{URL, ConnectionToken}
    • CopilotHomeBaseDirectory
    • RemoteEnableRemoteSessions
    • Client.ActualPort()Client.RuntimePort()
    • Drop public ConnectionState, State* constants, Client.State() — purely-internal state remains
    • LogLevel no longer defaults to "info": when empty, the SDK does not pass --log-level to the runtime at all (matches TS).
  • Phase ESessionMetadata / SessionLifecycleEventMetadata: StartTime / ModifiedTime stringtime.Time (wire is ISO 8601, parsed by time.Time's default JSON unmarshal)
  • Phase F — All six hook input types: Timestamp int64time.Time, with MarshalJSON / UnmarshalJSON pairs that serialize to/from Unix milliseconds on the wire
  • Phase G-K — misc cleanups
    • G: InputOptionsUiInputOptions
    • H: Convenience Session.SendPrompt(ctx, prompt) and Session.SendPromptAndWait(ctx, prompt)
    • I: SessionFsProvider method renames (Go-specific): MkdirMakeDirectory, ReaddirReadDirectory, ReaddirWithTypesReadDirectoryWithTypes, RmRemove. The adapter still implements the wire-protocol method names on rpc.SessionFsHandler unchanged.
    • J: CreateSessionFsHandler field → CreateSessionFsProvider
    • K: Remove deprecated Session.Destroy() (callers must use Session.Disconnect())
  • Phase L — Docs, samples, scenarios

Items explicitly NOT in this PR

Documented in the session plan:

  • SessionConfigBase extraction — skipped. Go has no struct inheritance; embedding would change JSON marshal behaviour and zero-value semantics, and the duplication is small. Documented divergence from TS/C#.
  • Sealing types — N/A in Go.
  • Removing named handler types in favour of plain func(...) — named handler types are idiomatic in Go and not noise the way C# delegate declarations are.
  • Discriminated lifecycle event type split (SessionCreatedEvent etc.) — keeping the flat SessionLifecycleEvent with a Type discriminator. Without sum-type narrowing in Go, a discriminated interface adds friction (forced type-switches at every callsite) without changing the wire shape.
  • LogLevel value-object — kept as string (TS/Go), but did drop the "info" default as part of Phase D.
  • AsyncDisposable — N/A; Disconnect() / Stop() are idiomatic.
  • _internalConnection / joinSession — feature not exposed in Go yet.

Already correct (no change needed)

GitHubToken casing, no "local" MCP alias, hidden session constructor, clean Stop() exit, MCP broadcast handling.

SteveSandersonMS and others added 7 commits May 21, 2026 14:45
- 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>
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>
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>
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>
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>
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>
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>
@github-actions

This comment has been minimized.

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>
@github-actions

This comment has been minimized.

@SteveSandersonMS SteveSandersonMS marked this pull request as ready for review May 21, 2026 14:50
@SteveSandersonMS SteveSandersonMS requested a review from a team as a code owner May 21, 2026 14:50
Copilot AI review requested due to automatic review settings May 21, 2026 14:50
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR applies the cross-language (C# / TypeScript) API review decisions to the Go SDK, including the new discriminated RuntimeConnection configuration model, multiple naming/shape alignments, and synchronized updates across Go tests, scenarios, samples, and documentation.

Changes:

  • Introduces RuntimeConnection (StdioConnection / TcpConnection / UriConnection) and migrates callers away from CLIPath/CLIUrl/UseStdio/Port fields.
  • Updates Go SDK API shapes for parity (e.g., Streaming *bool, hook timestamps to time.Time, GetMessagesGetEvents, provider/session fs renames).
  • Migrates Go E2E/unit tests, scenario apps, samples, and docs to the new API.
Show a summary per file
File Description
test/scenarios/transport/tcp/go/main.go Updates scenario to use ClientOptions.Connection with UriConnection.
test/scenarios/transport/reconnect/go/main.go Updates scenario to use ClientOptions.Connection with UriConnection.
test/scenarios/sessions/streaming/go/main.go Updates scenario to use Streaming: copilot.Bool(true).
test/scenarios/bundling/container-proxy/go/main.go Updates scenario to use ClientOptions.Connection with UriConnection.
test/scenarios/bundling/app-direct-server/go/main.go Updates scenario to use ClientOptions.Connection with UriConnection.
test/scenarios/bundling/app-backend-to-server/go/main.go Updates backend scenario to use ClientOptions.Connection with UriConnection.
go/types.go Adds RuntimeConnection types; aligns config/event types (timestamps, streaming pointers, renames) and MCP tool-list tagging.
go/types_test.go Updates provider-config JSON test for MaxPromptTokens.
go/session.go Adds SendPrompt/SendPromptAndWait, renames APIs (GetEvents, handler types, UiInputOptions), removes deprecated Destroy.
go/session_fs_provider.go Renames session fs provider methods and updates adapter to call renamed methods.
go/samples/manual_tool_resume/main.go Migrates sample to StdioConnection{Path: ...}.
go/samples/chat.go Migrates sample to StdioConnection{Path: ...}.
go/README.md Documents new Connection model, RuntimePort, Streaming *bool, and related renames.
go/internal/e2e/testharness/helper.go Migrates helper to GetEvents.
go/internal/e2e/testharness/context.go Updates test harness default client options to use StdioConnection.
go/internal/e2e/suspend_e2e_test.go Migrates suspend tests to UriConnection + connection token.
go/internal/e2e/streaming_fidelity_e2e_test.go Migrates streaming tests to Streaming: copilot.Bool(...) and GetEvents.
go/internal/e2e/session_fs_sqlite_e2e_test.go Migrates fs sqlite tests to renamed fs methods + CreateSessionFsProvider.
go/internal/e2e/session_fs_e2e_test.go Migrates fs tests to new connection model, RuntimePort, renamed fs APIs, and GetEvents.
go/internal/e2e/session_e2e_test.go Migrates session tests to GetEvents and time.Time metadata assertions.
go/internal/e2e/session_config_e2e_test.go Migrates config tests to GetEvents.
go/internal/e2e/rpc_shell_and_fleet_e2e_test.go Migrates polling logic to GetEvents.
go/internal/e2e/rpc_session_state_e2e_test.go Migrates session state tests to GetEvents.
go/internal/e2e/rpc_mcp_and_skills_e2e_test.go Updates CLI arg injection to flow via StdioConnection.Args.
go/internal/e2e/rpc_event_side_effects_e2e_test.go Migrates event side-effect tests to GetEvents.
go/internal/e2e/rpc_e2e_test.go Migrates RPC E2E tests to StdioConnection.
go/internal/e2e/per_session_auth_e2e_test.go Migrates auth test client creation to StdioConnection.
go/internal/e2e/pending_work_resume_e2e_test.go Migrates resume tests to UriConnection, RuntimePort, SuppressResumeEvent, GetEvents.
go/internal/e2e/multi_client_e2e_test.go Migrates multi-client tests to TcpConnection/UriConnection and RuntimePort.
go/internal/e2e/mode_handlers_e2e_test.go Migrates mode handler names to OnExitPlanModeRequest / OnAutoModeSwitchRequest.
go/internal/e2e/event_fidelity_e2e_test.go Migrates fidelity checks to GetEvents and updated error messages.
go/internal/e2e/error_resilience_e2e_test.go Migrates resilience checks to GetEvents.
go/internal/e2e/connection_token_test.go Migrates token tests to TcpConnection/UriConnection and RuntimePort.
go/internal/e2e/commands_and_elicitation_e2e_test.go Migrates commands/UI elicitation multi-client tests to new connection model and renames.
go/internal/e2e/client_options_e2e_test.go Updates options tests to new connection model and removes state-based assertions.
go/internal/e2e/client_lifecycle_e2e_test.go Removes state assertions after State() API removal.
go/internal/e2e/client_e2e_test.go Migrates client E2E tests to new connection model and removes state assertions.
go/internal/e2e/agent_and_compact_rpc_e2e_test.go Migrates agent-selection tests to StdioConnection.
go/internal/e2e/abort_e2e_test.go Migrates abort test to Streaming: copilot.Bool(true).
go/client.go Implements RuntimeConnection resolution, drops public state API, updates runtime spawning/args/log-level semantics.
go/client_test.go Updates unit tests for new connection model (but currently has formatting issues).
docs/troubleshooting/debugging.md Updates Go guidance to refer to Connection (UriConnection) (but still claims no extra-args support).
docs/setup/local-cli.md Updates Go examples to Connection, but also changes some cross-language wording.
docs/setup/bundled-cli.md Updates Go note to refer to Connection.
docs/setup/backend-services.md Updates Go backend-services example to use UriConnection.
docs/getting-started.md Updates Go examples to use Streaming: copilot.Bool(true) and UriConnection.
docs/features/streaming-events.md Updates Go streaming example to use Streaming: copilot.Bool(true).

Copilot's findings

Comments suppressed due to low confidence (1)

docs/setup/local-cli.md:16

  • The prose here says Node.js/Python/.NET can override the bundled CLI using the Connection option, but the Node and Python examples below still use cliPath / cli_path (and the diagram arrow is labeled cliPath). Consider rewording to be language-specific (e.g., Node: cliPath, Python: cli_path, .NET: Connection) to avoid confusing readers.
## How it works

By default, the Node.js, Python, and .NET SDKs include their own CLI dependency (see [Default Setup](./bundled-cli.md)). If you need to override this—for example, to use a system-installed CLI—you can use the `Connection` option.

```mermaid
flowchart LR
    subgraph YourMachine["Your Machine"]
        App["Your App"] --> SDK["SDK Client"]
        SDK -- "cliPath" --> CLI["Copilot CLI<br/>(your own binary)"]
        CLI --> Keychain["🔐 System Keychain<br/>(stored credentials)"]
  • Files reviewed: 47/47 changed files
  • Comments generated: 4

Comment thread go/types.go
Comment thread go/types.go
Comment thread go/client_test.go
Comment on lines 22 to 29
func TestClient_URLParsing(t *testing.T) {
t.Run("should parse port-only URL format", func(t *testing.T) {
client := NewClient(&ClientOptions{
CLIUrl: "8080",
Connection: UriConnection{URL: "8080"},
})

if client.actualPort != 8080 {
t.Errorf("Expected port 8080, got %d", client.actualPort)
}
Comment thread docs/troubleshooting/debugging.md
@github-actions

This comment has been minimized.

SteveSandersonMS and others added 3 commits May 21, 2026 17:16
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>
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>
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>
@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review

This PR applies the Go API review fixes in lockstep with C# (#1343) and TypeScript (#1357). Overall, the alignment is very strong — most renamed fields, config properties, and behavioral changes match exactly.

✅ Confirmed consistent with TypeScript and C#

Change Go TypeScript C#
BaseDirectory
EnableRemoteSessions
SuppressResumeEvent
OnExitPlanModeRequest field
OnAutoModeSwitchRequest field
MaxPromptTokens
Streaming *bool (nullable) ✅ (boolean?) ✅ (bool?)
GetEvents() semantics
RuntimeConnection concept

⚠️ Minor inconsistencies worth tracking

Three items I've left inline comments on:

  1. Connection struct names (go/types.go line 33): Go uses StdioConnection/TcpConnection/UriConnection, while TypeScript and C# both use StdioRuntimeConnection/TcpRuntimeConnection/UriRuntimeConnection. In TS/C#, these names are hidden behind factory methods, but in Go users write the struct names directly, making this difference visible. Likely intentional for Go idiom, but should be documented as a deliberate divergence and factored into the Python/Rust port.

  2. Handler type names (go/types.go line 356): Go uses ExitPlanModeRequestHandler/AutoModeSwitchRequestHandler while TypeScript uses ExitPlanModeHandler/AutoModeSwitchHandler. The config field (OnExitPlanModeRequest) is consistent; only the named type alias differs.

  3. RuntimePort() nullability (go/client.go line 1258): Returns int (0 when unavailable) in Go vs int? (null when unavailable) in C#. TypeScript doesn't expose this publicly. Worth aligning when Python/Rust are ported.

🔵 Language-idiomatic differences (not issues)

  • SendPrompt/SendPromptAndWait: Go adds these as named convenience methods because Go has no overloading. TypeScript/C# handle this via overloaded send(string) / SendAsync(string). This is the correct Go pattern.
  • ID vs Id in method names (e.g., GetLastSessionID vs GetLastSessionId): Follows Go's initialisms convention. Expected.
  • Handler types use func(...) (Result, error) instead of Func<..., Task<Result>> — expected Go idiom.

Overall this is a well-executed API review pass with clear documentation of deliberate deviations. The three items above are minor and mostly forward-looking concerns for when Python/Rust receive the equivalent treatment.

Generated by SDK Consistency Review Agent for issue #1360 · ● 2M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by SDK Consistency Review Agent for issue #1360 · ● 2M

Comment thread go/types.go

// StdioConnection spawns a runtime child process and communicates over its
// stdin/stdout pipes. This is the default when no connection is configured.
type StdioConnection struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-SDK naming note: The concrete connection struct names in Go (StdioConnection, TcpConnection, UriConnection) differ from the TypeScript and C# equivalents (StdioRuntimeConnection, TcpRuntimeConnection, UriRuntimeConnection).

Unlike TS/C# where users call factory functions (RuntimeConnection.forStdio(...), RuntimeConnection.ForStdio(...)) and rarely write the concrete type names, Go users will write these struct names directly in their code:

Connection: copilot.StdioConnection{Path: "..."}

The shorter Go names are arguably cleaner (the interface is already RuntimeConnection), but it's worth noting this difference explicitly since it means a developer switching SDKs won't find a StdioRuntimeConnection in Go. The PR description doesn't call this out as a deliberate divergence — just flagging it for the team to decide if this is intentional.

Comment thread go/types.go
Comment thread go/client.go
@SteveSandersonMS SteveSandersonMS merged commit cf0b305 into main May 21, 2026
43 checks passed
@SteveSandersonMS SteveSandersonMS deleted the stevesandersonms/go-sdk-api-review branch May 21, 2026 17:00
@SteveSandersonMS SteveSandersonMS mentioned this pull request May 21, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants