feat: close proto-to-SDK type gaps for full parity - #57
Merged
Conversation
The docs:check CI step was failing with 4 missing documentation gaps for the edge and fake packages. Add API docs pages and SUMMARY.md entries to get CI green. Assisted-By: 🤖 Claude Code
Add previously unmapped proto fields to SDK domain types: - ProviderProfile: annotations, source, scope - ProfileCredential: env_vars, auth_style, header_name, query_param, path_template, token_grant (with CredentialTokenGrant sub-types) - RefreshStrategy: add AWSStsAssumeRole enum value All new fields include deep-copy at proto/SDK boundaries and coverage tests using proto reflection to catch future drift. Assisted-By: 🤖 Claude Code
- SandboxPolicyRevision: add provenance map for audit metadata - SandboxTemplate: expose resources and driver_config as map[string]any using structpb.Struct round-trip (same pattern as middleware config) - Add coverage test for SandboxPolicyRevision proto fields Assisted-By: 🤖 Claude Code
- SandboxInterface.Create now accepts variadic ...CreateOptions with Annotations field for setting metadata at sandbox creation time - ConfigUpdate and ConfigUpdateResult gain Annotations for caller- provided and response metadata on config/policy updates - Update all SandboxInterface implementations (real client, fake, test stubs) for the new signature Assisted-By: 🤖 Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR expands SDK types and protobuf converters for metadata, credentials, refresh strategies, sandbox configuration, and annotations. It updates sandbox creation options, adds converter coverage, and documents the Edge and Fake APIs. ChangesSDK contracts and conversion support
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 #57 +/- ##
==========================================
+ Coverage 89.59% 89.84% +0.25%
==========================================
Files 78 78
Lines 4989 5102 +113
==========================================
+ Hits 4470 4584 +114
+ Misses 352 348 -4
- Partials 167 170 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/src/api/fake.md`:
- Around line 69-73: Update the PrincipalSubject value in the AddMember
workspace example to use an opaque OIDC sub-style identifier such as
“subject-123” instead of an email address, while leaving the surrounding
workspace and role example unchanged.
- Around line 22-24: Update the quick-start example around
client.Sandboxes().Create to pass &types.SandboxSpec{} instead of
&v1.SandboxSpec{}, and add the types package to the example imports. Preserve
the existing Create call and assertion behavior.
In `@openshell/v1/fake/sandbox.go`:
- Line 214: Update fakeSandboxClient.Create to retain the variadic CreateOptions
input: inspect the first option when provided and copy its Annotations into the
created sandbox’s sb.Annotations before storing or returning it, while
preserving current behavior when no options are supplied.
In `@openshell/v1/internal/converter/sandbox.go`:
- Around line 184-190: Handle and propagate errors returned by
structpb.NewStruct for both spec.Template.Resources and
spec.Template.DriverConfig in the converter path. Do not discard either error or
assign a nil field after conversion failure; return the conversion error so
malformed configuration is rejected before the request is created.
In `@openshell/v1/sandbox.go`:
- Line 56: Update the fake client’s Create implementation to preserve the
selected CreateOptions.Annotations when constructing the new sandbox, matching
the gRPC client’s request behavior. Locate the Create method in the fake sandbox
client and copy the annotations into the created sandbox metadata without
discarding the provided options.
🪄 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: b570224b-e366-4c05-b20c-4e8de398ef13
📒 Files selected for processing (26)
docs/src/SUMMARY.mddocs/src/api/edge.mddocs/src/api/fake.mdopenshell/v1/exec_client_test.goopenshell/v1/fake/sandbox.goopenshell/v1/internal/converter/coverage_test.goopenshell/v1/internal/converter/policy.goopenshell/v1/internal/converter/policy_test.goopenshell/v1/internal/converter/profile.goopenshell/v1/internal/converter/profile_test.goopenshell/v1/internal/converter/refresh.goopenshell/v1/internal/converter/refresh_test.goopenshell/v1/internal/converter/sandbox.goopenshell/v1/internal/converter/sandbox_test.goopenshell/v1/internal/converter/setting.goopenshell/v1/internal/converter/setting_test.goopenshell/v1/sandbox.goopenshell/v1/sandbox_client.goopenshell/v1/ssh_client_test.goopenshell/v1/tcp_client_test.goopenshell/v1/types/options.goopenshell/v1/types/policy.goopenshell/v1/types/profile.goopenshell/v1/types/refresh.goopenshell/v1/types/sandbox.goopenshell/v1/types/setting.go
There was a problem hiding this comment.
Pull request overview
This PR closes proto-to-SDK parity gaps by adding missing SDK fields, extending proto<->SDK converters (with deep-copy expectations), updating client interfaces to accept sandbox create annotations, and adding/expanding coverage tests that assert all proto fields are either handled or explicitly skipped.
Changes:
- Add new SDK fields for profiles/credentials/refresh strategies, sandbox templates, policy revisions, and config update annotations.
- Extend converters and tests to round-trip and deep-copy newly mapped fields (plus proto-reflection “coverage” tests to prevent future drift).
- Update the sandbox client interface to accept
...CreateOptions(withAnnotations) and adjust affected test mocks/fakes/docs.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openshell/v1/types/setting.go | Adds annotations to ConfigUpdate and ConfigUpdateResult. |
| openshell/v1/types/sandbox.go | Exposes SandboxTemplate.Resources and DriverConfig as map[string]any. |
| openshell/v1/types/refresh.go | Adds AWS STS assume-role refresh strategy constant. |
| openshell/v1/types/profile.go | Expands profile/credential types including token grant configuration. |
| openshell/v1/types/policy.go | Adds SandboxPolicyRevision.Provenance. |
| openshell/v1/types/options.go | Extends CreateOptions with Annotations. |
| openshell/v1/tcp_client_test.go | Updates resolver mock for variadic CreateOptions. |
| openshell/v1/ssh_client_test.go | Updates resolver mock for variadic CreateOptions. |
| openshell/v1/sandbox.go | Updates SandboxInterface.Create signature to accept ...CreateOptions. |
| openshell/v1/sandbox_client.go | Plumbs create annotations into CreateSandboxRequest. |
| openshell/v1/internal/converter/setting.go | Converts annotations for config update request/response. |
| openshell/v1/internal/converter/setting_test.go | Adds deep-copy tests for config update annotations. |
| openshell/v1/internal/converter/sandbox.go | Converts sandbox template resources/driver config via structpb. |
| openshell/v1/internal/converter/sandbox_test.go | Adds test coverage for resources/driver config conversions. |
| openshell/v1/internal/converter/refresh.go | Maps new refresh strategy between proto and SDK. |
| openshell/v1/internal/converter/refresh_test.go | Adds tests for AWS STS assume-role refresh strategy mapping. |
| openshell/v1/internal/converter/profile.go | Maps new provider profile fields and credential token grants. |
| openshell/v1/internal/converter/profile_test.go | Adds round-trip + deep-copy tests for new profile/credential fields. |
| openshell/v1/internal/converter/policy.go | Maps policy revision provenance from proto. |
| openshell/v1/internal/converter/policy_test.go | Adds deep-copy coverage for provenance mapping. |
| openshell/v1/internal/converter/coverage_test.go | Adds proto-reflection coverage tests for newly mapped messages. |
| openshell/v1/fake/sandbox.go | Adjusts fake sandbox client to match new Create signature. |
| openshell/v1/exec_client_test.go | Updates resolver mock for variadic CreateOptions. |
| docs/src/SUMMARY.md | Adds docs nav entries for edge and fake packages. |
| docs/src/api/fake.md | Adds new API documentation page for the fake client package. |
| docs/src/api/edge.md | Adds new API documentation page for the edge utilities package. |
Suppressed comments (1)
openshell/v1/fake/sandbox.go:218
- fakeSandboxClient.Create now accepts CreateOptions but currently ignores them, so sandbox-scoped annotations passed by callers won’t be reflected in the created sandbox. Also, the fake store claims to deep-copy objects at boundaries (store.go), but the sandbox copy helpers don’t appear to deep-copy the newly added SandboxTemplate.Resources / DriverConfig maps, which can allow caller mutation to affect stored state.
func (c *fakeSandboxClient) Create(_ context.Context, workspace, name string, spec *types.SandboxSpec, labels map[string]string, _ ...types.CreateOptions) (*types.Sandbox, error) {
if c.closedFunc() {
return nil, &types.StatusError{Code: types.ErrorUnavailable, Message: "client is closed"}
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Deep-copy Resources/DriverConfig in fake copySandboxTemplate (correctness) - Guard structpb.NewStruct with err==nil check (production consistency) - Apply CreateOptions.Annotations in fake Create (fake-real parity) - Add deep-copy mutation tests for Resources/DriverConfig and bytes ToProto - Use opaque OIDC subject in fake.md example (docs accuracy) Assisted-By: 🤖 Claude Code
Added tests to address Codecov coverage regression: - TestSandbox_Create_WithAnnotations: verify annotations pass-through - TestSandbox_Create_WithAnnotationsDeepCopy: mutation isolation - TestCopyAnyMap: nil, flat, nested map, nested slice, scalar types - TestCopySandboxTemplate_ResourcesDeepCopy: Resources/DriverConfig isolation Assisted-By: 🤖 Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
edgeandfakepackages (fixes CIdocs:check)ProviderProfile(annotations, source, scope),ProfileCredential(6 new fields including token grant sub-types),RefreshStrategy(AWS STS assume role)SandboxPolicyRevision.Provenanceand exposeSandboxTemplate.Resources/DriverConfigasmap[string]anyvia structpbSandboxInterface.Createnow accepts variadic...CreateOptionswithAnnotations;ConfigUpdate/ConfigUpdateResultgainAnnotationsAll new fields include deep-copy at proto/SDK boundaries and proto-reflection coverage tests to catch future drift.
Test plan
mise run cipasses all 5 checks (lint, build, test, proto:check, docs:check)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes