mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-03 07:18:03 +01:00
fix(review): pass 1: ahead and diverged labels, diff-scope needs shared history; preflight refreshes origin/<base> and stops on a diff far beyond the PR (GST-462, HARN-15)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
0d4846cc0a
commit
86b279fcae
8 files changed
+72
-9
No files matched your search
@@ -34,9 +34,9 @@ _default_base() {
|
||||
echo "main"
|
||||
}
|
||||
BASE="${1:-$(_default_base)}"
|
||||
if git show-ref --verify -q "refs/remotes/origin/${BASE}"; then BASE="origin/${BASE}"; fi
|
||||
HEAD_REF=""
|
||||
if [ "${2:-}" = "--head" ]; then HEAD_REF="${3:-}"; fi
|
||||
if [ "$BASE" != HEAD ] && git show-ref --verify -q "refs/remotes/origin/${BASE}" && git merge-base "origin/${BASE}" "${HEAD_REF:-HEAD}" >/dev/null 2>&1; then BASE="origin/${BASE}"; fi
|
||||
|
||||
FRONTEND=false
|
||||
BACKEND=false
|
||||
|
||||
@@ -148,7 +148,7 @@ 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: ${{ unrelated: `local ${baseRef} shares no history with HEAD`, missing: `no local branch ${baseRef}`, behind: `local ${baseRef} is behind ${base.ref}`, diverged: `local ${baseRef} has diverged from ${base.ref}` }[base.local]}; base is ${base.ref}`);
|
||||
if (base.local) console.error(`gstack-gate-diff: ${{ unrelated: `local ${baseRef} shares no history with HEAD`, missing: `no local branch ${baseRef}`, behind: `local ${baseRef} is behind ${base.ref}`, ahead: `local ${baseRef} is ahead of ${base.ref}`, diverged: `local ${baseRef} has diverged from ${base.ref}` }[base.local]}; 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`);
|
||||
|
||||
@@ -1,11 +1,13 @@
|
||||
**The /ship receipt now carries the targeted reader's record.** When the review plan asked for the targeted reader on money, gate, auth or secret code and the reader ran, `gstack-ship-queue ready` still printed `REVIEW_TARGETED: … receipt=legacy verdict=missing` (gstack-evals#95, #3682, #3612), because nothing passed the record from Step 11 to the receipt. Enforcement stays report-only (GST-464).
|
||||
|
||||
**A stale local base branch no longer stands in for the base (GST-462).** `gstack-review-plan --base main` read a local `main` that was behind `origin/main`, so commits the base already had counted as the branch's own change. #3597 was labeled gate code at full depth until it passed `--base origin/main`. gbrain hit the same problem with a stale local `master` (GBRA-218).
|
||||
**A stale base no longer stands in for the PR's base (GST-462).** `gstack-review-plan --base main` read a local `main` that was behind `origin/main`, so commits the base already had counted as the branch's own change. #3597 was labeled gate code at full depth until it passed `--base origin/main`, and gbrain hit the same problem with a stale local `master` (GBRA-218). On harness-evals#152, a stale base made /ship review a 17,000-file diff of main's history instead of the PR (HARN-15).
|
||||
|
||||
### Fixed
|
||||
- **Step 11** saves the targeted record to `.gstack/tmp/targeted.json` (`gstack-review-dispatch verify --targeted-out`), and the Step 19 and handoff `gstack-ship-receipt write` commands pass it as `--review-targeted` whenever that file exists. A run without the reader passes nothing, so its receipt stays legacy. Step 9 deletes the file on first entry, so a record from an earlier run can't carry over.
|
||||
- **`gstack-gate-diff`** prefers a base branch's remote-tracking twin whenever the twin shares history with HEAD. It says so when the local branch is behind or has diverged (`local main is behind origin/main; base is origin/main`). **`gstack-diff-scope`** diffs against `origin/<base>` whenever that ref exists, whether you name the base or let it detect the default. `gstack-review-plan` reads both, so its gate scan and scope now match the merge base it already took from `origin/<base>`.
|
||||
- **`gstack-ship-preflight`** fetches the base into `origin/<base>` with an explicit refspec, so a single-branch clone also diffs against the current base tip. When a PR is open at HEAD and the diff lists far more files than the PR changes (more than twice as many, plus 50), it stops before any reader runs and names the PR's base.
|
||||
- **`gstack-gate-diff`** prefers a base branch's remote-tracking twin whenever the twin shares history with HEAD. When the local branch is behind, ahead or has diverged, it says which (`local main is behind origin/main; base is origin/main`). **`gstack-diff-scope`** diffs against `origin/<base>` whenever that ref shares history with the head, whether you name the base or let it detect the default. `gstack-review-plan` reads both, so its gate scan and scope now match the merge base it already took from `origin/<base>`.
|
||||
|
||||
### Tests
|
||||
- `test/review-targeted.test.ts`: the generated sections' own commands carry the record from Step 11's verify to `gstack-ship-receipt write`, and ready's line reads `verdict=ok`. A run without the reader passes no flag, and Step 9 clears the file.
|
||||
- `test/review-plan-cli.test.ts`, `test/gate-diff-cli.test.ts`, `test/diff-scope.test.ts`: a local `main` that is behind `origin/main` yields to `origin/main`. The review plan of a docs-only branch that merged the newer `origin/main` stays docs; before the fix, the gate scan counted origin's skipped test.
|
||||
- `test/ship-preflight.test.ts`: a single-branch clone's preflight refreshes `origin/main`, and a 60-file diff against a 3-file PR stops at the pushed head. Negative controls: another head, or a matching size, goes on.
|
||||
- `test/review-plan-cli.test.ts`, `test/gate-diff-cli.test.ts`, `test/diff-scope.test.ts`: a local `main` that is behind, ahead of or diverged from `origin/main` yields to `origin/main`, and an unrelated `origin/main` or `HEAD` does not. The review plan of a docs-only branch that merged the newer `origin/main` stays docs; before the fix, the gate scan counted origin's skipped test.
|
||||
@@ -94,7 +94,7 @@ export function isShallow(cwd: string): boolean {
|
||||
* tracking ref is a fix and not a loop. `local` says why the twin won when the
|
||||
* local branch differs from it.
|
||||
*/
|
||||
export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'behind' | 'diverged' } | null {
|
||||
export function resolveBase(ref: string, cwd: string, head?: string): { ref: string; sha: string; local?: 'missing' | 'unrelated' | 'behind' | 'ahead' | 'diverged' } | null {
|
||||
const related = (sha: string) => !head || mergeBase(sha, head, cwd) !== null;
|
||||
const direct = resolvesToCommit(ref, cwd);
|
||||
if (isBranchName(ref)) {
|
||||
@@ -103,7 +103,8 @@ export function resolveBase(ref: string, cwd: string, head?: string): { ref: str
|
||||
const sha = resolvesToCommit(tracking, cwd);
|
||||
if (!sha || !related(sha)) continue;
|
||||
if (sha === direct) return { ref, sha };
|
||||
const local = !direct ? 'missing' : !related(direct) ? 'unrelated' : mergeBase(direct, sha, cwd) === direct ? 'behind' : 'diverged';
|
||||
const fork = direct && related(direct) ? mergeBase(direct, sha, cwd) : null;
|
||||
const local = !direct ? 'missing' : !fork ? 'unrelated' : fork === direct ? 'behind' : fork === sha ? 'ahead' : 'diverged';
|
||||
return { ref: tracking, sha, local };
|
||||
}
|
||||
}
|
||||
|
||||
+13
-1
@@ -32,7 +32,7 @@ function integrate(cwd: string, base: string, r: Report): boolean {
|
||||
if (!branch || branch === base || branch === head) { r.stop(`on the base branch (${branch || 'detached HEAD'}): ship from a feature branch`); return false; }
|
||||
const status = git(cwd, ['status', '--porcelain']).stdout.split('\n').filter(Boolean);
|
||||
r.add(`STATUS: modified=${status.filter(l => !l.startsWith('??')).length} untracked=${status.filter(l => l.startsWith('??')).length} (uncommitted changes ship with this run)`);
|
||||
const fetch = git(cwd, ['fetch', 'origin', base], 300_000);
|
||||
const fetch = git(cwd, ['fetch', 'origin', `+refs/heads/${base}:refs/remotes/origin/${base}`], 300_000);
|
||||
if (fetch.status !== 0) { r.stop(`git fetch origin ${base} failed: ${firstLine(fetch.stderr)}; restore access first`); return false; }
|
||||
const stat = git(cwd, ['diff', forkPoint(cwd, base), '--shortstat']).stdout.trim();
|
||||
const lines = [...stat.matchAll(/(\d+) (insertion|deletion)/g)].reduce((n, m) => n + Number(m[1]), 0);
|
||||
@@ -43,6 +43,7 @@ function integrate(cwd: string, base: string, r: Report): boolean {
|
||||
if (!pr) r.add('PR: not checked (origin is a local path, not a hosted remote gh can map; Step 0 uses git-native commands)');
|
||||
else if (pr.status !== 0) r.add(`CARRY: not checked (gh pr list failed: ${firstLine(pr.stderr)})`);
|
||||
else if (/^\d+$/.test(n)) {
|
||||
if (!prSized(cwd, base, n, r)) return false;
|
||||
const carry = bin(cwd, 'gstack-ship-receipt', ['carry', '--pr', n, '--base', base, '--dry-run'], 120_000);
|
||||
const line = (carry.stdout.match(/^CARRY: .*$/m) ?? [`CARRY: check failed (${firstLine(carry.stderr)})`])[0];
|
||||
r.add(`PR: #${n}`).add(line);
|
||||
@@ -64,6 +65,17 @@ function integrate(cwd: string, base: string, r: Report): boolean {
|
||||
return merge(cwd, base, r);
|
||||
}
|
||||
|
||||
/** The diff against origin/<base> must be about the PR's size: far more files (a stale or wrong base, HARN-15's 17,000-file review of main's history) stops before any reader runs. Checked only when HEAD is the PR's pushed head. */
|
||||
function prSized(cwd: string, base: string, n: string, r: Report): boolean {
|
||||
const view = gh(cwd, ['pr', 'view', n, '--json', 'changedFiles,headRefOid,baseRefName', '--jq', '"\\(.changedFiles) \\(.headRefOid) \\(.baseRefName)"']);
|
||||
const [pr, prHead, prBase] = view.status === 0 ? view.stdout.trim().split(' ') : [];
|
||||
if (!pr || !/^\d+$/.test(pr) || prHead !== git(cwd, ['rev-parse', 'HEAD']).stdout.trim()) return true;
|
||||
const local = git(cwd, ['diff', '--name-only', forkPoint(cwd, base), 'HEAD']).stdout.split('\n').filter(Boolean).length;
|
||||
if (local <= 2 * Number(pr) + 50) return true;
|
||||
r.stop(`the diff against origin/${base} lists ${local} files but PR #${n} changes ${pr} (its base is ${prBase}): origin/${base} is stale or not the PR's base; rerun with --base ${prBase} after git fetch origin +refs/heads/${prBase}:refs/remotes/origin/${prBase}`);
|
||||
return false;
|
||||
}
|
||||
|
||||
const forkPoint = (cwd: string, base: string) => git(cwd, ['merge-base', `origin/${base}`, 'HEAD']).stdout.trim() || `origin/${base}`;
|
||||
|
||||
function merge(cwd: string, base: string, r: Report): boolean {
|
||||
|
||||
@@ -88,6 +88,11 @@ describe('gstack-diff-scope', () => {
|
||||
expect(out).toContain('SCOPE_DOCS=true');
|
||||
expect(out).toContain('SCOPE_BACKEND=false');
|
||||
}
|
||||
git('symbolic-ref', 'refs/remotes/origin/HEAD', 'refs/remotes/origin/main');
|
||||
expect(spawnSync(SCRIPT, ['HEAD'], { cwd: dir, stdio: 'pipe', timeout: 5000 }).stdout.toString()).toContain('SCOPE_DOCS=false');
|
||||
git('update-ref', 'refs/remotes/origin/main', git('commit-tree', git('rev-parse', 'HEAD^{tree}').stdout.toString().trim(), '-m', 'unrelated').stdout.toString().trim());
|
||||
const unrelated = spawnSync(SCRIPT, ['main'], { cwd: dir, stdio: 'pipe', timeout: 5000 }).stdout.toString();
|
||||
expect(unrelated).toContain('SCOPE_BACKEND=true');
|
||||
});
|
||||
|
||||
test('detects frontend files', () => {
|
||||
|
||||
@@ -153,7 +153,7 @@ describe('gstack-gate-diff arguments and exit contract', () => {
|
||||
// commits the base already has (merged into the branch from origin) do not count as the branch's change (GST-462,
|
||||
// GBRA-218); fails_when=resolveBase returns the local ref whenever it resolves and shares history with HEAD; seam=a
|
||||
// tracking ref moved with update-ref, as a fetch would; no_claim=nothing about remotes other than origin.
|
||||
test('a local main related to HEAD but behind origin/main yields to origin/main and says so', () => {
|
||||
test('a local main related to HEAD but behind, ahead of or diverged from origin/main yields to origin/main and says which', () => {
|
||||
const src = repo({ fixture: 'rh1-skip', commit: true });
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-gate-diff-behind-'));
|
||||
repos.push(dir);
|
||||
@@ -174,6 +174,15 @@ describe('gstack-gate-diff arguments and exit contract', () => {
|
||||
const current = run(dir, ['main']);
|
||||
expect(summary(current.stdout).tagged).toBe('');
|
||||
expect(current.stderr).not.toContain('base is origin/main');
|
||||
const commitOn = (ref: string, name: string) => {
|
||||
const tree = gitIn(dir, ['rev-parse', `${ref}^{tree}`]).trim();
|
||||
const sha = gitIn(dir, ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit-tree', tree, '-p', ref, '-m', name]).trim();
|
||||
gitIn(dir, ['update-ref', ref, sha]);
|
||||
};
|
||||
commitOn('refs/heads/main', 'unpushed');
|
||||
expect(run(dir, ['main']).stderr).toContain('gstack-gate-diff: local main is ahead of origin/main; base is origin/main');
|
||||
commitOn('refs/remotes/origin/main', 'upstream moved');
|
||||
expect(run(dir, ['main']).stderr).toContain('gstack-gate-diff: local main has diverged from origin/main; base is origin/main');
|
||||
});
|
||||
|
||||
// Value: protects=a clone whose only remote is not called origin resolves the base through that remote and the fix line
|
||||
|
||||
@@ -85,6 +85,40 @@ describe('gstack-ship-preflight', () => {
|
||||
expect(lineOf(r.stdout, 'POLICY')).toContain('queue_mode=stamp-at-merge');
|
||||
});
|
||||
|
||||
// Value: protects=Step 1 refreshes origin/<base> itself, so a clone whose fetch refspec leaves out the base (a single-branch
|
||||
// clone) still diffs against the current base tip (GST-462, HARN-15); fails_when=preflight runs `git fetch origin <base>`,
|
||||
// which then updates only FETCH_HEAD; seam=remote.origin.fetch; no_claim=remotes other than origin
|
||||
test('a single-branch clone: the fetch refreshes origin/<base>, so DIFF is the branch alone', () => {
|
||||
const f = fx({ '.gstack/ship-policy.json': '{"queue_mode":"stamp-at-merge"}\n' });
|
||||
f.git('config', 'remote.origin.fetch', '+refs/heads/feat:refs/remotes/origin/feat');
|
||||
f.advanceBase({ 'docs/new.md': '# new\n' });
|
||||
f.git('fetch', '-q', f.origin, 'main');
|
||||
f.git('merge', '-q', '--no-edit', 'FETCH_HEAD');
|
||||
const stale = f.git('rev-parse', 'origin/main').stdout.trim();
|
||||
const r = f.run(PREFLIGHT, ['--base', 'main']);
|
||||
expect(r.status).toBe(0);
|
||||
expect(f.git('rev-parse', 'origin/main').stdout.trim()).not.toBe(stale);
|
||||
expect(lineOf(r.stdout, 'DIFF')).toMatch(/^DIFF: 1 file changed, 1 insertion\(\+\); commits=2$/);
|
||||
});
|
||||
|
||||
// Value: protects=a diff far larger than the PR's own (a stale or wrong base: HARN-15 reviewed 17,000 files of main's history)
|
||||
// stops before any reader runs, naming the PR's base; fails_when=preflight never compares the diff with the PR's changed
|
||||
// files; seam=gh pr view (changedFiles, headRefOid, baseRefName); no_claim=GitHub's own count
|
||||
test('a diff far beyond the PR\'s changed files stops at the pushed head; another head or a matching size goes on', () => {
|
||||
const files = Object.fromEntries(Array.from({ length: 60 }, (_, i) => [`src/f${i}.ts`, `export const f${i} = ${i};\n`]));
|
||||
const f = fx({}, files);
|
||||
const head = f.git('rev-parse', 'HEAD').stdout.trim();
|
||||
const stub = (changed: number, prHead: string) => f.gh(`case "$*" in *"pr list"*) echo 7 ;; *changedFiles*) echo "${changed} ${prHead} release" ;; esac`);
|
||||
stub(3, head);
|
||||
const r = f.run(PREFLIGHT, ['--base', 'main']);
|
||||
expect(r.status).toBe(3);
|
||||
expect(r.stdout).toContain('STOP: the diff against origin/main lists 60 files but PR #7 changes 3 (its base is release): origin/main is stale or not the PR\'s base; rerun with --base release');
|
||||
stub(3, 'f'.repeat(40));
|
||||
expect(f.run(PREFLIGHT, ['--base', 'main']).stdout).not.toContain('lists 60 files');
|
||||
stub(60, head);
|
||||
expect(f.run(PREFLIGHT, ['--base', 'main']).stdout).not.toContain('lists 60 files');
|
||||
});
|
||||
|
||||
// Value: protects=Step 1's scope check reads the fetched origin/<base>, so a clone without a local base branch still sees frontend files; fails_when=the preflight reports frontend=false because gstack-diff-scope could not resolve a local <base>;
|
||||
// why_new=GSTA-167 dogfood on #3633: a Capy checkout has no local main and SCOPE printed false; seam=gstack-diff-scope; no_claim=design review itself
|
||||
test('no local base branch: SCOPE still reads the fetched base', () => {
|
||||
|
||||
Reference in new issue
Block a user