Simplify the public client API around api.Runtime - #10
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
server/protocol_resource_test.go contains a non-compiling composite literal for map[string]*wirev1.Value (missing &wirev1.Value{...} for the map value).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR intentionally breaks the handwritten Go client alpha API by making client.New(ctx, conn) return the canonical api.Runtime interface and by removing the previously exported lower-level resource handles, option structs, and type aliases so consumers use the Unified API programming model directly over a borrowed gRPC transport.
Changes:
- Replace the public
clientsurface with a minimal export set (New,Error,ErrClosed,ErrExecutionCancelled) returningapi.Runtime. - Rework the internal client implementation to keep Wire connection/resource IDs and watch streams private while preserving lifecycle/cleanup semantics.
- Migrate tests and documentation to the canonical
api.Runtime/api.Plan/api.Sessionoperations and add contract guards for the new public surface.
File summaries
| File | Description |
|---|---|
| test/integration/harness/harness.go | Switch harness runtime construction to client.New. |
| server/runtime_optimization_presence_test.go | Update optimization tests to use canonical compile/debug methods and Plan.Close(). |
| server/runtime_adapter_benchmark_test.go | Update benchmark to construct runtime via client.New. |
| server/protocol_resource_test.go | Add protocol-level regression test ensuring RPC operations remain available. |
| server/integration_test.go | Migrate server integration tests to canonical runtime/plan/session APIs and handshake assertions. |
| README.md | Update public usage examples and migration guidance for client.New returning api.Runtime. |
| docs/protocol.md | Update protocol documentation to reflect canonical API projection and private Wire resources. |
| docs/client.md | Rewrite client documentation around client.New → api.Runtime and remove lower-level facade docs. |
| docs/architecture.md | Update architecture docs to match new client surface and private resource model. |
| client/session_handle.go | Update session handle wiring to new internal connection/plan/execution handle types. |
| client/runtime.go | Make New the public constructor returning api.Runtime; update runtime adapter methods/types. |
| client/runtime_transport.go | Update direct runtime run transport path to use configured session options. |
| client/runtime_session_options.go | Replace public parameter model with canonical map[string]any collection for session options. |
| client/runtime_contract_test.go | Add public-surface guard and constructor contract assertions. |
| client/runtime_allocation.go | Move allocation reclaim to connectionHandle. |
| client/run_options.go | Remove deprecated RunOptions. |
| client/remote_session.go | Update remote session to return api.Output and use new allocation/reclaim plumbing. |
| client/remote_plan.go | Update remote plan to return api.Session and use new session/debug session creation paths. |
| client/remote_debug_session.go | Update remote debugger adapter to wrap new debugSessionHandle. |
| client/plan.go | Replace exported Plan with private planHandle; migrate session/debug session creation APIs internally. |
| client/plan_test.go | Migrate plan tests to canonical session-based execution and new close semantics. |
| client/params.go | Remove exported Parameters type alias; keep encoder accepting map[string]any. |
| client/ownership_test.go | Update ownership tests for new session/execution path and service registration. |
| client/metadata.go | Remove runtime metadata/capabilities types and conversions. |
| client/metadata_test.go | Remove legacy runtime metadata defensive-copy test. |
| client/lifecycle_test.go | Update lifecycle tests to use internal newConnection/planHandle/executionHandle. |
| client/handle_lifecycle_test.go | Update handle lifecycle tests to new session-based execution fixture and handle types. |
| client/execution.go | Replace exported Execution with private executionHandle; remove explicit Cancel RPC and return api.Output. |
| client/execution_test.go | Update execution tests to use new internal execution fixture types. |
| client/execution_events.go | Make execution event receiver private (executionEvents). |
| client/execute_options.go | Remove deprecated ExecuteOptions. |
| client/errors.go | Update comments for new public runtime-centric surface while preserving error identities. |
| client/doc.go | Update package docs to describe canonical api.Runtime surface and private Wire resource model. |
| client/debug.go | Replace exported DebugSession with private debugSessionHandle; remove Stop. |
| client/debug_test.go | Update debug tests for private debug event receiver types. |
| client/debug_session_options.go | Remove deprecated DebugSessionOptions. |
| client/debug_inspection.go | Remove convenience SetBreakpoint; keep canonical SetBreakpointAt on private handle. |
| client/debug_events.go | Make debug event receiver private (debugEvents). |
| client/constructor_test.go | Add constructor failure/borrowing/context-detachment regression tests. |
| client/compile.go | Remove exported Client.Compile wrapper; keep internal compileConfigured on connectionHandle. |
| client/compile_options.go | Remove deprecated CompileOptions struct. |
| client/client.go | Replace exported Client with private connectionHandle and newConnection. |
| client/client_test.go | Migrate “Run owns resources” tests to api.Runtime.Run over client.New and add session service support. |
Review details
- Files reviewed: 43/43 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The handwritten client exposed both RPC resource handles and a remote Universal API adapter. This change makes
client.New(ctx, conn)returnapi.Runtime, so consumers use canonical compile, session, run, debugger, and close operations after configuring their transport.This is an intentional alpha API break. It adds no compatibility shims or version bump.
Export migration
New(ctx, conn) (*Client, error)New(ctx, conn) (api.Runtime, error); failure returns a nil interfaceNewRuntimeNewRuntime,Session,OutputaliasesapitypesClient,Plan,Execution,DebugSessionand their methodsExecutionEvents,DebugEventsand their methodsParameters,CompileOptions,ExecuteOptions,DebugSessionOptions,RunOptionsRuntimeInfo,CapabilitiesError,ErrClosed,ErrExecutionCancelledThe only package-level exports are now
New,Error,ErrClosed, andErrExecutionCancelled. An exact constructor-signature assertion and public-surface test guard this contract. The surface check reads sources from the package test working directory, so it also works with-trimpath.Ownership and remaining API gaps
Private connection, plan, session, execution, debugger, and stream owners stay in
client. Allocation methods consume the existing private option collectors; unused convenience operations and duplicate option types are deleted. Direct hostedRuntime.Run, reusable plans, durable sessions, sequential reuse, overlapping-run rejection, option ordering/aggregation, omitted optimization levels, encoded output, defensive copies, and debugger event mapping are preserved.Public
Close() errorretains bounded detached cleanup, retained results, nearest-owner reclamation, and sibling preservation. Runtime closure never closes the supplied transport. Cancellation, sanitization, portable-input validation, and resource limits retain their existing contracts.The Universal API has no equivalent structured remote-error taxonomy:
Errorstill exposes a Wire failure category, canonical diagnostics, and an unwrap-able transport cause. Terminal errors remain*failure.Failure; remote execution cancellation remains distinct from caller-context cancellation. The API also does not define a portable parameter subset, so Wire continues validating its transportable values locally. Private execution snapshots and watches serve protocol bookkeeping without adding public semantic types.grpc.ClientConnInterfaceremains the construction boundary because endpoint, credentials, TLS, and transport limits belong to the caller.Production server architecture, shared domain packages, API dependency, protobuf sources, and generated bindings are unchanged.
Tests and documentation
Migrated legacy client/server tests to canonical operations while retaining private conversion, stream, ownership, and lifecycle coverage. Added malformed/failed constructor, nil-interface, construction-context, and borrowed-transport checks. Generated RPC coverage preserves Execute, CancelExecution, Terminate, and handshake metadata assertions after removing their facade equivalents. Operation/cleanup errors remain discoverable.
All round-trip integration assertions are unchanged; their harness only renames
NewRuntimetoNew. README examples, package documentation, and client/architecture/protocol documentation describe the canonical resource model and correct the deferred-client-cleanup wording.Successful local validation (Go 1.26.5, darwin/arm64):
The full test and race targets include the round-trip integration suite. Formatting, vet, full tests/race, and build were rerun after the ownership self-review fix. Buf checks passed with network access for pinned tooling. Generation consistency produced no generated diff;
mainand fetchedorigin/mainwere bothe2eac36. Hosted CI supplies the repository's Go 1.25 and Linux/macOS/Windows matrix; those results are separate from this local validation.Benchmark and self-review
Repeated
go test ./server -run '^$' -bench '^BenchmarkRuntimeAdapterDurableSession$' -benchmem -count=5against an isolated source snapshot ofe2eac36and the final implementation, sequentially on an Apple M2 Max:No material regression. An earlier run overlapped tool compilation and had substantial timing variance, so both sides were repeated in isolation; the observed latency difference is not a claim of a statistically established speedup.
Mandatory complete-diff self-review covered correctness, lifecycle/concurrency, architecture/API, security, compatibility, and scope. It removed an obsolete direct-plan ownership branch, corrected unique session IDs in migrated test fixtures, and corrected stale documentation/assertion wording. Affected validation passed after the fixes. No remaining findings.