Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @alpe's task in 1m 29s —— View job Code Review
The change is small and focused:
|
| Severity | Issue |
|---|---|
| Minor bug | Error messages say > but condition is >= — misleading in logs and debugging |
| Test gap | No test for FSM initial state (Height=0, Timestamp=0 → first block) |
| Test gap | No test confirming height gaps are allowed (as stated in the comment) |
| Pre-existing | FSM.Apply returning error on invalid state may silently diverge followers |
The core logic change (> → >=) is correct and the test suite is a solid foundation. The error message fix and two additional test cases would make this production-ready.
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughModified validation boundary checks in RaftBlockState transitions from strict (>) to non-strict (>=) comparisons for height and timestamp fields. Added comprehensive test coverage for the updated assertValid function with scenarios covering normal progression, equal states, and boundary conditions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 #3167 +/- ##
==========================================
+ Coverage 60.20% 60.24% +0.03%
==========================================
Files 115 115
Lines 11902 11902
==========================================
+ Hits 7166 7170 +4
+ Misses 3925 3921 -4
Partials 811 811
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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)
pkg/raft/types_test.go (1)
9-44: Good test coverage; consider adding edge cases for initial state and height gaps.The table-driven tests cover the core validation scenarios well. Given the context from
node.gowhere FSM starts with an emptyRaftBlockState{}(Height=0, Timestamp=0), and the PR description noting that height gaps are allowed, consider adding these cases:Suggested additional test cases
"lower height": { s: &RaftBlockState{Height: 11, Timestamp: 100}, next: &RaftBlockState{Height: 10, Timestamp: 101}, expErr: true, }, + "from initial state": { + s: &RaftBlockState{Height: 0, Timestamp: 0}, + next: &RaftBlockState{Height: 1, Timestamp: 100}, + }, + "height gap allowed": { + s: &RaftBlockState{Height: 10, Timestamp: 100}, + next: &RaftBlockState{Height: 15, Timestamp: 200}, + }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/raft/types_test.go` around lines 9 - 44, Add two table-driven cases to TestAssertValid for the initial/zero-state and for allowed height gaps: include a case where s is &RaftBlockState{} (Height:0, Timestamp:0) and next is a valid non-zero state (e.g., Height>0, Timestamp>0) expecting no error, and a case where s has Height N and next has Height M where M > N+1 (a height gap) also expecting no error; update the specs map in TestAssertValid to reference RaftBlockState for these scenarios so the test covers FSM initial state and confirms height gaps are permitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/raft/types.go`:
- Around line 14-18: The error messages are misleading because the checks use >=
but the messages say '>' for s.Height/next.Height and
s.Timestamp/next.Timestamp; update the fmt.Errorf strings in the comparisons
that use s.Height >= next.Height and s.Timestamp >= next.Timestamp to reflect >=
(e.g. "invalid height: %d >= %d" and "invalid timestamp: %d >= %d") or use
wording like "not less than" so the message accurately matches the condition for
the symbols s.Height, next.Height, s.Timestamp and next.Timestamp.
---
Nitpick comments:
In `@pkg/raft/types_test.go`:
- Around line 9-44: Add two table-driven cases to TestAssertValid for the
initial/zero-state and for allowed height gaps: include a case where s is
&RaftBlockState{} (Height:0, Timestamp:0) and next is a valid non-zero state
(e.g., Height>0, Timestamp>0) expecting no error, and a case where s has Height
N and next has Height M where M > N+1 (a height gap) also expecting no error;
update the specs map in TestAssertValid to reference RaftBlockState for these
scenarios so the test covers FSM initial state and confirms height gaps are
permitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3087cb86-9ddc-4b55-baaf-0210abaccc0d
📒 Files selected for processing (2)
pkg/raft/types.gopkg/raft/types_test.go
| if s.Height >= next.Height { | ||
| return fmt.Errorf("invalid height: %d > %d", s.Height, next.Height) | ||
| } | ||
| if s.Timestamp > next.Timestamp { | ||
| if s.Timestamp >= next.Timestamp { | ||
| return fmt.Errorf("invalid timestamp: %d > %d", s.Timestamp, next.Timestamp) |
There was a problem hiding this comment.
Error messages are misleading for equality cases.
The conditions now use >= to reject equal values, but the error messages still show >. When s.Height == next.Height, the message "invalid height: 10 > 10" is confusing because 10 is not greater than 10.
Proposed fix to clarify error messages
func assertValid(s *RaftBlockState, next *RaftBlockState) error {
if s.Height >= next.Height {
- return fmt.Errorf("invalid height: %d > %d", s.Height, next.Height)
+ return fmt.Errorf("invalid height: current %d >= next %d", s.Height, next.Height)
}
if s.Timestamp >= next.Timestamp {
- return fmt.Errorf("invalid timestamp: %d > %d", s.Timestamp, next.Timestamp)
+ return fmt.Errorf("invalid timestamp: current %d >= next %d", s.Timestamp, next.Timestamp)
}
return nil
}📝 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.
| if s.Height >= next.Height { | |
| return fmt.Errorf("invalid height: %d > %d", s.Height, next.Height) | |
| } | |
| if s.Timestamp > next.Timestamp { | |
| if s.Timestamp >= next.Timestamp { | |
| return fmt.Errorf("invalid timestamp: %d > %d", s.Timestamp, next.Timestamp) | |
| if s.Height >= next.Height { | |
| return fmt.Errorf("invalid height: current %d >= next %d", s.Height, next.Height) | |
| } | |
| if s.Timestamp >= next.Timestamp { | |
| return fmt.Errorf("invalid timestamp: current %d >= next %d", s.Timestamp, next.Timestamp) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/raft/types.go` around lines 14 - 18, The error messages are misleading
because the checks use >= but the messages say '>' for s.Height/next.Height and
s.Timestamp/next.Timestamp; update the fmt.Errorf strings in the comparisons
that use s.Height >= next.Height and s.Timestamp >= next.Timestamp to reflect >=
(e.g. "invalid height: %d >= %d" and "invalid timestamp: %d >= %d") or use
wording like "not less than" so the message accurately matches the condition for
the symbols s.Height, next.Height, s.Timestamp and next.Timestamp.
2 participants
Overview
Ensure new raft state is increasing time and height. Not enforcing height + 1
Summary by CodeRabbit
Bug Fixes
Tests