From 7bc4f0ea06fa56a7b3e1e43b05af8ba562572ab1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:10:47 +0000 Subject: [PATCH 1/3] test(review): review-plan and gate-diff plan the same with a stale local main or a shadowing tag (GST-883, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- test/review-plan-origin-base.test.ts | 95 ++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 test/review-plan-origin-base.test.ts diff --git a/test/review-plan-origin-base.test.ts b/test/review-plan-origin-base.test.ts new file mode 100644 index 000000000..cb5f27bce --- /dev/null +++ b/test/review-plan-origin-base.test.ts @@ -0,0 +1,95 @@ +/** + * gstack-review-plan and gstack-gate-diff diff against origin/ (GST-883): with a local that lags + * origin/, review-plan handed the raw name to gstack-gate-diff, which merged against the stale local ref and + * scanned upstream commits the branch only inherited, so a code diff planned as gate class at full depth. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const PLAN = path.join(import.meta.dir, '..', 'bin', 'gstack-review-plan'); +const GATE_DIFF = path.join(import.meta.dir, '..', 'bin', 'gstack-gate-diff'); +const FIXTURES = path.join(import.meta.dir, 'fixtures', 'gate-diff'); +const dirs: string[] = []; +afterEach(() => { while (dirs.length) fs.rmSync(dirs.pop()!, { recursive: true, force: true }); }); + +function fixture() { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'rp-origin-'))); + dirs.push(root); + const work = path.join(root, 'work'); + const env = { ...process.env, GSTACK_STATE_ROOT: path.join(root, 'state'), GIT_AUTHOR_NAME: 't', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t', GIT_CONFIG_GLOBAL: path.join(root, 'gitconfig') }; + fs.writeFileSync(path.join(root, 'gitconfig'), '[commit]\n\tgpgsign = false\n'); + const sh = (cwd: string, c: string) => { const r = spawnSync('bash', ['-c', c], { cwd, env, encoding: 'utf8', timeout: 60_000 }); if (r.status !== 0) throw new Error(`${c}: ${r.stderr}`); return r.stdout.trim(); }; + sh(root, 'git init -q --bare -b main origin.git && git clone -q origin.git work 2>/dev/null'); + sh(work, `git apply '${FIXTURES}/rh1-skip.base.patch' && git add -A && git commit -qm base && git push -q origin main`); + sh(root, `git clone -q origin.git other && cd other && git apply '${FIXTURES}/rh1-skip.patch' && git commit -qam upstream && git push -q origin main`); + sh(work, "git fetch -q origin && git checkout -qb feat origin/main && echo 'export const v = 1;' > src/v.ts && git add -A && git commit -qm feat"); + return { work, env, sh }; +} + +const planOf = (work: string, env: NodeJS.ProcessEnv) => { + const r = spawnSync(PLAN, ['--base', 'main', '--json'], { cwd: work, env, encoding: 'utf8', timeout: 180_000 }); + expect(r.status, r.stdout + r.stderr).toBe(0); + const p = JSON.parse(r.stdout) as { class: string; depth: string; gate_scan: unknown }; + return { class: p.class, depth: p.depth, gate_scan: p.gate_scan }; +}; +const gateDiff = (work: string, env: NodeJS.ProcessEnv, base: string) => { + const r = spawnSync(GATE_DIFF, [base], { cwd: work, env, encoding: 'utf8', timeout: 120_000 }); + expect(r.status, r.stdout + r.stderr).toBe(0); + return { tagged: /tagged=\{([^}]*)\}/.exec(r.stdout)?.[1], candidate: /candidate=(\S+)/.exec(r.stdout)?.[1], stderr: r.stderr }; +}; + +describe('review-plan and gate-diff read the origin merge base (GST-883)', () => { + // Value: protects=a stale local never turns inherited upstream commits into gate hunks that force full depth; + // fails_when=gstack-gate-diff merges against the local while origin/ is ahead, or review-plan hands it a base the plan's own diff did not use; + // why_new=GEVA-117 paid ~15-20M tokens of full-depth readers for a gate hunk already on origin/main; seam=bin/gstack-review-plan, bin/gstack-gate-diff; + // no_claim=which gate rules fire + test('local main behind origin/main plans the same class, depth and gate-tagged count as a fresh main', () => { + const { work, env, sh } = fixture(); + expect(sh(work, 'git merge-base main HEAD')).not.toBe(sh(work, 'git merge-base origin/main HEAD')); + const stale = { plan: planOf(work, env), tags: gateDiff(work, env, 'main').tagged }; + sh(work, 'git branch -f main origin/main'); + const fresh = { plan: planOf(work, env), tags: gateDiff(work, env, 'main').tagged }; + expect(fresh).toEqual({ plan: { class: 'code', depth: 'light', gate_scan: expect.anything() }, tags: '' }); + expect(stale).toEqual(fresh); + }, 240_000); + + // Value: protects=a tag cannot stand in for the base branch, so it cannot move the merge base to HEAD and hide committed edits; + // fails_when=resolveBase or collectLive read `origin/main` or `main` by short name while a tag of that name exists; + // why_new=outside-voice finding on the whole diff (shadow tag reproduced); seam=bin/gstack-review-plan, bin/gstack-gate-diff + for (const [tag, branch] of [['origin/main', 'refs/remotes/origin/main'], ['main', 'refs/heads/main']] as const) { + test(`a tag named ${tag} at HEAD does not shadow ${branch}`, () => { + const { work, env, sh } = fixture(); + if (branch === 'refs/heads/main') sh(work, 'git remote remove origin'); + const before = { plan: planOf(work, env), candidate: gateDiff(work, env, 'main').candidate }; + sh(work, `git tag '${tag}' HEAD`); + const after = { plan: planOf(work, env), candidate: gateDiff(work, env, 'main').candidate }; + expect(after).toEqual(before); + expect(after.candidate?.startsWith(sh(work, `git merge-base ${branch} HEAD`).slice(0, 12))).toBe(true); + }, 240_000); + } + + // Value: protects=without a remote-tracking twin the local base still works and the fallback is named on stderr; + // fails_when=the origin preference turns a remote-less branch into no_base, or falls back silently; why_new=the fallback is new; seam=bin/gstack-gate-diff + test('no origin/: gate-diff falls back to the local ref and says so', () => { + const { work, env, sh } = fixture(); + sh(work, 'git branch trunk origin/main'); + const r = gateDiff(work, env, 'trunk'); + expect(r.tagged).toBe(''); + expect(r.stderr).toContain('gstack-gate-diff: no remote-tracking branch for trunk; base is the local trunk, which may lag its remote'); + }, 240_000); + + // Value: protects=the fallback note names why the local ref stands, and a remote-less repository stays quiet; + // fails_when=an unrelated origin/ is reported as missing, or a repository with no remote prints a lag note; why_new=review finding on the fallback; seam=bin/gstack-gate-diff + test('an unrelated origin/ is named in the note; no remote at all uses the local base quietly', () => { + const { work, env, sh } = fixture(); + sh(work, 'git branch -f main origin/main && git update-ref refs/remotes/origin/main "$(git commit-tree "HEAD^{tree}" -m orphan)"'); + expect(gateDiff(work, env, 'main').stderr).toContain('gstack-gate-diff: origin/main shares no history with HEAD; base is the local main, which may lag its remote'); + sh(work, 'git remote remove origin'); + const none = gateDiff(work, env, 'main'); + expect(none.tagged).toBe(''); + expect(none.stderr).not.toContain('base is'); + }, 240_000); +}); From dff41a922b872d945116810e11b790f5c348bcda Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:12:08 +0000 Subject: [PATCH 2/3] fix(review): gate-diff and review-plan resolve a branch base to origin/ by full ref name; review-plan scans the merge base its own diff used (GST-883) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-gate-diff | 6 ++-- bin/gstack-review-plan | 5 +-- docs/designs/HONEST_WORK_GATE_INTEGRITY.md | 9 +++-- lib/gate-diff/git.ts | 41 ++++++++++++++-------- lib/review-collect.ts | 2 +- 5 files changed, 41 insertions(+), 22 deletions(-) diff --git a/bin/gstack-gate-diff b/bin/gstack-gate-diff index 0a1af715c..d22d46075 100755 --- a/bin/gstack-gate-diff +++ b/bin/gstack-gate-diff @@ -39,7 +39,8 @@ const BIN_DIR = path.dirname(Bun.fileURLToPath(import.meta.url)); const USAGE = `usage: gstack-gate-diff [base-ref] [--format table|jsonl] [--commit ] [--help] Lists every hunk in a test, CI, runner/lint-config, snapshot or golden path of the -candidate (merge-base of and HEAD -> working tree + untracked files), +candidate (merge-base of and HEAD -> working tree + untracked files; +a branch name resolves to origin/, the local ref only when no remote has it), tags the gate edits it recognises (RH-1 RH-3 RH-4 RH-12 RH-13 RH-14 RH-15 RH-16), and prints the read-level listing after one GATE_SUMMARY: line. --commit scans that commit's first-parent delta instead of the working tree. @@ -148,7 +149,8 @@ function main(argv: string[]): number { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); return 2; } - if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : `no local branch ${baseRef}`}; base is ${base.ref}`); + if (base.local === 'fallback') console.error(`gstack-gate-diff: ${base.unrelatedRemote ? `${base.unrelatedRemote} shares no history with HEAD` : `no remote-tracking branch for ${baseRef}`}; base is the local ${baseRef}, which may lag its remote`); + else if (base.local) console.error(`gstack-gate-diff: ${base.local === 'unrelated' ? `local ${baseRef} shares no history with HEAD` : `no local branch ${baseRef}`}; base is ${base.ref}`); const mb = mergeBase(base.sha, head, root); if (!mb) { process.stdout.write(`GATE_ERROR=no_base ref=${baseRef} fix=${noBaseFix(baseRef, root)}\n`); diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 93f6aee5e..8cd1f8d2a 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -116,8 +116,9 @@ function main(): number { const m = /GATE_SUMMARY: .*?tagged=\{([^}]*)\}/.exec(g.stdout ?? ''); return g.status === 0 && m ? m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0) : null; }; - const scope = scopeOf([base]); - let gateTagged = flag('--gate-tagged') !== undefined ? Number(flag('--gate-tagged')) : gateTagsOf([base]); + const diffBase = collected.mergeBase ?? base; + const scope = scopeOf([diffBase]); + let gateTagged = flag('--gate-tagged') !== undefined ? Number(flag('--gate-tagged')) : gateTagsOf([diffBase]); if (gateTagged !== null && !Number.isFinite(gateTagged)) gateTagged = null; const force = flag('--force') === 'all' ? 'all' as const : list('--force'); const tiersSetting = runScriptSync(path.join(binDir, 'gstack-config'), ['get', 'review_effort_tiers'], { encoding: 'utf8', timeout: 15_000 }); diff --git a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md index adfad8d8c..28a8563d9 100644 --- a/docs/designs/HONEST_WORK_GATE_INTEGRITY.md +++ b/docs/designs/HONEST_WORK_GATE_INTEGRITY.md @@ -180,9 +180,12 @@ only history need is calibration, which `--commit ` covers by scanning one commit's first-parent delta without the working tree.) Positional base defaults the way `gstack-diff-scope` does (the remote's `HEAD` target, origin first, then the other remotes → `main` → `master`); an -already-qualified ref is used as given, and a branch name with no local -branch resolves through its remote-tracking twin (`origin/main` for `main`: -the merge-base is the same commit), noted once on stderr. stdout: one +already-qualified ref is used as given, and a branch name resolves through +its remote-tracking twin (`origin/main` for `main`), so a stale local `main` +never adds commits the branch only inherited (GST-883). Only when no remote +has the branch does the local ref stand, noted once on stderr (a repository +with no remote at all uses it quietly). Branches are read by full ref name, +so a tag named `main` or `origin/main` cannot stand in for them. stdout: one `GATE_SUMMARY: listed=N read=N tagged={RH-1:2,RH-15:1} unmatched=N coverage= languages_unlisted=<…> candidate=` line, then the read-level listing (bounded; the full listing goes to `--out`). diff --git a/lib/gate-diff/git.ts b/lib/gate-diff/git.ts index a9d4f07c0..3e3587dcd 100644 --- a/lib/gate-diff/git.ts +++ b/lib/gate-diff/git.ts @@ -85,24 +85,37 @@ export function isShallow(cwd: string): boolean { /** * The base as a ref that resolves here and, when `head` is given, shares - * history with it: the ref itself, else its remote-tracking branch - * (`origin/main` for `main`). A clone that checked out only the feature - * branch has no local `main`, and the merge-base against `origin/main` is the - * same commit; a stale local `main` that shares no history with HEAD (a - * fetched-over clone) yields to the twin too, so the fix line's fetch into the - * tracking ref is a fix and not a loop. + * history with it. A branch name resolves to its remote-tracking branch + * (`origin/main` for `main`, origin first) as review-collect's collectLive and + * review-dispatch do: a stale local `main` behind `origin/main` would scan + * upstream commits the branch only inherited (GST-883). The local branch is the + * fallback when no remote has the branch, or only one that shares no history + * with HEAD (`local: 'fallback'`, naming it as `unrelatedRemote`; a repository + * with no remote at all has nothing to lag behind, so it is plain). A clone + * that checked out only the feature branch has no local `main` (`missing`); a + * local `main` that shares no history with HEAD is `unrelated`, so the fix + * line's fetch into the tracking ref is a fix and not a loop. Branches are + * read by their full names (`refs/heads/`, `refs/remotes/`), so a tag named + * `main` or `origin/main` cannot stand in for them. */ -export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' } | null { +export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'fallback'; unrelatedRemote?: string } | null { const related = (sha: string) => !head || mergeBase(sha, head, cwd) !== null; - const direct = resolvesToCommit(ref, cwd); - if (direct && related(direct)) return { ref, sha: direct }; - if (!isBranchName(ref)) return direct ? { ref, sha: direct } : null; - for (const remote of remoteNames(cwd)) { + const direct = () => resolvesToCommit(ref, cwd); + if (!isBranchName(ref)) { const sha = direct(); return sha ? { ref, sha } : null; } + const qualified = resolvesToCommit(`refs/remotes/${ref}`, cwd); + if (qualified) return { ref, sha: qualified }; + const local = resolvesToCommit(`refs/heads/${ref}`, cwd); + const remotes = remoteNames(cwd); + let unrelatedRemote: string | undefined; + for (const remote of remotes) { const tracking = `${remote}/${ref}`; - const sha = resolvesToCommit(tracking, cwd); - if (sha && related(sha)) return { ref: tracking, sha, local: direct ? 'unrelated' : 'missing' }; + const sha = resolvesToCommit(`refs/remotes/${tracking}`, cwd); + if (sha && related(sha)) return { ref: tracking, sha, ...(!local ? { local: 'missing' as const } : !related(local) ? { local: 'unrelated' as const } : {}) }; + if (sha) unrelatedRemote ??= tracking; } - return direct ? { ref, sha: direct } : null; + if (local) return { ref, sha: local, ...(remotes.length ? { local: 'fallback' as const, ...(unrelatedRemote ? { unrelatedRemote } : {}) } : {}) }; + const sha = direct(); + return sha ? { ref, sha } : null; } /** A word for a printed shell line: as is when it is plain, single-quoted otherwise (ref names may hold `;`, `$`, `(`, `{1,2}`). */ diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 7fbfdae12..1b15bf12a 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -171,7 +171,7 @@ export function collectCommit(root: string, baseSha: string, headSha: string): C /** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ export function collectLive(root: string, base: string): Collected { const errors: DepthError[] = []; - const ref = git(root, ['rev-parse', '--verify', '-q', `origin/${base}^{commit}`]).ok ? `origin/${base}` : base; + const ref = [`refs/remotes/origin/${base}`, `refs/heads/${base}`].find(r => git(root, ['rev-parse', '--verify', '-q', `${r}^{commit}`]).ok) ?? base; const registry = loadRegistryAt(root, ref); const mb = git(root, ['merge-base', ref, 'HEAD']); if (!mb.ok) { From 76ec4259aa8015ca1c4f9fc655592e025d4571a1 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sun, 1 Nov 2026 23:12:08 +0000 Subject: [PATCH 3/3] docs(changelog): GST-883 fragment Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-883-review-plan-origin-base.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog.d/gst-883-review-plan-origin-base.md diff --git a/changelog.d/gst-883-review-plan-origin-base.md b/changelog.d/gst-883-review-plan-origin-base.md new file mode 100644 index 000000000..10c43e6ba --- /dev/null +++ b/changelog.d/gst-883-review-plan-origin-base.md @@ -0,0 +1,8 @@ +**A stale local `main` no longer turns a /ship review into a full-depth gate review (GST-883).** `gstack-review-plan` handed the raw base name to `gstack-gate-diff`, which diffed against the local branch. With a local `main` four commits behind `origin/main`, the gate scan read a test edit that was already on `origin/main`, so a plain code diff planned as gate class at full depth. On gstack-evals#123 that cost about 15-20M tokens of readers the change did not need (GEVA-117). + +### Fixed +- **`gstack-gate-diff `** resolves a branch name to its remote-tracking branch (`origin/main` for `main`, origin first), the way `gstack-review-plan`'s own diff and review dispatch already did (#3780). It uses the local branch only when no remote has the branch, and says so on stderr: `no remote-tracking branch for `, or `/ shares no history with HEAD`. A repository with no remote at all uses the local branch without a note. Branches are read by their full ref names, so a tag named `main` or `origin/main` can no longer stand in for the base and hide committed edits from the scan. +- **`gstack-review-plan`** passes the merge base its own diff used to `gstack-diff-scope` and `gstack-gate-diff`, so the scope flags, the gate scan and the line counts all read the same commits. Its diff reads `refs/remotes/origin/` and `refs/heads/` by full name too. + +### Tests +- `test/review-plan-origin-base.test.ts`: a fixture whose `origin/main` is one commit ahead of the local `main`, with a test edit on `origin/main` and a branch forked from it. `gstack-review-plan --base main` and `gstack-gate-diff main` plan the same class, depth and gate-tagged count as with a fresh `main` (before the fix: gate, full, `RH-1:1` against code, light, none). A tag named `origin/main` or `main` at HEAD leaves the plan and the scanned range unchanged. Also covers the stderr note for a branch no remote has, an unrelated `origin/main`, and the quiet path with no remote.