git: add bounded rewrite scenario orchestration#5
Conversation
Introduce detect/intervene/verify rerun orchestration with max-attempt guards so rewrite pipelines can fail predictably and report pass-by-pass state. Made-with: Cursor
📝 WalkthroughWalkthroughThis PR introduces a rewrite orchestration utility in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
bugbot-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
🧹 Nitpick comments (1)
git/rewrite_scenario_test.go (1)
250-279: Add explicit tests for cancellation and nil-context fallback.Given cancellation is part of runtime control flow, add targeted tests for a canceled context and for
ctx == nilto lock down these contracts.🧪 Suggested test additions
+func TestRunRewriteScenario_CancelledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := RunRewriteScenario(ctx, RewriteScenarioOptions{ + MaxAttempts: 2, + Detect: func(_ context.Context, _ int) (bool, error) { return true, nil }, + Intervene: func(_ context.Context, _ int) error { return nil }, + Verify: func(_ context.Context, _ int) (bool, error) { return false, nil }, + }) + if err == nil { + t.Fatal("RunRewriteScenario() error = nil, want cancellation error") + } + if result.Attempts != 0 { + t.Fatalf("result.Attempts = %d, want 0", result.Attempts) + } +} + +func TestRunRewriteScenario_NilContextFallback(t *testing.T) { + result, err := RunRewriteScenario(nil, RewriteScenarioOptions{ + MaxAttempts: 1, + Detect: func(_ context.Context, _ int) (bool, error) { return false, nil }, + Intervene: func(_ context.Context, _ int) error { return nil }, + Verify: func(_ context.Context, _ int) (bool, error) { return true, nil }, + }) + if err != nil { + t.Fatalf("RunRewriteScenario() error = %v", err) + } + if !result.Clean { + t.Fatalf("result.Clean = false, want true") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@git/rewrite_scenario_test.go` around lines 250 - 279, Add two explicit unit tests to TestRunRewriteScenario (or new tests) to cover cancellation and nil-context fallback: call RunRewriteScenario with a context canceled before invocation (use context.WithCancel and cancel()) and assert it returns a context/cancellation-related error; also call RunRewriteScenario passing nil as the ctx and assert it behaves the same as context.Background() (i.e., does not panic and honors callbacks), using the same RewriteScenarioOptions callbacks (Detect, Intervene, Verify) as in existing tests to verify expected handling. Ensure you reference RunRewriteScenario and RewriteScenarioOptions and keep assertions specific to cancellation and nil-context behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@git/rewrite_scenario.go`:
- Around line 72-107: The loop currently only checks ctx.Err() at the start of
each attempt; add cancellation checks immediately after each callback phase
(after calling opts.Detect and after opts.Intervene, and optionally after
opts.Verify if you want mid-verify cancellation) so we don't proceed to
subsequent callbacks when ctx is cancelled. On detecting cancellation populate
result.Attempts and result.Passes consistently (e.g., set result.Attempts =
attempt, append the current RewriteAttempt pass), and return a wrapped error
like fmt.Errorf("rewrite scenario cancelled before <phase> on attempt %d: %w",
attempt, ctx.Err()) to preserve the original context error; update the logic
around RewriteAttempt, opts.Detect, opts.Intervene, and opts.Verify to perform
these checks and early returns.
---
Nitpick comments:
In `@git/rewrite_scenario_test.go`:
- Around line 250-279: Add two explicit unit tests to TestRunRewriteScenario (or
new tests) to cover cancellation and nil-context fallback: call
RunRewriteScenario with a context canceled before invocation (use
context.WithCancel and cancel()) and assert it returns a
context/cancellation-related error; also call RunRewriteScenario passing nil as
the ctx and assert it behaves the same as context.Background() (i.e., does not
panic and honors callbacks), using the same RewriteScenarioOptions callbacks
(Detect, Intervene, Verify) as in existing tests to verify expected handling.
Ensure you reference RunRewriteScenario and RewriteScenarioOptions and keep
assertions specific to cancellation and nil-context behavior.
🪄 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: adf60cd2-c371-4eca-9352-8e4b84787b83
📒 Files selected for processing (3)
README.mdgit/rewrite_scenario.gogit/rewrite_scenario_test.go
|
@cursoragent address feedback then kick off a bugbot review after |
|
Here is what was done. Code changes
Tests
Git / PR
Bugbot review
Tests run:   |
Check ctx.Err after detect (when intervention needed) and after intervene so later callbacks are skipped when cancelled. Add tests for pre-cancelled context, nil context fallback, and mid-attempt cancellation before intervene and verify. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
|
@cursoragent review |
Review outcomeCodeRabbit’s “cancellation between phases” point is already satisfied on One gap I fixed: on the last attempt, if Verify returns Change (now on the branch): after a non-clean verify, check Pushed as Optional follow-ups (not changed): CodeRabbit’s docstring coverage warning is tooling noise unless you want docstrings on every exported symbol; Bugbot already ran on |
When verify reports not clean on the last attempt and the context is cancelled during verify, return a cancellation error instead of ErrRewriteAttemptsExceeded. Add a regression test. Co-authored-by: Ben Schellenberger <bschellenberger2600@users.noreply.github.com>
2 participants
Summary
RunRewriteScenarioto orchestrate bounded detect/intervene/verify rewrite passesTest plan
go test -count=1 ./git -run RunRewriteScenariogo test -count=1 ./...Made with Cursor
Note
Low Risk
Additive, isolated orchestration helper with extensive unit coverage and no changes to existing behavior.
Overview
Adds a new
git.RunRewriteScenarioAPI that runs a bounded detect → intervene → verify loop, returns pass-by-pass telemetry (RewriteScenarioResult.Passes), and fails withErrRewriteAttemptsExceededwhen the repo never becomes clean withinMaxAttempts.Includes a comprehensive test suite covering clean short-circuiting, multi-pass behavior, callback failures, context cancellation at multiple phases, and option validation, plus README documentation and local test commands for validating the orchestration flow.
Reviewed by Cursor Bugbot for commit 4e4e936. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation