feat(cli): --rain TUI, default mainline fetch, --sync full hydrate#2
Merged
bschellenberger2600 merged 4 commits intomainfrom Apr 16, 2026
Merged
Conversation
Replace the interactive --select flow with --rain (git-fire parity). Default invocation scans repos then runs targeted mainline git fetch per remote; add --sync for full RainRepository hydration. Remove --select entirely. Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 39 minutes and 9 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change splits git-rain into a default mainline remote-tracking fetch path and an explicit Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant TUI
participant Controller
participant GitLayer
participant Remote
CLI->>Controller: start (flags: --rain?, --sync?, --risky?)
alt interactive (--rain)
Controller->>TUI: open picker
TUI-->>Controller: selected repos
end
Controller->>Controller: computeFullSync(flags, cfg)
loop per repo
Controller->>GitLayer: if fullSync -> RainRepository(repo)
alt not fullSync
Controller->>GitLayer: MainlineFetchRemotes(repo, opts)
GitLayer->>Remote: git fetch <remote> --prune <branches...>
Remote-->>GitLayer: fetch result
end
GitLayer-->>Controller: per-branch outcomes
end
Controller-->>CLI: aggregated report (fetched/synced/failed/etc)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
Config mainline_patterns apply to default mainline fetch (fetched) and to full hydrate (synced). The scenario asserts a fast-forward, so run with --sync to match the intended integration check. Made-with: Cursor
![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
🧹 Nitpick comments (2)
cmd/root_test.go (1)
67-91: Cover explicit--branch-mode mainline.The subtle edge here is
branchCfg="mainline"staying on the default fetch path while an explicitbranchFlag="mainline"should force full sync. The table only exercises a non-mainline explicit flag today, so this contract can regress unnoticed.As per coding guidelines "cmd/root.go: When --sync flag is used, branch_mode is set, or risky mode is active, run full local branch hydration via RainRepository".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root_test.go` around lines 67 - 91, TestComputeFullSync is missing a case for an explicit branch flag set to "mainline": computeFullSync should treat an explicit branchFlag="mainline" as forcing a full sync. Add a table entry in TestComputeFullSync (the tests slice in cmd/root_test.go) that sets branchFlag to "mainline" (branchCfg can be ""), expects want true, and ensure the test calls computeFullSync(...) to assert this behavior so the contract in computeFullSync/branch handling is preserved.internal/git/fetch_mainline_test.go (1)
23-69: Add a renamed-local-branch regression case.This only locks down
main -> origin/main. It won't catch the case where a differently named local branch tracks a mainline upstream (for examplework/main -> origin/main) and the default fetch path skips it. A second integration case here would cover the new behavior much better.Based on learnings "Applies to internal/git/*_test.go : Prefer integration-style tests that shell out to
gitfor internal/git package functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/git/fetch_mainline_test.go` around lines 23 - 69, Add a second integration test to cover the renamed-local-branch regression: create a new test (e.g., TestMainlineFetchRemotes_RenamedLocalBranchTracksOrigin) that follows the same setup as TestMainlineFetchRemotes_UpdatesRemoteTrackingRefNotLocalBranch but creates a local branch with a different name (e.g., "work/main") that is set to track "origin/<defaultBranch>" (use git branch --track or git branch --set-upstream-to) before pushing a change from a separate clone to advance origin; then call MainlineFetchRemotes and assert that result.Updated > 0, that revParse(repo.Path(), "origin/<defaultBranch>") has advanced, and that the local branch SHA for "work/main" did not change (use testutil.GetCurrentSHA with the branch checked out or rev-parse on refs/heads/work/main) to ensure fetch updates the remote-tracking ref but does not move the differently named local branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/git/fetch_mainline.go`:
- Around line 63-73: The code treats any error from inferUpstreamRef(repoPath,
b.Branch) as a hard failure; change the handler so that if the error indicates
multiple candidate upstreams (the "ambiguous upstream" condition returned by
inferUpstreamRef) you set the RainBranchResult.Outcome to
RainOutcomeSkippedAmbiguousUpstream (leave Upstream empty and preserve
Branch/Current/Message) and do NOT increment result.Failed, while all other
errors should keep the existing RainOutcomeFailed behavior and increment
result.Failed; detect the ambiguous case by matching the specific error
value/type returned by inferUpstreamRef (or an exported sentinel like
ErrAmbiguousUpstream) rather than relying on string matching.
- Around line 35-46: The code currently filters local branches using b.Branch
before resolving upstream, so upstream mainline refs (e.g., work/main ->
origin/main) are skipped; change the logic in fetch_mainline.go to first call
splitUpstreamRemoteBranch (or otherwise resolve the upstream into rb) for each b
returned by listLocalBranchesWithUpstream(repoPath) and then apply
isMainlineBranch(rb) or matchesPatterns(rb, opts.MainlinePatterns) to decide
inclusion, updating the variable used for fetching to the resolved upstream
(rb); apply the same change to the similar block around the other occurrence
(lines 89-107) so MainlineFetchRemotes refreshes remote-tracking refs rather
than local branch names.
---
Nitpick comments:
In `@cmd/root_test.go`:
- Around line 67-91: TestComputeFullSync is missing a case for an explicit
branch flag set to "mainline": computeFullSync should treat an explicit
branchFlag="mainline" as forcing a full sync. Add a table entry in
TestComputeFullSync (the tests slice in cmd/root_test.go) that sets branchFlag
to "mainline" (branchCfg can be ""), expects want true, and ensure the test
calls computeFullSync(...) to assert this behavior so the contract in
computeFullSync/branch handling is preserved.
In `@internal/git/fetch_mainline_test.go`:
- Around line 23-69: Add a second integration test to cover the
renamed-local-branch regression: create a new test (e.g.,
TestMainlineFetchRemotes_RenamedLocalBranchTracksOrigin) that follows the same
setup as TestMainlineFetchRemotes_UpdatesRemoteTrackingRefNotLocalBranch but
creates a local branch with a different name (e.g., "work/main") that is set to
track "origin/<defaultBranch>" (use git branch --track or git branch
--set-upstream-to) before pushing a change from a separate clone to advance
origin; then call MainlineFetchRemotes and assert that result.Updated > 0, that
revParse(repo.Path(), "origin/<defaultBranch>") has advanced, and that the local
branch SHA for "work/main" did not change (use testutil.GetCurrentSHA with the
branch checked out or rev-parse on refs/heads/work/main) to ensure fetch updates
the remote-tracking ref but does not move the differently named local branch.
🪄 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: 34d68b45-8fda-46e0-9c17-f5fb4a133fa4
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdcmd/root.gocmd/root_test.gointernal/git/fetch_mainline.gointernal/git/fetch_mainline_test.gointernal/git/rain.goscripts/uat.sh
…nfer is skip - MainlineFetchRemotes: classify mainline using remote branch (rb) after splitUpstreamRemoteBranch, so locals like integrate/x tracking origin/main still fetch main. - Export ErrAmbiguousUpstream from inferUpstreamRef; treat as RainOutcomeSkippedAmbiguousUpstream (no CLI failure) instead of Failed. - Add regression test TestMainlineFetchRemotes_NonMainlineLocalNameTracksMainlineUpstream. Addresses CodeRabbit review on PR #2. Made-with: Cursor
Member
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/git/fetch_mainline.go (1)
146-149: Unnecessary error suppression.The
_ = fetchErron line 148 appears to be a leftover. The error is already handled by storing the failure reason, and the assignment serves no purpose.♻️ Remove unnecessary assignment
cmd := exec.Command("git", args...) cmd.Dir = repoPath if output, fetchErr := cmd.CombinedOutput(); fetchErr != nil { failedReason[remote] = fetchFailureReason(output) - _ = fetchErr }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/git/fetch_mainline.go` around lines 146 - 149, The code captures cmd.CombinedOutput() into output and fetchErr but then assigns fetchErr to a blank identifier; remove the unnecessary suppression. In the fetch logic (the CombinedOutput() call and its error handling), drop the `_ = fetchErr` line and simply use the existing handling that records failedReason[remote] = fetchFailureReason(output); ensure no other uses of fetchErr remain in the surrounding block (function fetchFailureReason and variable failedReason referenced) so the compiler has no unused variable errors.internal/git/fetch_mainline_test.go (1)
12-21: Minor: potential panic on empty git output.If
git rev-parsesomehow returns empty output (e.g., unusual git error), the slice operationout[:len(out)-1]will panic with index out of range. Consider adding a length check.🛡️ Defensive fix
func revParse(t *testing.T, repoPath, ref string) string { t.Helper() cmd := exec.Command("git", "rev-parse", ref) cmd.Dir = repoPath out, err := cmd.Output() if err != nil { t.Fatalf("git rev-parse %s: %v", ref, err) } - return string(out[:len(out)-1]) // trim newline + return strings.TrimSuffix(string(out), "\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/git/fetch_mainline_test.go` around lines 12 - 21, The revParse helper can panic if git returns empty output because it slices out[:len(out)-1]; update revParse to defensively handle empty or nil output from exec.Command by checking len(out) before slicing (or use trimming like strings.TrimSpace/bytes.TrimSuffix) and return a clear fatal error via t.Fatalf if output is empty; reference the revParse function and the out variable/len(out)-1 slice so you replace that unsafe slice with a safe trim-or-check-and-fatal approach.cmd/root.go (1)
569-599: Consider extracting duplicated branch result printing logic.The branch result formatting loop (lines 581-592) is nearly identical to the one in
runRain(lines 259-270) and thefullSyncpath (lines 613-627). While acceptable for now, extracting a helper would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 569 - 599, Extract the duplicated branch-result printing/formatting into a helper function (e.g., printBranchResults or formatAndPrintBranches) and call it from the non-fullSync block, the runRain block, and the fullSync block; the helper should accept the slice of branch results (the type returned in res.Branches) and any context needed to print (uses weatherSymbol, outcomeLabel, safety.SanitizeText, and strings.TrimSpace) and either return the aggregated counts (Updated, Skipped, Frozen, Failed) or accept pointers to accumulate them so you can replace the repeated loop that builds 'line' strings and updates totalUpdated/totalSkipped/totalFrozen/totalFailed with calls to this single helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/root.go`:
- Around line 569-599: Extract the duplicated branch-result printing/formatting
into a helper function (e.g., printBranchResults or formatAndPrintBranches) and
call it from the non-fullSync block, the runRain block, and the fullSync block;
the helper should accept the slice of branch results (the type returned in
res.Branches) and any context needed to print (uses weatherSymbol, outcomeLabel,
safety.SanitizeText, and strings.TrimSpace) and either return the aggregated
counts (Updated, Skipped, Frozen, Failed) or accept pointers to accumulate them
so you can replace the repeated loop that builds 'line' strings and updates
totalUpdated/totalSkipped/totalFrozen/totalFailed with calls to this single
helper.
In `@internal/git/fetch_mainline_test.go`:
- Around line 12-21: The revParse helper can panic if git returns empty output
because it slices out[:len(out)-1]; update revParse to defensively handle empty
or nil output from exec.Command by checking len(out) before slicing (or use
trimming like strings.TrimSpace/bytes.TrimSuffix) and return a clear fatal error
via t.Fatalf if output is empty; reference the revParse function and the out
variable/len(out)-1 slice so you replace that unsafe slice with a safe
trim-or-check-and-fatal approach.
In `@internal/git/fetch_mainline.go`:
- Around line 146-149: The code captures cmd.CombinedOutput() into output and
fetchErr but then assigns fetchErr to a blank identifier; remove the unnecessary
suppression. In the fetch logic (the CombinedOutput() call and its error
handling), drop the `_ = fetchErr` line and simply use the existing handling
that records failedReason[remote] = fetchFailureReason(output); ensure no other
uses of fetchErr remain in the surrounding block (function fetchFailureReason
and variable failedReason referenced) so the compiler has no unused variable
errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73db73fe-5899-4ca8-ab18-69ece282683a
📒 Files selected for processing (8)
CLAUDE.mdREADME.mdcmd/root.gocmd/root_test.gointernal/git/fetch_mainline.gointernal/git/fetch_mainline_test.gointernal/git/rain.goscripts/uat.sh
This was referenced Apr 16, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
git fetch <remote> --prunefor mainline branches only viaMainlineFetchRemotes— updates remote-tracking refs, not local branch tips.--sync: previous behavior — fullRainRepositoryhydration (fetch all remotes, fast-forward / skip / risky reset).--rain: interactive TUI repo picker (git-fire--fireparity).--selectis removed (no hidden alias).computeFullSync:--sync,--risky/global.risky_mode, explicit--branch-mode, or non-mainline configbranch_modeselects the full hydrate path.Docs
Tests
go test -race -count=1 ./...TestMainlineFetchRemotes_UpdatesRemoteTrackingRef,TestComputeFullSync; existing cmd integration tests setrainSync = truewhere full hydrate is required.Summary by CodeRabbit
New Features
Documentation
Tests
Chores