build(proto): switch to buf and gate gen on breaking check#586
Conversation
|
Warning Review limit reached
More reviews will be available in 54 minutes and 19 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces ChangesProto tooling migration to buf
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the Protobuf generation process to the buf toolchain, introducing buf.yaml and buf.gen.yaml configurations and a breaking change check. The review feedback recommends strengthening the check-tools target in the Makefile to strictly enforce versioning for buf and its plugins, ensuring build reproducibility. Additionally, it is suggested to change the breaking change detection category from FILE to WIRE to allow for backward-compatible API evolution without unnecessary failures.
| @installed_buf=$$($(BUF) --version 2>/dev/null); \ | ||
| if [ "$$installed_buf" != "$(BUF_VERSION)" ]; then \ | ||
| echo "warning: buf version mismatch (expected $(BUF_VERSION), got $$installed_buf)"; \ | ||
| fi | ||
| @if [ "$$(protoc-gen-go --version)" != "$(PROTOC_GEN_GO_VERSION)" ]; then \ | ||
| echo "expected $(PROTOC_GEN_GO_VERSION), got $$(protoc-gen-go --version)"; \ | ||
| @command -v protoc-gen-go >/dev/null 2>&1 || { \ | ||
| echo "protoc-gen-go not found; install with:"; \ | ||
| echo " go install google.golang.org/protobuf/cmd/protoc-gen-go@$(PROTOC_GEN_GO_VERSION)"; \ | ||
| exit 1; \ | ||
| fi | ||
| @if [ "$$(protoc-gen-go-grpc --version)" != "$(PROTOC_GEN_GO_GRPC_VERSION)" ]; then \ | ||
| echo "expected $(PROTOC_GEN_GO_GRPC_VERSION), got $$(protoc-gen-go-grpc --version)"; \ | ||
| } | ||
| @command -v protoc-gen-go-grpc >/dev/null 2>&1 || { \ | ||
| echo "protoc-gen-go-grpc not found; install with:"; \ | ||
| echo " go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v$(PROTOC_GEN_GO_GRPC_VERSION)"; \ | ||
| exit 1; \ | ||
| } |
There was a problem hiding this comment.
The check-tools target has been weakened compared to the previous implementation. It now only issues a warning for buf version mismatches and completely omits version verification for protoc-gen-go and protoc-gen-go-grpc.
To ensure reproducible builds and prevent unexpected diffs in generated code caused by toolchain drift, these checks should be strict (exit with an error) and cover all plugins. Additionally, the version variables are inconsistent: PROTOC_GEN_GO_VERSION includes a v prefix while PROTOC_GEN_GO_GRPC_VERSION does not, which makes the install commands and version checks harder to maintain.
@installed_buf=$$($(BUF) --version 2>/dev/null); \
if [ "$$installed_buf" != "$(BUF_VERSION)" ]; then \
echo "error: buf version mismatch (expected $(BUF_VERSION), got $$installed_buf)"; \
exit 1; \
fi
@command -v protoc-gen-go >/dev/null 2>&1 || { \
echo "protoc-gen-go not found; install with:"; \
echo " go install google.golang.org/protobuf/cmd/protoc-gen-go@$(PROTOC_GEN_GO_VERSION)"; \
exit 1; \
}
@if [ "$$(protoc-gen-go --version 2>/dev/null)" != "protoc-gen-go $(PROTOC_GEN_GO_VERSION)" ]; then \
echo "error: protoc-gen-go version mismatch (expected $(PROTOC_GEN_GO_VERSION), got $$(protoc-gen-go --version))"; \
exit 1; \
fi
@command -v protoc-gen-go-grpc >/dev/null 2>&1 || { \
echo "protoc-gen-go-grpc not found; install with:"; \
echo " go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v$(PROTOC_GEN_GO_GRPC_VERSION)"; \
exit 1; \
}
@if [ "$$(protoc-gen-go-grpc --version 2>/dev/null)" != "protoc-gen-go-grpc $(PROTOC_GEN_GO_GRPC_VERSION)" ]; then \
echo "error: protoc-gen-go-grpc version mismatch (expected $(PROTOC_GEN_GO_GRPC_VERSION), got $$(protoc-gen-go-grpc --version))"; \
exit 1; \
fi
| - path: . | ||
| breaking: | ||
| use: | ||
| - FILE |
There was a problem hiding this comment.
The FILE breaking change category is extremely restrictive as it fails on any modification to the Protobuf descriptors, including backward-compatible changes like adding a new field or a new service method.
If the intent is to allow the API to evolve while preventing wire-incompatible changes (as suggested in the PR summary), the WIRE category is typically more appropriate. FILE is generally reserved for projects that require absolute stability of the file structure and descriptors.
- WIRE
|
/gemini review @codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e312f96afc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @if git -C .. rev-parse --verify main >/dev/null 2>&1; then \ | ||
| $(BUF) breaking --against '$(BREAKING_AGAINST)'; \ | ||
| else \ | ||
| echo "skip breaking check: main branch not found"; \ |
There was a problem hiding this comment.
gen currently depends on breaking, but breaking silently succeeds when git -C .. rev-parse --verify main fails; in single-branch clones or detached checkouts where only origin/main exists, this prints “skip breaking check” and still runs buf generate, so wire-incompatible proto changes are regenerated without any gate. Since this commit’s goal is to enforce buf breaking before generation, the target should resolve a reachable baseline (for example origin/main) or fail instead of skipping.
Useful? React with 👍 / 👎.
Replace protoc-based proto/Makefile with buf (v1.55.1). `make -C proto gen` now runs `buf breaking` against main before `buf generate`, so wire- incompatible changes fail locally before regenerating. Add a Proto workflow that runs the same breaking check, regenerates, and fails if the committed *.pb.go drift from the generated output. Regenerate all *.pb.go under buf; the only functional diff is normalized internal symbol names in internal.pb.go (no exported API change).
- Swap breaking rule FILE -> WIRE so non-wire-breaking evolution (field renames, etc.) is no longer flagged as breaking. - Strictly enforce buf, protoc-gen-go, and protoc-gen-go-grpc versions in check-tools; mismatches now fail rather than warn. - Fix Proto CI: explicitly fetch refs/remotes/origin/main and compare against it (branch=main is not materialized in PR checkouts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bee4bef309
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run: buf generate | ||
| - name: Fail on regenerated diff | ||
| run: | | ||
| if ! git diff --exit-code -- proto/; then |
There was a problem hiding this comment.
The Fail on regenerated diff step can miss missing generated artifacts because git diff --exit-code -- proto/ only compares tracked changes (index/worktree) and returns success when buf generate creates new untracked *.pb.go files. In that case CI can pass even though generation output is not fully committed (e.g., adding a new .proto file and forgetting to git add the new generated file), so this gate does not reliably enforce the intended “committed output matches generated output” invariant.
Useful? React with 👍 / 👎.
- Makefile breaking target no longer silently skips when main is missing. It auto-detects main or origin/main, lets BREAKING_AGAINST override, and fails closed when no baseline is reachable. - CI drift check now runs `git add -N proto/` before `git diff --exit-code` so newly generated .pb.go files (e.g. after adding a .proto but forgetting to commit the generated output) surface as a diff instead of slipping past the gate.
|
/gemini review @codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 851919521e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "no breaking baseline found: neither 'main' nor 'origin/main' is reachable"; \ | ||
| echo "fetch main first (git fetch origin main) or override with BREAKING_AGAINST=..."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| if [ -n "$(BREAKING_AGAINST)" ]; then baseline='$(BREAKING_AGAINST)'; fi; \ |
There was a problem hiding this comment.
BREAKING_AGAINST is documented as an override, but the breaking recipe exits in the else branch before that override is applied. In environments without reachable main/origin/main refs (for example shallow or single-branch clones), make gen BREAKING_AGAINST=... still fails with “no breaking baseline found,” so the advertised override path is unusable.
Useful? React with 👍 / 👎.
| if git -C .. rev-parse --verify main >/dev/null 2>&1; then \ | ||
| baseline='../.git#subdir=proto,branch=main'; \ | ||
| elif git -C .. rev-parse --verify origin/main >/dev/null 2>&1; then \ | ||
| baseline='../.git#subdir=proto,ref=refs/remotes/origin/main'; \ |
There was a problem hiding this comment.
The baseline selection in breaking prefers local main before origin/main, which can silently compare against a stale branch on developer machines. If local main lags remote, local make gen may miss breaking changes that CI later rejects (CI uses refs/remotes/origin/main in .github/workflows/proto.yml), making the gate inconsistent across environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7babe860fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var File_internal_proto protoreflect.FileDescriptor | ||
|
|
||
| const file_proto_internal_proto_rawDesc = "" + | ||
| const file_internal_proto_rawDesc = "" + | ||
| "\n" + | ||
| "\x14proto/internal.proto\"G\n" + | ||
| "\x0einternal.proto\"G\n" + |
There was a problem hiding this comment.
For consumers that use the generated descriptor API, this regeneration changes the registered file path from proto/internal.proto to internal.proto and removes the previous exported File_proto_internal_proto symbol. That breaks code using protoregistry.GlobalFiles.FindFileByPath("proto/internal.proto") or pb.File_proto_internal_proto even though the wire format is unchanged, so keep the old source path or add a compatibility alias if this package is imported outside the repo.
Useful? React with 👍 / 👎.
| - name: buf breaking (against main) | ||
| if: github.ref != 'refs/heads/main' | ||
| working-directory: proto | ||
| run: buf breaking --against '../.git#subdir=proto,ref=refs/remotes/origin/main' |
There was a problem hiding this comment.
In the PR workflow, origin/main is fetched only as a remote-tracking ref in the runner checkout, but Buf's git input clones the local .git and then checks out the requested ref; that clone will not contain the caller's refs/remotes/origin/main unless it came from a local branch named main, so this can either fail CI on proto PRs or compare against stale local main. Fresh evidence beyond the earlier baseline comments is that the current command now passes the remote-tracking ref directly to Buf's local-git input; use a real branch in the cloned input or a remote URL/commit instead.
Useful? React with 👍 / 👎.
| run: buf breaking --against '../.git#subdir=proto,ref=refs/remotes/origin/main' | ||
| - name: buf generate | ||
| working-directory: proto | ||
| run: buf generate |
There was a problem hiding this comment.
When a .proto file is removed or renamed, this in-place buf generate leaves the old tracked *.pb.go and *_grpc.pb.go files untouched; because those files are already tracked and unchanged, the later git add -N/git diff -- proto/ still exits successfully and stale generated APIs can remain committed. Clean or explicitly remove only the generated outputs before regenerating so deletions are surfaced by the drift check.
Useful? React with 👍 / 👎.
| - path: . | ||
| breaking: | ||
| use: | ||
| - WIRE |
There was a problem hiding this comment.
Using only WIRE means this new required breaking gate ignores generated source/API breakage; Buf's rules docs state that WIRE detects binary encoding breakage and does not check generated source code. In this repo's public gRPC protos, a PR can delete or rename an RPC such as RawKV.RawPut and still pass buf breaking, while generated clients/servers and existing gRPC callers break; use PACKAGE/FILE or add the RPC/service deletion rules if the gate is meant to prevent API-breaking proto changes.
Useful? React with 👍 / 👎.
|
@codex review 対応しました。
確認:
|
![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
🤖 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 @.github/workflows/proto.yml:
- Around line 27-33: The GitHub Actions in the proto workflow are using mutable
version tags (v6) which violates hardened CI policy and creates upstream tag
retarget risk. Replace the mutable `@v6` tags for both the actions/checkout and
actions/setup-go actions with pinned commit SHAs. Additionally, add the
persist-credentials: false parameter to the actions/checkout action to reduce
token exposure and improve security posture.
🪄 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: 84e4259d-0b51-4089-b2fb-ed8491ae18a0
⛔ Files ignored due to path filters (10)
proto/distribution.pb.gois excluded by!**/*.pb.goproto/distribution_grpc.pb.gois excluded by!**/*.pb.goproto/dynamodb_internal.pb.gois excluded by!**/*.pb.goproto/etcd_raft.pb.gois excluded by!**/*.pb.goproto/etcd_raft_grpc.pb.gois excluded by!**/*.pb.goproto/internal.pb.gois excluded by!**/*.pb.goproto/internal_grpc.pb.gois excluded by!**/*.pb.goproto/redis_internal.pb.gois excluded by!**/*.pb.goproto/service.pb.gois excluded by!**/*.pb.goproto/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
.github/workflows/proto.ymlproto/Makefileproto/buf.gen.yamlproto/buf.yamlproto/internal.proto
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d917069c1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run: buf breaking --against '../.git#subdir=proto,ref=refs/remotes/origin/main' | ||
| - name: buf generate | ||
| working-directory: proto | ||
| run: buf generate |
There was a problem hiding this comment.
Because this runs a plain buf generate before diffing, generated Go files for a .proto that was removed or renamed are left untouched; the Buf docs describe clean generation as the mechanism that prevents removed .proto types from leaving stale generated files. In a PR that deletes proto/foo.proto but forgets foo.pb.go, the following git diff remains clean and go build can pass against the stale API, so this gate misses that drift; add a safe generated-file cleanup (not a blind clean of out: .) before generating.
Useful? React with 👍 / 👎.
|
@codex review Addressed the current proto workflow findings:
Validation: actionlint .github/workflows/proto.yml; buf breaking against buf-breaking-base; make -C proto gen with buf 1.55.1; generated Go files have no drift; git diff --check; GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go build ./.... Note: I did not raise buf breaking from WIRE to PACKAGE/FILE in this branch because the branch is behind current main and that stricter mode currently fails on unrelated admin proto additions from main as deletions. That should be done after rebasing/updating this PR onto current main so the stricter API gate does not fail for unrelated history skew. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # proto/Makefile # proto/internal.pb.go
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
proto/Makefilewith buf (pinned to v1.55.1);make -C proto gennow runsbuf breakingagainstmainbeforebuf generate, so wire-incompatible changes fail locally before regenerating..github/workflows/proto.ymlthat installs pinned buf + plugins, runs the same breaking check, regenerates, and fails if committed*.pb.godrift from generated output. Also runsgo build ./....proto/*.pb.gounder buf. Only functional diff is normalized internal symbol names ininternal.pb.go(e.g.file_proto_internal_proto_*->file_internal_proto_*); exported API and wire format are unchanged.Lint is intentionally left disabled — enabling
MINIMALwould require addingpackagedeclarations and renaming services/RPC types, both of which change gRPC wire FQNs.Test plan
buf breaking --against '../.git#subdir=proto,branch=main'passesbuf generateis idempotent against committed outputgo build ./...passesSummary by CodeRabbit
Release Notes