From ea098ceb963e4e93651ebc163a9613a06a84be89 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Sat, 25 Jul 2026 19:40:40 -0700 Subject: [PATCH] Accept remote refs as diff bases; honor non-origin upstreams Both review bots found real gaps in the first pass: candidates were verified as local branch names only, so an origin/HEAD target with no local checkout fell through, and stripOrigin() dropped upstreams on any remote not named origin (fork workflows tracking upstream/release). Candidates now carry a display name plus the revs to try in order: the upstream's remote rev wins outright (it tracks the actual merge target, so it beats a possibly stale local branch of the same name), origin/HEAD tries the local branch then the remote-tracking ref, and the conventional names each try local then origin/. git.base keeps reporting the friendly branch name while the diff runs against whichever rev resolved. Two new failing-first tests: remote-only default branch, and an upstream on a remote named upstream with no local base branch. Prepared with AI assistance (Claude Code), directed by @pbakaus. Co-Authored-By: Claude Code --- skill/scripts/context-signals.mjs | 43 ++++++++++++++++++++++--------- tests/context-signals.test.mjs | 43 +++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 12 deletions(-) diff --git a/skill/scripts/context-signals.mjs b/skill/scripts/context-signals.mjs index d480e77e5..0140dccbd 100644 --- a/skill/scripts/context-signals.mjs +++ b/skill/scripts/context-signals.mjs @@ -96,24 +96,43 @@ function gitSignals(cwd) { // are withheld when the current branch IS one of them: sitting on main // in a repo that also has develop must not diff the two integration // branches against each other. - const stripOrigin = (ref) => (ref && ref.startsWith('origin/') ? ref.slice('origin/'.length) : null); - const upstreamBase = stripOrigin(run(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'])); - const remoteHeadBase = stripOrigin(run(['symbolic-ref', '--short', 'refs/remotes/origin/HEAD'])); + // Candidates carry a display name (what git.base reports) and the revs to + // try, in order. A remote ref like `upstream/release` (fork workflows) or + // an origin/HEAD target with no local checkout is a perfectly good diff + // base, so revs are not limited to local branch names. + const splitRemoteRef = (ref) => { + const i = ref ? ref.indexOf('/') : -1; + return i > 0 ? { name: ref.slice(i + 1), rev: ref } : null; + }; + const upstream = splitRemoteRef(run(['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{u}'])); + const remoteHead = splitRemoteRef(run(['symbolic-ref', '--short', 'refs/remotes/origin/HEAD'])); const conventional = ['develop', 'main', 'master']; - const candidates = new Set([ - ...[upstreamBase, remoteHeadBase].filter(Boolean), - ...(conventional.includes(branch) ? [] : conventional), - ]); + const candidates = []; + const seen = new Set(); + const addCandidate = (name, revs) => { + if (!name || name === branch || seen.has(name)) return; + seen.add(name); + candidates.push({ name, revs }); + }; + // The upstream tracks the actual merge target, so its remote rev wins over + // a possibly stale local branch of the same name. + if (upstream) addCandidate(upstream.name, [upstream.rev]); + if (remoteHead) addCandidate(remoteHead.name, [remoteHead.name, remoteHead.rev]); + if (!conventional.includes(branch)) { + for (const name of conventional) addCandidate(name, [name, `origin/${name}`]); + } let base = null; - for (const b of candidates) { - if (b === branch) continue; - if (run(['rev-parse', '--verify', '--quiet', b]) !== null) { - base = b; + let baseRev = null; + for (const c of candidates) { + const rev = c.revs.find((r) => run(['rev-parse', '--verify', '--quiet', r]) !== null); + if (rev) { + base = c.name; + baseRev = rev; break; } } const diffBase = base && branch && branch !== base ? base : null; - const fromDiff = diffBase ? run(['diff', '--name-only', `${diffBase}...HEAD`]) : null; + const fromDiff = diffBase ? run(['diff', '--name-only', `${baseRev}...HEAD`]) : null; // porcelain lines are `XY PATH`: a 2-char status + a space, then the path. // Don't trim the combined output — an unstaged-modified line starts with a // leading space (` M path`), and a global trim would eat the first line's diff --git a/tests/context-signals.test.mjs b/tests/context-signals.test.mjs index 62ef39491..b75101518 100644 --- a/tests/context-signals.test.mjs +++ b/tests/context-signals.test.mjs @@ -266,6 +266,49 @@ describe('gatherSignals', () => { assert.deepEqual(s.git.changedFiles, ['src/App.tsx']); }); + it('uses the remote-tracking ref when the base has no local branch (#302)', async () => { + const { execFileSync } = await import('node:child_process'); + const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' }); + git('init', '-q', '-b', 'develop'); + git('config', 'user.email', 't@example.com'); + git('config', 'user.name', 'Test'); + write('src/App.tsx', 'export default 1;\n'); + git('add', '.'); + git('commit', '-qm', 'init'); + git('update-ref', 'refs/remotes/origin/develop', 'HEAD'); + git('symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/develop'); + git('checkout', '-q', '-b', 'feature/w'); + git('branch', '-q', '-D', 'develop'); // remote default exists, local doesn't + write('src/Hero.tsx', 'export const Hero = () => null;\n'); + git('add', '.'); + git('commit', '-qm', 'feature work'); + const s = await gatherSignals(scratch); + assert.equal(s.git.base, 'develop'); + assert.deepEqual(s.git.changedFiles, ['src/Hero.tsx']); + }); + + it('honors an upstream on a non-origin remote (fork workflow) (#302)', async () => { + const { execFileSync } = await import('node:child_process'); + const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' }); + git('init', '-q', '-b', 'release'); + git('config', 'user.email', 't@example.com'); + git('config', 'user.name', 'Test'); + write('src/App.tsx', 'export default 1;\n'); + git('add', '.'); + git('commit', '-qm', 'init'); + git('remote', 'add', 'upstream', '.'); + git('update-ref', 'refs/remotes/upstream/release', 'HEAD'); + git('checkout', '-q', '-b', 'feature/v'); + git('branch', '-q', '--set-upstream-to=upstream/release'); + git('branch', '-q', '-D', 'release'); // the tracked base lives only on the fork parent + write('src/Hero.tsx', 'export const Hero = () => null;\n'); + git('add', '.'); + git('commit', '-qm', 'feature work'); + const s = await gatherSignals(scratch); + assert.equal(s.git.base, 'release'); + assert.deepEqual(s.git.changedFiles, ['src/Hero.tsx']); + }); + it('never diffs one integration branch against another (#302)', async () => { const { execFileSync } = await import('node:child_process'); const git = (...args) => execFileSync('git', args, { cwd: scratch, stdio: 'ignore' });