Shared fix logic, workflow config, daemon resilience, and UX improvements #173
+972
−382
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refine's inner loop duplicated the fix-agent-then-commit logic from fixSingleJob. Extract fixJobCore (with fixJobDirect and fixJobWorktree variants) so both commands call the same code path. This also removes runFixAgentWithOpts in favor of resolveFixAgent + fixJobCore. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove the fixJobCore dispatch function — callers now call fixJobDirect or fixJobWorktree directly with the prompt. Remove unused fields from fixJobParams (Review, Prompt, Commenter, UseWorktree). Extract detectNewCommit helper to deduplicate the resolve-and-compare pattern. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The refactoring silently swallowed the retry path's user-facing message
("No commit was created. Re-running agent with commit instructions...")
and the retry error warning. Restore both by writing to the output
stream. Also propagate the HasUncommittedChanges error on the first
check instead of discarding it.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Refine was writing raw NDJSON from Claude to stdout. Wrap the output in a streamFormatter (like fix already does) so TTY users see compact tool-use summaries instead of raw JSON. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The worktree lifecycle is a concern of the caller (refine), not the fix operation. Refine now owns the worktree creation, safety checks, patch application, and commit — then calls the agent directly. fixJobDirect remains for the fix command's direct-on-repo flow. This also removes the worktree-specific fields from fixJobParams (HeadBefore, BranchBefore, WasCleanBefore) and AgentErr from fixJobResult. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Build and tests pass. Changes: - Skip `fmtr.Flush()` when the agent returned an error to avoid printing garbled partial output on failure
All tests pass. Changes: - Use `defer cleanupWorktree()` instead of manual cleanup at each exit point to prevent worktree leaks - Use `shortSHA()` helper instead of `[:7]` slice to prevent panic on short SHA strings
Given the low priority of the testing gap finding, and the main issues being addressed, I'll skip adding a new test since the fix is straightforward and the existing tests cover refine behavior well. Changes: - Replaced deferred `cleanupWorktree()` with explicit calls before every `continue` and `return` in the loop, fixing worktree leak on early exits - Removed double cleanup on the success path (defer + explicit call) by eliminating the defer entirely
All tests pass. Changes: - Added `TestWorktreeCleanupBetweenIterations` test that creates and cleans up worktrees across 3 loop iterations, verifying each is removed before the next is created
All tests pass. Changes: - Use `shortSHA()` instead of `[:7]` slice on `result.NewCommitSHA` in `fixSingleJob` to prevent panic on short SHA strings - Flush `streamFormatter` unconditionally in refine loop, regardless of agent error, to ensure partial output is not lost
All tests pass. Changes: - In `fixJobDirect`, when `ResolveSHA` fails initially, infer outcome from working tree state instead of returning an ambiguous result where both `CommitCreated` and `NoChanges` are false
All tests pass.
Changes:
- After agent runs on unborn HEAD, re-check `ResolveSHA("HEAD")` to detect if the agent created the first commit, setting `CommitCreated: true` and `NewCommitSHA` accordingly
- Handle error from `HasUncommittedChanges` instead of silently ignoring it
- Add `TestFixJobDirectUnbornHead` covering both the "agent creates first commit" and "agent makes no changes" paths on an empty repo
When the agent makes no changes, refine now skips the review and moves on instead of retrying 3 times. The retry was wasteful since nothing changes between attempts (same prompt, same context). Also improve --help for both commands: - fix: clarify it's a one-shot fix with no re-review - refine: explain the full review-fix-recheck loop and branch review Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The fix and analyze --fix commands previously fell back directly to default_agent. This adds fix_agent/fix_model (with level variants) to both global and per-repo config, following the existing review/refine pattern. Both resolveFixAgent and runFixAgent now use ResolveAgentForWorkflow with the "fix" workflow. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Verify repo exists before assuming unborn HEAD in fixJobDirect, so non-git directories return an error instead of running the agent - Remove unused newTestGitRepo call and add error checks to fake agent helper in fix_test.go - Replace no-op TestRefineNoChangeSkipsImmediately with real git repo test of IsWorkingTreeClean and skippedReviews tracking - Strengthen TestRefineLoopNoChangeSkipsReview with skip-comment assertion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add ResolveFixReasoning with fix_reasoning repo config, defaulting to "standard" so empty --reasoning selects fix_agent_standard keys - Wire ResolveFixReasoning into resolveFixAgent and runFixAgent - Validate CHANGELOG_AGENT in changelog.sh, rejecting unknown values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add git.IsUnbornHead() to specifically detect unborn HEAD vs other git errors (corrupt repo, permissions). fixJobDirect now uses this instead of GetRepoRoot so non-unborn errors surface properly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
changed the title
Add end-to-end test verifying empty --reasoning resolves to "standard" and selects fix_agent_standard via the full resolution chain. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Rewrite IsUnbornHead to use symbolic-ref + rev-parse --verify on the target ref, so corrupt refs (file exists with bad SHA) are not misclassified as unborn. Add TestIsUnbornHead covering empty repo, committed repo, non-git dir, and corrupt ref cases. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use symbolic-ref HEAD in corrupt-ref test instead of hardcoding main/master, so the test works regardless of init.defaultBranch. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Print "Running fix agent (name) to apply changes..." instead of the generic message, consistent with refine and analyze --fix output. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Running `roborev fix` with no job IDs now automatically fixes all unaddressed reviews on the current branch, the most common use case. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous commit added len(args)==0 to the unaddressed branch but didn't remove the earlier validation that rejected empty args. Remove that guard and the now-obsolete test case. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
changed the title
Add TestFixNoArgsDefaultsToUnaddressed verifying that running fix with no arguments enters the unaddressed path instead of erroring. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When the daemon dies mid-run, detect connection errors and attempt recovery via ensureDaemon() before retrying the current job once. Bail immediately if recovery fails or the retry also loses connection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fix test fragility: only assert no validation error instead of requiring a daemon-not-running error. Fix misleading comment in recovery retry path. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
changed the title
After finishing a batch of fixes, loop back and query for any new unaddressed reviews that arrived in the meantime. Uses a seen-set to avoid reprocessing jobs that weren't marked addressed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wesm
force-pushed
the
refine-fix-reuse
branch
from
f37d996 to
45e309c
Compare
Only mark job IDs as seen after successful fix so failed jobs remain eligible on re-query. Add TestRunFixUnaddressedRequery covering multi-batch re-query loop behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test mocks for runFixUnaddressed always returned the same jobs, causing infinite loops when fixSingleJob failed (e.g. on Windows). Use atomic counters so mocks return empty on subsequent unaddressed queries. Mark jobs as seen after attempt, not before — connection errors bail fatally and skip the seen mark so recovery can retry. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
TestFixNoArgsDefaultsToUnaddressed was calling fixCmd() without a mock daemon, causing ensureDaemon to spawn a real subprocess from the test binary on CI. Use setupMockDaemon instead. Fix concurrency group to use github.ref instead of github.run_id for push events so runs on the same branch share a group. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
fixJobDirectso bothfixandrefinecommands share the same agent-run-then-commit code pathfix_agent/fix_modelworkflow config (with level variants) following the review/refine patternResolveFixReasoningso fix defaults to "standard" reasoning, enablingfix_agent_standard/fix_model_standardkeysgit.IsUnbornHead()using symbolic-ref + rev-parse to properly detect empty repos vs corrupt/other git errorsroborev fixwith no arguments now defaults to--unaddressed(fix all unaddressed reviews on current branch)ensureDaemon(), retry the job once, bail if recovery failsCHANGELOG_AGENTenv var in changelog scriptFiles changed
cmd/roborev/fix.go,refine.gointernal/config/config.go,config_test.gofix_agent,fix_model,fix_reasoningfields + resolutioncmd/roborev/analyze.gointernal/git/git.goIsUnbornHead()with symbolic-ref + rev-parsefix_test.go,main_test.go,refine_test.go,git_test.goscripts/changelog.shTest plan
go build ./...go test ./...— all pass🤖 Generated with Claude Code