Skip to content

fix: stop CoS agent worktree branches from tracking the default branch - #4182

Merged
atomantic merged 2 commits into
mainfrom
claim/issue-4172
Aug 14, 2026
Merged

fix: stop CoS agent worktree branches from tracking the default branch#4182
atomantic merged 2 commits into
mainfrom
claim/issue-4172

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

A CoS agent worktree branch was being created with branch.<name>.merge = refs/heads/main. That is not a stray --track anywhere in PortOS — it is git's default branch.autoSetupMerge, which auto-tracks whenever a new branch is cut from a remote-tracking ref, and git worktree add -b <branch> <path> origin/main is exactly that shape.

It matters because /do:pr deliberately derives its push destination from that config rather than from the local branch name (git push "$PUSH_REMOTE" "HEAD:$PUSH_BRANCH"), so a branch whose upstream is named differently still pushes to the right ref. With merge=refs/heads/main the same guard resolves to HEAD:refs/heads/main and pushes the agent's work straight to main, reporting success while the branch is never published and no PR is opened. Neither existing carve-out catches it: the branch has an upstream, and the remote is a real remote, not ..

Fixed at the creation sites, per the issue's decision:

  • --no-track on every new-branch git worktree addcreateWorktree (CoS agent worktrees) and createPersistentWorktree's new-branch arm. The --track -b … origin/<branchName> arm is left alone: it tracks the branch's own ref, which is correct.
  • server/lib/branchUpstreamGuard.js — the assertion the issue asked for, as a reusable guard. The invariant is "an agent branch's upstream is either absent or names its own ref" (deliberately broader than "is it the default branch" — any foreign ref is a mis-aimed push, main is just the worst instance). It is repair-then-verify rather than refuse: it drops the bogus upstream and logs loudly, and throws only if the repair does not take. Repair matters because branches created before this fix keep their bad upstream, and a review-loop agent re-attaching to one would inherit it — so the guard also runs on the existing-branch attach path.
  • agentWorkspacePrep.js — the JIRA feature-branch bootstrap runs the same check. checkout -b off a local branch does not auto-track under git's default, but a repo configured branch.autoSetupMerge=always records the base branch as the new branch's upstream.

Acceptance criteria: a freshly created agent worktree branch now has no upstream ✓; git push "$(git config --get branch.$BR.remote)" "HEAD:$(git config --get branch.$BR.merge)" cannot land on main ✓ (executed as a test, below); the config shape is covered by tests ✓.

Test plan

server/lib/branchUpstreamGuard.test.js runs against real git repositories in a temp dir with a real bare remote — the whole premise is a git default, and a mocked execGit would only assert what the test author believed git config prints.

  • A bypass probe runs the un-fixed worktree add -b … origin/main and asserts git records refs/heads/main, so the --no-track fix is demonstrably load-bearing; the paired test shows --no-track leaves it empty.
  • The acceptance criterion is executed, not asserted about: commit in a repaired worktree, push, then check origin/main is unmoved and origin/<branch> carries the commit.
  • Repair, healthy-branch no-op (a branch tracking its own remote ref is untouched), untracked no-op, missing-argument tolerance, and the loud-throw path (a merge with no remote, which --unset-upstream refuses to clear).
  • server/services/worktreeManager.test.js adds wiring coverage: the add carries --no-track before -b, the guard fires on both the new-branch and existing-branch paths, and a healthy branch is left alone.
cd server && NODE_ENV=test npx vitest run lib/branchUpstreamGuard.test.js services/worktreeManager.test.js lib/index.test.js
→ 119 passed

Full server suite: 28453 passed. Five files failed under parallel load (setup-data-drift, chiptuneRender, settings.secretsStrip, imageGenQuota, privacyNeverFederates, imageGen.*) with 10s-timeout errors; all 8 pass when re-run in isolation, and none touch this code.

Closes #4172

#4172)

`git worktree add -b <branch> <path> origin/main` does not leave the new branch
untracked: with git's default `branch.autoSetupMerge`, branching off a
remote-tracking ref records `branch.<name>.merge = refs/heads/main`. `/do:pr`
deliberately derives its push destination from that config (`git push $REMOTE
HEAD:$MERGE`) so a branch whose upstream is named differently still pushes to
the right ref — which here resolves to `HEAD:refs/heads/main` and lands the
agent's commits straight on main, reporting success while no PR is ever opened.
Neither existing carve-out fires: the branch HAS an upstream, and the remote is
a real remote rather than `.`.

Every new-branch `worktree add` now passes `--no-track`, and a new
`lib/branchUpstreamGuard.js` asserts the invariant after the fact — an agent
branch's upstream is either absent or names its own ref. A foreign upstream is
dropped and logged loudly (which also repairs branches created before this fix
and re-attached by a review-loop agent); it throws only if the repair does not
take, because at that point every downstream push helper is still aimed at the
wrong ref.
…closed on an unreadable upstream (#4172)

Both review passes landed on the same gap: enforceSafeBranchUpstream throws
AFTER `git worktree add` has already succeeded, and that throw was outside the
`cleanupOrphanBranch` catch that only wraps the add itself — so a refusal
returned a failed create while leaving a registered worktree (and, for a fresh
`-b` add, an orphan branch) on disk. Worst on the persistent feature-agent
path: it lives outside WORKTREES_DIR, so no reaper sweeps it, its only caller
does not catch, and a retry's add then fails 'already exists' until a human
prunes. enforceUpstreamOrUndoAdd now removes the tree on refusal, deleting the
branch only where this add created it.

Second finding: readBranchUpstream collapsed a failed config read into '',
which reads as 'no upstream' — so a wedged git or an unreadable repo would wave
through a branch that really does track main, at the one moment the guard most
needs to hold. It now distinguishes '' (unset, exit 1) from null (could not
read), isSafeBranchUpstream rejects null, and the guard refuses rather than
guessing. Mirrors the sentinel-and-validate rule in CLAUDE.md.
@atomantic

Copy link
Copy Markdown
Owner Author

Review pass complete — codex and claude both ran headless against the diff and independently landed on the same finding, fixed in 1f5dc6b:

1. The guard's throw could strand a worktree. enforceSafeBranchUpstream runs after git worktree add has already succeeded, and its throw sat outside the cleanupOrphanBranch catch that only wraps the add itself — so a refusal returned a failed create while leaving a registered worktree (and, for a fresh -b add, an orphan branch) on disk. The claude pass added the detail that made this worth fixing rather than leaving to the sweeper: on the persistent feature-agent path the tree lives outside WORKTREES_DIR, so neither cleanupOrphanedWorktrees nor reapMergedWorktrees ever reaps it, its only caller has no .catch, and a retry's add then fails already exists until a human prunes. New enforceUpstreamOrUndoAdd removes the tree on refusal and deletes the branch only where this add created it (never on an attach — that branch pre-dates us and may hold commits, the same distinction cleanupOrphanBranch draws).

2. readBranchUpstream failed open. It collapsed a failed config read into '', which reads as "no upstream" — so a wedged git or unreadable repo would wave through a branch that really does track main, at exactly the moment the guard most needs to hold. It now separates '' (unset, git exit 1) from null (could not read, exit 128 or a throw); isSafeBranchUpstream rejects null, and the guard refuses instead of guessing. This is the sentinel-and-validate rule from CLAUDE.md.

Both reviewers confirmed no flow relied on the auto-set upstream (the only @{upstream} consumer, primaryCheckoutGuard.js, reads the primary checkout's branch), and that the two intentionally-tracking arms track their own ref and so pass the guard untouched.

Also closed the coverage gap claude named: the persistent feature-agent arm now has its own tests for both --no-track and the undo-on-refusal path.

Affected suites: 137 passed. CI is green on the rerun — the earlier SongBookViewer metronome failure was a client-side timing flake (3061ms on the runner vs 70ms locally) on a branch that touches no client files.

@atomantic

Copy link
Copy Markdown
Owner Author

Round 2 (confirmation pass on the fix commit): codex VERDICT: CLEAN, claude VERDICT: CLEAN — no new correctness defects. Both independently verified the four things the fix could plausibly have gotten wrong:

  • deleteBranch at each of the three call sites. false on the attach path (the branch pre-dates the add, or is a local copy of a remote branch whose commits live on origin); true on the fresh --no-track -b add, where a pre-existing branch would have failed the add before the guard runs, so anything reaching the guard was created by this add; !localBranchExists on the persistent path, which correctly splits the attach arm from the two -b arms.
  • No bad interaction with cleanupOrphanBranch's isPreexistingRefError skip — neither guard error message contains "already exists" wording, so the regex cannot falsely suppress the delete, and the worktree is removed before branch -D so the delete isn't blocked by "used by worktree".
  • The exit-code mapping is right: git config --get exits 1 for an unset key and 128 for a bad repo, so healthy untracked branches still read '' (pinned by the real-repo test) while genuine failures map to null.
  • Fail-closed rejects only transient git failures during the read — the deliberate tradeoff. Every caller handles it: the create-worktree callers already handle a rejected create, and the JIRA bootstrap's existing .catch logs and nulls the branch name.

Reviewers satisfied. Merging once CI finishes.

@atomantic
atomantic merged commit 06c3e19 into main Aug 14, 2026
6 checks passed
@atomantic
atomantic deleted the claim/issue-4172 branch August 14, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CoS agent worktree branches track refs/heads/main, so /do:pr's config-derived push lands on main

1 participant