Skip to content

feat: sync upstream PR #2271 changes for Drop A parity - #53

Merged
rhuss merged 5 commits into
mainfrom
sync-upstream-pr-2271
Aug 8, 2026
Merged

feat: sync upstream PR #2271 changes for Drop A parity#53
rhuss merged 5 commits into
mainfrom
sync-upstream-pr-2271

Conversation

@rhuss

@rhuss rhuss commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Summary

Ports fixes and improvements from NVIDIA/OpenShell PR #2271 to keep both repos on par for Drop A.

Changes

  • Proto sync: Add CredentialHandle message and Provider.credential_handles field to datamodel.proto, regenerate Go bindings
  • Provider types: Add ProfileWorkspace, CredentialHandles, CredentialHandle to domain types and converter (both directions)
  • WaitReady fix: Detect SandboxDeleting phase and return immediately instead of polling indefinitely
  • Watch goroutine fixes:
    • defer streamCancel() to prevent context leaks
    • EventAdded type for first event
    • Blocking error delivery with FromGRPCError conversion (fixes IsUnavailable(ev.Err) on mid-stream errors)
  • Plaintext auth rejection: http:// scheme detection, InsecureSkipVerify semantics for TLS Insecure, reject auth providers that require transport security over plaintext
  • Tests: Provider converter unit tests, proto field coverage tests for Provider/CredentialHandle, mid-stream watch error test, EventDeleted test, fixed StopOnTerminal=false test
  • Makefile removed: mise tasks are the canonical build system

Related

Summary by CodeRabbit

  • New Features

    • Added provider credential handles and profile workspace support.
    • Added cause-aware error handling for cancellation and timeouts.
    • Improved sandbox watch events with initial, deletion, and error states.
  • Bug Fixes

    • Improved readiness handling during deletion, cancellation, and timeouts.
    • Corrected HTTP, HTTPS, and unspecified-address connection security behavior.
    • Added validation for incomplete client certificate configurations.
  • Tests

    • Expanded coverage for provider conversion, connections, errors, and sandbox watches.

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>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@rhuss, you've reached your PR review limit, so we couldn't start this review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7b16d51f-f018-4e58-8ebe-73cf693e9749

📥 Commits

Reviewing files that changed from the base of the PR and between e0039c7 and c2fa08d.

📒 Files selected for processing (6)
  • openshell/v1/context_errors.go
  • openshell/v1/context_errors_test.go
  • openshell/v1/internal/converter/provider_test.go
  • openshell/v1/internal/grpc/conn.go
  • openshell/v1/internal/grpc/conn_test.go
  • openshell/v1/sandbox_client.go
📝 Walkthrough

Walkthrough

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

Changes

SDK behavior updates

Layer / File(s) Summary
Cause-aware error contracts
openshell/v1/types/errors.go, openshell/v1/grpc_errors.go, openshell/v1/errors_test.go, openshell/v1/fake/sandbox.go, openshell/v1/grpc_errors_test.go, openshell/v1/fake/sandbox_test.go
StatusError unwraps an underlying cause. Context errors map to typed status errors while preserving the cause.
Provider credential-handle conversion
proto/datamodel.proto, openshell/v1/types/provider.go, openshell/v1/internal/converter/provider.go, openshell/v1/internal/converter/provider_test.go
Provider data carries profile workspaces and credential handles in both conversion directions.
gRPC transport selection
openshell/v1/internal/grpc/conn.go, openshell/v1/internal/grpc/conn_test.go
NewConnection selects plaintext or TLS from the address scheme, validates authentication and certificate settings, and applies insecure TLS configuration.
Protobuf converter coverage
openshell/v1/internal/converter/coverage_test.go
Reflection-based tests detect unhandled fields, stale handled-field entries, and explicitly skipped fields.
Sandbox readiness and watch events
openshell/v1/types/watch.go, openshell/v1/sandbox_client.go, openshell/v1/sandbox_client_test.go
Readiness handles deletion and context errors. Watch events classify initial objects as added, carry stream errors, preserve open streams when configured, and classify deleting objects as deleted.

Development command removal

Layer / File(s) Summary
Make target removal
Makefile
The Makefile and its mise check, phony declarations, and development, validation, build, CI, documentation, and proto targets are removed.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: porting upstream PR #2271 changes to maintain Drop A parity.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync-upstream-pr-2271

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.61%. Comparing base (f7b4752) to head (c2fa08d).

Files with missing lines Patch % Lines
openshell/v1/fake/sandbox.go 75.00% 2 Missing ⚠️
openshell/v1/internal/grpc/conn.go 87.50% 1 Missing and 1 partial ⚠️
openshell/v1/sandbox_client.go 96.55% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (2)
openshell/v1/internal/grpc/conn_test.go (1)

15-31: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Execute a request before claiming transport coverage.

NewConnection returns 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 and TLSParams.Insecure behavior.

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 win

Use testify assertions for coverage failures.

This integration test must use testify assertions, so replace the direct t.Errorf calls in assertAllFieldsCovered with assert.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

📥 Commits

Reviewing files that changed from the base of the PR and between f7b4752 and 9762bbb.

⛔ Files ignored due to path filters (1)
  • proto/datamodelv1/datamodel.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (14)
  • Makefile
  • openshell/v1/errors_test.go
  • openshell/v1/grpc_errors.go
  • openshell/v1/internal/converter/coverage_test.go
  • openshell/v1/internal/converter/provider.go
  • openshell/v1/internal/converter/provider_test.go
  • openshell/v1/internal/grpc/conn.go
  • openshell/v1/internal/grpc/conn_test.go
  • openshell/v1/sandbox_client.go
  • openshell/v1/sandbox_client_test.go
  • openshell/v1/types/errors.go
  • openshell/v1/types/provider.go
  • openshell/v1/types/watch.go
  • proto/datamodel.proto
💤 Files with no reviewable changes (1)
  • Makefile

Comment thread openshell/v1/internal/converter/provider.go
Comment thread openshell/v1/internal/grpc/conn_test.go
Comment thread openshell/v1/internal/grpc/conn_test.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/sandbox_client.go
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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CredentialHandle and Provider.credential_handles, regenerated bindings, and added converter coverage tests to catch proto drift.
  • Extended provider domain types + converters for ProfileWorkspace and 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.

Comment thread openshell/v1/internal/converter/provider.go
Comment thread openshell/v1/types/watch.go

@rhuss rhuss left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc-review Summary

What Went Well

  • Proto drift detection via reflection (coverage_test.go): The assertAllFieldsCovered pattern 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 of http:// 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

Comment thread openshell/v1/types/errors.go
Comment thread openshell/v1/fake/sandbox.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/context_errors.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/converter/provider_test.go

@rhuss rhuss left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc-review Summary

What Went Well

  • Proto drift detection via reflection (coverage_test.go): The assertAllFieldsCovered pattern 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 of http:// 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

Comment thread openshell/v1/types/errors.go
Comment thread openshell/v1/fake/sandbox.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/context_errors.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/converter/provider_test.go

@rhuss rhuss left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc-review Summary

What Went Well

  • Proto drift detection via reflection (coverage_test.go): The assertAllFieldsCovered pattern 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 of http:// 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

Comment thread openshell/v1/types/errors.go
Comment thread openshell/v1/fake/sandbox.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/sandbox_client.go Outdated
Comment thread openshell/v1/sandbox_client.go
Comment thread openshell/v1/context_errors.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/grpc/conn.go
Comment thread openshell/v1/internal/converter/provider_test.go
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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ebadf86 and 6fc1eda.

📒 Files selected for processing (3)
  • openshell/v1/fake/sandbox_test.go
  • openshell/v1/grpc_errors_test.go
  • openshell/v1/sandbox_client_test.go

Comment thread openshell/v1/fake/sandbox_test.go Outdated
rhuss added 2 commits August 8, 2026 06:26
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
@rhuss
rhuss merged commit ab39ac0 into main Aug 8, 2026
7 checks passed
@rhuss
rhuss deleted the sync-upstream-pr-2271 branch August 8, 2026 05:12
rhuss added a commit that referenced this pull request Aug 8, 2026
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants