feat(sdk/go): add Go SDK foundation, types, and sandbox client (A)

[rhuss]

Context

This is the first PR in a 6-PR decomposition of the Go SDK contribution (#2044). The decomposition was discussed in the contributor meeting on 2026-07-14 to make the review process more approachable.

The first PR is intentionally the largest because it carries the shared foundation. After this merge, the SDK is usable end-to-end for sandbox management. Each subsequent PR then incrementally adds one more resource group, and after every merge the SDK is fully working with an expanded API surface.

PR	What	Code	Tests	Status
This PR (A)	Foundation + types + sandbox	~4.6K	~8.5K	ready for review
B	Exec + file + health	~1.2K	~1.8K	after A merges
C	Provider + profile + config + refresh	~2.2K	~3.3K	after B merges
D	Policy + service + TCP + SSH	~2.6K	~3.9K	after C merges
E	Gateway + OIDC + edge + fakes	TBD	TBD	after D merges
F	Docs + CI	~0.5K	-	after E merges

What's in this PR

• Module setup: go.mod, go.sum, Makefile, mise.toml
• All domain types: types/ package (14 files) covering every SDK resource
• Full ClientInterface: all 10 sub-client accessors defined upfront
• Shared infrastructure: errors, auth primitives, gRPC connection, logging
• Sandbox client: fully functional with converter and tests
• Stub clients: all other resources return Unimplemented errors linking to feat(sdk/go): Go SDK PR decomposition plan #2270. Each subsequent PR replaces stubs with real implementations.

sdk/go/
├── go.mod, go.sum, Makefile, mise.toml
├── proto/                              # Proto definitions + generated .pb.go
├── openshell/v1/
│   ├── types/                          # All domain types (14 files)
│   ├── internal/converter/             # Proto-to-SDK converters
│   ├── internal/grpc/                  # Connection management
│   ├── client.go                       # ClientInterface + Client struct
│   ├── sandbox.go + sandbox_client.go  # Sandbox (real implementation)
│   ├── stub_clients.go                 # Stubs for resources not yet implemented
│   ├── auth*.go                        # Auth providers
│   ├── errors.go                       # Typed errors with IsNotFound() etc.
│   └── {exec,file,health,...}.go       # Interface definitions for all resources

How to Review

Review zones

Zone	Files	What to do
Must-review	client.go, types/*.go, errors.go, auth*.go, sandbox.go, sandbox_client.go, internal/grpc/conn.go	These define the API surface and core logic. Read carefully.
Pattern-review	sandbox_client_test.go, internal/converter/sandbox.go, internal/converter/sandbox_test.go	Review sandbox_client_test.go as the test pattern exemplar. Converter tests follow table-driven patterns.
Skim	stub_clients.go, go.sum, Makefile, mise.toml, doc.go, interface-only files (exec.go, file.go, etc.)	Stubs are mechanical. Interface files are just type declarations.
Skip	proto/*.pb.go, proto/*_grpc.pb.go	Generated code.

Key design decisions

• client-go conventions: typed sub-clients per resource, watch primitives, typed errors
• Domain types separate from proto: types/ package has no proto imports, insulating consumers from wire format changes
• Stub pattern for incremental delivery: stubs return ErrorUnimplemented with a link to the tracking issue. Each follow-up PR replaces stubs with real implementations without modifying client.go.
• All types upfront: the full domain model ships in this PR (~1K lines) so reviewers see the complete API shape once

What to look for

• ClientInterface covers the right API surface
• Type definitions match gRPC API semantics
• Error handling follows typed error conventions (IsNotFound(), etc.)
• Auth provider interface is extensible
• Sandbox client test coverage is adequate

Testing

All 130 tests pass:

go test ./...   # 130 passed in 7 packages

Resolves #2044 (with remaining PRs B-F)
Part of #2270

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[russellb]

🤖 This review was generated with Claude Code using Opus 4.8 (1M context). Findings were verified by building the module, running go vet, the test suite, staticcheck, and diffing the vendored proto against the canonical proto/. Treat as reviewer input, not ground truth.

Principal Engineer Review — Go SDK foundation (A)

Reviewed by checking out the branch and reading every non-generated file. go build/go vet are clean and the 130 tests pass at ~73% package coverage. Findings are ordered by severity; all are actionable.

Blocking

1. Dead code will fail the project's own lint gate — sdk/go/openshell/v1/internal/converter/copy.go:51

boolCount is defined but never referenced anywhere in the module (verified across all .go including tests). unused is a default golangci-lint linter, and mise run ci depends on lint. Confirmed with staticcheck:

copy.go:51:6: func boolCount is unused (U1000)

This contradicts the "CI green" claim — mise run lint should be red. Delete boolCount (or wire it into the log-option validation it was presumably written for).

2. Three public Config fields are silent no-ops — sdk/go/openshell/v1/types/config.go:13-15, client.go:64

Config.Timeout, Config.RetryPolicy, and Config.Logger are declared on the public config struct but never read anywhere in the client or connection path (verified by grep). A user who sets Timeout: 30*time.Second or a RetryPolicy gets zero behavioral change, with no error and no doc warning. For the PR that "carries the shared foundation," shipping config knobs that do nothing bakes in an interface people will rely on, and later wiring them up becomes a behavior change. Either implement them (dial/context timeout, gRPC retryPolicy service-config, connection logging) or drop them from this PR and add each alongside its implementation. At minimum, document them as reserved/no-op.

High

3. Vendored proto is hand-edited and drifted from canonical proto/, and proto:sync will clobber the edits — sdk/go/proto/*, sdk/go/mise.toml:204

• UPSTREAM_VERSION pins 29ce6a70…, which is not resolvable in this repo's history — the snapshot isn't reproducible/verifiable from here.
• The vendored openshell.proto has been manually stripped of import "options.proto" and every [(…secret) = true] annotation (because options.proto isn't vendored).
• It has diverged from main: e.g. volume_claim_templates = 9 is still present here but is reserved 9 on main; the newer annotations = 4 request field on main is missing.
• proto:sync does a raw cp "$UPSTREAM_PATH/*.proto", which will re-introduce import "options.proto" and the secret annotations, immediately breaking proto:gen (protoc can't find options.proto).

Net: the generated bindings are built against a stale, hand-modified contract, and the "sync" automation is not idempotent with the manual edits. proto:check only verifies .pb.go matches the local .proto, not that the local .proto matches upstream — so drift is undetected. Recommend vendoring options.proto (or applying a scripted, repeatable transform), making proto:sync reproduce the exact committed state, and adding a check that the vendored proto equals the pinned upstream.

4. Package doc advertises functionality that returns Unimplemented in this PR — sdk/go/openshell/v1/doc.go

The package overview gives copy-paste examples for Exec().Run, Services().Expose, Providers().Profiles(), SSH().CreateSession/Tunnel, TCP().Forward, Config().Update/GetSandbox, and Policy().List (doc.go:37-357). Every one is a stub returning ErrorUnimplemented until PRs B–F. After A merges, pkg.go.dev presents these as working, and a user following the Quick Start past Sandboxes() hits runtime Unimplemented with no compile-time signal. Scope the package doc to what actually works in A, or clearly mark the not-yet-available sections.

Medium

5. internal/grpc/conn.go has zero test coverage — sdk/go/openshell/v1/internal/grpc/conn.go

The connection package ([no test files]) contains the only security-sensitive logic in the PR: TLS default selection, buildTLSCredentials, CA-pool loading, mTLS keypair loading, and the both-CertFile-and-KeyFile invariant. None of it is tested. NewConnection is easily unit-testable (temp cert files; assert error paths for bad CA / half-configured client cert / scheme stripping). Given this is the shared foundation, the crypto path deserves tests now.

6. WatchOptions fields silently ignored — sdk/go/openshell/v1/types/options.go:29-30, sandbox_client.go:174-190

Watch reads only StopOnTerminal; TimeoutSeconds and LabelSelector are never applied. Same silent no-op problem as #2. For a single-object watch, LabelSelector is meaningless — drop it (or document intent), and either honor TimeoutSeconds or remove it in favor of the documented "use context for timeout."

7. EventAdded is defined and re-exported but never emitted — sdk/go/openshell/v1/sandbox_client.go:211

The doc claims the watcher is "Modeled after k8s.io/apimachinery/pkg/watch.Interface," but the initial object and all updates are delivered as EventModified; EventAdded is dead. k8s consumers expect the first delivery to be ADDED. Either emit EventAdded for the first event (the code already handles first separately, so it's a one-line branch) or drop the constant to avoid implying semantics you don't provide.

Low / nits

8. Watch error events can be silently dropped — sandbox_client.go:233-236

The terminal EventError is sent with a non-blocking select { … default: }. If the 64-slot buffer is full (slow consumer), the stream error is dropped and the consumer only sees a closed channel — indistinguishable from clean EOF. Consider a dedicated error field on the watcher, or block on the send guarded by w.done.

9. Watch blocks until the first server event — sandbox_client.go:196

stream.Recv() runs synchronously before Watch returns, so the call blocks (bounded only by ctx) until the gateway produces the first event. Callers reasonably expect Watch to return promptly and stream thereafter. Document it, or move the first Recv into the goroutine.

10. FromGRPCError loses detail and lets non-status errors bypass typing — internal/converter/errors.go:35-52

StatusError.Details is never populated (the field is dead across the SDK), and when status.FromError returns ok=false the raw error is returned unwrapped, so a caller's IsX() checks silently return false for it. The unused Details field is misleading API surface.

11. Unchecked int → uint32 conversions — sandbox_client.go:54,57

uint32(opts[0].Limit) / uint32(opts[0].Offset) truncate on large values (only guarded against negatives by > 0). Not caught by the default linters, but would trip gosec G115 if adopted; cheap to bound-check.

12. Mock server has unsynchronized map access — sandbox_client_test.go:72,102-106,113-117

CreateSandbox, ListSandboxes, and DeleteSandbox touch s.sandboxes without holding s.mu, while GetSandbox/setPhase do lock. -race passes today only because tests don't currently overlap those calls with setPhase goroutines; latent flakiness in the file nominated as the test-pattern exemplar. Lock consistently.

13. The only non-skipped integration test can't pass — integration_test.go:25-34

TestIntegration_HealthCheck calls Health().Check(), which is a stub returning Unimplemented, so against a real gateway require.NoError fails; the others are t.Skip("TODO"). It's build-tagged so normal CI skips it, but as written it's a broken test. Skip it too, or gate on Health landing in PR B.

14. refreshableAuth holds the write lock across the network refresh — auth_refresh.go:83-112

source.Token() (a network call) runs under mu.Lock(), so every concurrent RPC's metadata fetch blocks for the full refresh duration. Correct single-flight behavior, but "coalesced" undersells that it fully serializes callers during refresh. Acceptable; worth a note.

---

Overall: the structure (typed sub-clients, types/ isolated from proto, converters, typed errors, watch primitives) is sound and the sandbox path is well tested. The items I'd gate merge on are #1 (lint failure), #2 (no-op config fields in the foundation), and #3/#4 (proto-sync integrity + docs overstating current capability); #5 (conn.go tests) is strongly recommended given this PR's role as the base. The rest are cleanups.

[russellb]

[codex:gpt-5.5] Finding 1: The Go SDK still drops active sandbox policy fields from the handwritten types/converters. The synced proto includes credential_signing, signing_service, signing_region, json_rpc_max_body_bytes, mcp, and params on L7 allow/deny rules, but PolicyNetworkEndpoint, L7Allow, L7DenyRule, and the converters omit them. Since Create sends SandboxSpecToProto, Go clients cannot express current SigV4/MCP/JSON-RPC policy controls, and server-returned policies lose these fields on round-trip. Please add SDK fields and bidirectional converter coverage for every current proto policy field. Refs: sdk/go/openshell/v1/types/network_policy.go:19, sdk/go/openshell/v1/internal/converter/network_policy.go:65, proto/sandbox.proto:131, proto/sandbox.proto:211.

[codex:gpt-5.5] Finding 2: mapToStruct ignores structpb.NewStruct errors for SandboxTemplate.Resources and DriverConfig. Invalid UTF-8 keys or unsupported map[string]any values make NewStruct return nil, err, but the SDK silently sends nil, so user-provided template config can disappear without an error. Please make sandbox spec conversion fallible, validate before CreateSandbox, or expose a safer typed representation, and add tests for invalid values. Refs: sdk/go/openshell/v1/internal/converter/copy.go:60, sdk/go/openshell/v1/internal/converter/sandbox.go:170, sdk/go/openshell/v1/sandbox_client.go:28.

[rhuss]

My agent's response to #2271 (comment). Most of the things are because of this artificial split to get the PRs down to something more consumable (which was also important as I hight some size limits for code agent's review when I dropped it). But thank you very much for jumping on it, I've addressed the comments (and delayed some until we get the full combo in)

---

Thanks for the review. Here is my assessment, classifying each finding by root cause:

Already addressed (in a prior fix commit 3b81351):

• Proto drift (finding 3): Synced all protos from canonical proto/ sources and regenerated Go bindings. The options.proto concern is moot: the canonical protos do not import it, so proto:sync will not break proto:gen. The volume_claim_templates field is now correctly reserved 9.
• conn.go untested (finding 5): Added conn_test.go with tests covering all scheme/TLS paths (http:// plaintext, https:// TLS, no-scheme TLS, Insecure config).

Fixed now (commit dab9329):

• Dead boolCount (finding 1): Deleted.
• EventAdded never emitted (finding 7): First watch event now emits EventAdded, subsequent events emit EventModified. Tests updated.
• Mock server races (finding 12): Added mu.Lock/Unlock to all mock server methods accessing s.sandboxes.
• Broken integration test (finding 13): HealthCheck test now t.Skips like the others.
• doc.go scope (finding 4): All sub-client sections not available in PR A are marked "(available in a future release)".
• No-op fields (findings 2, 6): Config.Timeout, RetryPolicy, Logger and WatchOptions.TimeoutSeconds, LabelSelector are documented as "reserved for future use".

Deferred to later PRs (expected from the A-F split):

• Config wiring (finding 2): The actual Timeout/RetryPolicy/Logger implementation requires the full client paths that land in PRs B-F. Documented as reserved for now.
• LabelSelector (finding 6): Meaningless for single-object watch (reviewer agrees). Will revisit when multi-object watch is added.

Accepted as low-priority (not blocking):

• Watch error drop (finding 8): Valid edge case with the 64-slot buffer. Will address if it surfaces in practice.
• Watch blocks on first Recv (finding 9): Documentation issue. The blocking behavior is inherent to the name-to-ID resolution + initial state delivery pattern.
• FromGRPCError detail loss (finding 10): Details field is dead. Will clean up alongside error handling improvements.
• int to uint32 truncation (finding 11): Bounds would only matter at >4B. Low risk but easy to add.
• Refresh lock scope (finding 14): Correct single-flight behavior as noted.

[russellb]

My agent's response to #2271 (comment). Most of the things are because of this artificial split to get the PRs down to something more consumable (which was also important as I hight some size limits for code agent's review when I dropped it). But thank you very much for jumping on it, I've addressed the comments (and delayed some until we get the full combo in)

Sounds good. I figured some of it would be off, but that your agent would sort it out. :)

[drew]

Are these just getting copied from the root protos/?

[rhuss]

Yes, they are copies from the root proto/ directory, with a proto:sync task and UPSTREAM_VERSION file to track provenance.

This is intentional. The copy+sync approach decouples SDK maintenance from core API changes. Without this, any contributor who touches a proto file would also need to fix all in-tree SDK bindings in the same PR. That's a high bar, especially as the number of SDK languages grows. Not everyone who adds a field to a proto can (or should have to) fix the Go, TypeScript, and future Python clients. Instead, a sync workflow detects proto changes and opens issues/PRs so the respective SDK maintainers can handle the adaptation on their own timeline.

Kubernetes uses a similar pattern (staging directories + publishing-bot), and Envoy's go-control-plane syncs protos via GitHub Actions rather than referencing them in-place. The trade-off is duplication vs. independence.

The alternative would be separate repositories per SDK, which is what Kubernetes and Temporal do. But that requires a common GitHub organization we can govern and control. With company-owned orgs, adding new repos typically involves heavy internal processes, so that's not a practical path right now.

That said, I agree we need alignment across SDKs. #2122 (TypeScript SDK by @maxdubrinsky) uses buf which references root protos directly, so we'd have two different approaches. @maxdubrinsky, could you weigh in on whether the TypeScript SDK would also benefit from a decoupled copy+sync model, or whether you see advantages to the buf approach that should inform the Go SDK too? Given how quickly the protos are still evolving, reducing friction for contributors who aren't SDK specialists is a key goal IMO.

[drew]

This seems superfluous given our use of mise. Is there a reason to have this?

[rhuss]

The Makefile is intentional as an entry point for Go developers. In the Go ecosystem, mise is essentially unknown, and Makefiles are the standard build tool (most Go projects use make test, make lint, etc.). Since the SDK targets external Go contributors who may never have seen mise, the Makefile picks them up where they are and reduces onboarding friction.

That said, it's a thin convenience layer and I don't feel strongly about it. Happy to drop it if the team prefers mise-only consistency across the repo.

[drew]

By convention this would go into something like tasks/sdk/go.toml. Is there a reason to create a new mise root here?

[krishicks]

We should also get some alignment between this PR and #2122, which adds the TypeScript SDK.

[rhuss]

The mise.toml here serves as a self-contained SDK development root, not just a task file. It declares SDK-specific tool dependencies (go, protoc, golangci-lint, protoc-gen-go, protoc-gen-go-grpc) alongside the tasks. A tasks/sdk/go.toml include can only define tasks, not tool versions.

This lets a Go contributor cd sdk/go && mise install and get a complete, isolated development environment without pulling in the root project's Rust/Python/Node toolchain. It follows the same self-containment principle as the proto copy discussed above.

That said, I don't feel as strongly about this as the proto decoupling. Moving tasks to tasks/sdk/go.toml and tool versions into the root mise.toml is a one-time change and not a big burden, so happy to adapt if the team prefers consistency.

On alignment with #2122: agreed, already pinged @maxdubrinsky on the proto thread above.

[rhuss]

THinking again, I will update the build for better consistency.

[rhuss]

Moved to the centralized pattern in acac231: Go tools are now in root mise.toml, tasks in tasks/go.toml with go: namespace and dir = "sdk/go". The standalone sdk/go/mise.toml is removed.

mise tasks --hidden | grep go: confirms all 11 tasks are registered and mise run go:test passes.

[drew]

How do we envision the SDK getting developed? Pinning the upstream version seems to break the monorepo convention where all code is intended to work at the tip of main.

To provide a concrete scenario: if i add a new field to a proto and associated services implemention, should the PR update the SDKs for that proto change as well?

[rhuss]

The UPSTREAM_VERSION pin is a leftover from the original standalone repo prototype where this connection was needed. I'm happy to remove it since in a monorepo, git diff between sdk/go/proto/ and proto/ already tells you if the SDK is behind. CI can check this directly.

But the underlying question stands regardless of the version file: should a PR that adds a proto field and updates the service implementation also need to update all SDK bindings? I'd argue no. That's the friction we're trying to avoid.

The deeper motivation is about community handover. In a project with SDKs in Go, TypeScript, Python, and a core in Rust, no single contributor is a domain expert in all of these. I have basic TypeScript knowledge but wouldn't trust myself with idiomatic TypeScript patterns, and I'm still learning Rust. Others may be TypeScript experts who've never written Go. Forcing every proto change through all SDKs means either blocking on specialists or accepting non-idiomatic fixes from generalists. It's better to let people do what they're best at and optimize the handover points.

What we should add is automation to make this handover smooth: a CI job that diffs root protos against each SDK's copies and flags divergence, plus a hard release gate requiring all SDKs to be green before shipping. That way the decoupling doesn't become invisible drift.

I agree the copy approach creates a visible deviation from the "tip of main" monorepo convention. But the convention assumes a single team maintaining all code. Whether we copy protos or reference them in-place is the design decision we need to align on across all SDKs (see the proto thread above with @maxdubrinsky).

[rhuss]

Removed the UPSTREAM_VERSION file and its references in db55f26. It was a leftover from the standalone repo prototype. In a monorepo, git diff sdk/go/proto/ proto/ already shows drift, and CI can enforce this directly.

The proto:sync task now defaults to ../../proto (the root proto directory) and no longer writes a version file.

[krishicks]

We should also get some alignment between this PR and #2122, which adds the TypeScript SDK.

[krishicks]

In #2122 buf is used for generating the protos, which I think is what we will prefer here over invoking protoc. Is it possible to update this PR to match #2122's implementation?

[rhuss]

Aligning on buf vs protoc makes sense, and I'm open to switching. But this choice is downstream of the proto management decision we're discussing on the other threads: whether SDKs copy protos locally or reference them from the root.

If we go with copy+sync (my preference, for the decoupling reasons above), either tool works fine for generating from local copies. If we go with buf's workspace model (referencing root protos directly), then buf is the natural fit.

I'd suggest we settle the proto management approach first (already pinged @maxdubrinsky above), then align the generation tooling across both SDKs to match.

[rhuss]

Follow-up: #2344 implements the SDK proto sync CI automation we discussed in the threads above.

It adds go:proto:drift and go:proto:build-check mise tasks, a non-blocking PR gate workflow, and a scheduled dashboard that creates agent-actionable issues when SDK protos drift from root definitions. This is the automation layer that makes the copy+sync model practical at scale.

Depends on this PR (#2271) for the tasks/go.toml foundation.