Repository navigation
refactor: switch proto generation from protoc to buf - #31
Merged
Merged
Conversation
Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code
Assisted-By: 🤖 Claude Code
Replace raw protoc invocations with buf, aligning with the monorepo SDK approach (PR NVIDIA/OpenShell#2271). - Add buf.yaml and buf.gen.yaml for declarative proto generation - Replace protoc-based mise tasks (proto:gen, proto:check) with buf - Remove proto:sync, proto:clean tasks (no longer needed) - Remove UPSTREAM_VERSION (proto sources are now the source of truth) - Sync proto files from upstream (includes options.proto) - Add buf 1.72.0 to mise.toml, remove standalone protoc dependency - Add proto target to Makefile - Regenerate all .pb.go files via buf pipeline All 1035 tests pass. Signed-off-by: Roland Huß <rhuss@redhat.com>
📝 WalkthroughWalkthroughThe PR switches protobuf generation to Buf, adds generation and validation configuration, expands OpenShell workspace, provider, policy, gateway, sandbox, middleware, and secret-field schemas, and adds brainstorming documents for SDK documentation, authentication, and upstream migration planning. ChangesProto generation workflow
Metadata and API contracts
SDK planning records
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 #31 +/- ##
==========================================
+ Coverage 88.51% 88.56% +0.04%
==========================================
Files 70 70
Lines 4163 4163
==========================================
+ Hits 3685 3687 +2
+ Misses 330 328 -2
Partials 148 148 ☔ 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)
proto/openshell.proto (1)
2099-2105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
annotationstoCreateWorkspaceRequestfor consistency.To align with the
Workspacemodel (whereObjectMetasupports bothlabelsandannotations) and other create requests likeCreateSandboxRequest, consider adding anannotationsfield here.♻️ Proposed refactor
message CreateWorkspaceRequest { // Workspace name. Must be a valid DNS-1123 label. string name = 1; // Optional labels for the workspace (key-value metadata). map<string, string> labels = 2; + // Optional annotations for the workspace (non-selector metadata). + map<string, string> annotations = 3; }🤖 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 `@proto/openshell.proto` around lines 2099 - 2105, Add an annotations map field to the CreateWorkspaceRequest message alongside the existing labels field, matching the Workspace ObjectMeta and CreateSandboxRequest conventions; assign it a new, unused protobuf field number.
🤖 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 `@proto/sandbox.proto`:
- Around line 163-209: Update the canonical protocol field documentation to
include "mcp" and "json-rpc" among the supported values, matching the McpOptions
usage and JSON-RPC method references. Keep the existing protocol values and
descriptions intact while making the enumeration the complete source of truth.
---
Nitpick comments:
In `@proto/openshell.proto`:
- Around line 2099-2105: Add an annotations map field to the
CreateWorkspaceRequest message alongside the existing labels field, matching the
Workspace ObjectMeta and CreateSandboxRequest conventions; assign it a new,
unused protobuf field number.
🪄 Autofix (Beta)
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
Run ID: 2a334fbf-77d8-4d4f-924c-08f9d3741da2
⛔ Files ignored due to path filters (5)
proto/datamodelv1/datamodel.pb.gois excluded by!**/*.pb.goproto/openshellv1/openshell.pb.gois excluded by!**/*.pb.goproto/openshellv1/openshell_grpc.pb.gois excluded by!**/*.pb.goproto/optionsv1/options.pb.gois excluded by!**/*.pb.goproto/sandboxv1/sandbox.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
Makefilebrainstorm/00-overview.mdbrainstorm/022-auth-guide.mdbrainstorm/023-schema-docs.mdbrainstorm/024-upstream-pr.mdbuf.gen.yamlbuf.yamlmise.tomlproto/UPSTREAM_VERSIONproto/datamodel.protoproto/openshell.protoproto/options.protoproto/sandbox.proto
💤 Files with no reviewable changes (1)
- proto/UPSTREAM_VERSION
Comment on lines
+163
to
+209
| // Proxy-side credential signing mode: "sigv4" for AWS SigV4 re-signing. | ||
| // When set, the proxy strips the client's Authorization header and computes | ||
| // a fresh SigV4 signature using real credentials from the provider. | ||
| string credential_signing = 19; | ||
| // AWS signing service name override. Required when credential_signing is | ||
| // "sigv4" — e.g. "bedrock" for bedrock-runtime endpoints. | ||
| string signing_service = 20; | ||
| // AWS region override for SigV4 signing. When set, takes precedence over | ||
| // hostname-based region extraction. Required for non-standard endpoints. | ||
| string signing_region = 21; | ||
| // Maximum JSON-RPC-over-HTTP request body bytes to buffer for inspection. | ||
| // Defaults to 65536 when unset. | ||
| uint32 json_rpc_max_body_bytes = 22; | ||
| // MCP-only policy and inspection options. Only used when protocol is "mcp". | ||
| McpOptions mcp = 23; | ||
| } | ||
|
|
||
| // MCP options are grouped so MCP-specific policy can grow without adding more | ||
| // top-level NetworkEndpoint fields. Current enforcement targets the active | ||
| // 2025-11-25 Streamable HTTP/tools behavior, while preserving space for | ||
| // version-profile policy if OpenShell adopts 2026-07-28 draft behavior later. | ||
| // | ||
| // Planned policy extensions should use OpenShell-owned static definitions for | ||
| // MCP method/version profiles rather than treating dependency enums as the | ||
| // policy contract. Candidate profile checks include request metadata/header | ||
| // validation, response/SSE introspection, trusted annotation handling, | ||
| // resultType/cache metadata validation, x-mcp-header tool-definition checks, | ||
| // and subscriptions/listen handling. | ||
| // | ||
| // Sources: | ||
| // - https://modelcontextprotocol.io/specification/2025-11-25/server/tools | ||
| // - https://modelcontextprotocol.io/specification/draft/changelog | ||
| // - https://modelcontextprotocol.io/specification/draft/basic/transports/streamable-http | ||
| // - https://modelcontextprotocol.io/specification/draft/server/tools | ||
| message McpOptions { | ||
| // Hardening boundary for tools/call params.name. When unset or true, the | ||
| // supervisor enforces the MCP recommended tool-name syntax | ||
| // ^[A-Za-z0-9_.-]{1,128}$ before policy evaluation. Set false only for | ||
| // compatibility with servers that intentionally use non-recommended names. | ||
| // | ||
| // Source: | ||
| // - https://modelcontextprotocol.io/specification/2025-11-25/server/tools#tool-names | ||
| optional bool strict_tool_names = 1; | ||
| // Method-layer default for MCP endpoints. When true, OpenShell allows parsed | ||
| // MCP-family methods at the method layer unless a tool-name policy narrows | ||
| // tools/call. When unset or false, explicit method rules are required. | ||
| optional bool allow_all_known_mcp_methods = 2; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
protocol field enum doc doesn't list "mcp" (or "json-rpc"), but the new mcp field depends on it.
McpOptions mcp (line 177) states it's "Only used when protocol is "mcp"," yet the canonical protocol field enumeration ("rest", "websocket", "graphql", "sql", or "" — line 105-106) doesn't include "mcp". The changed method comments on L7DenyRule/L7Allow (lines 226-227, 256-257) similarly reference "JSON-RPC method name" as a supported form, which is also absent from that enumeration. Readers relying on the protocol field doc as the source of truth for valid values will miss these.
📝 Suggested doc update (outside this range, at line 105-106)
- // Application protocol for L7 inspection: "rest", "websocket", "graphql", "sql", or "" (L4-only).
+ // Application protocol for L7 inspection: "rest", "websocket", "graphql", "sql", "json-rpc", "mcp", or "" (L4-only).🤖 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 `@proto/sandbox.proto` around lines 163 - 209, Update the canonical protocol
field documentation to include "mcp" and "json-rpc" among the supported values,
matching the McpOptions usage and JSON-RPC method references. Keep the existing
protocol values and descriptions intact while making the enumeration the
complete source of truth.
6 tasks done
5 of 6 tasks
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
protoctobuffor proto code generation, aligning with the monorepo SDK approach (NVIDIA/OpenShell#2271)options.protothat was previously stripped)UPSTREAM_VERSION,proto:sync, andproto:cleantasksChanges
buf.yamlandbuf.gen.yamlfor declarative proto generationproto:gen,proto:check) with buf equivalentsproto:sync,proto:cleantasks (no longer needed)buf 1.72.0tomise.toml, remove standaloneprotocdependencyprototarget to Makefile.pb.gofiles via buf pipelineTesting
mise run proto:checkconfirms generated files are up to datego build ./...compiles cleanlySummary by CodeRabbit