feat(zerogit): auto-create a conventional branch before push/pr on default branch - #671
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds fail-closed Git branch detection and creation APIs, then wires automatic feature-branch selection into ChangesAutomatic feature branch routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runChangesPush
participant ensureFeatureBranch
participant zerogit
participant Push
User->>runChangesPush: invoke changes push
runChangesPush->>ensureFeatureBranch: determine branch target
ensureFeatureBranch->>zerogit: resolve default branch and ahead count
ensureFeatureBranch->>zerogit: create computed feature branch
runChangesPush->>Push: push branch to resolved remote
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
|
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.
|
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.
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 an unreachable remote from clearing the default-branch guard
internal/zerogit/zerogit.go:639-646
A localrefs/remotes/<remote>/HEADis a cache, not evidence that a differently named branch is safe. If a server renames its default frommaintotrunk, the local record remainsorigin/HEAD -> origin/main, andls-remotethen times out or fails, pushingtrunkreturnsfalsehere. Both the preflight andPushrepeat that result, so the command pushes the newly protected branch without--yes, despite the intended fail-closed behavior. Only use a cached match to block a branch; when it does not match after live verification fails, return the unknown-default error. -
[P1] Honor
--diff-bytesbefore sending a branch-name prompt to the provider
internal/cli/workflows.go:870,928,1057,1072-1079,1115-1121
changes push/praccept and document--diff-bytes, but neither call threadsoptions.maxDiffBytesintoensureFeatureBranch, which invokesInspectwith onlyCwd. With--auto, the resulting unboundedsummary.Diffis embedded in the provider request. A user who supplies a cap to limit proprietary source sent for LLM naming therefore uploads the complete diff; pass the option through toInspectOptionsjust as the commit path does. -
[P2] Refuse an auto-branch push when there is no commit to publish
internal/cli/workflows.go:1057-1096
The new path branches solely from working-tree status and never establishes that HEAD is ahead of the selected default branch. On a clean, up-to-date default branch it creates and pushesuser/<HEAD-subject>at exactly the default tip; with only uncommitted edits it names a branch from those edits but pushes the unchanged HEAD, leaving the edits local.changes prthen leaves that remote branch behind before GitHub rejects the empty comparison. Check that there is a publishable commit range (and report no changes otherwise) before creating or pushing the branch. -
[P2] Do not treat an LLM preamble as the generated branch slug
internal/cli/workflows.go:1144-1155
The helper claims to tolerate non-compliant model responses but returns the first non-empty line verbatim. A common reply such asHere is a suggested branch name:\nadd-login-pagecreatesuser/here-is-a-suggested-branch-name; a fenced reply begins with```and falls back silently. Parse a valid slug line or strip the supported wrappers before slugifying, and cover preamble and fenced responses.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Pushed fixes for the review findings:
|
5b0131b to
a079996
Compare
a079996
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] Terminate options before passing the remote to
ls-remote
internal/zerogit/zerogit.go:630
remotecan come from--remote=<value>or branch configuration, but it is inserted before theHEADpositional argument without--. A value such as--upload-pack=/bin/echois parsed by Git as an option;git ls-remote --symref --upload-pack=/bin/echo HEADinvokes that program (and fails with its output as a protocol error). Put--before the remote and add a dash-prefixed-remote regression test so this preflight cannot turn a remote value into Git options. -
[P1] Avoid colliding with branches that exist only on the target remote
internal/zerogit/zerogit.go:756
The suffix loop probes onlyrefs/heads/<name>locally, thenensureFeatureBranchpushes the generated name to the resolved remote. A pre-existing remote-only<user>/<slug>(for example an old/open PR whose tip is already inmain, or a branch absent after pruning) is not detected;git push -ucan fast-forward it and silently append the new work to that unrelated remote branch/PR. Check the chosen names against the target remote as well, or fail before creating/pushing, and cover a remote-only collision. -
[P1] Permit a first feature-branch push to an empty remote
internal/zerogit/zerogit.go:640
An empty newly created remote has neither aHEADsymref fromls-remotenor a localrefs/remotes/<remote>/HEAD. After the new flow creates a non-default feature branch,Pushtherefore returns “default branch … unknown” and requires--yes;git remote set-head --autocannot repair an empty remote. This regresses the first push of a safe non-default branch and conflicts with the preflight’s stated legitimate-first-push path. Distinguish an unborn remote (or otherwise allow this non-default first push) while keeping the default-branch guard fail-closed.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
This round landed well — the suffix-based collision handling, threading the resolved remote through the default-branch check and the push, gating the LLM naming behind --auto, and the preamble/fence slug parsing are all what I wanted to see, and CI is green across the board. But I'm with jatmn on his latest pass, so holding approval. The one I care most about: a brand-new empty remote is now a dead end — ls-remote returns no symref, there's no local refs/remotes//HEAD, and the error's own suggested fix (git remote set-head --auto) can't work on an empty remote, so the very first push of a fresh repo needs --yes. That's the exact dead-end UX this PR exists to remove; please carve out the unborn-remote case while keeping the guard fail-closed otherwise (zerogit.go ~640-656). Second, the suffix loop only probes local refs/heads, so a stale same-name branch that exists only on the target remote (an old merged PR branch, say) gets silently fast-forwarded by push -u and inherits the new commits — probe the remote too, or fail before pushing (zerogit.go ~756). The missing -- before the remote in ls-remote predates this PR, but you rewrote that function and already did it right in Push's args, so add it while you're in there. My earlier two notes (the main/master short-circuit and the quiet LLM fallback) still don't gate.
|
Pushed aabd6f2 for the three open items: (1) isDefaultBranch terminates options with -- before the remote (dash-prefixed-remote regression test included); (2) an unborn remote — ls-remote succeeds with zero refs — now counts as proof there is no protected default, so the first feature-branch push of a fresh repo is no longer a --yes dead end, while every failure path stays fail-closed and main/master stay guarded by the name heuristic; (3) CreateBranch probes the target remote's heads once (bounded, with --) so a remote-only stale branch counts as taken in the suffix loop, and an unreachable remote fails visibly before anything is created or pushed. |
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] Do not treat a missing remote HEAD symref as proof that the remote is empty
internal/zerogit/zerogit.go:642
git ls-remote --symref <remote> HEADalso returns no output when a non-empty remote has a dangling or missing HEAD symref. In that state this returnsfalse, nil, which clears the new fail-closed default-branch guard for branches such astrunk;Push/ensureFeatureBranchcan then publish without--yes. Confirm that the remote has no heads before allowing the unborn-repository exception, or keep the default branch unknown and require an explicit override. -
[P1] Refuse when the working tree cannot be represented by the branch being pushed
internal/cli/workflows.go:1069
This only checks whetherHEADis ahead and then names a branch from the working-tree snapshot, butCreateBranchandPushpublish commits only. With an ahead commit plus additional unstaged edits, the command creates a PR named after the edits while omitting them; iforigin/mainis absent locally, the ignoredCommitsAheaderror permits the same empty-branch result with only uncommitted changes. Require a clean working tree (or explicitly commit/stage it) and fail when the publishable commit range cannot be determined, rather than silently creating a partial or empty PR. -
[P2] Make the remote branch collision check atomic with the push
internal/zerogit/zerogit.go:784
The remote-head snapshot is taken before checkout, while the ordinarygit push -uoccurs later. Another client can create the same generated name in that window; when its ref is at the default tip, this push fast-forwards it and silently appends this work to the other branch/PR. Use a push-time “destination must not exist” condition (or retry a fresh suffix after rejection) so the collision protection cannot be bypassed by a concurrent creator. -
[P2] Do not make ordinary feature-branch pushes depend on a fixed five-second HEAD lookup
internal/zerogit/zerogit.go:628
Every non-main/masterPushnow givesls-remotefive seconds. A reachable SSH/VPN remote whose handshake or credential negotiation exceeds that limit falls through to the fail-closed error and requires--yesbefore the normal push can even run; the same cap also blocks the new collision probe. Honor a caller/user deadline or otherwise avoid turning a slow, established remote into a default-branch-verification failure.
|
Pushed 130b986. This addresses the latest review, all four points. Fixed:
Nothing left open, all four points from the latest review are addressed. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head. All three of my asks are resolved. The unborn/empty-remote case is carved out and now confirmed with a second ls-remote --heads probe before the fail-closed guard is cleared, remote-only collisions are detected and made race-safe with a zero-value force-with-lease, and the missing -- before the remote in the ls-remote preflight is in. It also picked up jatmn's later findings (the HEAD-symref-as-empty case and the unrepresentable-working-tree one).
CI is green and CodeRabbit approved on this head. My two remaining notes (a repo whose real default is "trunk" being treated as a feature branch, and the message left dangling if LLM slug generation fails) are non-gating. Approving.
521d4ba
|
Pushed 521d4ba (on top of 130b986) for the remaining item from the latest review. From 130b986 (already on this head when the review was filed against aabd6f2):
From 521d4ba (this push):
Covered by |
521d4ba to
ef8e072
Compare
|
Addressed the latest review findings: [P1] Missing remote HEAD symref is not proof the remote is empty [P1] Refuse when the working tree cannot be represented by the branch being pushed [P2] Remote branch collision check atomic with push [P2] Ordinary feature-branch pushes no longer depend on a fixed 5s HEAD lookup Also rebased onto current |
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 the nonexistence lease on a retry after a collision
internal/cli/workflows.go:1068
When the initial auto-branch push loses the race with another creator, the explicit--force-with-lease=<branch>:correctly rejects it but leaves the local generated branch checked out. Retrying takes this non-default early return and clearsRequireNewRemoteBranch, so the ordinary push can fast-forward the concurrently-created branch (and append to its PR) when it started at the same default tip. Preserve the lease or recheck/disambiguate the remote branch on this retry path. -
[P2] Resolve
main/masteragainst the selected remote before treating it as protected
internal/zerogit/zerogit.go:632
This shortcut now refuses or auto-branches any branch namedmain/masterwithout consulting the requested remote. A repository whose remote default istrunkcan legitimately have a non-default localmain(including one trackingorigin/trunk); the prior implementation accepted the live symref result, while this path either creates an unnecessary branch or subsequently fails looking up the nonexistentorigin/main. Apply the conventional-name fallback only when the remote default cannot be determined. -
[P2] Validate publishability against the live default, not a stale tracking ref
internal/cli/workflows.go:1092
The preflight has already contacted the remote, but it decides whether there is work to publish fromremote/currentBranchin the local ref database. Iforigin/mainis stale at A while the remote has already advanced to local HEAD C, this count is positive and the command creates/pushes a feature branch whose PR has an empty comparison. Fetch or otherwise compare with the advertised default ref before creating the branch and deriving its diff.
…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.
… 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.
…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.
…guard When the live ls-remote default-branch lookup fails, isDefaultBranch fell back to the local refs/remotes/<remote>/HEAD cache and returned branch == cachedName. That cache is only a hint: if the server renamed its default (main -> trunk) the record still names main, so checking whether pushing trunk is safe returned false and the newly protected branch was pushed without --yes. Trust the cache only to block a push (a positive match proves the branch is the recorded default); on a mismatch fall through to the existing fail-closed unknown-default error instead of treating the branch as unprotected.
… naming changes push/pr accept and document --diff-bytes, but ensureFeatureBranch inspected the working tree with only Cwd set. With --auto the resulting unbounded diff was embedded in the branch-name request to the provider, so a user who capped the diff to limit how much proprietary source is uploaded for naming still sent the complete diff. Thread options.maxDiffBytes through ensureFeatureBranch into InspectOptions, matching how the commit path already bounds the diff.
The auto-branch path decided to branch solely from working-tree status and never checked that HEAD was ahead of the default branch. On a clean, up-to-date default branch it would create and push user/<subject> at the exact default tip; with only uncommitted edits it named a branch from those edits but pushed the unchanged HEAD, leaving the edits local, and changes pr then abandoned the empty branch before the host rejected the comparison. Add zerogit.CommitsAhead and consult it before branching: when HEAD is provably not ahead of <remote>/<default>, report "no changes to publish" instead of creating the branch. An indeterminate count (for example a remote-tracking ref that was never fetched) is treated as "cannot tell" so a legitimate first push is not blocked.
The slug helper returned the first non-empty line verbatim, so a reply like "Here is a suggested branch name:\nadd-login-page" produced user/here-is-a-suggested-branch-name, and a reply wrapped in a code fence slugified the ``` line to nothing and silently fell back to the local name. Drop Markdown code-fence lines and prefer a line that already reads as a kebab-case slug, falling back to the first quoted/plain line only when no slug-shaped line exists. Cover preamble and fenced responses with tests.
Three review findings: - isDefaultBranch passed the remote to ls-remote before the HEAD positional with no --, so a remote value shaped like an option (--upload-pack=/bin/echo) was parsed as one. Terminate options with -- and cover a dash-prefixed remote. - An unborn remote (freshly created, zero refs) answered ls-remote with empty output, fell through to the fail-closed unknown-default error, and made the very first feature-branch push a --yes dead end that git remote set-head --auto cannot repair. ls-remote succeeding with no refs now counts as proof there is no protected default; main/master stay guarded by the name heuristic and every failure path stays fail-closed. - CreateBranch probed only local refs/heads for collisions, so a branch existing only on the target remote (an old merged-PR branch) was silently fast-forwarded by the later push -u. The target remote's heads are now probed once (bounded, with --) and count as taken; an unreachable remote fails visibly before anything is created. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Require a confirmed unborn HEAD, not just an empty ls-remote --symref, before granting the first-push exception, since a dangling remote HEAD looked the same. Resolve branch naming and ahead-count checks against the remote branch instead of a working-tree snapshot, which could describe uncommitted edits a commit-only push would never publish. Guard concurrent branch creation with a force-with-lease push. Scope the ls-remote timeout to the default-branch preflight only, so it no longer caps Push's own unrelated check.
Refuse ensureFeatureBranch when the working tree is dirty or the ahead count against the remote default cannot be determined, so a default-branch push cannot leave uncommitted edits behind or publish an empty comparison under a guessed name.
IsDefaultBranch and CreateBranch applied a hard 5s bound on ls-remote, so ordinary feature-branch pushes and the collision probe failed on slow but reachable remotes (SSH/VPN) before Push could run. Honor the caller's context instead; callers that need a bound pass a deadline.
…unborn remote jatmn flagged that the confirmed-unborn-remote exception this PR added to IsDefaultBranch never actually helps the main/master case it was written for: when the local branch is named main, isDefaultBranch returns true from the name heuristic before it ever consults the remote, so ensureFeatureBranch still unconditionally calls commitsAhead against <remote>/<branch>. On a genuinely empty remote that ref cannot exist, so `zero changes push`/`pr` from a local default branch against a brand-new empty remote still dead-ended with "cannot determine whether HEAD is ahead of origin/main" - the exact bug this PR set out to fix. Tracing the flow further, the same problem also hit the diff-base Inspect call right after: it uses the same nonexistent <remote>/<branch> ref, so bypassing only commitsAhead would have left the dead end one line later. ensureFeatureBranch now probes the new zerogit.IsUnbornRemote (ls-remote --heads on the remote) whenever commitsAhead fails, and only bypasses both checks when that probe positively confirms the remote has no refs at all. An unreachable remote, or one that answers with refs, still fails closed exactly as before - the fail-closed "unknown default branch" behavior is unchanged and covered by TestEnsureFeatureBranchFailsWhenAheadCountUnknown and the new TestEnsureFeatureBranchFailsWhenUnbornCheckErrors. TestEnsureFeatureBranchCreatesBranchOnConfirmedUnbornRemote is the regression test: confirmed failing (with the exact dead-end error) against the pre-fix logic, passing after.
isDefaultBranch short-circuited on the literal branch names main/master before ever consulting the remote. A repository whose remote default is genuinely something else (e.g. trunk) could still have a local main branch tracking that remote, and the shortcut wrongly treated it as protected instead of trusting the live symref result. The conventional-name check now only applies as a last-resort fallback, after both the live ls-remote symref lookup and the local remotes/<remote>/HEAD cache fail to answer.
Two related gaps in ensureFeatureBranch's auto-branch preflight: - When an auto-branch push loses a force-with-lease race against a concurrent creator, the generated branch is left checked out locally with no successful push behind it. A retry took the "already off the default branch" early return and dropped RequireNewRemoteBranch, letting the next push silently fast-forward whatever the concurrent creator published. The lease now stays required unless the branch already has a configured upstream (proof a push already landed). - The ahead-of-default check compared HEAD against the local remote-tracking ref, which is only refreshed at clone or the last fetch. A stale ref can under-report how far ahead HEAD is, or say zero when the remote has actually moved past it, so ensureFeatureBranch now refreshes that tracking ref before trusting it, and fails closed if the refresh itself cannot be confirmed. Adds regression coverage for both: a retry after a lease collision keeps requiring the lease (and an already-published branch does not), and a stale tracking ref is refreshed before the ahead check runs (with a fail-closed case when that refresh cannot be confirmed).
4a3cd0c to
b1dc729
Compare
|
Addressed the three findings from the latest review. Keep the nonexistence lease on a retry after a collision (workflows.go:1068): when ensureFeatureBranch is called again with the current branch already off the default (the case a retry after a lost force-with-lease race hits), it now checks whether that branch has a configured upstream before deciding whether Push needs RequireNewRemoteBranch. No upstream means no push has landed for it yet, so the lease stays required; an already-published branch (has an upstream) does not need it reapplied. Added zerogit.HasUpstream for this check. Resolve main/master against the selected remote before treating it as protected (zerogit.go:632): isDefaultBranch no longer short-circuits on the literal names before consulting the remote. It now tries the live ls-remote symref result first, then the local remotes//HEAD cache, and only falls back to the conventional main/master names when neither can answer. A repo whose real default is something else (trunk, say) with a local main tracking it is no longer misidentified. Validate publishability against the live default, not a stale tracking ref (workflows.go:1092): before checking whether HEAD is ahead of the remote default, ensureFeatureBranch now refreshes the local remote-tracking ref for that branch (a targeted fetch of just that ref) so a merely-cached origin/main can't make a real publishable range look like zero commits ahead. If that refresh can't be confirmed, it fails closed the same way an unresolvable ahead count already did, instead of trusting whatever the stale ref reports. Tests added: a retry-after-lease-collision case (and its companion, an already-published branch not requiring the lease) in internal/cli/workflow_test.go; a non-default local main tracking a remote whose live default is something else in internal/zerogit/zerogit_test.go; and a stale-tracking-ref-but-remote-has-advanced case plus a fail-closed case when the refresh itself fails, in internal/cli/workflow_test.go. Rebased onto upstream/main (one unrelated cron fix was ahead) before making these changes. go build, go vet, go test, and gofmt all pass on internal/cli and internal/zerogit. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/workflow_test.go (1)
1107-1141: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDiff-bytes cap is only proven to reach
Inspect, not the provider request.The comment justifies the cap by the
--autoLLM-upload privacy path ("proprietary source sent for LLM naming"), but this test runs withautoNaming=false, so the provider is never constructed and the truncated diff is never asserted to land in the provider request. The privacy guarantee the comment describes is therefore untested end-to-end; only the cap→Inspecthop is covered.Consider adding an
--autovariant that stubsnewProviderand asserts the capturedzeroruntime.CompletionRequestcarries the capped diff (themockCommitMsgProvider.reqcapture at Line 721 already makes this cheap).🤖 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 1107 - 1141, Extend TestEnsureFeatureBranchThreadsDiffBytesToInspect with an auto-naming path that enables provider creation, stubs newProvider using the existing mockCommitMsgProvider request capture, and asserts zeroruntime.CompletionRequest contains the capped diff. Preserve the existing assertion that options.MaxDiffBytes reaches inspectChanges, while adding the end-to-end provider-request verification described by the test comment.
🤖 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/cli/workflow_test.go`:
- Around line 1107-1141: Extend TestEnsureFeatureBranchThreadsDiffBytesToInspect
with an auto-naming path that enables provider creation, stubs newProvider using
the existing mockCommitMsgProvider request capture, and asserts
zeroruntime.CompletionRequest contains the capped diff. Preserve the existing
assertion that options.MaxDiffBytes reaches inspectChanges, while adding the
end-to-end provider-request verification described by the test comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 433dc4c3-84e5-49d0-8956-3e40991231f4
📒 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 (4)
- internal/cli/app.go
- internal/cli/workflows.go
- internal/zerogit/zerogit.go
- internal/zerogit/zerogit_test.go
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 conventional default branches on the auto-branch path for an unborn remote
internal/zerogit/zerogit.go:641
A newly created remote makes bothls-remote --symrefand--headsreturn empty, so this returnsfalsebefore reaching themain/masterfallback. Consequently,ensureFeatureBranchtakes its non-default early return andchanges pushpublishes localmaindirectly without--yes, rather than creating the promised feature branch. This also makes the confirmed-unborn handling below inensureFeatureBranchunreachable in production: its regression test stubsisDefaultBranchto true. Preserve the default-branch classification for conventional local defaults while allowing the later unborn-remote logic to handle the missing tracking ref, and cover the realIsDefaultBranch→ensureFeatureBranchpath. -
[P2] Do not push before attempting
changes pragainst an unborn remote
internal/cli/workflows.go:930
Correcting the default-branch classification above reaches the confirmed-unborn branch path, which pushes a generated branch and then unconditionally invokesgh pr create. An empty GitHub repository has no independent base branch: its first pushed branch becomes the default, so there is no comparison for a pull request and the command fails after leaving a remote branch behind. Detect this unsupportedprcase before the push (or bootstrap an explicit base as part of a defined workflow), rather than reporting a failed PR after the side effect. -
[P2] Do not infer a collision retry from a missing upstream
internal/cli/workflows.go:1083
Every non-default branch without a configured upstream is markedRequireNewRemoteBranch, including an ordinary local checkout of an existing remote feature branch (for example,git switch --no-track origin/feature). The subsequentgit push --force-with-lease=<branch>:requires the remote branch not to exist and rejects that normal upstream-establishment push as stale. The previous push flow supported it; retain explicit provenance for a generated branch that lost the creation race, or otherwise distinguish it from manually checked-out no-upstream branches.
…ify main on unborn
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 the new-branch lease when
--forceis requested
internal/zerogit/zerogit.go:604
Auto-created branches setRequireNewRemoteBranchspecifically to prevent a branch created after the preflight probe from being updated. However,Forcewins this switch and replaces the explicit--force-with-lease=<branch>:with the generic form. With a stale localrefs/remotes/<remote>/<branch>at the default tip, a concurrent creator can publish that branch at the same tip during the probe-to-push window; generic--force-with-leasethen accepts and fast-forwards their branch. Preserve the explicit nonexistence lease (or reject this flag combination) wheneverRequireNewRemoteBranchis set. -
[P2] Do not discard the generated-branch marker write failure
internal/cli/workflows.go:1187
The retry path retains the nonexistence lease only whenIsGeneratedBranchreads the marker written here. Ifgit config branch.<name>.zeroAutoBranch truefails after checkout (for example due to config-lock contention), the initial push can lose its lease race, but the retry sees an unmarked non-default branch and dropsRequireNewRemoteBranch; it can then fast-forward the concurrent branch. Return the marker error or retain the lease through another durable mechanism. -
[P3] Reject an unborn remote before switching branches for
changes pr
internal/cli/workflows.go:930
On a default branch with a confirmed-unborn remote,ensureFeatureBranchcreates and checks out a generated branch before lines 935–939 reject PR creation and tell the user to push the initial default branch. The failed command therefore strands the user on the feature branch; following that instruction now requires manually switching back first. Check the remote before branch creation or restore the original branch on this error path.
…agate marker write errors, check unborn remote before branch switch
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 the collision lease when a generated branch is pushed to a different remote
internal/cli/workflows.go:1087-1092
The retry path treats any configured upstream as proof that the generated branch has already been published, without checking whether it is theremoteselected for this invocation. After an auto-created branch has been pushed toorigin,zero changes push --remote upstreamdropsRequireNewRemoteBranch; ifupstreamalready has the same branch name, the ordinary push can fast-forward that stale branch. Compare the configured upstream remote with the selected remote, or re-probe/assert nonexistence on every different target remote. -
[P2] Preserve collision protection after the generated-branch marker write fails
internal/cli/workflows.go:1192-1195
CreateBranchhas already switched to the new branch whenMarkGeneratedBranchcan fail (for example while.git/config.lockis present). Returning the error leaves that branch checked out but unmarked; on retry it follows the ordinary non-default path and dropsRequireNewRemoteBranch. A concurrent creator can then create the same remote ref after the first probe and have the retry fast-forward it. Roll back safely or retain durable/recomputed nonexistence protection across this partial-success state. -
[P2] Let
--yesactually bypass the default-branch preflight for PR creation
internal/cli/workflows.go:930-932
changes pr --yesis documented as bypassing the auto-branch step, and bothensureFeatureBranchandPushhonor that override. The new unconditional preliminaryisDefaultBranchcall still fails when the remote HEAD cannot be resolved or the remote is temporarily unavailable, so the command aborts before the allowed push. Skip this verification under--yes, or resolve the destination remote without requiring a default-branch determination.
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 invoked from the default branch, with remote resolution and preflight inspection.--autogenerates AI-assisted commit messages and feature-branch naming, including normalization of inconsistent responses.--diff-bytesis respected during inspection.--yes, and pushes won’t overwrite existing remote branches.--auto/--messageand related flags.