fix: [ENG-2835] checkout from unborn HEAD to fetched ref#656
Conversation
The checkout snapshot resolved the source branch via git.listFiles before delegating, which throws NotFoundError when HEAD is on an unborn branch (post-init, pre-commit). The handler then mapped the error to "no commits yet", masking the real cause and blocking the normal init + fetch + checkout flow against a remote-only ref. Resolve the source ref first; on unresolved (unborn) source, skip the listFiles snapshot and pass undefined into guardStagedConflicts. The guard now accepts a missing source — every staged blob is treated as new — and resolves the target via a small DWIM helper so a real conflict still trips on a remote-only target.
|
Claude finished @bao-byterover's task in 3m 4s —— View job Review — PR #656
SummaryOverall score: 4 / 5 — solid bug fix with a clear root-cause writeup, good integration-test coverage of the unborn-HEAD path, and the author has been honest about the multi-remote DWIM risk. The patch is minimal and targeted: it does not touch sibling handlers, doesn't widen scope, and the new What works well
Issues — see inline comments
No blocking issues. Recommend addressing (1) — at minimum a test or a PR-body callout — and (3)/(5) as cheap cleanups before merge. (2) is fine to defer to a follow-up given the documented mitigation. The rest are nits. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Extract `tryResolveRef` helper to replace inline `.catch(() => {})` (one
call site, three previous duplications). Lint rule
`unicorn/no-useless-undefined` blocks `.catch(() => undefined)`, so the
helper is the lint-clean way to land the explicit-undefined semantics.
- Thread `sourceOid` through `guardStagedConflicts` instead of re-resolving
the source branch inside the helper. One fewer fs round-trip per checkout.
- `resolveCheckoutRef` now refuses on multi-remote ambiguity (collect all
matches, return undefined when more than one), matching native git's DWIM
refuse-on-ambiguity instead of first-wins.
- Annotate `guardStagedConflicts`'s early return on missing target OID so
the trade-off (resolver could disagree with `git.checkout`'s internal
resolution) is documented in-source.
- Drop the now-redundant `sourceBranchResolves` flag; use truthy `sourceOid`
directly. Update the gate comment to mention detached HEAD explicitly.
- Add an integration test asserting the guard now also trips on
detached-HEAD + staged-conflict (previously skipped, leaving the
documented data-loss bug unprotected — fixed as a side effect of the
ENG-2835 gate change).
- Hoist test cleanup into an `extraTmpDirs` array drained in `afterEach`,
so tmp dirs no longer leak when an assertion mid-test throws.
|
Claude finished @bao-byterover's task in 3m 16s —— View job Review — PR #656 (re-review after
|
| # | Prior issue | Status |
|---|---|---|
| 1 | Stealth detached-HEAD broadening | ✅ Resolved — new integration test at test/integration/infra/git/isomorphic-git-service.test.ts:889 exercises detached-HEAD + staged conflict, gate comment at src/server/infra/git/isomorphic-git-service.ts:205-209 calls it out explicitly |
| 2 | DWIM ambiguity (first-remote-wins) | ✅ Resolved — resolveCheckoutRef now refuses on ambiguity (matches.length === 1), matches native git |
| 3 | Helper name vs. return type (resolveCheckoutRef returns OID) |
|
| 4 | Silent skip when target OID missing | ✅ Resolved — inline trade-off comment at lines 1084-1088 |
| 5 | Redundant source-ref resolution | ✅ Resolved — sourceOid threaded through guardStagedConflicts |
| 6 | .catch(() => {}) style |
✅ Resolved — extracted tryResolveRef helper since unicorn/no-useless-undefined blocked .catch(() => undefined) |
| 7 | Test hygiene (tmp-dir leaks on assertion failure) | ✅ Resolved — extraTmpDirs array drained in afterEach |
Residual issues (all non-blocking, see inline)
tryResolveRefswallows all errors, not just NotFoundError (isomorphic-git-service.ts:1490). A filesystem permission error or corrupted ref store silently degrades to "ref doesn't exist", which then degrades the guard. Narrow togit.Errors.NotFoundError.resolveCheckoutRefnaming carry-over — returns an OID, name reads like a ref. Cheap rename, can be deferred.- PR body "What did NOT change" is stale — the detached-HEAD broadening should be added to the scope statement so it shows up in release notes /
git log --merges. Commit message has it; PR body doesn't. - Test precondition assertion shape — the new detached-HEAD test has a great
expect(branchNow, 'precondition: HEAD must be detached')assert; the unborn-HEAD tests would benefit from a similar precondition assert thatresolveRef('main')actually throws (otherwise the test still "passes" if isomorphic-git ever softens that behavior).
Optional follow-up (carryover, low priority)
- DWIM now refuses on ambiguity (good), but doesn't honor
checkout.defaultRemoteor HEAD's tracking-remote preference the way native git does. Fine for single-remotebrv vcusers; worth a ticket if the multi-remote story expands.
· branch fix/ENG-2835
isomorphic-git's `git.checkout({ref: sha})` creates a local branch from the
SHA instead of detaching HEAD, and the library exposes no `--detach` mode.
A filesystem write to `.git/HEAD` is the only way to reach detached state
for the staged-conflict test. Document that in-test so the next reader does
not wonder whether the API was tried first.
|
Claude finished @bao-byterover's task in 2m 11s —— View job Review — PR #656 (post-
|
| # | Prior issue | Status | Notes |
|---|---|---|---|
| 1 | tryResolveRef swallows all errors |
⏭️ Deferred (justified) | Author noted isomorphic-git-service.ts already uses broad .catch(() => null/[]/{}) at lines 102, 114, 328-329, 360, 1405, etc. Verified — narrowing only tryResolveRef would in fact reduce consistency. Worth a follow-up ticket for a codebase-wide hygiene pass, not in-scope here |
| 2 | resolveCheckoutRef name vs. OID return |
⏭️ Deferred (justified) | Author cited the matching pattern from git.resolveRef itself — accepting |
| 3 | PR body "What did NOT change" missed detached-HEAD broadening | ✅ Resolved | "Intentional side effect" bullet added; merge-commit and release-note views now match commit message + in-source comment |
| 4 | Test precondition assertion shape | ✅ Partially resolved | New comment on filesystem-write rationale. Precondition-style assert not spread to unborn-HEAD tests; author argued the existing getCurrentBranch === 'main' check is the moral equivalent, with explicit deferral logged. Reasonable trade-off — see inline note for the residual gap |
Code quality assessment
Final state of the patch:
checkout()(line 189-235): clean. Source-ref resolution viatryResolveRef, gate isif (!params.force)with contract spelled out in comment block.sourceOidthreaded throughguardStagedConflictsto avoid redundant resolution.guardStagedConflicts(line 1071-1106): clean signature (sourceOid: string | undefined), comment block at line 1084-1088 honestly documents the resolver-disagreement trade-off.resolveCheckoutRef(line 1401-1415): DWIM-with-refuse-on-ambiguity (matches.length === 1) matches native git. Helper extraction (tryResolveRef, line 1490) tidied up the.catch(() => undefined)repetition.- Test coverage (
test/integration/infra/git/isomorphic-git-service.test.ts:760-964): 4 new tests cover unborn + DWIM, stale-local-main regression, unborn + staged conflict, unborn + untracked conflict, plus the detached-HEAD + staged-conflict guard.extraTmpDirsarray drained inafterEach(line 67-75) — tmp-dir leaks on assertion failure closed.
Open follow-up tickets worth filing (out of scope here):
- Codebase-wide hygiene pass on broad-catch patterns in
isomorphic-git-service.ts(narrow to specific error classes where the failure mode matters). - Honor
checkout.defaultRemoteand the current branch's tracking-remote preference inresolveCheckoutRefwhen the multi-remote story expands.
Recommendation: LGTM, ready to merge. Inline note on the test file is informational only.
· branch fix/ENG-2835
|
Follow-up commit on this branch: What it fixes: another user hit Fix: drop the disambiguation. After ENG-2835 the only Test: Honest note: this is covered by an acceptance criterion I wrote in the original ticket md (" Origin of the bug (per Live verification: Tests: |
… count
The checkout-handler catch block previously used `log({depth: 1}).length === 0`
to decide between BRANCH_NOT_FOUND and NO_COMMITS. That proxy was added in
4dd3ffd (ENG-1656) to make `brv vc init && brv vc checkout main` (current
branch is unborn) say "no commits yet" instead of "branch not found".
After ENG-2835 the proxy collapses two distinct cases into the same wrong
answer when HEAD is unborn:
(a) user typoed the target name (e.g. `checkout zorqplex_nonexistent_4242`)
→ message should be BRANCH_NOT_FOUND with the `-b` hint.
(b) user asked to checkout the branch they are already on, but it is unborn
(`init && checkout main`)
→ message should be NO_COMMITS with the add/commit/pull hint.
Both cases produce zero commits, so commit-count cannot separate them. The
target-name vs current-branch comparison can: case (a) differs, case (b)
matches. Switch the disambiguator to `data.branch === previousBranch`.
Detached HEAD (`previousBranch === undefined`) falls through to (a) which is
correct — checking out a typoed ref from a detached state is still
target-not-found.
Two unit tests added:
- `should throw BRANCH_NOT_FOUND (not NO_COMMITS) when target ref missing
from unborn HEAD` — the user-reported `zorqplex_nonexistent_4242` flow.
- `should throw NO_COMMITS when checking out the current unborn branch by
name` — the ENG-1656 flow this handler was originally written to protect.
Verified live with `brv` against byterover-cli.git: all three flows now
print the right message (typo → branch-not-found, current-unborn → no
commits, real ref → switched).
|
Claude finished @bao-byterover's task in 2m 16s —— View job Review — PR #656 (after
|
| Scenario | previousBranch |
data.branch |
Predicate | Emitted code | Correct? |
|---|---|---|---|---|---|
| (a) Typo from unborn HEAD | 'main' |
'zorqplex_…' |
differ | BRANCH_NOT_FOUND |
✅ |
(b) init && checkout main (current = target, unborn) |
'main' |
'main' |
match | NO_COMMITS |
✅ |
| (c) Typo from detached HEAD | undefined |
'zorqplex_…' |
differ | BRANCH_NOT_FOUND |
✅ |
(d) Typo from born main (pre-existing test, L3026) |
'main' |
'nonexistent' |
differ | BRANCH_NOT_FOUND |
✅ |
After ENG-2835 the source-ref listFiles no longer throws, so the catch only sees target-ref-missing — the author's reasoning holds. The dead gitService.log({depth:1, directory}) call is correctly removed, eliminating one filesystem side-effect from the error-handling path.
What works well
- Predicate is the natural one — the only ambiguity the old proxy collapsed was "same-branch unborn" vs "different-target unborn", and the target-name comparison separates exactly those two. Detached HEAD falls through to
BRANCH_NOT_FOUNDbecause'foo' !== undefined, which is the right default. - Improved NO_COMMITS message —
"… or pull from a remote."is the actionable add given the new post-init+fetch flow this PR enables. Before, the message was hostile after fetch. - Two-case test coverage — both cases the disambiguator separates are now locked in (
vc-handler.test.ts:3048and:3074). The negative-assertionexpect(error.message).to.not.include('does not have any commits yet')in the typo test is a nice belt-and-braces check that would catch a regression if someone restored the old default. - In-source comment block at
vc-handler.ts:560-567explains the why including the prior-art reference (ENG-1656, commit4dd3ffd69e). Future readers will not have to dig. - Honesty in the PR comment about the ticket-acceptance-criterion vs verification gap is a useful signal — the AC was written, never tested, and a separate review nit on this taxonomy was deferred as "cosmetic" when it was actually user-facing. Process improvement worth noting.
Residual nits (all non-blocking)
- No explicit test for detached HEAD + typo — the table above shows case (c) is correct by construction, but it isn't covered by a test. A two-line variant of
vc-handler.test.ts:3048withdeps.gitService.getCurrentBranch.resolves(undefined)would lock it in. Low priority; the pre-existing test at:3026indirectly exercises the "differ" branch. - Dead
gitService.log.resolves(...)stubs in the new test atvc-handler.test.ts:3054(deps.gitService.log.resolves([])) and the pre-existing test atvc-handler.test.ts:3029-3031. The new code path never callslog(), so these stubs are now no-ops. Cosmetic, but worth a sweep — leaving them suggests the test still depends onlogand could mislead the next reader. - PR-body comment vs actual commit drift — the comment at
2026-05-14T10:38:25Zdescribes a "drop disambiguation / always BRANCH_NOT_FOUND" approach. The pushed commit9a8b4c8(title:disambiguate NotFoundError by branch name, not commit count) keeps the disambiguator. A one-line addendum to the PR body's "Root cause" or "What changed" section, explaining the better-than-described fix, would keep the merge-commit / release-note view honest. The commit message itself is excellent — this is purely about the PR body. - Predicate could be defensively guarded —
data.branch === previousBranchis correct becausedata.branchis a non-empty string (Zod-validated) andpreviousBranchisstring | undefined, so apreviousBranch === undefinedcannot spuriously matchdata.branch. Worth a one-line comment noting the schema guarantee, or no change at all if you treat the schema as the contract. - Edge: fully-qualified refs —
brv vc checkout refs/heads/mainwhile on unbornmainwould emitBRANCH_NOT_FOUND(notNO_COMMITS) because'refs/heads/main' !== 'main'. Real users don't type fully-qualified refs at the CLI, but it's a behavioral asymmetry vsgit's own normalization. Not worth fixing here — flag if someone surfaces it.
Confidence
Tests should be re-run locally to confirm 113 integration + the new 2 unit tests pass — the PR comment claims they do, no reason to doubt. The implementation is small, the diff is readable, and the comment hygiene is excellent.
Recommendation: LGTM. Merge after nit (3) — the PR body addendum — if you want the durable record to match the commit. Everything else is deferrable.
· branch fix/ENG-2835 ·
|
Claude finished @bao-byterover's task in 2m 2s —— View job Review — PR #656 (post-
|
2 participants
Summary
brv vc checkout <branch>errors withYour current branch does not have any commits yet. Run 'brv vc add' and 'brv vc commit' first.afterbrv vc init + remote add + fetch, even when the target branch exists as a remote-tracking ref. The flow is identical togit checkout <fetched-branch>on native git, which succeeds by creating a local tracking branch.brv vcusers (init, configure remote, fetch, switch to a branch). With the bug, users cannot adoptbrv vcagainst an existing remote without manually committing first, which is misleading and inconsistent with native git muscle memory.IsomorphicGitService.checkoutnow resolves the source ref before snapshotting tracked files. When the source ref is unborn (post-init, pre-commit), the listFiles snapshot is skipped and the staged-conflict guard receivesundefinedfor the source branch. The guard treats every staged blob as new in that case, and resolves the target ref via a small DWIM helper (resolveCheckoutRef) so a real conflict still trips on a remote-only target.brv vc checkout mainto restore a wiped workdir), other vc handlers (reset,commit,merge,pull,isAncestor), and the existing checkout error taxonomy invc-handler. Sibling NotFoundError audit findings are documented inagent-tmp/vc-checkout-sibling-audit.mdfor separate tickets.guardStagedConflictsgate widened fromif (!params.force && sourceBranch)toif (!params.force). Previously, detached HEAD bypassed the guard entirely, leaving the documented isomorphic-git data-loss bug (silent overwrite of staged conflicts) unprotected in that mode. Now covered, withtest/integration/infra/git/isomorphic-git-service.test.ts: "blocks checkout from detached HEAD when a staged file conflicts with target tree".Type of change
Scope (select all touched areas)
Linked issues
Root cause (bug fixes only, otherwise write
N/A)IsomorphicGitService.checkoutsnapshots the source-branch file set viagit.listFiles({ref: sourceBranch})at the start of the method.getCurrentBranchreturns'main'afterbrv vc initbecause HEAD points atrefs/heads/main, but no commit has been written, sorefs/heads/maindoes not resolve.git.listFilesthrowsNotFoundError: Could not find main, the error bubbles up tohandleCheckout, which catchesNotFoundError, runslog({depth: 1})to disambiguate, finds zero commits, and returns the misleadingNO_COMMITSerror. The actual checkout never executed.checkout()test intest/integration/infra/git/isomorphic-git-service.test.tsset up a committed seed file inbeforeEach, so the unborn-source code path was never exercised. The bug only fires when the user reachesgit.checkoutwithout ever committing locally, a flow no test covered.Test plan
test/integration/infra/git/isomorphic-git-service.test.tsguardStagedConflictsstill trips, no silent data loss.CheckoutConflictErrorstill trips, no silent overwrite.User-visible changes
brv vc checkout <branch>afterbrv vc init + remote add + fetchnow succeeds when<branch>exists as a remote-tracking ref, instead of erroring with "no commits yet".Evidence
Failing before, passing after (single test):
Before (main code):
After (this branch):
End-to-end against the live remote (
brvbinary, fresh daemon):Checklist
tsc --noEmitclean on both root andsrc/webui/tsconfig.json)npm run buildclean, dist regenerated)fix: [ENG-2835] checkout from unborn HEAD to fetched ref)main(rebased ontoorigin/maina8594cbdabefore commit)Risks and mitigations
resolveCheckoutRefdoes DWIM lookup across all configured remotes by iteratinggit.listRemotes. If the user has multiple remotes that each have a branch with the same short name, the first remote's ref wins and the guard compares against that ref. The chosen ref may differ from whatgit.checkoutitself selects, which could in theory causeguardStagedConflictsto miss or over-report a conflict in a multi-remote setup.git.listRemotes, which mirrors the order remotes were added. In practicebrv vcusers have a singleoriginremote, so the ambiguity does not arise. If a multi-remote regression surfaces later, narrow the lookup to the tracking remote of the current branch.