From 2f5b9f9edad8acad453e57b5f5fae5cecf0b6896 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Thu, 3 Sep 2026 15:43:45 +0000 Subject: [PATCH] fix: stop branch-reconcile probing origin on a repo that has none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read-only leftover-branch detector calls gatherBranchState once per managed app, and an app's repoPath can be a directory that was never a clone (a static-site folder, a checkout whose remote was never added). gatherBranchState resolved origin and computed hasOrigin, then ran `git ls-remote --heads origin` anyway — so every detector cycle logged `❌ branch-reconcile: git ls-remote origin failed` for that app, for a repo with no remote to read and no candidate branches to judge. It does not throw on a non-repo either, so the detector's own try/catch never fired and nothing named the app. Gate the remote read on the hasOrigin the gather already has, matching the gate reconcile() applies at both of its own remote reads for exactly this reason. `null` there is the established "could not ask" value that reconcile() already passes explicitly for an origin-less repo. Also name the repo and git's own reason in the failure line: one log line serves three call sites across every managed app, so a bare "ls-remote failed" told you neither which repo went unread nor whether it was a network blip, an unauthenticated remote, or no remote at all. Claude-Session: https://claude.ai/code/session_01FBgckWpLV6qzytK9vKnVvU --- server/services/branchReconcile.js | 24 ++++++++++--- server/services/branchReconcile.test.js | 47 +++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/server/services/branchReconcile.js b/server/services/branchReconcile.js index b17a984768..e64c85398d 100644 --- a/server/services/branchReconcile.js +++ b/server/services/branchReconcile.js @@ -542,9 +542,14 @@ export function parseRemoteHeads(stdout) { */ export async function listRemoteHeads(repoPath) { const res = await execGit(['ls-remote', '--heads', RECONCILED_REMOTE], repoPath, { ignoreExitCode: true }) - .catch(() => null); + .catch((err) => ({ error: err.message })); if (!res || res.exitCode !== 0) { - console.error(`❌ branch-reconcile: git ls-remote ${RECONCILED_REMOTE} failed — remote branch state unknown this cycle`); + // One log line serves every caller across every managed app, so it has to + // name the repo that went unread and git's own reason — without both, a + // network blip, an unauthenticated remote and a repo with no `origin` at + // all are one indistinguishable line and the operator has nothing to act on. + const why = (res?.error || res?.stderr || '').trim().split('\n')[0] || `exit ${res?.exitCode}`; + console.error(`❌ branch-reconcile: git ls-remote ${RECONCILED_REMOTE} failed in ${repoPath} (${why}) — remote branch state unknown this cycle`); return null; } return parseRemoteHeads(res.stdout); @@ -679,8 +684,9 @@ async function worktreeAgeMs(worktreePath) { * `activeAgentIds` distinguishes a live agent's worktree from an abandoned one (see * `isAbandonedAgentWorktree`); omitting it leaves every agent worktree protected. * `remoteHeads` is `listRemoteHeads`' answer when the caller already has it; - * omitted, this reads it itself, and `null` (unreadable remote) is carried - * through as "we could not ask" rather than "the remote is empty". + * omitted, this reads it itself when the repo has an origin, and `null` + * (unreadable remote, or no origin to read) is carried through as "we could + * not ask" rather than "the remote is empty". * `hasOrigin` is `getOriginInfo`'s verdict when the caller already has it (same * rationale); omitted, this reads it itself, so a standalone caller still gets a * truthful answer rather than the fail-closed default. @@ -705,11 +711,19 @@ export async function gatherBranchState(repoPath, { defaultBranch, activeAgentId ? Boolean(origin?.hasOrigin) : Boolean(providedHasOrigin); + // The remote read is gated on `hasOrigin` for the same reason reconcile's own + // two are: a repo with no `origin` has no remote to ask, and probing one + // anyway logs a failure every cycle. This gather runs once per managed app in + // the read-only leftover-branch detector, and an app's repoPath can be a + // directory that was never a clone — `null` there means "could not ask", + // which is exactly what an origin-less repo can answer. const [branches, worktrees, prsByHeadOrNull, remoteHeads] = await Promise.all([ getBranches(repoPath), listWorktrees(repoPath).catch(() => []), getOpenPrsByHead(repoPath, origin), - providedRemoteHeads === undefined ? listRemoteHeads(repoPath) : providedRemoteHeads + providedRemoteHeads !== undefined ? providedRemoteHeads + : hasOrigin ? listRemoteHeads(repoPath) + : null ]); // null = the forge could not be read (see getOpenPrsByHead). Carried onto every // input so the classifier can refuse to conclude "no PR" from an unread forge. diff --git a/server/services/branchReconcile.test.js b/server/services/branchReconcile.test.js index b3272dde08..8be9d8ea3c 100644 --- a/server/services/branchReconcile.test.js +++ b/server/services/branchReconcile.test.js @@ -470,6 +470,35 @@ describe('gatherBranchState', () => { expect(inputs.find((i) => i.branch === 'next/issue-88').openPr).toBeNull(); expect(execGh).not.toHaveBeenCalled(); }); + + // The read-only leftover-branch detector calls this once per managed app, and + // an app's repoPath can be a directory with no origin (a static-site folder, + // a checkout whose remote was never added). `origin` is already resolved by + // the time the reads fan out, so probing the remote anyway bought nothing and + // logged `❌ … git ls-remote origin failed` for that app on every cycle — the + // exact per-cycle failure reconcile's own two remote reads are gated to avoid. + it('skips the remote probe on a repo with no origin', async () => { + getOriginInfo.mockResolvedValue({ + hasOrigin: false, isGithub: false, host: null, fullName: null + }); + git.getBranches.mockResolvedValue([ + { name: 'local/only', isDefault: false, current: false, tracking: null, merged: false } + ]); + wt.listWorktrees.mockResolvedValue([]); + git.isBranchMergedInto.mockResolvedValue(false); + execGit.mockImplementation(async (args) => (args[0] === 'ls-remote' + ? { stdout: '', stderr: "fatal: 'origin' does not appear to be a git repository", exitCode: 128 } + : { stdout: '', exitCode: 0 })); + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const inputs = await gatherBranchState('/repo', { defaultBranch: 'main' }); + expect(inputs.find((i) => i.branch === 'local/only').hasOrigin).toBe(false); + expect(execGit).not.toHaveBeenCalledWith( + expect.arrayContaining(['ls-remote']), expect.anything(), expect.anything() + ); + expect(errors).not.toHaveBeenCalled(); + errors.mockRestore(); + }); }); describe('isAbandonedAgentWorktree', () => { @@ -1543,6 +1572,24 @@ describe('orphaned remote branches', () => { expect(git.deleteBranch).toHaveBeenCalledWith('/repo', 'stale/merged', { remote: true }); }); + it('names the repo and git\'s own reason when the remote cannot be read', async () => { + // One shared log line serves every caller across every managed app, so a + // bare "ls-remote failed" tells the operator neither which repo went + // unread nor whether it was a network blip or a broken remote. + execGit.mockResolvedValue({ + stdout: '', stderr: 'ssh: Could not resolve hostname github.com', exitCode: 128 + }); + git.getBranches.mockResolvedValue([]); + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const res = await reapOrphanedRemotes('/repo', 'main'); + expect(res.remoteUnavailable).toBe(true); + const line = errors.mock.calls.map(([msg]) => String(msg)).find((m) => m.includes('ls-remote')); + expect(line).toContain('/repo'); + expect(line).toContain('Could not resolve hostname'); + errors.mockRestore(); + }); + it('merge-checks the SHA ls-remote reported, not the branch name', async () => { // A stale `origin/` ref can say "merged" about commits origin has // since moved past; the live SHA is the only safe thing to judge.