feat: add Drop D proto gap types (MCP, middleware, SigV4) - #55
Conversation
|
Warning Review limit reached
Next review available in: 49 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 (2)
📝 WalkthroughWalkthroughThe PR adds SDK types and protobuf conversions for SigV4, JSON-RPC, MCP, and network middleware policy fields. It also adds round-trip, deep-copy, and converter coverage tests, plus design and implementation documentation. ChangesNetwork Policy Proto Gaps
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
+ Coverage 89.57% 89.59% +0.01%
==========================================
Files 78 78
Lines 4901 4967 +66
==========================================
+ Hits 4390 4450 +60
- Misses 345 350 +5
- Partials 166 167 +1 ☔ 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: 1
🧹 Nitpick comments (1)
openshell/v1/internal/converter/policy_test.go (1)
689-703: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover
Configdeep-copy isolation.This test mutates only
Endpoints.Include. Add aConfigvalue, mutate the source proto configuration after conversion, and assert thatpolicy.NetworkMiddlewares["test"].Configretains the original value.Based on learnings, mutable proto-backed maps must be deep-copied across the proto/SDK boundary.
🤖 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/policy_test.go` around lines 689 - 703, Extend TestSandboxPolicyMiddlewareDeepCopy by adding a Config value to the test middleware, mutate the corresponding source proto Config after SandboxPolicyFromProto conversion, and assert policy.NetworkMiddlewares["test"].Config still contains the original value. Keep the existing Endpoints.Include isolation assertion.Sources: Coding guidelines, Learnings
🤖 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/policy.go`:
- Around line 185-190: Update the converter containing SandboxPolicyToProto so
structpb.NewStruct errors for m.Config are propagated to the caller instead of
silently omitting result.Config. Change the relevant converter signatures and
call sites to return and handle the error, or validate
NetworkMiddlewareConfig.Config before request construction, while preserving the
existing successful conversion behavior.
---
Nitpick comments:
In `@openshell/v1/internal/converter/policy_test.go`:
- Around line 689-703: Extend TestSandboxPolicyMiddlewareDeepCopy by adding a
Config value to the test middleware, mutate the corresponding source proto
Config after SandboxPolicyFromProto conversion, and assert
policy.NetworkMiddlewares["test"].Config still contains the original value. Keep
the existing Endpoints.Include isolation assertion.
🪄 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: c0734b27-6ec7-4974-adfa-756f838e06c6
📒 Files selected for processing (9)
docs/superpowers/plans/2026-08-08-proto-gap-drop-d.mddocs/superpowers/specs/2026-08-08-proto-gap-drop-d-design.mdopenshell/v1/internal/converter/coverage_test.goopenshell/v1/internal/converter/network_policy.goopenshell/v1/internal/converter/network_policy_test.goopenshell/v1/internal/converter/policy.goopenshell/v1/internal/converter/policy_test.goopenshell/v1/types/network_policy.goopenshell/v1/types/policy.go
| if m.Config != nil { | ||
| s, err := structpb.NewStruct(m.Config) | ||
| if err == nil { | ||
| result.Config = s | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all conversion call sites that must propagate validation errors.
rg -n -C 3 --type go '\bSandboxPolicyToProto\s*\(' .
rg -n -C 3 --type go '\bmiddlewareConfigToProto\s*\(' .
rg -n -C 3 --type go '\bNewStruct\s*\(' openshell/v1/internal/converterRepository: rhuss/openshell-sdk-go
Length of output: 8493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the converter signatures, call sites, and concrete middleware config handling.
sed -n '1,220p' openshell/v1/internal/converter/policy.go
sed -n '140,210p' openshell/v1/internal/converter/sandbox.go
sed -n '170,200p' openshell/v1/internal/converter/setting.go
# Search for API/write/validate calls around Policy or middleware config to see whether errors are usable downstream.
rg -n -C 2 --type go '\b(Policy|MergeOperations|SandboxPolicy|NetworkMiddlewares|PolicyPolicies|Middleware|Config)\b' openshell/v1/internal openshell/v1 --glob '!**/*_test.go' | head -n 200Repository: rhuss/openshell-sdk-go
Length of output: 22350
Return Config conversion errors.
structpb.NewStruct can reject values in NetworkMiddlewareConfig.Config. The current code drops the conversion and sends a middleware config without Config, changing the policy.
Make the conversion caller-propagated, either by changing these converters to return an error or by validating Config before SandboxPolicyToProto builds the request.
🤖 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/policy.go` around lines 185 - 190, Update the
converter containing SandboxPolicyToProto so structpb.NewStruct errors for
m.Config are propagated to the caller instead of silently omitting
result.Config. Change the relevant converter signatures and call sites to return
and handle the error, or validate NetworkMiddlewareConfig.Config before request
construction, while preserving the existing successful conversion behavior.
There was a problem hiding this comment.
Valid observation. The SDK converter pattern does not return errors (all ToProto functions have void returns), so propagating the structpb.NewStruct error would break the established API contract across all converters.
In practice, Config values always originate from structpb.AsMap() on the FromProto path, which produces only JSON-compatible types. A comment has been added (d0be11f) explaining this constraint: non-JSON-compatible values in user-constructed maps are silently dropped.
There was a problem hiding this comment.
@rhuss, the added comment documents the behavior, but it does not resolve the data-loss case.
NetworkMiddlewareConfig.Config is public and user-constructible as map[string]any. If it contains a value that structpb.NewStruct rejects, the SDK sends a different policy without Config.
I understand that the current ToProto contract has no error return. That constraint means this change needs validation before conversion, or another explicit invalid-config behavior. The converter must not silently omit policy configuration.
✏️ Learnings added
Learnt from: rhuss
Repo: rhuss/openshell-sdk-go PR: 55
File: openshell/v1/internal/converter/policy.go:185-192
Timestamp: 2026-08-08T14:36:47.934Z
Learning: In the Go SDK, established `ToProto` converters use void return values and do not return conversion errors. In `openshell/v1/internal/converter/policy.go`, `middlewareConfigToProto` documents that it silently drops `types.NetworkMiddlewareConfig.Config` when `structpb.NewStruct` rejects user-constructed non-JSON-compatible map values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
Pull request overview
Adds the remaining “Drop D” network-policy proto gap types/fields to the Go SDK’s domain layer and converters, with coverage + unit tests to ensure new proto fields are represented and round-trip correctly.
Changes:
- Extend network endpoint policy types/converters to support SigV4 signing fields, JSON-RPC max body size, MCP options, and L7 allow/deny
Paramsmatching. - Add network middleware domain types plus
SandboxPolicy.NetworkMiddlewares, with policy converters and unit tests. - Update converter field-coverage tests and add design/plan docs for the Drop D proto gap work.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openshell/v1/types/policy.go | Adds NetworkMiddlewares to SandboxPolicy and introduces middleware domain types. |
| openshell/v1/types/network_policy.go | Adds SigV4/JSON-RPC/MCP fields to PolicyNetworkEndpoint and Params to L7 rule types. |
| openshell/v1/internal/converter/policy.go | Implements middleware conversions and wires them into SandboxPolicy conversions. |
| openshell/v1/internal/converter/policy_test.go | Adds middleware conversion, round-trip, and deep-copy tests. |
| openshell/v1/internal/converter/network_policy.go | Maps new endpoint fields, MCP options, and L7 Params between proto and domain types. |
| openshell/v1/internal/converter/network_policy_test.go | Adds unit tests for new endpoint fields, MCP, and Params. |
| openshell/v1/internal/converter/coverage_test.go | Marks new proto fields/messages as handled via coverage tests. |
| docs/superpowers/specs/2026-08-08-proto-gap-drop-d-design.md | Documents the design/mapping decisions for the proto-gap fixes. |
| docs/superpowers/plans/2026-08-08-proto-gap-drop-d.md | Implementation plan/checklist for the Drop D proto-gap work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
PolicyNetworkEndpoint(closes Add SigV4 credential signing and JSON-RPC fields on NetworkEndpoint #37)McpOptionstype andParamsfield onL7Allow/L7DenyRule(closes Add MCP policy types (McpOptions, L7 MCP filtering) #35)NetworkMiddlewareConfig,MiddlewareEndpointSelectortypes andSandboxPolicy.NetworkMiddlewares(closes Add network middleware types (NetworkMiddlewareConfig, SupervisorMiddlewareService) #36)All changes follow the established 3-layer pattern: domain types, converters with deep-copy, coverage tests, and round-trip/deep-copy unit tests.
JIRA: RHAIENG-6595
Test plan
mise run testpassesmise run lintpasses (0 issues)mise run proto:checkpasses (generated files up to date)TestConverterCoversAllProtoFields_*coverage tests pass (13/13)skippedfields remain inSandboxPolicycoverage testSummary by CodeRabbit