feat(zerogit): auto-create a conventional branch before push/pr on default branch#671
Conversation
…fault branch zero changes push/pr refused to push straight to the default branch but never offered an alternative, unlike other agent tooling that names and checks out a feature branch automatically. Add CreateBranch, IsDefaultBranch, CurrentGitUser, and branch-name slugify/build helpers to zerogit, and wire push/pr to auto-create a "<user>/<slug>" branch (slug from an LLM when a provider is configured, otherwise a deterministic fallback from the diff) whenever the current branch is the default one and --yes/--dry-run weren't passed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Git branch-management APIs and wires them into CLI workflows so ChangesAutomatic feature branch routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runChangesPush
participant ensureFeatureBranch
participant zerogit
participant pushChanges
User->>runChangesPush: invoke changes push
runChangesPush->>ensureFeatureBranch: determine branch target
ensureFeatureBranch->>zerogit: resolve default branch and Git user
ensureFeatureBranch->>zerogit: create computed feature branch
runChangesPush->>pushChanges: push to ensured branch and remote
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/zerogit/zerogit_test.go (2)
836-848: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CurrentGitUser's fallback branches (OS username, literal"user") are untested.Only the
git config user.namesuccess path is covered. The two fallback tiers (L715-718 in zerogit.go) have no coverage, at least a case where the fake runner errors/returns empty output to exercise the OS-username 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/zerogit/zerogit_test.go` around lines 836 - 848, Extend TestCurrentGitUser to cover CurrentGitUser’s fallback behavior when git config returns an error or empty output, asserting the OS-username fallback and command invocation; also add coverage for the final literal "user" fallback when no OS username is available.
731-792: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing coverage for
CreateBranchfailing to check out (e.g. branch already exists).All three subtests exercise success/dry-run/empty-name paths; none exercise the
git checkout -bfailure branch (L700-702 in zerogit.go), which is exactly the scenario that occurs on a name collision. Worth adding a subtest that returns a non-nil error/non-zero exit from the checkout call and asserts the wrapped error message.🤖 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/zerogit/zerogit_test.go` around lines 731 - 792, The TestCreateBranch coverage should include checkout failure, such as an existing branch name. Add a subtest alongside HappyPath and DryRunDoesNotCheckout using fakeRunner to return a non-zero/error result for the checkout invocation, then assert CreateBranch returns an error containing the wrapped checkout failure message.internal/cli/workflows.go (1)
1029-1060: 🚀 Performance & Scalability | 🔵 TrivialExtra LLM round-trip added to every default-branch push/PR without
--yes.When on the default branch (and no
--yes/dry-run),ensureFeatureBranchnow performs its ownStreamCompletioncall to name the branch, on top of any existing commit-message-generation LLM call in the same flow. That's an additional ~60s-capped network round trip and provider cost on a very common path (any push straight frommain). Worth being aware of for latency/cost budgeting, and consider whether the fallback slug is "good enough" to skip the LLM call by default (e.g., behind a flag) rather than always attempting it whenever a provider is configured.🤖 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/cli/workflows.go` around lines 1029 - 1060, The ensureFeatureBranch flow should not automatically perform an LLM request on every default-branch push or PR. Use fallbackBranchSlug(summary) by default and gate generateAutoBranchSlug behind an explicit opt-in configuration or flag, preserving the existing branch-generation behavior when that opt-in is enabled.internal/cli/workflow_test.go (1)
918-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo CLI-level test for
runChangesPR+ensureFeatureBranchintegration.Coverage is added for
ensureFeatureBranchin isolation and forrunChangesPush(Lines 918-980), butrunChangesPRalso now routes throughensureFeatureBranchand forwards the ensured branch intoPushOptions.Branch(workflows.go Lines 924-928, 937). GivenprhardcodesdryRun=falseunlikepush, this path deserves its own targeted test (e.g. mirroringTestRunChangesPushCreatesFeatureBranchWhenOnDefaultforchanges pron the default branch) to lock in the new behavior before it ships.Want me to draft that test?
🤖 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/cli/workflow_test.go` around lines 918 - 980, The CLI tests cover feature-branch creation for runChangesPush but not the equivalent runChangesPR flow. Add a targeted test for changes pr on the default branch, verifying ensureFeatureBranch creates the expected branch and that runChangesPR forwards it through PushOptions.Branch, while preserving the existing dryRun=false behavior.
🤖 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/cli/workflows.go`:
- Around line 1087-1116: Update generateAutoBranchSlug to normalize
collected.Text before passing it to zerogit.SlugifyBranchComponent: select the
first non-empty line, trim surrounding whitespace and quotes, then slugify that
single-line value. Preserve the existing empty-slug error handling and provider
error propagation.
In `@internal/zerogit/zerogit.go`:
- Around line 637-662: Bound the remote lookup performed by IsDefaultBranch,
especially the isDefaultBranch call that may execute git ls-remote, with a short
context timeout even when the caller supplies context.Background(). On timeout
or remote lookup failure, preserve the existing local main/master fallback so
changes push/pr do not stall.
- Around line 677-704: Update CreateBranch to handle an already-existing local
branch before treating branch creation as failed: detect whether the requested
name exists locally, check it out and return the same BranchResult when it does,
while retaining checkout -b for new branches and preserving DryRun behavior.
---
Nitpick comments:
In `@internal/cli/workflow_test.go`:
- Around line 918-980: The CLI tests cover feature-branch creation for
runChangesPush but not the equivalent runChangesPR flow. Add a targeted test for
changes pr on the default branch, verifying ensureFeatureBranch creates the
expected branch and that runChangesPR forwards it through PushOptions.Branch,
while preserving the existing dryRun=false behavior.
In `@internal/cli/workflows.go`:
- Around line 1029-1060: The ensureFeatureBranch flow should not automatically
perform an LLM request on every default-branch push or PR. Use
fallbackBranchSlug(summary) by default and gate generateAutoBranchSlug behind an
explicit opt-in configuration or flag, preserving the existing branch-generation
behavior when that opt-in is enabled.
In `@internal/zerogit/zerogit_test.go`:
- Around line 836-848: Extend TestCurrentGitUser to cover CurrentGitUser’s
fallback behavior when git config returns an error or empty output, asserting
the OS-username fallback and command invocation; also add coverage for the final
literal "user" fallback when no OS username is available.
- Around line 731-792: The TestCreateBranch coverage should include checkout
failure, such as an existing branch name. Add a subtest alongside HappyPath and
DryRunDoesNotCheckout using fakeRunner to return a non-zero/error result for the
checkout invocation, then assert CreateBranch returns an error containing the
wrapped checkout failure message.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f58092c2-d708-4e9f-b966-136d674d41ae
📒 Files selected for processing (5)
internal/cli/app.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.go
… existing branches Address CodeRabbit's review on the auto-branch-naming PR: - generateAutoBranchSlug now takes the first non-empty, quote-trimmed line of the model's response before slugifying, instead of folding any preamble or wrapping quotes into the branch name. - isDefaultBranch's ls-remote lookup is now bounded by a 5s timeout so a slow or unreachable remote can't stall push/pr; falls back to the local main/master heuristic as before. - CreateBranch checks whether the target branch already exists locally and checks it out instead of failing checkout -b on the collision. Also add regression coverage: CurrentGitUser's OS-username fallback tier, CreateBranch's existing-branch and generic checkout-failure paths, a CLI-level test for changes pr (previously untested) mirroring the existing changes push coverage, and a messy-LLM-response case for the slug normalization fix.
|
Addressed the review, including the nitpicks. Actionable:
Nitpicks fixed:
One nitpick I'm leaving as-is: the extra LLM round-trip on every default-branch push without --yes. That's the intended behavior, not a bug: the LLM slug is the actual point of this feature, --yes/--dry-run already bypass it for anyone who wants to skip it, and gating it behind a further flag is new config surface for a cost/latency observation rather than a correctness issue. go vet, go build ./..., and go test -race -count=1 ./internal/zerogit/... ./internal/cli/... are all clean. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep branch generation from selecting an unrelated local branch
internal/cli/workflows.go:1042-1063,internal/zerogit/zerogit.go:709-718
changes pushnormally runs afterchanges commit, so the working tree inspected here is clean and the fallback name is alwaysuser/changes(the provider path likewise receives an empty diff). On a later push, or whenever a low-entropy fallback/LLM name already exists locally,CreateBranchchecks out that arbitrary existing ref instead of creating a branch at the current default-branch HEAD. The subsequent push/PR then publishes the stale branch while leaving the user's new commit onmain. Do not treat a name collision as a retry unless its history is proven to be the intended current work; use a unique name or fail visibly, and cover the ordinary commit-then-push sequence. -
[P1] Preserve and use the actual target remote throughout auto-branching
internal/cli/workflows.go:866-878,internal/cli/workflows.go:924-940,internal/cli/workflows.go:1034,internal/zerogit/zerogit.go:574-585
The preflight always checksorigin, while the original push may target--remoteor the current branch's configured upstream. After creating a branch, that new branch has no tracking configuration, soPushfalls back tooriginas well. In a fork/upstream setup this can either leave the advertised default-branch dead end intact (the target remote identifies the branch as default after theoriginpreflight skipped creation) or push/create the PR againstoriginrather than the source branch's upstream. Resolve the remote once before branching, pass it toIsDefaultBranch, and pass the same resolved remote toPush; add non-origin/upstream coverage. -
[P1] Do not fail open when default-branch lookup times out
internal/zerogit/zerogit.go:616-629
The new five-second deadline applies to the existing guard inPushas well as the new preflight. Ifls-remote --symreftakes longer than five seconds for a repository whose default istrunk/develop, the fallback only recognizesmainandmaster;Pushthen proceeds without--yes. Before this change it waited for the remote answer and preserved the confirmation guard. A lookup timeout needs to fail closed (or use a trusted local default reference), not be treated as evidence that an arbitrary branch is unprotected. -
[P1] Do not upload a diff during ordinary push/PR without an explicit opt-in
internal/cli/workflows.go:1048-1055
A configured provider now causes every default-branchchanges push/changes prto send the full change diff to that provider. These commands were previously git-only; the existing LLM behavior is explicitly requested throughchanges commit --auto.redactChangeSummaryremoves secret-shaped values but intentionally retains ordinary source code, so this silently exports proprietary code (and JSON mode gives no notice). Keep the deterministic local naming path by default and require an explicit LLM opt-in before constructing this completion request.
…ail-closed, and opt-in for LLM naming - CreateBranch never checks out an existing branch on a name collision: it picks a unique suffixed name off the current HEAD (name-2 .. name-9) and fails visibly when the namespace is exhausted, so a stale branch with unrelated history can no longer be published while the new commit stays behind on the default branch. - ensureFeatureBranch resolves the remote once (explicit --remote, then the original branch's configured upstream, then origin) via IsDefaultBranch, which now reports the resolved remote, and push/pr thread that remote into Push, so a freshly created branch without tracking configuration no longer falls back to origin in fork setups. - The default-branch check fails closed: main and master count as default with no network, an unreachable remote falls back to the local refs/remotes/<remote>/HEAD record, and when neither answers the push is refused with guidance instead of silently dropping the confirmation guard for repositories whose default is trunk or develop. - LLM branch naming is now opt-in via --auto on push/pr, matching commit --auto: a configured provider alone no longer causes the change diff to be uploaded during ordinary git-only pushes. - After the ordinary commit-then-push sequence the tree is clean, so the fallback name comes from the HEAD commit subject instead of the constant user/changes.
|
Pushed e3ec76f (plus gofmt fixup 68bcce3) for all four findings.
go build, go vet, and the zerogit and cli suites pass locally. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/zerogit/zerogit_test.go (1)
527-668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
Push's new fail-closed error path.
TestPushBranchesToRemoteexercisesisDefault == true(RejectsDefaultBranch) andisDefault == false(HappyPath,FlagsForceAndDryRun,FallbackRemoteToOrigin), but none of the subtests driveisDefaultBranchinto returning an error (remote lookup fails and no localrefs/remotes/<remote>/HEADrecord) to verifyPush's new wrapping at Line 585-587 ("cannot verify %q is not the default/protected branch: %w; use --yes to override"). This is the actual new logic added toPushin this PR and is otherwise only exercised indirectly viaIsDefaultBranch's ownFailsClosedWhenDefaultBranchUnknowntest, which doesn't go throughPush.✅ Suggested additional subtest
t.Run("FailsWhenDefaultBranchCannotBeVerified", func(t *testing.T) { root := t.TempDir() runner := &fakeRunner{results: []CommandResult{ {Stdout: root + "\n"}, {Stdout: "feat/some-feature\n"}, {Stdout: "origin\n"}, {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails {ExitCode: 1}, // no local refs/remotes/origin/HEAD record }} _, err := Push(context.Background(), PushOptions{ Cwd: root, RunGit: runner.Run, }) if err == nil || !strings.Contains(err.Error(), "use --yes to override") { t.Fatalf("expected fail-closed error, got %v", err) } })🤖 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/zerogit/zerogit_test.go` around lines 527 - 668, Add a FailsWhenDefaultBranchCannotBeVerified subtest to TestPushBranchesToRemote that makes ls-remote fail and the local refs/remotes/origin/HEAD lookup fail, then assert Push returns an error containing “use --yes to override.” Ensure the fake runner sequence reaches Push’s wrapped isDefaultBranch error path rather than the existing successful or protected-branch paths.
🤖 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.
Nitpick comments:
In `@internal/zerogit/zerogit_test.go`:
- Around line 527-668: Add a FailsWhenDefaultBranchCannotBeVerified subtest to
TestPushBranchesToRemote that makes ls-remote fail and the local
refs/remotes/origin/HEAD lookup fail, then assert Push returns an error
containing “use --yes to override.” Ensure the fake runner sequence reaches
Push’s wrapped isDefaultBranch error path rather than the existing successful or
protected-branch paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16f16bb3-6e00-490a-b235-ab360363f2d3
📒 Files selected for processing (5)
internal/cli/app.gointernal/cli/workflow_test.gointernal/cli/workflows.gointernal/zerogit/zerogit.gointernal/zerogit/zerogit_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/app.go
- internal/cli/workflows.go
|
Pushed a subtest for CodeRabbit's coverage nitpick: TestPushBranchesToRemote/FailsWhenDefaultBranchCannotBeVerified drives the remote lookup and the local refs/remotes/origin/HEAD record to fail and asserts Push itself refuses with the use --yes guidance, exercising the new fail-closed wrapping directly rather than only through IsDefaultBranch. |
There was a problem hiding this comment.
Approving. The feature is well-gated branch creation skips on --yes/--dry-run, and the LLM naming path is opt-in via --auto with the diff redacted before it leaves the machine and I like that you hardened isDefaultBranch to fail closed instead of silently downgrading to the main/master name heuristic when the remote lookup times out. Two small things worth a glance: isDefaultBranch now short-circuits on the literal names main/master before consulting the remote, so a repo whose real default is trunk would treat a local feature branch named main as the default (safe direction it only blocks a push, never permits one but slightly surprising); and if the LLM slug generation fails after printing "Generating branch name using LLM..." it silently falls back to the deterministic slug with no follow-up message, which could confuse a user waiting on the LLM. Neither blocks merge.
3 participants
Summary
zero changes pushandzero changes prcurrently refuse to push straight to the default branch (main/master) but don't offer any alternative, so hitting that guard is a dead end.CreateBranch,IsDefaultBranch,CurrentGitUser,SlugifyBranchComponent, andBuildBranchNametointernal/zerogit.push/prnow call a newensureFeatureBranchstep: if the current branch is the default branch and neither--yesnor--dry-runwas passed, it generates a short slug for the diff (via the configured LLM provider, falling back to a deterministic slug derived from the changed files if no provider is configured) and checks out<git user>/<slug>before pushing.--yesand--dry-runbypass this entirely, preserving the existing refuse/preview behavior.Linked issue
None. This came out of a direct discussion about zero having no defined branch-naming convention, not a filed issue.
Test plan
go build ./...go vet ./...go test ./...(all green except a pre-existing, unrelated failure ininternal/contextreportthat also fails on unmodifiedmain)gofmt -lclean on all changed filesinternal/zerogit/zerogit_test.go(CreateBranch,IsDefaultBranch,CurrentGitUser,SlugifyBranchComponent,BuildBranchName)internal/cli/workflow_test.gocoveringensureFeatureBranchdirectly (default-branch creation with/without a provider, skip when already off default, skip on--yes/--dry-run) and end-to-end throughzero changes pushSummary by CodeRabbit
changes pushandchanges prnow auto-create and use a non-default feature branch when run from the default branch.user/slug) and optional AI-assisted naming forpush/prwhen enabled.--messagevalidation is improved, and--autois supported forcommit,push, andpr.--yesand dry-run behavior are respected.--yesedge cases.