Breaking Change Detection V1#95
Conversation
WalkthroughThis update introduces breaking change detection for Protocol Buffer schemas using Bufrnix. It adds configuration options, documentation, and an example Go project demonstrating the feature. The build logic is extended to run Buf's breaking change checks, and automated tests verify detection of both safe and breaking changes. Several package versions are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NixBuild
participant BufrnixShell
participant Buf
participant Git
User->>NixBuild: nix build/run on example
NixBuild->>BufrnixShell: Launches shell script
alt Breaking change detection enabled
BufrnixShell->>Git: Check if repo & base reference exist
Git-->>BufrnixShell: Repo and base status
BufrnixShell->>Buf: Run buf breaking --against baseRef [options]
Buf-->>BufrnixShell: Breaking check result
alt Breaking changes detected
BufrnixShell->>User: Error or warning (configurable)
else No breaking changes
BufrnixShell->>User: Continue to code generation
end
else Breaking change detection disabled
BufrnixShell->>User: Proceed to code generation
end
Suggested labels
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
doc/src/content/docs/reference/languages.mdx (1)
1266-1268: Prefer HTML entity to avoid a visible back-slashEscaping the
<with a back-slash renders the back-slash in HTML.
Use<100KB RAMor wrap the whole predicate in back-ticks instead.-- Memory is severely constrained (\<100KB RAM) +- Memory is severely constrained (<100KB RAM)doc/src/content/docs/reference/configuration.mdx (2)
74-80: Example omitsfailOnBreaking; readers may assume success on breakThe minimal snippet enables detection but does not show
failOnBreaking = true;which is critical for CI. Consider adding it so the quick-start is production-safe.
170-187: Clarify precedence betweenbaseReferenceandbasePathLater you document both keys, but only
baseReferenceis shown in the block.
Explicitly state thatbasePathoverridesbaseReference(or vice-versa) to avoid user confusion.examples/go-breaking-change/README.md (1)
30-50: Remove trailing colons in headings (markdown-lint MD026)-### Run both server and client together (default): +### Run both server and client together -### Run only the server: +### Run only the server -### Run only the client: +### Run only the client -### Customize the port: +### Customize the porttest-examples.sh (1)
237-240: Potential sed command portability issue.The sed command uses a complex multi-line replacement that might not work consistently across different platforms (macOS vs Linux).
Consider using a more portable approach for file modification:
- # Add optional field to proto - sed -i.bak 's/google.protobuf.Timestamp last_login = 6;/google.protobuf.Timestamp last_login = 6;\ - \ - \/\/ New optional field - backward compatible\ - string phone_number = 7;/' proto/user/v1/user.proto + # Add optional field to proto + cp proto/user/v1/user.proto proto/user/v1/user.proto.bak + cat >> proto/user/v1/user.proto << 'EOF' + + // New optional field - backward compatible + string phone_number = 7; +EOFdoc/src/content/docs/guides/breaking-changes.mdx (1)
367-367: Example reference uses incorrect path format.The link to the example uses a format that may not work correctly in the documentation site.
Use a more standard documentation link format:
-See the [go-breaking-change example](/examples/go-breaking-change/) for a complete demonstration +See the [go-breaking-change example](https://github.com/conneroisu/bufrnix/tree/main/examples/go-breaking-change) for a complete demonstrationdoc/bun.nix (1)
33-44: Review the nested package structure complexity.The addition of nested package entries under
@astrojs/mdxintroduces dependency complexity. The deeply nested path structure (@astrojs/mdx/node_modules/@astrojs/node_modules/markdown-remark/node_modules/@astrojs/node_modules/prism) may indicate version conflicts requiring specific resolution.Consider documenting why these nested dependencies are necessary and whether they represent version conflicts that could be resolved at the top level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
doc/bun.lockis excluded by!**/*.lockexamples/go-breaking-change/flake.lockis excluded by!**/*.lockexamples/go-breaking-change/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
doc/bun.nix(4 hunks)doc/package.json(1 hunks)doc/src/content/docs/guides/breaking-changes.mdx(1 hunks)doc/src/content/docs/reference/configuration.mdx(2 hunks)doc/src/content/docs/reference/languages.mdx(1 hunks)examples/go-breaking-change/README.md(1 hunks)examples/go-breaking-change/flake.nix(1 hunks)examples/go-breaking-change/go.mod(1 hunks)examples/go-breaking-change/main.go(1 hunks)examples/go-breaking-change/proto/user/v1/user.proto(1 hunks)src/lib/bufrnix-options.nix(1 hunks)src/lib/mkBufrnix.nix(2 hunks)test-examples.sh(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
examples/go-breaking-change/README.md
30-30: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
36-36: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
42-42: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
48-48: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Buf (1.54.0)
examples/go-breaking-change/proto/user/v1/user.proto
3-3: Files with package "user.v1" must be within a directory "user/v1" relative to root but were in directory "examples/go-breaking-change/proto/user/v1".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (20)
doc/package.json (1)
4-8: Confirm that the Astro minor bumps introduce no breaking CSS/MDX changesThe jump to
astro@5.9.xand@astrojs/starlight@0.34.4should be safe, but both packages had non-trivial internals changes in their latest minors (e.g. collections & image pipeline).
Please run the docs build once (pnpm run buildorbun run astro build) before merging.examples/go-breaking-change/go.mod (1)
3-4: Go 1.24.3 is not released yetIf you intend to pin to Go tip, use
go 1.22(latest LTS) and letnixprovide a newer compiler, or wait until 1.24 is officially tagged – otherwisego installon non-Nix systems will fail.examples/go-breaking-change/proto/user/v1/user.proto (1)
1-92: Well-designed protobuf schema for breaking change demonstration.The schema follows protobuf best practices with:
- Clear message structure with appropriate field types
- Comprehensive enum definitions with unspecified default
- Complete CRUD service interface
- Proper field numbering and comments
Note: The static analysis hint about package directory mismatch is a false positive - the file is correctly placed in the examples directory structure.
test-examples.sh (2)
149-303: Comprehensive breaking change detection test implementation.The test function covers all essential scenarios:
- Git repository setup and validation
- No-change baseline testing
- Safe change detection (adding optional fields)
- Breaking change detection (removing fields)
- Proper error handling and cleanup
403-414: Test integration follows established patterns.The breaking change example is properly integrated with the existing test framework, maintaining consistency with other example tests.
src/lib/mkBufrnix.nix (3)
115-120: Clean integration of breaking change package defaults.The buf package is properly added to the packageDefaults structure, maintaining consistency with other language configurations.
278-281: Proper conditional runtime inputs.The runtime inputs correctly include buf and git only when breaking change detection is enabled, avoiding unnecessary dependencies.
355-371: Robust error handling for breaking change detection.The timeout and exit code handling properly distinguishes between different failure scenarios and respects the
failOnBreakingconfiguration.doc/src/content/docs/guides/breaking-changes.mdx (2)
1-400: Comprehensive and well-structured documentation.The documentation excellently covers:
- Clear overview and benefits
- Detailed configuration options with examples
- Different compatibility modes with specific use cases
- Best practices and common pitfalls
- Troubleshooting section for common issues
- Integration examples for CI/CD workflows
The documentation aligns well with the implementation and provides clear guidance for users.
78-78:basePathis implemented in the code
I found thebasePathoption declared insrc/lib/bufrnix-options.nix(lines ~97–102), matching the documentation. No changes to the guides are needed.src/lib/bufrnix-options.nix (1)
77-147: Excellent implementation of breaking change detection configuration.The breaking change detection options are well-structured with proper types, sensible defaults, and comprehensive documentation. The configuration covers all essential aspects of schema compatibility checking and integrates cleanly with the existing Bufrnix options schema.
examples/go-breaking-change/main.go (4)
18-86: Well-implemented gRPC server with proper concurrency handling.The server implementation demonstrates excellent practices:
- Proper use of
sync.RWMutexfor thread-safe operations- Comprehensive input validation with meaningful error messages
- Consistent error handling using response fields rather than gRPC status codes
- Clean separation between CreateUser (write lock) and GetUser (read lock) operations
88-105: Standard and correct gRPC server setup.The server initialization follows gRPC best practices with proper error handling and includes reflection registration for debugging capabilities.
107-191: Comprehensive client implementation demonstrating best practices.The client code provides an excellent example of gRPC usage with:
- Proper connection management and cleanup
- Context with timeout for all operations
- Comprehensive demonstration of both success and error scenarios
- Detailed logging for educational purposes
193-232: Well-structured main function with flexible execution modes.The main function provides excellent flexibility with:
- Clean command-line flag configuration
- Useful "both" mode for demos and testing
- Proper retry logic with timeout for server readiness
- Comprehensive error handling for all execution paths
doc/bun.nix (5)
11-13: Version update appears correct but verify hash integrity.The
@astrojs/compilerversion bump from 2.12.0 to 2.12.2 follows semantic versioning correctly. However, ensure the SHA-512 hash corresponds to the actual package at the specified URL.
45-50: New package addition looks correct.The addition of
@astrojs/prism@3.3.0as a top-level package follows the established pattern correctly with proper URL and hash formatting.
59-61: Package version updates are structurally correct.Both
@astrojs/starlight(0.34.3 → 0.34.4) andastro(5.8.0 → 5.9.4) version updates follow semantic versioning and maintain consistent URL/hash formatting.Also applies to: 998-1000
1-3352: Inconsistency between PR objectives and file contents.The PR is titled "Breaking Change Detection V1" and mentions Protocol Buffer schema breaking change detection using Bufrnix, but this file only contains Astro package version updates. This suggests either:
- This file was included in the wrong PR, or
- The PR description is incorrect, or
- There are missing files that implement the breaking change detection
Please clarify why Astro package updates are included in a PR focused on Protocol Buffer breaking change detection.
Likely an incorrect or invalid review comment.
23-25: Nested markdown-remark version is intentional and matches @astrojs/mdx’s own dependencyThe top-level entry in doc/bun.nix pins markdown-remark@6.3.2, while the nested block under “@astrojs/mdx” (lines 33–38) remains at 6.3.1 because @astrojs/mdx@4.2.6 still depends on that version. This discrepancy is expected until mdx itself is updated to require 6.3.2. No change is needed here.
• doc/bun.nix lines 23–25: direct markdown-remark@6.3.2
• doc/bun.nix lines 33–38: nested markdown-remark@6.3.1 under @astrojs/mdx
| module github.com/conneroisu/bufrnix/examples/simple-flake | ||
|
|
There was a problem hiding this comment.
Module path looks incorrect
module github.com/conneroisu/bufrnix/examples/simple-flake does not match the directory examples/go-breaking-change.
This mismatch breaks go list ./... and any code-gen that relies on import paths.
-module github.com/conneroisu/bufrnix/examples/simple-flake
+module github.com/conneroisu/bufrnix/examples/go-breaking-change🤖 Prompt for AI Agents
In examples/go-breaking-change/go.mod at lines 1 to 2, the module path is set to
github.com/conneroisu/bufrnix/examples/simple-flake, which does not match the
directory path examples/go-breaking-change. Update the module path to
github.com/conneroisu/bufrnix/examples/go-breaking-change to align with the
directory structure and ensure go list and code generation tools work correctly.
| ```sh | ||
| nix run .\#packages.x86_64-linux.bufrnix generate | ||
| ``` |
There was a problem hiding this comment.
Windows path separator leaks into nix command
The back-slash before # will break on POSIX shells.
-nix run .\#packages.x86_64-linux.bufrnix generate
+nix run .#packages.x86_64-linux.bufrnix generate📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```sh | |
| nix run .\#packages.x86_64-linux.bufrnix generate | |
| ``` |
🤖 Prompt for AI Agents
In examples/go-breaking-change/README.md around lines 18 to 20, the nix command
uses a Windows path separator (backslash) before the '#' character, which will
break on POSIX shells. Replace the backslash with a POSIX-compatible separator
or remove it entirely so the command works correctly on all platforms.
| breaking = { | ||
| enable = false; # Disabled for testing | ||
| mode = "backward"; | ||
| baseReference = "HEAD~1"; | ||
| failOnBreaking = true; | ||
| outputFormat = "text"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent breaking change configuration in example.
This example is meant to demonstrate breaking change detection, but the feature is disabled (enable = false). The shell hook instructions mention running breaking change detection, but it won't work with this configuration.
Consider either:
- Enable breaking change detection by default to demonstrate the feature
- Add clear instructions in the shell hook about enabling it manually
- breaking = {
- enable = false; # Disabled for testing
+ breaking = {
+ enable = true; # Enable for demonstration
mode = "backward";
baseReference = "HEAD~1";
failOnBreaking = true;
outputFormat = "text";
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| breaking = { | |
| enable = false; # Disabled for testing | |
| mode = "backward"; | |
| baseReference = "HEAD~1"; | |
| failOnBreaking = true; | |
| outputFormat = "text"; | |
| }; | |
| breaking = { | |
| enable = true; # Enable for demonstration | |
| mode = "backward"; | |
| baseReference = "HEAD~1"; | |
| failOnBreaking = true; | |
| outputFormat = "text"; | |
| }; |
🤖 Prompt for AI Agents
In examples/go-breaking-change/flake.nix around lines 54 to 60, the breaking
change detection feature is disabled by default with 'enable = false', which
contradicts the example's purpose and shell hook instructions. To fix this, set
'enable = true' to activate breaking change detection by default, or
alternatively, update the shell hook instructions to clearly explain how to
enable breaking change detection manually before running it.
| case "${cfg.breaking.mode}" in | ||
| "backward") | ||
| buf_cmd="$buf_cmd --against-input-config" | ||
| ;; | ||
| "forward") | ||
| buf_cmd="$buf_cmd --against-input-config" | ||
| ;; | ||
| "full") | ||
| buf_cmd="$buf_cmd --against-input-config" | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Identical case statements in mode switch.
All three compatibility modes (backward, forward, full) use identical buf command flags. This suggests either the implementation is incomplete or the modes are handled differently by buf itself.
Verify that the buf CLI handles these modes differently with the same flags, or if different flags should be used for each mode:
🏁 Script executed:
#!/bin/bash
# Description: Check buf breaking command documentation for mode-specific flags
# Expected: Different flags or confirmation that modes are handled by buf internally
# Check buf breaking help for mode-specific options
buf breaking --help | grep -A 10 -B 10 -i "mode\|backward\|forward\|full"
# Check if buf has different subcommands for different modes
buf breaking --help | grep -E "(backward|forward|full)"Length of output: 133
Add explicit --mode flags to the buf breaking command
All three branches currently use only --against-input-config, so the selected mode has no effect. You should pass --mode <backward|forward|full> for each case.
• File: src/lib/mkBufrnix.nix
Lines: 314–324
Suggested diff:
case "${cfg.breaking.mode}" in
"backward")
- buf_cmd="$buf_cmd --against-input-config"
+ buf_cmd="$buf_cmd --against-input-config --mode backward"
;;
"forward")
- buf_cmd="$buf_cmd --against-input-config"
+ buf_cmd="$buf_cmd --against-input-config --mode forward"
;;
"full")
- buf_cmd="$buf_cmd --against-input-config"
+ buf_cmd="$buf_cmd --against-input-config --mode full"
;;
esac
Please verify against the buf CLI documentation that --mode is the correct flag name and adjust if needed.
🤖 Prompt for AI Agents
In src/lib/mkBufrnix.nix between lines 314 and 324, the buf breaking command
uses --against-input-config in all cases but does not specify the mode, so the
selected mode has no effect. Fix this by appending --mode backward, --mode
forward, or --mode full to the buf_cmd variable in each respective case branch.
Verify the buf CLI documentation to confirm that --mode is the correct flag name
before applying the change.
| esac | ||
|
|
||
| # Add base reference | ||
| buf_cmd="$buf_cmd '.git#branch=${cfg.breaking.baseReference}'" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Git reference formatting could be more robust.
The git reference is directly interpolated into the command string, which could be vulnerable to shell injection if the reference contains special characters.
Use proper shell escaping for the git reference:
- buf_cmd="$buf_cmd '.git#branch=${cfg.breaking.baseReference}'"
+ buf_cmd="$buf_cmd '.git#branch=$(printf %q "${cfg.breaking.baseReference}")'"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/mkBufrnix.nix at line 327, the git reference is directly interpolated
into the shell command string, which risks shell injection if it contains
special characters. To fix this, properly escape the git reference value before
including it in the command string, ensuring any special characters are safely
handled to prevent injection vulnerabilities.
| "sync" | ||
| "time" | ||
|
|
||
| pb "github.com/conneroisu/bufrnix/examples/simple-flake/proto/gen/go/simple/v1" |
There was a problem hiding this comment.
Fix the incorrect import path.
The import path references examples/simple-flake/ but this file is in examples/go-breaking-change/. This appears to be a copy-paste error that will prevent the code from compiling correctly.
Apply this diff to fix the import path:
- pb "github.com/conneroisu/bufrnix/examples/simple-flake/proto/gen/go/simple/v1"
+ pb "github.com/conneroisu/bufrnix/examples/go-breaking-change/proto/gen/go/user/v1"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pb "github.com/conneroisu/bufrnix/examples/simple-flake/proto/gen/go/simple/v1" | |
| - pb "github.com/conneroisu/bufrnix/examples/simple-flake/proto/gen/go/simple/v1" | |
| + pb "github.com/conneroisu/bufrnix/examples/go-breaking-change/proto/gen/go/user/v1" |
🤖 Prompt for AI Agents
In examples/go-breaking-change/main.go at line 12, the import path incorrectly
points to examples/simple-flake/proto/gen/go/simple/v1, which is wrong for this
file's location. Update the import path to correctly reference the proto package
relative to examples/go-breaking-change, ensuring it matches the actual
directory structure to fix the compilation error.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests