build(proto): switch to buf and gate gen on breaking check#586
build(proto): switch to buf and gate gen on breaking check#586
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (10)
📒 Files selected for processing (4)
✨ 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 👍 / 👎.
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 ./...passes