feat: sync upstream PR #2271 changes for Drop A parity - #53
Conversation
Port fixes and improvements from NVIDIA/OpenShell PR #2271: - Proto sync: add CredentialHandle message and Provider.credential_handles - Fix WaitReady to detect SandboxDeleting phase - Fix Watch goroutine: defer streamCancel, EventAdded type, blocking error delivery with FromGRPCError conversion - Reject plaintext auth when provider requires transport security - Add provider converter unit tests and proto field coverage tests - Remove Makefile (mise tasks are the canonical build system) Signed-off-by: Roland Huß <rhuss@redhat.com>
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe SDK now preserves error causes, supports provider credential handles, selects gRPC transport by address scheme, updates sandbox readiness and watch events, adds protobuf converter coverage checks, and removes the Makefile command wrappers. ChangesSDK behavior updates
Development command removal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxClient
participant WatchStream
participant contextError
participant EventConsumer
SandboxClient->>WatchStream: Receive sandbox event
WatchStream-->>SandboxClient: Return event or stream error
SandboxClient->>contextError: Convert non-EOF error
contextError-->>SandboxClient: Return StatusError
SandboxClient->>EventConsumer: Emit EventError with Err
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 89.09% 89.61% +0.51%
==========================================
Files 77 78 +1
Lines 4842 4901 +59
==========================================
+ Hits 4314 4392 +78
+ Misses 361 343 -18
+ Partials 167 166 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
openshell/v1/internal/grpc/conn_test.go (1)
15-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExecute a request before claiming transport coverage.
NewConnectionreturns before a handshake or RPC. These tests can pass even if the selected transport does not match the listener. Register a minimal service and execute an RPC to verify plaintext transport andTLSParams.Insecurebehavior.Also applies to: 52-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshell/v1/internal/grpc/conn_test.go` around lines 15 - 31, Extend TestNewConnectionHTTPSchemeUsesPlaintext and the corresponding TLSParams.Insecure test to register a minimal gRPC service on the test server and execute a real RPC through the connection. Assert the RPC succeeds, so both the http:// transport selection and insecure TLS configuration are validated against the listener rather than only connection creation.openshell/v1/internal/converter/coverage_test.go (1)
210-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse testify assertions for coverage failures.
This integration test must use testify assertions, so replace the direct
t.Errorfcalls inassertAllFieldsCoveredwithassert.Failf. Keep the existing failure message so field coverage drift is still reported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshell/v1/internal/converter/coverage_test.go` around lines 210 - 215, Update assertAllFieldsCovered to replace the direct t.Errorf coverage failure with testify's assert.Failf, preserving the existing failure message and formatting arguments so uncovered or unjustifiably skipped fields remain clearly reported.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshell/v1/internal/converter/provider.go`:
- Around line 89-99: Remove CredentialHandles from provider create/update RPC
payloads while preserving ProviderToProto for other conversions. Update the
Create, Update, and Ensure request-building flows to use request-specific
conversion or explicitly clear the protobuf CredentialHandles field before
sending CreateProviderRequest or UpdateProviderRequest, ensuring internal
gateway state is never serialized in these RPCs.
In `@openshell/v1/internal/grpc/conn_test.go`:
- Around line 6-13: Update the tests in conn_test.go to use testify require or
assert calls instead of direct t.Fatal and t.Fatalf assertions, while retaining
testing.T in test function signatures and adding the appropriate testify
assertion import.
- Around line 1-4: Add the //go:build integration constraint immediately before
the package declaration in conn_test.go, marking this real TCP/gRPC server test
for integration-only builds while keeping the existing package and license
header unchanged.
In `@openshell/v1/internal/grpc/conn.go`:
- Around line 53-56: Update the connection setup validation around
TLSParams.Insecure and the auth provider check to reject transport-secure
per-RPC credentials whenever certificate verification is disabled, before
grpc.WithPerRPCCredentials is configured. Preserve the existing plaintext
rejection and allow insecure TLS only when no transport-secure credentials are
supplied or an explicitly supported unauthenticated-TLS mode is selected.
In `@openshell/v1/sandbox_client.go`:
- Around line 164-165: Update the fake client’s WaitReady implementation in
fake/sandbox.go to map context deadline and cancellation errors through the same
StatusError conversion used by the real client, preserving ctx.Err() as the
cause. Keep non-context errors and successful readiness behavior unchanged so
IsDeadlineExceeded and IsCancelled work consistently across implementations.
---
Nitpick comments:
In `@openshell/v1/internal/converter/coverage_test.go`:
- Around line 210-215: Update assertAllFieldsCovered to replace the direct
t.Errorf coverage failure with testify's assert.Failf, preserving the existing
failure message and formatting arguments so uncovered or unjustifiably skipped
fields remain clearly reported.
In `@openshell/v1/internal/grpc/conn_test.go`:
- Around line 15-31: Extend TestNewConnectionHTTPSchemeUsesPlaintext and the
corresponding TLSParams.Insecure test to register a minimal gRPC service on the
test server and execute a real RPC through the connection. Assert the RPC
succeeds, so both the http:// transport selection and insecure TLS configuration
are validated against the listener rather than only connection creation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 194bef51-2e06-4d6a-9e7a-a48b527a3519
⛔ Files ignored due to path filters (1)
proto/datamodelv1/datamodel.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
Makefileopenshell/v1/errors_test.goopenshell/v1/grpc_errors.goopenshell/v1/internal/converter/coverage_test.goopenshell/v1/internal/converter/provider.goopenshell/v1/internal/converter/provider_test.goopenshell/v1/internal/grpc/conn.goopenshell/v1/internal/grpc/conn_test.goopenshell/v1/sandbox_client.goopenshell/v1/sandbox_client_test.goopenshell/v1/types/errors.goopenshell/v1/types/provider.goopenshell/v1/types/watch.goproto/datamodel.proto
💤 Files with no reviewable changes (1)
- Makefile
Applied fixes from bot review comments: - Comment #3723395311: use testify require/assert in conn_test.go - Comment #3723395336: wrap context errors in fake WaitReady as StatusError for fake-real parity Assisted-By: 🤖 Claude Code
There was a problem hiding this comment.
Pull request overview
Ports upstream NVIDIA/OpenShell PR #2271 into the Go SDK to maintain Drop A parity, including proto additions (credential handles), provider domain/converter updates, sandbox readiness/watch fixes, and gRPC connection security/scheme handling with expanded tests.
Changes:
- Synced protos to add
CredentialHandleandProvider.credential_handles, regenerated bindings, and added converter coverage tests to catch proto drift. - Extended provider domain types + converters for
ProfileWorkspaceand credential handle metadata. - Fixed sandbox lifecycle handling (WaitReady deletion exit, watch event typing + mid-stream error delivery) and tightened gRPC connection transport/auth validation.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/datamodel.proto | Adds CredentialHandle and credential_handles to Provider schema. |
| proto/datamodelv1/datamodel.pb.go | Regenerated Go bindings reflecting new proto messages/fields. |
| openshell/v1/types/watch.go | Extends watch events to carry an Err for error events. |
| openshell/v1/types/provider.go | Adds provider spec fields for ProfileWorkspace and CredentialHandles. |
| openshell/v1/types/errors.go | Adds cause-aware error chaining via Cause + Unwrap(). |
| openshell/v1/grpc_errors.go | Introduces helper to wrap context cancellation/deadline into StatusError. |
| openshell/v1/sandbox_client.go | Fixes WaitReady on deleting + improves Watch event typing, cancellation, and mid-stream error propagation. |
| openshell/v1/sandbox_client_test.go | Adds/updates tests for WaitReady context errors and Watch behavior (added/deleted/error + StopOnTerminal semantics). |
| openshell/v1/internal/grpc/conn.go | Implements scheme-driven plaintext vs TLS selection and additional TLS/client-cert validation. |
| openshell/v1/internal/grpc/conn_test.go | Adds tests for scheme handling, TLS defaults, and plaintext+auth rejection. |
| openshell/v1/internal/converter/provider.go | Converts new provider fields including credential handles. |
| openshell/v1/internal/converter/provider_test.go | Adds/updates converter unit tests to cover new provider fields and nil/empty cases. |
| openshell/v1/internal/converter/coverage_test.go | Adds proto-field coverage tests using protobuf reflection to detect converter drift. |
| openshell/v1/fake/sandbox.go | Aligns fake WaitReady context-error behavior with real client (cause-aware StatusError). |
| openshell/v1/errors_test.go | Updates tests to validate StatusError cause unwrapping. |
| Makefile | Removed (build/test now driven directly via mise tasks). |
Files not reviewed (1)
- proto/datamodelv1/datamodel.pb.go: Generated file
Suppressed comments (1)
openshell/v1/internal/grpc/conn.go:75
- Setting InsecureSkipVerify=true while still accepting CAFile is misleading: the CA bundle is loaded but will not be used for verification when Insecure is true. Consider rejecting this combination (or at least returning a clear error) so users don’t think they’re pinning a CA when verification is disabled.
if cfg.CAFile != "" {
caCert, err := os.ReadFile(cfg.CAFile)
if err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- Proto drift detection via reflection (
coverage_test.go): TheassertAllFieldsCoveredpattern uses protobuf reflection to catch unhandled fields at test time. High-leverage testing pattern. - Deep copy discipline at boundaries (
converter/provider.go:46-52): CredentialHandle.Metadata maps are deep-copied via CopyStringMap in both directions. - Blocking error delivery in watch goroutine (
sandbox_client.go:248-252): Guarantees the consumer receives the error event before the goroutine exits. - Scheme-driven transport selection (
conn.go:31-38): Clean separation ofhttp://plaintext vs TLS.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | types/errors.go:60 |
StatusError.Details replaced with Cause (undeclared breaking change) | goal-alignment |
| Important | AGENTS.md:5 |
Stale make references after Makefile deletion |
architecture, goal-alignment |
| Important | converter/errors.go:48 |
FromGRPCError doesn't set Cause unlike contextError | architecture |
| Important | fake/sandbox.go:298 |
contextError logic duplicated between fake and real client | architecture, correctness |
| Important | sandbox_client.go:149 |
WaitReady terminal-phase checks duplicated pre-loop and in-loop | architecture |
| Important | sandbox_client.go:247 |
Watch spurious error after Stop() due to select nondeterminism | production |
| Important | sandbox_client_test.go:545 |
WaitReady test: weak assertions, no SandboxDeleting test | test-quality |
| Important | converter/provider_test.go:20 |
No deep-copy verification for CredentialHandles | test-quality |
| Minor | grpc_errors.go:1 |
File named grpc_errors.go but contains context error handling | architecture |
| Minor | sandbox_client.go:239 |
WHAT comment on StopOnTerminal restates code | architecture |
| Minor | conn.go:41 |
TLSParams silently ignored when http:// scheme used | production, security |
| Minor | types/provider.go:23 |
Provider Credentials sent in body over plaintext | security |
| Minor | conn.go:73 |
CAFile path read without sanitization | security |
| Minor | errors_test.go |
IsUnauthenticated not tested despite new ErrorCode | test-quality |
| Minor | conn_test.go:48 |
Error message not verified in auth rejection test | test-quality |
| Minor | sandbox_client.go:164 |
WaitReady context error wrapping undeclared | goal-alignment |
| Minor | conn.go:91 |
CertFile/KeyFile mutual validation undeclared | goal-alignment |
| Minor | sandbox_client_test.go:949 |
Mock CreateSandbox mutex added undeclared | goal-alignment |
| Minor | sandbox_client.go:219 |
Watch leak if abandoned without Stop() | production |
| Minor | edge/doc.go:66 |
Edge tunnel doc example stale after Insecure semantic change | architecture |
Review Details
- Findings posted: 20 (8 Important, 12 Minor)
- Findings reviewed and not posted: 11 (4 Important rejected, 7 Notable skipped)
- Gate outcome: FAIL (8 Important)
- Participating agents: correctness, architecture, security, production, test-quality, goal-alignment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- Proto drift detection via reflection (
coverage_test.go): TheassertAllFieldsCoveredpattern uses protobuf reflection to catch unhandled fields at test time. High-leverage testing pattern. - Deep copy discipline at boundaries (
converter/provider.go:46-52): CredentialHandle.Metadata maps are deep-copied via CopyStringMap in both directions. - Blocking error delivery in watch goroutine (
sandbox_client.go:248-252): Guarantees the consumer receives the error event before the goroutine exits. - Scheme-driven transport selection (
conn.go:31-38): Clean separation ofhttp://plaintext vs TLS.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | types/errors.go:60 |
StatusError.Details replaced with Cause (undeclared breaking change) | goal-alignment |
| Important | AGENTS.md:5 |
Stale make references after Makefile deletion |
architecture, goal-alignment |
| Important | converter/errors.go:48 |
FromGRPCError doesn't set Cause unlike contextError | architecture |
| Important | fake/sandbox.go:298 |
contextError logic duplicated between fake and real client | architecture, correctness |
| Important | sandbox_client.go:149 |
WaitReady terminal-phase checks duplicated pre-loop and in-loop | architecture |
| Important | sandbox_client.go:247 |
Watch spurious error after Stop() due to select nondeterminism | production |
| Important | sandbox_client_test.go:545 |
WaitReady test: weak assertions, no SandboxDeleting test | test-quality |
| Important | converter/provider_test.go:20 |
No deep-copy verification for CredentialHandles | test-quality |
| Minor | grpc_errors.go:1 |
File named grpc_errors.go but contains context error handling | architecture |
| Minor | sandbox_client.go:239 |
WHAT comment on StopOnTerminal restates code | architecture |
| Minor | conn.go:41 |
TLSParams silently ignored when http:// scheme used | production, security |
| Minor | types/provider.go:23 |
Provider Credentials sent in body over plaintext | security |
| Minor | conn.go:73 |
CAFile path read without sanitization | security |
| Minor | errors_test.go |
IsUnauthenticated not tested despite new ErrorCode | test-quality |
| Minor | conn_test.go:48 |
Error message not verified in auth rejection test | test-quality |
| Minor | sandbox_client.go:164 |
WaitReady context error wrapping undeclared | goal-alignment |
| Minor | conn.go:91 |
CertFile/KeyFile mutual validation undeclared | goal-alignment |
| Minor | sandbox_client_test.go:949 |
Mock CreateSandbox mutex added undeclared | goal-alignment |
| Minor | sandbox_client.go:219 |
Watch leak if abandoned without Stop() | production |
| Minor | edge/doc.go:66 |
Edge tunnel doc example stale after Insecure semantic change | architecture |
Review Details
- Findings posted: 20 (8 Important, 12 Minor)
- Findings reviewed and not posted: 11 (4 Important rejected, 7 Notable skipped)
- Gate outcome: FAIL (8 Important)
- Participating agents: correctness, architecture, security, production, test-quality, goal-alignment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- Proto drift detection via reflection (
coverage_test.go): TheassertAllFieldsCoveredpattern uses protobuf reflection to catch unhandled fields at test time. High-leverage testing pattern. - Deep copy discipline at boundaries (
converter/provider.go:46-52): CredentialHandle.Metadata maps are deep-copied via CopyStringMap in both directions. - Blocking error delivery in watch goroutine (
sandbox_client.go:248-252): Guarantees the consumer receives the error event before the goroutine exits. - Scheme-driven transport selection (
conn.go:31-38): Clean separation ofhttp://plaintext vs TLS.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | types/errors.go:60 |
StatusError.Details replaced with Cause (undeclared breaking change) | goal-alignment |
| Important | AGENTS.md:5 |
Stale make references after Makefile deletion |
architecture, goal-alignment |
| Important | converter/errors.go:48 |
FromGRPCError doesn't set Cause unlike contextError | architecture |
| Important | fake/sandbox.go:298 |
contextError logic duplicated between fake and real client | architecture, correctness |
| Important | sandbox_client.go:149 |
WaitReady terminal-phase checks duplicated pre-loop and in-loop | architecture |
| Important | sandbox_client.go:247 |
Watch spurious error after Stop() due to select nondeterminism | production |
| Important | sandbox_client_test.go:545 |
WaitReady test: weak assertions, no SandboxDeleting test | test-quality |
| Important | converter/provider_test.go:20 |
No deep-copy verification for CredentialHandles | test-quality |
| Minor | grpc_errors.go:1 |
File named grpc_errors.go but contains context error handling | architecture |
| Minor | sandbox_client.go:239 |
WHAT comment on StopOnTerminal restates code | architecture |
| Minor | conn.go:41 |
TLSParams silently ignored when http:// scheme used | production, security |
| Minor | types/provider.go:23 |
Provider Credentials sent in body over plaintext | security |
| Minor | conn.go:73 |
CAFile path read without sanitization | security |
| Minor | errors_test.go |
IsUnauthenticated not tested despite new ErrorCode | test-quality |
| Minor | conn_test.go:48 |
Error message not verified in auth rejection test | test-quality |
| Minor | sandbox_client.go:164 |
WaitReady context error wrapping undeclared | goal-alignment |
| Minor | conn.go:91 |
CertFile/KeyFile mutual validation undeclared | goal-alignment |
| Minor | sandbox_client_test.go:949 |
Mock CreateSandbox mutex added undeclared | goal-alignment |
| Minor | sandbox_client.go:219 |
Watch leak if abandoned without Stop() | production |
| Minor | edge/doc.go:66 |
Edge tunnel doc example stale after Insecure semantic change | architecture |
Review Details
- Findings posted: 20 (8 Important, 12 Minor)
- Findings reviewed and not posted: 11 (4 Important rejected, 7 Notable skipped)
- Gate outcome: FAIL (8 Important)
- Participating agents: correctness, architecture, security, production, test-quality, goal-alignment
Added tests to address Codecov coverage regression: - openshell/v1/grpc_errors_test.go (all branches of contextError) - openshell/v1/fake/sandbox_test.go (DeadlineExceeded in WaitReady) - openshell/v1/sandbox_client_test.go (SandboxDeleting in WaitReady) Assisted-By: 🤖 Claude Code
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshell/v1/fake/sandbox_test.go`:
- Around line 228-230: Update the context setup in the affected test to use an
already expired deadline when calling context.WithTimeout, and remove the
time.Sleep call. Keep the existing cancel cleanup and ensure the test still
exercises the expired-context behavior deterministically.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf97f186-f945-44e4-95cc-bca71b203172
📒 Files selected for processing (3)
openshell/v1/fake/sandbox_test.goopenshell/v1/grpc_errors_test.goopenshell/v1/sandbox_client_test.go
Applied fixes from bot review comments: - Comment #3736086034: use already-expired deadline for deterministic test Assisted-By: 🤖 Claude Code
- Extract checkTerminalPhase helper to DRY duplicate phase checks in WaitReady - Fix watch goroutine race: check w.done before delivering error after Stop() - Remove WHAT comment that restates code on StopOnTerminal - Rename grpc_errors.go to context_errors.go (content is context error wrapping) - Add validation error when TLS params conflict with plaintext http:// address - Add deep-copy mutation tests for CredentialHandles in provider converter Assisted-By: 🤖 Claude Code
- brainstorm/030-upstream-review-findings.md: review findings from PR #53 triage that should flow upstream with the next SDK contribution PR - brainstorm/idea-inbox.md: add deferred triage findings (credential-handles write behavior, context-error extraction, undeclared PR changes) Assisted-By: 🤖 Claude Code
Summary
Ports fixes and improvements from NVIDIA/OpenShell PR #2271 to keep both repos on par for Drop A.
Changes
CredentialHandlemessage andProvider.credential_handlesfield todatamodel.proto, regenerate Go bindingsProfileWorkspace,CredentialHandles,CredentialHandleto domain types and converter (both directions)SandboxDeletingphase and return immediately instead of polling indefinitelydefer streamCancel()to prevent context leaksEventAddedtype for first eventFromGRPCErrorconversion (fixesIsUnavailable(ev.Err)on mid-stream errors)http://scheme detection,InsecureSkipVerifysemantics for TLSInsecure, reject auth providers that require transport security over plaintextmisetasks are the canonical build systemRelated
Summary by CodeRabbit
New Features
Bug Fixes
Tests