fix: GST-774 review round 1 — merged resume depth, aliased and detached PR checkouts, develop/trunk defaults

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-01 18:04:32 +00:00
1 parent 0eb4748d1f
commit 82207609aa
8 files changed
+93 -28

No files matched your search

+9 -3
View File
@@ -5,6 +5,9 @@
# Or: gstack-diff-scope <base> --head <sha> → the committed range <base>...<sha> only
# (no working tree, no untracked files): what a receipt writer and the
# merge queue compute for one commit, from any checkout (GSTA-147).
# Or: gstack-diff-scope <base> --head <sha> --merged → the same for a head GitHub
# already merged into the base (land resuming a true merge): the empty range
# is the answer, so the base-contains-head refusal below does not apply.
#
# Output contract (#2526 — all-false must be distinguishable from "we could
# not look" and from "nothing matched"):
@@ -31,8 +34,9 @@ set -euo pipefail
# Detect the repo's default branch when no arg is given (#703-class
# platform-agnostic rule, lib/default-base.ts): origin/HEAD unless it names the
# checked-out branch (a PR flow's clone can point it at the PR branch, GST-511,
# GST-774) -> the PR's base (gh, 5 s) -> origin/main -> origin/master -> main.
# checked-out branch, its upstream or HEAD's commit (a PR flow's clone can point
# it at the PR branch, GST-511, GST-774) -> the PR's base (gh, 5 s) -> the
# remote's live HEAD (ls-remote, 5 s) -> origin/main -> origin/master -> main.
# An origin/HEAD skipped that way with nothing past it resolving leaves BASE
# empty, and that fails closed; so does a default-base.ts that cannot run.
GSTACK_BUN_CONFIG=/dev/null; case "${OSTYPE:-}" in msys*|cygwin*) if command -v cygpath >/dev/null 2>&1; then GSTACK_BUN_CONFIG=NUL; fi ;; esac
@@ -50,7 +54,9 @@ if [ -z "$BASE" ]; then
esac
fi
HEAD_REF=""
MERGED_HEAD=false
if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi
if [ -n "$HEAD_REF" ] && [ "${4:-}" = "--merged" ]; then MERGED_HEAD=true; fi
FRONTEND=false
BACKEND=false
@@ -108,7 +114,7 @@ fi
# itself, all false with exit 0 (GST-774). lib/default-base.ts decides whether
# that is the base branch itself or a fresh branch (today's output) or a
# refusal; it is asked only when git says the base contains the head.
if git merge-base --is-ancestor "${HEAD_REF:-HEAD}" "${BASE}" >/dev/null 2>&1; then
if [ "$MERGED_HEAD" = false ] && git merge-base --is-ancestor "${HEAD_REF:-HEAD}" "${BASE}" >/dev/null 2>&1; then
_rc=0
_why=$(_default_base_ts check "$BASE" "${HEAD_REF:-HEAD}" gstack-diff-scope 2>/dev/null) || _rc=$?
if [ "$_rc" -ne 0 ]; then
+1 -1
View File
@@ -1 +1 @@
- On a checkout whose `origin/HEAD` points at the checked-out branch itself (a PR-branch clone), every base-relative step resolved the base to HEAD and reported zero changes, so review depth, `ready`, pregate and the docs audit passed on an empty diff (GST-774). Base resolution now skips a remote HEAD that names the checked-out branch and uses the PR's base ref when `gh` finds one, then `main` or `master`. Whatever the cause, `gstack-diff-scope`, `gstack-gate-diff`, `gstack-review-plan`, `gstack-pregate`, `gstack-docs-candidate`, `/ship` policy base resolution and the receipt's depth binding refuse with `BASE_CONTAINS_HEAD` instead of reporting no changes when the resolved base equals HEAD or contains it. For a checkout the refusal is narrowed to a base that contains HEAD while the real base does not, so a fresh branch with only uncommitted work still runs as before; a PR head is refused whenever the base contains it. `gstack-ship-queue land` resuming a merge-only PR that GitHub already merged with a true merge commit still lands instead of hitting the new refusal. See docs/troubleshooting.md#base-contains-head.
- On a checkout whose `origin/HEAD` points at the checked-out branch itself (a PR-branch clone), every base-relative step resolved the base to HEAD and reported zero changes, so review depth, `ready`, pregate and the docs audit passed on an empty diff (GST-774). Base resolution now skips a remote HEAD that names the checked-out branch, its upstream or HEAD's own commit (so a local branch under another name and a detached checkout of the PR branch are caught too), then uses the PR's base ref when `gh` finds one, then the remote's live default branch from `git ls-remote --symref`, then `main` or `master`. A repository whose default branch is `develop` or `trunk` keeps it. When the resolved base equals HEAD or contains it while the branch has commits its real base lacks, `gstack-diff-scope`, `gstack-gate-diff`, `gstack-review-plan`, `gstack-pregate`, `gstack-docs-candidate`, `/ship` policy base resolution and the receipt's depth binding refuse with `BASE_CONTAINS_HEAD` instead of reporting no changes. A pull request's head is refused whenever the base contains it; on the base branch itself, on a fresh branch with only uncommitted work, and on a detached HEAD given an explicit base, the step runs as before. `gstack-ship-queue land` resuming a merge-only PR that GitHub already merged with a true merge commit still lands, its depth check included. See docs/troubleshooting.md#base-contains-head.
+5 -3
View File
@@ -2311,8 +2311,9 @@ first parent is missing from a shallow history. The tool never guesses a
base, because an empty diff against a wrong or absent base would read as "no
gate edits" and pass review. `gstack-diff-scope` prints `SCOPE_ERROR=no_base`
(all flags false, exit 2) for the same cause, and also when no base was given
and none resolves because `origin/HEAD` names the checked-out branch and no PR
base, `origin/main` or `origin/master` exists (its comment line says so).
and none resolves because `origin/HEAD` names the checked-out branch (or its
upstream, or HEAD's own commit) and no PR base, live remote HEAD, `origin/main`
or `origin/master` resolves (its comment line says so).
**Fix.** Run the command the line prints. It is built for this checkout:
`git fetch origin +<branch>:refs/remotes/origin/<branch>` for an unfetched
@@ -5223,7 +5224,8 @@ has commits its real base lacks. The comparison would read the branch against
itself and report no changes, a false green. The usual cause is a checkout
made by a PR flow: `refs/remotes/origin/HEAD` names the PR branch and there is
no local `main` (GST-774). The detail names the base, the head, and the real
base (the PR's base from `gh`, else `origin/main`, else `origin/master`). For
base (the PR's base from `gh`, else the remote's live HEAD from
`git ls-remote --symref`, else `origin/main`, else `origin/master`). For
a pull request's head the queue refuses whenever the base contains it. On the
base branch itself, on a detached HEAD, and on a fresh branch with no commits
past its base, the step runs as before. `gstack-diff-scope` reports it as
+15 -7
View File
@@ -1,12 +1,14 @@
/**
* default-base — the base branch a command uses when none is given (GST-511), and the
* refusal every base-relative step shares when its base is the branch itself (GST-774).
* A remote's HEAD counts only when it names a branch other than the checked-out one: a
* clone made by a PR flow can point origin/HEAD at the PR branch, and that base diffs
* the branch against itself, an empty diff every gate passes. Past it: the PR's base
* from gh (a short timeout, prompts off, any failure ignored; none for a PR from another
* repository), then `<remote>/main`, then `<remote>/master`. A detached HEAD keeps the
* remote's HEAD. bin/gstack-diff-scope reads both through this file's command line.
* A remote's HEAD counts only when it names a branch other than the checked-out one, its
* upstream, or HEAD's own commit: a clone made by a PR flow can point origin/HEAD at the PR
* branch, and that base diffs the branch against itself, an empty diff every gate passes.
* Past it: the PR's base from gh, then the remote's live HEAD (`git ls-remote --symref`,
* which keeps a default branch such as develop or trunk), then `<remote>/main`, then
* `<remote>/master`. Both network reads use a 5 s timeout with prompts off and ignore any
* failure; gh names no base for a PR from another repository. bin/gstack-diff-scope reads
* both through this file's command line.
*/
import { spawnSync } from 'node:child_process';
import { resultLine } from './result-codes';
@@ -36,17 +38,23 @@ const ancestor = (cwd: string, a: string, b: string) => spawnSync('git', ['merge
export function defaultBaseBranch(cwd: string, remotes: readonly string[] = ['origin']): DefaultBase {
const current = read('git', ['symbolic-ref', '--quiet', 'HEAD'], cwd, 30_000).replace(/^refs\/heads\//, '');
const upstream = current ? read('git', ['rev-parse', '--abbrev-ref', '--symbolic-full-name', '@{upstream}'], cwd, 30_000) : '';
const headSha = commit(cwd, 'HEAD');
let skipped: DefaultBase['skipped'];
for (const remote of remotes) {
const prefix = `refs/remotes/${remote}/`;
const target = read('git', ['symbolic-ref', '--quiet', `${prefix}HEAD`], cwd, 30_000);
if (!target.startsWith(prefix)) continue;
const branch = target.slice(prefix.length);
if (branch !== current) return { branch, remote };
if (branch !== current && upstream !== `${remote}/${branch}` && commit(cwd, target) !== headSha) return { branch, remote };
skipped ??= { remote, branch };
}
const pr = current ? read('gh', ['pr', 'view', '--json', 'baseRefName,isCrossRepository', '-q', PR_BASE], cwd, 5_000).split('\n')[0] : '';
if (pr) return { branch: pr, skipped };
if (skipped) {
const live = /^ref: refs\/heads\/(\S+)\tHEAD$/m.exec(read('git', ['ls-remote', '--symref', skipped.remote, 'HEAD'], cwd, 5_000))?.[1];
if (live) return { branch: live, remote: skipped.remote, skipped };
}
for (const name of ['main', 'master']) {
if (remotes.some((remote) => commit(cwd, `refs/remotes/${remote}/${name}`))) return { branch: name, skipped };
}
+7 -7
View File
@@ -41,9 +41,9 @@ function git(root: string, args: string[]): { ok: boolean; out: string } {
return { ok: r.status === 0, out: (r.stdout ?? '').trim() };
}
/** gstack-diff-scope on the committed range `base`...`head` (`--head`): the same signals at write, carry and the queue. */
function scopeOfRange(root: string, binDir: string, base: string, head: string): Record<string, string> {
const sr = runScriptSync(path.join(binDir, 'gstack-diff-scope'), [base, '--head', head], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024 });
/** gstack-diff-scope on the committed range `base`...`head` (`--head`): the same signals at write, carry and the queue; `merged` (`--merged`) for a head GitHub already merged into the base. */
function scopeOfRange(root: string, binDir: string, base: string, head: string, merged = false): Record<string, string> {
const sr = runScriptSync(path.join(binDir, 'gstack-diff-scope'), [base, '--head', head, ...(merged ? ['--merged'] : [])], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 16 * 1024 * 1024 });
const scope: Record<string, string> = {};
for (const l of (sr.stdout ?? '').split('\n')) { const m = /^(SCOPE_[A-Z_]+)=(.*)$/.exec(l); if (m) scope[m[1]!] = m[2]!; }
if (sr.status !== 0 && !scope.SCOPE_ERROR) scope.SCOPE_ERROR = `exit ${sr.status}`;
@@ -156,10 +156,10 @@ export interface DepthVerdict { ok: boolean; line: string; message?: string }
/** The head's committed range against the base, read once for the depth verdict and the targeted line. */
interface HeadInputs { baseSha: { ok: boolean; out: string }; c: ReturnType<typeof collectCommit>; scope: Record<string, string> }
function headInputs(root: string, baseRef: string, head: string): HeadInputs {
function headInputs(root: string, baseRef: string, head: string, merged = false): HeadInputs {
const baseSha = git(root, ['rev-parse', '--verify', '--quiet', `${baseRef}^{commit}`]);
const baseRev = baseSha.ok ? baseSha.out : baseRef;
return { baseSha, c: collectCommit(root, baseRev, head), scope: scopeOfRange(root, BIN_DIR, baseRev, head) };
return { baseSha, c: collectCommit(root, baseRev, head), scope: scopeOfRange(root, BIN_DIR, baseRev, head, merged) };
}
/** The receipt's recorded gate scan when it names this head; otherwise null (unavailable). */
const boundScan = (r: ShipReceipt, head: string): number | null => {
@@ -190,7 +190,7 @@ export function receiptDepthVerdict(root: string, baseRef: string, r: ShipReceip
return { ok: false, line, message: `the receipt ${why}, but head ${head.slice(0, 12)} needs ${need.depth} (${need.reasons.join(', ')}${errs ? `; ${errs}` : ''}); the receipt is unchanged; run /ship on ${head.slice(0, 12)}` };
}
/** bindReceipt plus the owner exceptions and the review depth: what READY and train build accept; the targeted line is reported, never refused on. A base that already contains the head refuses (BASE_CONTAINS_HEAD, GST-774) before any depth is computed, except for a PR GitHub already merged (`merged`: land resuming after a merge-only true merge). */
/** bindReceipt plus the owner exceptions and the review depth: what READY and train build accept; the targeted line is reported, never refused on. A base that already contains the head refuses (BASE_CONTAINS_HEAD, GST-774) before any depth is computed, except for a PR GitHub already merged (`merged`: land resuming after a merge-only true merge), whose depth then reads the range with diff-scope's `--merged`. */
export function bindShippedReceipt(root: string, baseRef: string, body: string, pr: string | undefined, accepted: readonly string[], merged = false):
| { ok: true; receipt: ShipReceipt; head: string; line: string; depthLine: string; targetedLine: string }
| { ok: false; code: ReceiptBindingCode | 'BASE_CONTAINS_HEAD' | 'QUEUE_RECEIPT_DEPTH_RAISED' | 'QUEUE_RECEIPT_EXCEPTION_INVALID'; message: string; line: string; depthLine?: string; targetedLine?: string } {
@@ -201,7 +201,7 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string,
const ancestor = (commit: string) => b.receipt.carry?.ancestry === 'rewritten' || git(root, ['merge-base', '--is-ancestor', commit, b.head]).ok;
const problems = exceptionProblems(body, b.receipt, ancestor);
if (problems.length) return { ok: false, code: 'QUEUE_RECEIPT_EXCEPTION_INVALID', message: `the receipt's owner exception does not stand: ${problems.join('; ')}`, line: b.line };
const at = headInputs(root, baseRef, b.head);
const at = headInputs(root, baseRef, b.head, merged);
const d = receiptDepthVerdict(root, baseRef, b.receipt, b.head, at);
const gateTagged = boundScan(b.receipt, b.head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0);
const targetedLine = receiptTargetedLine(targetedClasses({ ...at.c, scope: at.scope, gateTagged }).classes, b.receipt);
+42 -2
View File
@@ -21,7 +21,7 @@ const ROOT = path.resolve(import.meta.dir, '..');
const BIN = (name: string) => path.join(ROOT, 'bin', name);
const big = Array.from({ length: 80 }, (_, i) => `exports.f${i} = () => ${i};`).join('\n') + '\n';
interface Fx { root: string; origin: string; clone: string; control: string; noMain: string; home: string; failGh: string; releaseGh: string; env: Record<string, string> }
interface Fx { root: string; origin: string; clone: string; control: string; noMain: string; alias: string; trunk: string; develop: string; home: string; failGh: string; releaseGh: string; env: Record<string, string> }
let fx: Fx;
function run(cmd: string, args: string[], cwd: string, env: Record<string, string> = fx.env) {
@@ -51,7 +51,7 @@ beforeAll(() => {
const failGh = stub(path.join(root, 'gh-fail'), 'exit 1');
const releaseGh = stub(path.join(root, 'gh-release'), 'echo release');
const env = { HOME: home, GSTACK_STATE_ROOT: home, GSTACK_HOME: home, GIT_CONFIG_GLOBAL: gitconfig, GIT_CONFIG_NOSYSTEM: '1', PATH: `${failGh}${path.delimiter}${process.env.PATH}` };
fx = { root, origin: path.join(root, 'origin.git'), clone: path.join(root, 'clone'), control: path.join(root, 'control'), noMain: path.join(root, 'nomain'), home, failGh, releaseGh, env };
fx = { root, origin: path.join(root, 'origin.git'), clone: path.join(root, 'clone'), control: path.join(root, 'control'), noMain: path.join(root, 'nomain'), alias: path.join(root, 'alias'), trunk: path.join(root, 'trunk'), develop: path.join(root, 'develop'), home, failGh, releaseGh, env };
const seed = path.join(root, 'seed');
run('git', ['init', '-q', '--bare', '-b', 'main', fx.origin], root);
run('git', ['init', '-q', '-b', 'main', seed], root);
@@ -80,6 +80,18 @@ beforeAll(() => {
git(nomainOrigin, 'symbolic-ref', 'HEAD', 'refs/heads/trunk');
run('git', ['clone', '-q', '--branch', 'pr-branch', nomainOrigin, fx.noMain], root);
git(fx.noMain, 'symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/pr-branch');
git(fx.noMain, 'remote', 'set-url', 'origin', path.join(root, 'offline.git'));
run('git', ['clone', '-q', nomainOrigin, fx.trunk], root);
run('git', ['clone', '-q', fx.origin, fx.alias], root);
git(fx.alias, 'checkout', '-q', '-b', 'work', '--track', 'origin/pr-branch');
git(fx.alias, 'symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/pr-branch');
const developOrigin = path.join(root, 'develop.git');
run('git', ['clone', '-q', '--bare', fx.origin, developOrigin], root);
git(developOrigin, 'branch', 'develop', 'pr-branch');
git(developOrigin, 'symbolic-ref', 'HEAD', 'refs/heads/develop');
run('git', ['clone', '-q', developOrigin, fx.develop], root);
});
afterAll(() => { if (fx) fs.rmSync(fx.root, { recursive: true, force: true }); });
@@ -278,3 +290,31 @@ describe('baseContainsHead keeps today\'s result where the branch is not compare
});
}, 120_000);
});
describe('a remote HEAD naming the branch under another name is skipped; a real default branch is kept', () => {
// Value: protects=a local branch tracking the PR branch, one cut from its tip, and a detached checkout of it never take the PR
// branch as their base, while a repository whose default is develop or trunk keeps it; fails_when=the skip compares names only,
// so a differently named checkout reads the PR against itself (an empty diff with exit 0), or the skip falls through to a
// stale main or to no base on the default branch itself; why_new=the outside voice and the adversarial pass reproduced both;
// seam=lib/default-base.ts
test('upstream, same commit and detached checkouts resolve main; develop and trunk resolve from the remote\'s live HEAD', () => {
withEnv(fx.env, () => {
expect(defaultBaseBranch(fx.alias).branch).toBe('main');
expect(baseContainsHead(fx.alias, 'origin/pr-branch')).toContain('on work, whose commits origin/main lacks; the real base is main');
const aliased = bin('gstack-diff-scope', [], fx.alias);
expect(aliased.status, aliased.out).toBe(0);
expect(aliased.out).toContain('SCOPE_BACKEND=true');
git(fx.alias, 'checkout', '-q', '--no-track', '-b', 'work-2', 'origin/pr-branch');
try { expect(defaultBaseBranch(fx.alias).branch).toBe('main'); } finally { git(fx.alias, 'checkout', '-q', 'work'); }
git(fx.alias, 'checkout', '-q', '--detach', 'origin/pr-branch');
try { expect(defaultBaseBranch(fx.alias).branch).toBe('main'); } finally { git(fx.alias, 'checkout', '-q', 'work'); }
expect(defaultBaseBranch(fx.trunk)).toEqual({ branch: 'trunk', remote: 'origin', skipped: { remote: 'origin', branch: 'trunk' } });
expect(defaultBaseBranch(fx.develop).branch).toBe('develop');
for (const cwd of [fx.trunk, fx.develop]) {
const onDefault = bin('gstack-diff-scope', [], cwd);
expect(onDefault.status, onDefault.out).toBe(0);
expect(onDefault.out).not.toContain('=true');
}
});
}, 120_000);
});
+2
View File
@@ -155,6 +155,8 @@ const SCANNER_EXEMPT: Record<string, string> = {
'git fetch appears only in an error-message string',
'bin/gstack-worktree-audit':
'read-only audit: best-effort fetch of the user\'s own repo\'s base branch so the MERGED column is current (skipped with --no-fetch) — a user-repo dev-workflow op like gstack-next-version\'s, not gstack-state egress',
'lib/default-base.ts':
'read-only `git ls-remote --symref <remote> HEAD` of the user\'s own repo (5 s timeout, prompts off) to learn its live default branch, run only when the cached origin/HEAD names the checked-out branch (GST-774) — a user-repo dev-workflow op like gstack-next-version\'s, not gstack-state egress',
'bin/gstack-state-matrix':
'git fetch appears only in an error-message string (the fix the user runs when origin/<base> has no merge base)',
'bin/gstack-redact-prepush':
+12 -5
View File
@@ -14,9 +14,11 @@ import { EXIT } from '../lib/headless-artifacts';
import { BASE_POLICY, cleanup, makeFixtureRepo, readFile, runBin, type FixtureRepo } from './helpers/restamp-fixture';
import { shippedBody } from './helpers/ship-queue-train-fixture';
import { resolveReleaseConflicts } from '../lib/ship-queue-land';
import { receiptDepthAtHead } from '../lib/ship-receipt-depth';
import { mergePolicy } from '../lib/ship-policy';
import { persist, repoIdentity } from '../lib/ship-queue-train-record';
const ROOT = path.resolve(import.meta.dir, '..');
const POLICY = { ...BASE_POLICY, release_tool: null, release_outputs: [], changelog_fragments: { dir: 'changelog.d' }, min_gstack_version: '1.91.336.0' };
const repos: FixtureRepo[] = [];
afterEach(() => { while (repos.length) cleanup(repos.pop()!); });
@@ -564,16 +566,21 @@ describe('land: one pass, base, lock and history', () => {
}, 90_000);
// Value: protects=a merge-only land interrupted after GitHub merged resumes to LANDED, though a true merge puts the pin inside
// the base; fails_when=the receipt check refuses the merged pin as BASE_CONTAINS_HEAD and the lock is never released;
// why_new=GST-774's base guard, and the shim only squashed; seam=none; no_claim=GitHub's merge-commit shape beyond two parents
test('merge-only: a true merge that errored after GitHub merged resumes to LANDED, not BASE_CONTAINS_HEAD', () => {
// the base; fails_when=the receipt check refuses the merged pin as BASE_CONTAINS_HEAD, or its depth check reads the empty
// range as a scope error and raises the depth, and the lock is never released; why_new=GST-774's base guard, and the shim
// only squashed; seam=none; no_claim=GitHub's merge-commit shape beyond two parents
test('merge-only: a true merge that errored after GitHub merged resumes to LANDED with a depth-bearing receipt', () => {
const fx = setup({ ...POLICY, history: 'merge-only' });
openPr(fx, '14');
const head = openPr(fx, '14');
const depth = receiptDepthAtHead(fx.repo.work, 'main', path.join(ROOT, 'bin'), head);
expect(depth.depth).toBe('light');
fx.write(d => { d.prs['14'].body = shippedBody('14', head, { gstack_version: '1.91.400.0', review_depth: depth }); });
land(fx, ['--pr', '14']);
fx.write(d => { d.trueMerge = true; d.mergeErrorAfter = true; });
expect(land(fx, ['--pr', '14']).err).toContain('HTTP 502');
const again = land(fx, ['--pr', '14']);
expect(again.out).not.toContain('BASE_CONTAINS_HEAD');
expect(again.out + again.err).not.toMatch(/BASE_CONTAINS_HEAD|QUEUE_RECEIPT_DEPTH_RAISED/);
expect(again.out).toMatch(/^REVIEW_DEPTH: receipt=light required=light /m);
expect(again.out).toContain('LANDED: ');
}, 90_000);
});