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 |
…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).
…ify main on unborn
…agate marker write errors, check unborn remote before branch switch
…back failed branch marking Compare target remote against configured upstream remote to retain non-existence lease on target, roll back created branch if marking fails, and allow --yes to bypass PR default-branch preflight. Refs Gitlawb#671
…nborn-remote guard
f6583ce to
c5f42e5
Compare
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
-
[P2] Do not leave the commit on the protected local branch after auto-branching
internal/cli/workflows.go:1203
The normal flow commits onmainfirst and only then createsuser/slugat the sameHEAD; the original localmainref is never moved back. After the PR is squash-merged, the user’s localmainstill points at the pre-squash commit and diverges fromorigin/main, so a later pull/push requires manual recovery and can re-publish the old change. Either create the feature branch before committing or restore the original default ref once the new branch owns the commit, with failure-safe handling and coverage for the commit-then-push path. -
[P2] Resolve the actual upstream before the
--yesunborn-remote check
internal/cli/workflows.go:930
Withchanges pr --yesand no--remote, this bypassesisDefaultBranchand callsbranchUpstreamRemotewith an empty branch name, which queriesbranch..remoteand falls back toorigin.Pushsubsequently resolves the actual current branch and can target its configured upstream instead. In a fork setup this tests the wrong remote: an unbornoriginblocks a valid upstream PR, while a populatedoriginlets an unborn upstream pass the preflight and fails only after pushing. Resolve the current branch/upstream through the same path even under--yes, and cover a non-origin upstream.
…upstream Do not leave auto-branch commits on the protected local default branch, and resolve the real upstream for the unborn-remote guard under --yes instead of always probing origin.
|
Addressed @jatmn findings in
Please re-review when you can. |
Thread --auto and a mock provider through DiffBytes coverage so the cap is checked on the real LLM request path, not only Inspect.
|
Both P2s are on tip ( Ready for re-review. |
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 auto-create a feature branch on an unborn remote
internal/cli/workflows.go:1160-1173,internal/cli/workflows.go:1218-1252
TheunbornRemoteexception treats the missing<remote>/<currentBranch>tracking ref as proof that every local commit is publishable, then creates and pushes onlyuser/slug. For a new GitHub/bare repository whose HEAD is configured formain, that leaves the remote HEAD dangling: it has a feature branch but no default branch. The nextchanges prcannot recover: its preflight no longer sees an empty remote, whileIsDefaultBranchsees an unresolved HEAD plus an existing head and fails closed. Pushingmainwith--yesafterward publishes the same commit asuser/slug, so there is no remaining PR diff. The root cause is conflating “there is no tracking ref to compare” with “a feature branch is safe as the first remote branch.” Model the initial-repository state separately: require the initial default branch to be established before auto-branching, or create/push that default branch as an explicit initialization operation before creating the feature branch. Add an end-to-end bare-remote test that runschanges pushfollowed bychanges prand proves the workflow remains usable. -
[P1] Keep the new-branch lease after a failed generated-branch push
internal/cli/workflows.go:1113-1122
The retry path clearsRequireNewRemoteBranchas soon asbranch.<name>.remoteequals the target remote, but this configuration is not proof that the generated branch was published. Withbranch.autoSetupMerge=inherit,git checkout -b user/slugcopiesremote=originandmerge=refs/heads/mainfrom the source branch before the first push. If the first lease-protected push loses a collision race, the generated branch remains marked locally; its retry seesremote=origin, drops the lease, and can fast-forward the other creator'sorigin/user/slug. The root cause is using a partial local config field as a publication-state marker. Track the outcome of Zero's successfulpush -ufor the exact<remote>/<generated branch>ref (or query that exact upstream relationship), and retain the zero-value lease for every other generated-branch retry. The addedHasUpstreamhelper is not currently called and, by itself, must not accept an inherited upstream toorigin/main; add tests forbranch.autoSetupMerge=inherit, a lost lease, and a same-name concurrent branch. -
[P2] Restore the original default branch against its own upstream
internal/cli/workflows.go:1240-1246
--remoteis documented as the push-destination override, but the cleanup step unconditionally resets the local default branch to<selected remote>/<branch>. In a fork setup where localmaintracksorigin/main,changes push --remote upstreamtherefore creates the feature branch and then rewrites localmaintoupstream/main. Those refs can differ or be unrelated; the branch still tracksorigin, so later pulls/pushes now run from an unexpected base. The root cause is using one resolved remote for two distinct roles: the destination chosen for this publish and the original branch's baseline. Capture the source branch's actual upstream ref before creating the feature branch, and restore that ref (or do not rewrite the source branch when its upstream differs from the explicit destination). Cover a fork fixture whereorigin/mainandupstream/mainhave different tips and assert that--remote upstreamleaves localmainat its original upstream tip.
Refuse auto-creating a feature branch when the target remote has no refs yet.
Publishing only user/slug leaves remote HEAD dangling, so the next changes pr
fails closed and a later main push has no PR diff. Require push --yes first.
Keep the nonexistence lease unless branch@{upstream} is exactly the target
remote/generated-branch ref from a successful push -u. Inherited origin/main
from branch.autoSetupMerge=inherit must not drop the lease.
Restore the original default branch against its own upstream tip, not the
push destination, so --remote upstream does not rewrite a fork main that
tracks origin/main.
|
Addressed the latest review:
|
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
-
[P2] Preserve successful publication when
push -ucannot update local config
internal/cli/workflows.go:1115
The retry path treats a generated branch as published only when its local upstream config equals the target ref. That is not a reliable publication record:git push -ucan create the remote branch and then fail to write.git/config(for example, while another Git process holds.git/config.lock). Git reportsunable to write upstream branch configurationon stderr but exits zero;Pushtreats that as success and discards stderr. The generated-branch marker remains, butbranchUpstreamRefis empty. If the user makes another commit before retrying,ensureFeatureBranchsetsRequireNewRemoteBranch, and the next push uses--force-with-lease=<branch>:against the branch that the first push already created. Git then rejects it as stale, sochanges pushandchanges prcannot proceed without manual upstream repair.Please fix the root cause by making the successful remote publication and the local upstream setup separate, observable outcomes. For example, surface an upstream-write failure instead of reporting the push as successful, or record/recover the exact remote ref state before deciding to apply the nonexistence lease on a retry. The retry decision must not infer remote nonexistence solely from local branch configuration. Add an integration-style regression test for a successful remote push whose
-uconfiguration write fails, followed by a new commit and retry. -
[P3] Skip one-word LLM acknowledgements before accepting a slug
internal/cli/workflows.go:1365
extractBranchSlugchecksslugLineRebefore theisPreambleTextfilter. A common noncompliant response such asSure\nadd login pagetherefore returnsSure: the one-word acknowledgement satisfies the kebab-case regex and the function exits before it reaches the actual suggestion on the next line. This producesuser/sureinstead of the intended branch name, and it bypasses the explicitsure/certainlyentries already present in the preamble list.Please make preamble classification part of candidate selection, before the strict-slug early return. Preserve the intentional inline form (
Branch name: add-login-page) by applying the check to the extracted value when a label was removed, rather than accepting every regex-shaped original line. Add regression coverage for one-word acknowledgements followed by both spaced and kebab-case slug suggestions.
Push -u can publish a remote branch then fail to write local upstream config. Treat that as incomplete, recover with set-upstream-to, and probe the remote before reasserting the nonexistence lease on retry. Also skip one-word LLM acknowledgements before accepting a slug-shaped line. Refs Gitlawb#671
|
Addressed the latest review on tip
Tests: Push recovery/failure cases, real-git |
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.