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. :)

[rhuss]

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

It's mostly meant for illustrative purpose how such a copy-sync flow could look like.

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.

[rhuss]

Pushed the buf migration in 85fd246, addressing the proto management discussion:

• Dropped vendored .proto sources from sdk/go/proto/. Go SDK now generates directly from root proto/ at tip.
• Switched from raw protoc to buf. Added repo-level buf.yaml (identical to what feat(sdk): add TypeScript SDK (@nvidia/openshell-sdk) #2122 proposes for the TS SDK) and sdk/go/buf.gen.yaml for Go-specific generation config.
• Rewrote mise tasks: go:proto:gen and go:proto:check now use buf generate. Removed go:proto:sync and go:proto:clean (no longer needed).
• Added buf 1.72.0 to root mise.toml tool dependencies.
• Included options.proto in generation (was stripped from the vendored copies). New proto/optionsv1/ package appears.
• All 134 tests pass, go:proto:check confirms generated files match.

If #2122 lands first, the buf.yaml merge is trivial since the content is identical. Also rebased on latest upstream/main.

[rhuss]

One note on sequencing: if the TS SDK (#2122) merges first and the repo-level buf.yaml changes from what I have here, happy to rebase and align. The current buf.yaml is a copy of what #2122 proposes, so if that evolves (e.g. additional lint rules, different breaking policy), I'll update this PR to match so we ship a single consistent buf config across both SDKs.

[rhuss]

Hey @russellb @drew @krishicks @maxdubrinsky, thank you all for the thorough review and the great feedback so far. I've addressed all the review comments across the last few rounds of fixups, and the PR is rebased on the latest main with CI green.

Is there still something outstanding or blocking a merge? From my side, all review concerns have been addressed, and the PR is in good shape: single commit, full test coverage, proto-converter drift detection in place (with a follow PR for CI automation --> #2344, @maxdubrinsky that might be interesting for the TypeScript SDK, too).

I'd like to gently asked for merging this soon so we can start landing the subsequent drops (B through F) and keep the momentum going toward the Beta milestone. Since the Go SDK is entirely new surface area with no existing consumers, the risk of merging is low. If anything comes up after merge, we can address it incrementally in the follow-up PRs rather than gating everything on this single PR.

Happy to discuss anything that's still open, though!

[rhuss]

Note on workspace scoping: This PR (Drop A) operates exclusively in the default workspace. The workspace field on request messages (e.g., CreateSandboxRequest.workspace) is not yet exposed in the SDK client API, so the gateway receives an empty string and normalizes it to "default".

The read side is covered: Sandbox.Workspace and Provider.Workspace fields are populated from ObjectMeta.workspace in the response, so consumers can see which workspace a resource belongs to.

Full workspace support (targeting non-default workspaces, all_workspaces on list operations, and the workspace CRUD/membership RPCs) is planned for Drop C.