Skip to content

Fix review screen showing worktree branch instead of stored branch#492

Merged
wesm merged 5 commits intomainfrom
fix/worktree-misattrib
Mar 10, 2026
Merged

Fix review screen showing worktree branch instead of stored branch#492
wesm merged 5 commits intomainfrom
fix/worktree-misattrib

Conversation

@wesm
Copy link
Copy Markdown
Collaborator

@wesm wesm commented Mar 10, 2026

Summary

  • The TUI review screen resolved the branch name dynamically via git name-rev, which can return a worktree branch when the same SHA is reachable from multiple refs. The queue view already handled this correctly by preferring the stored job.Branch.
  • fetchReview() now checks review.Job.Branch first and only falls back to git.GetBranchName() when the stored value is empty (for older jobs that predate the Branch column).
  • Added a regression test for the stored-branch priority.

🤖 Generated with Claude Code

The review screen's fetchReview() always resolved the branch via
git name-rev, which can return a worktree branch when the same SHA
is reachable from multiple refs. The queue view already handled this
correctly by preferring the stored job.Branch. Apply the same
priority in fetchReview(): use job.Branch first, fall back to the
git lookup only when the stored value is empty.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 10, 2026

roborev: Combined Review (39144a2)

Verdict: The PR successfully
updates review loading to prefer stored job branches over dynamic git resolution, but introduces a potential display regression for range reviews and lacks proper test coverage for the modified logic.

Medium

Potential behavior regression for range reviews
File: cmd/roborev/tui/fetch.go:508- 511
branchName = review.Job.Branch is now applied unconditionally before the range check. This means start..end range reviews (which previously stayed branchless in this code path) can now inappropriately display a branch if job.Branch is populated.
Suggested
fix:
Gate the stored-branch usage with the same single-commit condition to preserve existing behavior for range reviews (and optionally exclude "dirty" if that should remain branchless too).

Ineffective regression test
File: cmd/roborev/tui/review_branch_test.go: 33-53 (TestTUIReviewMsgUsesStoredBranch)
The new regression test hardcodes reviewMsg.branchName = "main" instead of invoking the modified fetchReview() logic. It only verifies the Bubbletea model's message handling, completely bypassing and failing to validate the
new branch resolution logic.
Suggested fix: Update the test to call fetchReview() (or the relevant command generator), execute the returned tea.Cmd, and assert the computed branchName on the resulting reviewMsg prefers review.Job.Branch.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

Address CI review feedback:
- Extract branch resolution from fetchReview into a standalone
  reviewBranchName function so the logic is directly testable.
- Replace the ineffective regression test with table-driven tests
  that exercise reviewBranchName for nil job, stored branch,
  range reviews, and missing repo path cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 10, 2026

roborev: Combined Review (f46a69d)

Verdict: The PR successfully refactors branch name resolution to prioritize stored job branches, but requires a
fix to prevent the "(none)" sentinel from rendering in the UI.

Medium

  • cmd/roborev/tui/fetch.go:515-520, cmd/roborev/tui/render_review.go:52-53

    reviewBranchName now returns any non-empty stored job.Branch verbatim. In this codebase, "(none)" is already a client-side sentinel for “no branch” (cmd/roborev/tui/helpers.go:24-25), and
    the same commit already backfills that sentinel into jobs for dirty/task/remote/detached cases (cmd/roborev/tui/fetch.go:395-413). With this change, those reviews will render headers like ... on (none), which is a user-visible regression
    . Suggested fix: treat branchNone the same as empty in reviewBranchName and only display real branch names.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

GetReviewByJobID and GetReviewByCommitSHA omitted j.branch from
their SELECT/Scan, so review.Job.Branch was always empty in the
fetchReview path. The TUI helper reviewBranchName prefers the stored
branch, but it was never populated, leaving the git name-rev
fallback in effect.

Add j.branch to both queries and scan it into job.Branch. Add
storage-level tests verifying the branch round-trips through both
GetReviewByJobID and GetReviewByCommitSHA.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 10, 2026

roborev: Combined Review (ab31e87)

Verdict: The changes successfully update the review fetch logic to prefer the stored branch name, and no medium or higher severity issues were found.

All agents reviewed the commits and agree
the code is clean. No critical, high, or medium severity issues were identified.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

The TUI backfill path persists "(none)" as the branch for jobs
where branch can't be resolved (task, dirty, remote, detached HEAD).
reviewBranchName returned this sentinel verbatim, which would render
as "on (none)" in the review header. Treat it as empty so the branch
is omitted instead.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 10, 2026

roborev: Combined Review (24879f5)

Verdict: The changes successfully wire the stored branch through the review-loading path to improve TUI display accuracy, but a medium-severity issue exists with the fallback behavior for the branch sentinel.

Medium

  • cmd/roborev/tui/fetch. go:519-523 (Commit 24879f5)
    reviewBranchName() treats branchNone as "don't return the stored branch", but it still falls through to git.GetBranchName() for single-commit reviews with a RepoPath . That defeats the commit's stated behavior of omitting the branch for sentinel-backed jobs, and can reintroduce the original wrong-branch display if the SHA is reachable from another local branch/worktree ref.
    Suggested fix: Return "" immediately when job.Branch == branchNone, or
    only run the git fallback when job.Branch == "" (not when it is the sentinel). Add a test covering Branch: branchNone with a non-empty RepoPath.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

When job.Branch is "(none)", the function fell through to the git
name-rev lookup instead of returning empty, reintroducing the
original wrong-branch display for sentinel-backed jobs with a local
repo path. Check for the sentinel first and return "" immediately.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Mar 10, 2026

roborev: Combined Review (76cbabb)

Verdict: All review agents agree the
code is clean and found no issues.


Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm merged commit 06ea80a into main Mar 10, 2026
14 of 15 checks passed
@wesm wesm deleted the fix/worktree-misattrib branch March 10, 2026 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant