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.

🤖 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.

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.

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

Review verdict: Request changes

PR #2271 (#2271) has a sound overall structure, but I found four merge-blocking issues:

1. [P1] Go checks are not wired into repository CI.
   go:ci exists, but root ci, test, lint, fmt, and pre-commit never invoke it. Required checks can pass without compiling or testing the SDK. tasks/ci.toml (

   OpenShell/tasks/ci.toml

   Lines 47 to 59 in a7e939b

   	[fmt]
   	description = "Format code"
   	depends = ["rust:format", "python:format", "markdown:format"]
   	hide = true
   	[lint]
   	description = "Run repository lint checks"
   	depends = ["license:check", "rust:format:check", "rust:lint", "python:format:check", "python:lint", "helm:lint", "helm:docs:check", "markdown:lint"]
   	hide = true
   	[ci]
   	description = "Run full checks (lint, compile/type checks, and tests)"
   	depends = ["lint", "check", "test"]

   ), tasks/go.toml
   (

   OpenShell/tasks/go.toml

   Lines 36 to 38 in a7e939b

   	["go:ci"]
   	description = "Run Go SDK full CI pipeline"
   	depends = ["go:lint", "go:build", "go:test", "go:proto:check", "go:docs:check"]

   )

2. [P2] The public refresh API omits AWS STS.
   The canonical proto includes AWS_STS_ASSUME_ROLE, but RefreshStrategy stops at Google service-account JWT. This contradicts the PR’s “all domain types upfront” contract. refresh.go
   (

   OpenShell/sdk/go/openshell/v1/types/refresh.go

   Lines 12 to 18 in a7e939b

   	const (
   	RefreshStrategyStatic RefreshStrategy = "Static"
   	RefreshStrategyExternal RefreshStrategy = "External"
   	RefreshStrategyOAuth2RefreshToken RefreshStrategy = "OAuth2RefreshToken"
   	RefreshStrategyOAuth2ClientCredentials RefreshStrategy = "OAuth2ClientCredentials"
   	RefreshStrategyGoogleServiceAccountJWT RefreshStrategy = "GoogleServiceAccountJWT"
   	)

   ), openshell.proto (

   OpenShell/proto/openshell.proto

   Lines 1474 to 1482 in a7e939b

   	enum ProviderCredentialRefreshStrategy {
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_UNSPECIFIED = 0;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_STATIC = 1;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_EXTERNAL = 2;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_OAUTH2_REFRESH_TOKEN = 3;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_OAUTH2_CLIENT_CREDENTIALS = 4;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_GOOGLE_SERVICE_ACCOUNT_JWT = 5;
   	PROVIDER_CREDENTIAL_REFRESH_STRATEGY_AWS_STS_ASSUME_ROLE = 6;
   	}

   )

3. [P2] WaitReady bypasses typed errors.
   Cancellation returns raw ctx.Err(), so advertised helpers such as IsDeadlineExceeded and IsCancelled return false. Convert context errors into StatusError. sandbox_client.go
   (

   OpenShell/sdk/go/openshell/v1/sandbox_client.go

   Lines 159 to 163 in a7e939b

   	for {
   	select {
   	case <-ctx.Done():
   	return nil, ctx.Err()
   	case <-ticker.C:

   )

4. [P2] Invalid template maps are silently discarded.
   structpb.NewStruct errors are ignored. Unsupported values in Resources or DriverConfig therefore become nil, and Create proceeds with different configuration. The conversion needs to return an error. copy.go
   (

   OpenShell/sdk/go/openshell/v1/internal/converter/copy.go

   Lines 60 to 65 in a7e939b

   	func mapToStruct(m map[string]any) *structpb.Struct {
   	if m == nil {
   	return nil
   	}
   	s, _ := structpb.NewStruct(m)
   	return s

   )

Additional cleanup:

• gofmt -l openshell reports copy.go and sandbox_test.go.
• go:fmt calls goimports, but goimports is not declared in mise.toml.
• Several commits contain Assisted-By: Claude Code, and six lack Signed-off-by; both violate repository commit rules.
• The agent-infrastructure check found that AGENTS.md and CONTRIBUTING.md do not list the new sdk/go/ component.

Validation: go test ./..., go test -race ./..., and go vet ./... all passed locally. The typed sub-client architecture, proto/domain separation, and sandbox test coverage are otherwise strong.

Thanks for the thorough review. Addressed 3 of the 4 code findings in ad76876:

[P2] RefreshStrategy missing AWS STS - Added RefreshStrategyAWSStsAssumeRole to match proto enum value 6 and re-exported in the public package.

[P2] WaitReady bypasses typed errors - Added contextError() helper that wraps context.DeadlineExceeded and context.Canceled into StatusError, so IsDeadlineExceeded() and IsCancelled() now work correctly.

[P2] Invalid template maps silently discarded - Changed mapToStruct to return an error, propagated through SandboxSpecToProto (now returns (*pb.SandboxSpec, error)), and surfaced as InvalidArgument in Create().

[P1] CI wiring - This is tracked separately in PR #2344 (SDK proto sync CI). The root ci task intentionally does not include Go yet since the SDK is not merged. Once PR A lands, we'll wire go:ci into the root task in a follow-up.

Re: gofmt/goimports and Signed-off-by - will clean up in the next push.

• Partially fixed. Commit 163589a (163589a) resolves three of four principal findings:

Finding Status
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━━━━
Go omitted from root CI ❌ Unfixed
────────────────────────────────────────── ────────────
Missing AWS STS refresh strategy ✅ Fixed
────────────────────────────────────────── ────────────
WaitReady returns untyped context errors ✅ Fixed
────────────────────────────────────────── ────────────
Invalid template maps silently discarded ✅ Fixed

Validation passed: go test ./..., race tests, and go vet ./....

Still blocking approval:

• Root ci, test, lint, and fmt still exclude Go tasks.
• gofmt -l now reports three files: copy.go, sandbox.go, and sandbox_test.go.
• No focused regression tests were added for context-error classification or invalid maps.
• goimports remains undeclared; coverage.out remains unignored.
• Older commits still contain AI attribution and missing sign-offs.
• AGENTS.md and CONTRIBUTING.md still omit sdk/go/.

Verdict: keep Request changes until CI integration and formatting are fixed; the three implementation fixes themselves look correct.

Addressed all remaining items in 41b2a4a:

• Root CI wiring: go:ci added to root ci task depends in tasks/ci.toml
• gofmt: all 3 files reformatted (gofmt -l now reports 0 files)
• goimports: declared in mise.toml tools
• coverage.out: added to .gitignore
• AGENTS.md: added Go SDK section with build/test/convention guidance
• CONTRIBUTING.md: added sdk/go/ to project structure table
• Regression tests: added TestSandboxWaitReady_ContextCancelled (verifies IsCancelled()), strengthened existing timeout test to assert IsDeadlineExceeded(), and added TestSandboxSpecToProto_InvalidMapReturnsError
• No panics: SandboxToProto now returns (*pb.Sandbox, error) instead of using a must-wrapper; grep -rn panic sdk/go/openshell/ returns 0 hits

Test count: 146 (was 144). All pass with -race.

Still outstanding (historical, cannot fix without interactive rebase):

• AI attribution and missing Signed-off-by on older commits

Re-review: the original functional issues are fixed. Regression tests cover them, go test -race ./... passes, and formatting/diff checks are clean.

One remaining concern: goimports is set to latest (

Also, the latest head now includes root CI wiring despite the earlier deferral. AI attribution/sign-off cleanup on squash is fine.

Verdict: code fixes look ready; one small tooling change requested.

Blockers

1. Token auth is unusable against any non-production gateway. StaticToken, RefreshableToken, and anything wrapped in WithExtraHeaders all return RequireTransportSecurity() == true. NewConnection maps both http:// and TLS.Insecure: true to insecure.NewCredentials(), and gRPC
   rejects that combination at construction. I verified all three empirically:

NewClient(http:// + StaticToken) err=grpc connect: grpc: the credentials require transport level security
NewClient(https:// + Insecure:true + StaticToken) err=
NewClient(http:// + RefreshableToken) err=

NewClient never returns a usable client. This breaks the entire local-dev and k3d onboarding path — you cannot authenticate to a dev gateway at all.

2. TLS.Insecure means "plaintext", not "skip cert verification" (internal/grpc/conn.go:41). This diverges from the Rust SDK in this same repo, which deliberately keeps the two separate — crates/openshell-sdk/src/config.rs:72 says "insecure_skip_verify is a separate flag rather
   than a third AuthConfig variant because it's a transport concern (cert verification) that's orthogonal to auth", and transport.rs:81 documents the branch table. A Go user pointing Insecure: true at an HTTPS gateway with a self-signed cert sends plaintext at a TLS port.
   Implementing this as real TLS with InsecureSkipVerify (leaving http:// as the only plaintext trigger) also fixes ci: add GitHub Actions CI workflow with lint, test, and image build #1.

3. Six policy proto fields are still silently dropped. Codex flagged this on 2026-07-15 and the thread is resolved, but the fields are absent from both types.PolicyNetworkEndpoint (18 fields, covering proto 1–18) and both converter directions:

• NetworkEndpoint 19–23: credential_signing, signing_service, signing_region, json_rpc_max_body_bytes, mcp
• L7Allow.params and L7DenyRule.params (field 9 on both)

Go clients can't express SigV4 re-signing, MCP options, or JSON-RPC body limits, and a Get → modify → Create round-trip strips them. Silently dropping credential_signing is a security-relevant downgrade, not just a gap.

4. The new drift guard doesn't guard. coverage_test.go:124 reports unhandled fields via t.Logf, so drift never fails. It also only covers 5 messages — none of which are NetworkEndpoint, L7Allow, or L7DenyRule, i.e. exactly where the drift in Sandbox logging #3 lives. The comment defers
   enforcement to "a separate CI workflow (planned)". Make unhandled fields t.Errorf with an explicit skip-set, and extend coverage to the policy messages.

5. No CI runs any of this. tasks/ci.toml adds go:ci to the local ci task, but branch-checks.yml invokes granular per-language tasks (rust:lint, test:rust, python:lint, …) and never calls mise run ci. Grepping every workflow for sdk/go, go:ci, or golangci returns nothing. 8K lines
   land with 130 tests that never execute in PR CI — and since CI is deferred to PR F, PRs A through E all merge unverified. The Go job belongs in this PR.

Should fix

6. Every godoc example is broken. Commit 2110f15 added a leading workspace parameter to all methods; doc.go was never updated. Create(ctx, "my-sandbox", spec, nil), Get(ctx, "missing"), WaitReady(ctx, sandbox.Name), Watch(ctx, sandbox.Name), Services().Expose(...),
   Policy().List(ctx, "secure-sandbox"), Config().GetSandbox(...), Config().Update(...) — none compile. This is the pkg.go.dev landing page. Moving them to example_test.go files makes the compiler enforce them permanently.

7. Watch discards the error. Event[T] carries only Type and Object, so Event{Type: EventError, Object: nil} (sandbox_client.go:251) tells the caller nothing about what failed. It's sent with select/default, so a full buffer drops even that signal. Add Err error to Event[T] —
   keyed-literal compatible — and block on w.done instead.

8. Unauthenticated collapses into PermissionDenied (converter/errors.go:18). For a bearer-token SDK, 401 (re-authenticate) vs 403 (give up) is precisely the distinction callers branch on. Worth adding ErrorUnauthenticated/IsUnauthenticated() now — doing it later silently changes
   behavior for existing IsPermissionDenied() callers.

9. StatusError has no Unwrap(), and Details is never populated — it's declared at types/errors.go:60 and written nowhere. Callers can't recover gRPC status details or match context.DeadlineExceeded. Wire both up or drop the dead field before it becomes public API.

Minor

• WaitReady treats only Ready/Error as terminal; a sandbox entering Deleting spins until the context deadline instead of failing fast.
• Watch maps phase Deleting → EventDeleted — "deleting" isn't "deleted", and actual delete completion emits no event.
• No .golangci.yml anywhere, so go:lint runs default linters only — no gosec or errorlint on a security-adjacent SDK.
• uint32(opts[0].Limit) (sandbox_client.go:62) truncates silently above 2³².
• 9 of 10 sub-clients runtime-fail with Unimplemented. Reasonable for incremental delivery, but make sure sdk/go isn't go get-able on a release tag until the series lands.

What's good

The proto-free types package with an enforced boundary is the right call and well executed. Deep-copy at every converter boundary, closeOnce on Close, and the double-checked-locking refresh with stale-token fallback and structured logging are all solid. The buf migration
documents each lint exception with a real justification, mise.lock pins with checksums and provenance, integration tests are correctly build-tagged, and mapToStruct now propagates errors (codex's finding #2 genuinely fixed).

Findings 1, 2, and 3 are the ones I'd consider merge-blocking on their own — 1 because the SDK can't authenticate anywhere but production, and 3 because it drops a security control silently.

Addressed all 5 blockers from the latest review:

1. + 2. TLS.Insecure = skip cert verify (58f30d1): Insecure: true now uses TLS with InsecureSkipVerify instead of plaintext, matching the Rust SDK's insecure_skip_verify semantics. http:// + token auth works via an insecureAuthWrapper that overrides RequireTransportSecurity. Transport decision table matches crates/openshell-sdk/src/transport.rs.

3. Missing policy proto fields (6686187): Added all 6 fields: CredentialSigning, SigningService, SigningRegion, JsonRpcMaxBodyBytes, Mcp (new McpOptions type) on PolicyNetworkEndpoint; Params on both L7Allow and L7DenyRule. Both converter directions wired.

4. Coverage test enforcement (12daaba): Changed t.Logf to t.Errorf for unhandled fields. Extended from 5 messages to 9: added NetworkEndpoint (23 fields), L7Allow (8), L7DenyRule (8), McpOptions (2). Proto drift now fails the build.

5. CI workflow (b0d375f): Added go job to branch-checks.yml running mise run go:ci on every PR.

Test count: 151 (was 147). All pass with -race. mise run go:ci clean.

Re-reviewed at b0d375fa, verifying each fix against the code rather than the summary. All 5 blockers are genuinely fixed. Two new items and a decision request below.

Blockers — all confirmed fixed

#1 + #2 TLS/auth (58f30d1) — Fixed, and confirmed at the wire level rather than just checking the error changed. All six auth/transport combinations now construct a client. Against a real gRPC server presenting a self-signed cert with a deliberately wrong CN:

Insecure:true   RPC err=<nil>   -> server saw authorization="Bearer tok-A"
Insecure:false  RPC err=... tls: failed to verify certificate: x509: certificate signed by unknown authority

That proves both halves: Insecure: true completes a genuine TLS handshake (plaintext could not) and delivers the bearer token, while Insecure: false still rejects a bad cert — so skip-verify is doing real work and wasn't just swapped for plaintext. usePlaintext is now set only by the http:// prefix, matching the Rust SDK's branch table in crates/openshell-sdk/src/transport.rs.

#3 Policy fields (6686187) — Fixed. PolicyNetworkEndpoint now carries all 23 proto fields, McpOptions is a new type with both optional bools, and Params is wired on L7Allow and L7DenyRule in both directions. I round-tripped a NetworkPolicyRule with every previously-dropped field populated through FromProto → ToProto and compared with proto.Equal: lossless.

#4 Coverage enforcement (12daaba) — Fixed. t.Logf → t.Errorf, and coverage went from 5 to 9 messages including the three where the drift actually lived. Confirmed the guard fires rather than assuming it — removing credential_signing from the handled set produces a real failure:

--- FAIL: TestConverterCoversAllProtoFields_NetworkEndpoint
    proto openshell.sandbox.v1.NetworkEndpoint field "credential_signing" is not handled
    by the converter and not explicitly skipped.

#5 CI (b0d375f) — Fixed and correctly gated. Traced required-ci-gates.yml: it evaluates the whole "Branch Checks" workflow conclusion, so a Go failure blocks the merge rather than being advisory.

Full suite passes with -race.

Two new items

Go formatting is unchecked, and the fix commit is already unformatted. gofmt -l flags openshell/v1/internal/converter/coverage_test.go, introduced by 12daaba5 itself. go:ci is go:lint, go:build, go:test, go:proto:check, go:docs:check — no format check; go:fmt only writes (-w) and isn't in the pipeline; and there's still no .golangci.yml, so golangci-lint runs defaults, which exclude gofmt. The repo wires rust:format:check and python:format:check into branch-checks.yml, so Go should match. Adding a go:format:check running gofmt -l . | tee /dev/stderr | (! read) to go:ci closes it and would have caught this commit.

go:docs:check passes vacuously. Its glob is openshell/v1/*/doc.go, and the only subdirectories are internal/ and types/ — both explicitly skipped, so the loop body never executes. It cannot fail today and never covers openshell/v1/doc.go itself. Not a blocker, but it's a green check that verifies nothing. Its error text also refers to "Constitution XIII", which exists nowhere in this repo — a leftover from the standalone SDK repo.

Findings 6–9 unchanged

Consistent with your comment, which only claimed the blockers. Restating for triage, since these are API-surface decisions that get harder to change after release:

• Broken godoc examples — no example_test.go files yet; Create(ctx, "my-sandbox", …), Get(ctx, "missing"), WaitReady(ctx, sandbox.Name), and Watch(ctx, sandbox.Name) all still omit the workspace parameter added in 2110f15. This is the pkg.go.dev landing page and it doesn't compile.
• Watch drops the error — Event[T] is still {Type, Object}; sandbox_client.go:251 still emits Object: nil, so the cause is unrecoverable.
• Unauthenticated → PermissionDenied — converter/errors.go:18 unchanged.
• StatusError.Details still dead, and no Unwrap() — types/errors.go:60 is declared but never written.

Of these I'd push for #8 and #9 in this PR: adding ErrorUnauthenticated later silently changes behavior for existing IsPermissionDenied() callers, and Details is public API that never gets populated. #6 and #7 are safely additive later — Err can be added to Event[T] without breaking keyed literals.

Summary

The blocker work is solid and independently verified. I'd ask for the go:format:check task before merge since it's one line and the tree is currently unformatted, plus a decision on #8/#9 now rather than post-release. Everything else can follow in B–F.

Addressed all should-fix items (#6-#9) in 58374d8:

• Agents are running inside the sandbox with secure access to frontier models #6: Fixed all godoc examples in doc.go with workspace parameter
• Security feedback: SSH hardening and policy validation #7: Added Err error field to Event[T] so watch errors carry the underlying cause
• bug(bootstrap/cli): remote gateway_endpoint may resolve to endpoint serving wrong certificate #8: Separated Unauthenticated from PermissionDenied (new ErrorUnauthenticated code + IsUnauthenticated() helper, gRPC mapping updated)
• perf: SSH sandbox sessions have high interactive latency due to missing TCP_NODELAY and other inefficiencies #9: Replaced dead Details field with Cause error + Unwrap() on StatusError, enabling errors.Is/As unwrapping. FromGRPCError and contextError both populate Cause.

Re: .golangci.yml with gosec/errorlint: deferring to Drop F (docs + CI). Adding security linters mid-review risks a new findings cycle, and gosec would flag the intentional InsecureSkipVerify we just added. Better to introduce it alongside proper nolint exemptions in the CI PR.

151 tests, all pass with -race. mise run go:ci clean.

Checked at 58374d8e. All four should-fix items (#6–#9) are genuinely fixed and verified. One item from my previous comment is still outstanding, and it isn't covered by the stated deferral.

#6–#9 confirmed fixed

#6 godoc examples — Fixed, and I verified more than the claim. Rather than just checking the workspace arg was added, I transcribed all 23 call sites from doc.go into a real compiled test file; it builds clean under go vet. That validates more than arity: every struct field (RefreshConfig{Provider, CredentialKey, Strategy, Material}, ConfigUpdate{Name, Policy, SettingKey, SettingValue}, SettingValue{Type, IntVal}), every constant (RefreshStrategyOAuth2ClientCredentials, SettingValueInt, ProfileCategoryInference), and every accessor (status.NextRefreshAt, gwCfg.SettingsRevision, sbCfg.ConfigRevision, session.HostKeyFingerprint) resolves against the real API. Config().GetGateway(ctx) correctly has no workspace arg.

#7 Event.Err — Field added and populated with recvErr at sandbox_client.go:251.

#8 Unauthenticated — Verified both directions, no leakage:

Unauthenticated  -> IsUnauthenticated=true  IsPermissionDenied=false
PermissionDenied -> IsUnauthenticated=false IsPermissionDenied=true

Also a good call appending the new code at the end of the iota block so existing ErrorCode values don't renumber.

#9 Cause + Unwrap() — Verified the full unwrap chain:

status.FromError(Unwrap(err)) ok=true code=NotFound
errors.Is(deadline, context.DeadlineExceeded)=true
errors.Is(cancel,   context.Canceled)=true
errors.As -> code=NotFound cause=true

Details is gone entirely, and FromGRPCError plus all three contextError branches populate Cause.

151 tests, pass with -race — matches your count.

Still outstanding: gofmt

The .golangci.yml/gosec/errorlint deferral to Drop F is reasonable, and you're right that gosec would flag the intentional InsecureSkipVerify. But gofmt is a separate item from that and wasn't addressed:

$ gofmt -l .
openshell/v1/internal/converter/coverage_test.go

go:ci is still ["go:lint", "go:build", "go:test", "go:proto:check", "go:docs:check"], and the task list is go:test, go:test:integration, go:lint, go:fmt, go:build, go:ci, go:docs:check, go:proto:gen, go:proto:check — there's no go:format:check, and go:fmt only writes.

This doesn't carry the risks cited for the deferral: gofmt isn't a security linter, can't flag InsecureSkipVerify, and can't open a new findings cycle — it's whitespace. The actual defect is column misalignment in the NetworkEndpoint handled-set map introduced by 12daaba5, fixed by mise run go:fmt. Rust and Python both gate on *:format:check in branch-checks.yml; merging Go without it means the tree lands unformatted and stays that way until Drop F.

Two lines closes it:

["go:format:check"]
dir = "sdk/go"
run = "gofmt -l . | tee /dev/stderr | (! read)"

...added to go:ci's depends.

Also unaddressed, both minor and fine to carry to Drop F: go:docs:check still passes vacuously (its openshell/v1/*/doc.go glob only matches internal/ and types/, both explicitly skipped), and its error message still references the nonexistent "Constitution XIII".

Summary

Everything substantive is done and independently verified — the blockers and all four should-fix items. The only thing I'd still ask for before merge is mise run go:fmt plus the format-check task; that's a two-line change with no review risk. LGTM once that lands.

Added go:format:check to go:ci in 2515210. It runs gofmt -l and fails if any file is unformatted. Fixed the unformatted coverage_test.go in the same commit. Tree is clean:

mise run go:ci   # format-check + lint + build + test + proto-check + docs-check — all pass

Note: the should-fix items #6-#9 were already addressed in 58374d8 (pushed before the re-review at b0d375f). That commit adds Err to Event[T], separates Unauthenticated from PermissionDenied, replaces Details with Cause + Unwrap(), and fixes all godoc examples. The review at b0d375f predates that push.

25152106 closes the last outstanding item. Verified at that commit:

go:format:check added and wired. The task is in tasks/go.toml and is first in go:ci's depends, so the go job in branch-checks.yml picks it up. I confirmed three things rather than just reading the diff:

• gofmt -l . is clean across the whole module, including generated proto/.
• The script passes as written when run verbatim.
• It genuinely fails on drift — injecting unformatted code into errors.go produced ERROR: The following files are not gofmt-formatted: openshell/v1/errors.go with exit=1.

The coverage_test.go reformat was whitespace-only. Since that commit rewrote 46 lines in the file holding the drift guard, I compared the handled-set entries before and after: 74 entries, identical. No field was silently dropped while realigning columns — which would have quietly re-opened blocker #3's detection gap.

The coverage guard still fires after the reformat: removing json_rpc_max_body_bytes from the handled set fails the test as expected.

151 tests pass with -race.

Where the PR stands

Everything raised across the review rounds is now resolved and independently verified:

Two minor items remain deliberately deferred to Drop F, both fine to carry: go:docs:check still passes vacuously (its openshell/v1/*/doc.go glob only matches internal/ and types/, both explicitly skipped), and its error text references the nonexistent "Constitution XIII". Neither blocks anything — the check just can't fail today. Worth a tracking note so they don't get lost.

Thanks for the quick turnaround on all of these, and for splitting the contribution into reviewable drops — it made this tractable.

No further review comments from me. This looks ready to merge once CI is green.

/ok to test 2515210

/ok to test 02717b8

/ok to test 5ed39ca