Fix Edge Case Detector Max File Size bounds#531
Conversation
Co-authored-by: theRebelliousNerd <187437903+theRebelliousNerd@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR introduces defensive bounds handling to prevent integer overflow in the edge case detector. It clamps ChangesDefensive bounds in edge case detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/campaign/edge_case_detector_test.go`:
- Around line 347-350: Replace the weak upper-bound assertion after calling
detector.queryComplexity(&decision, "huge_file.go") with a deterministic
equality check against the expected capped complexity for math.MaxInt32: compute
expected := float64(math.MaxInt32) / 50.0 and assert decision.Complexity ==
expected (or use a strict comparison with t.Fatalf/t.Errorf if not equal),
referencing detector.queryComplexity and decision.Complexity so the test fails
if the value is 0 or any other incorrect value.
In `@internal/campaign/edge_case_detector.go`:
- Around line 368-379: The fallback branch that computes Complexity when
d.kernel == nil is unreachable because gatherMetrics only calls queryComplexity
when d.kernel is non-nil; update gatherMetrics (or the call site in
AnalyzeFiles) to invoke queryComplexity regardless of d.kernel so the nil-kernel
fallback inside queryComplexity runs, or alternatively move the nil check from
queryComplexity into gatherMetrics/AnalyzeFiles and compute the same
safeLineCount fallback there; ensure you reference and adjust the
queryComplexity method and the gatherMetrics call path so the d.kernel == nil
branch can execute.
🪄 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: 8e319971-1a10-45b4-bafb-078cdceed717
📒 Files selected for processing (2)
internal/campaign/edge_case_detector.gointernal/campaign/edge_case_detector_test.go
| detector.queryComplexity(&decision, "huge_file.go") | ||
| if decision.Complexity > float64(10000000)/50.0 { | ||
| t.Errorf("Complexity was not properly bounded, got %v", decision.Complexity) | ||
| } |
There was a problem hiding this comment.
Complexity bound assertion is too weak for this edge-case test.
The current check only enforces an upper bound, so a regression that leaves complexity at 0 would still pass. Assert the deterministic capped value for math.MaxInt32.
Proposed fix
detector.queryComplexity(&decision, "huge_file.go")
- if decision.Complexity > float64(10000000)/50.0 {
- t.Errorf("Complexity was not properly bounded, got %v", decision.Complexity)
- }
+ expected := float64(10000000) / 50.0
+ if decision.Complexity != expected {
+ t.Errorf("Expected bounded complexity %v, got %v", expected, decision.Complexity)
+ }
}📝 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.
| detector.queryComplexity(&decision, "huge_file.go") | |
| if decision.Complexity > float64(10000000)/50.0 { | |
| t.Errorf("Complexity was not properly bounded, got %v", decision.Complexity) | |
| } | |
| detector.queryComplexity(&decision, "huge_file.go") | |
| expected := float64(10000000) / 50.0 | |
| if decision.Complexity != expected { | |
| t.Errorf("Expected bounded complexity %v, got %v", expected, decision.Complexity) | |
| } |
🤖 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 `@internal/campaign/edge_case_detector_test.go` around lines 347 - 350, Replace
the weak upper-bound assertion after calling detector.queryComplexity(&decision,
"huge_file.go") with a deterministic equality check against the expected capped
complexity for math.MaxInt32: compute expected := float64(math.MaxInt32) / 50.0
and assert decision.Complexity == expected (or use a strict comparison with
t.Fatalf/t.Errorf if not equal), referencing detector.queryComplexity and
decision.Complexity so the test fails if the value is 0 or any other incorrect
value.
| if d.kernel == nil { | ||
| // Just estimate from line count if kernel is not available | ||
| decision.Complexity = 0 | ||
| if decision.LineCount > 0 { | ||
| safeLineCount := decision.LineCount | ||
| if safeLineCount > 10000000 { | ||
| safeLineCount = 10000000 | ||
| } | ||
| decision.Complexity = float64(safeLineCount) / 50.0 | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Nil-kernel complexity fallback is unreachable from the main analysis flow.
queryComplexity now handles d.kernel == nil, but gatherMetrics only invokes it when the kernel is non-nil (Line 334), so this branch never runs during normal AnalyzeFiles execution.
Proposed fix
func (d *EdgeCaseDetector) gatherMetrics(decision *FileDecision, path string, intel *IntelligenceReport) {
@@
- // Query kernel for dependencies
- if d.kernel != nil {
- d.queryDependencies(decision, path)
- d.queryComplexity(decision, path)
- }
+ // Query kernel for dependencies when available
+ if d.kernel != nil {
+ d.queryDependencies(decision, path)
+ }
+ // Always compute complexity (queryComplexity handles nil kernel fallback)
+ d.queryComplexity(decision, path)
}🤖 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 `@internal/campaign/edge_case_detector.go` around lines 368 - 379, The fallback
branch that computes Complexity when d.kernel == nil is unreachable because
gatherMetrics only calls queryComplexity when d.kernel is non-nil; update
gatherMetrics (or the call site in AnalyzeFiles) to invoke queryComplexity
regardless of d.kernel so the nil-kernel fallback inside queryComplexity runs,
or alternatively move the nil check from queryComplexity into
gatherMetrics/AnalyzeFiles and compute the same safeLineCount fallback there;
ensure you reference and adjust the queryComplexity method and the gatherMetrics
call path so the d.kernel == nil branch can execute.
1 participant
Implemented bounds checking for
LineCountininternal/campaign/edge_case_detector.goto preventfloat64precision loss and potential integer overflow issues in complexity heuristics.Specifically, bounds checking was added in
gatherMetricsprior to rough line count estimations viasymbolCount * 25, and withinqueryComplexityprior to calculation using a 10_000_000 line soft limit cap. Additionally, tests were added inedge_case_detector_test.goto verify behavior handlesmath.MaxInt32and safely caps output complexity, directly resolving the missing edge case TODO.PR created automatically by Jules for task 10505337179896057081 started by @theRebelliousNerd
Summary by CodeRabbit
Bug Fixes
Tests