Skip to content

Commit 6a252d2

Browse files
test22345claude
andcommitted
fix(land): resolve rebased merge SHA by patch-id + honor GSTACK_HOME (codex review)
Codex second-opinion findings on the wave diff, both in garrytan#1815's bin/gstack-merge: - P1: the rebase-null edge recorded origin/<base> tip as the landed SHA, which can belong to a LATER change — deploy-matching and reverts could target the wrong commit. Now resolves the rebased commit by patch-id among the 50 newest base commits; when no unique match exists, the state records sha_exact:false, read-state emits LAND_SHA_EXACT=0, and land-and-deploy refuses SHA-based deploy-matching and reverts. - P2: landing state read/wrote ~/.gstack unconditionally; now honors GSTACK_HOME like every other gstack state tool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent b3b842b commit 6a252d2

5 files changed

Lines changed: 80 additions & 14 deletions

File tree

‎bin/gstack-merge‎

Lines changed: 47 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ import {
4141
type PrCheck,
4242
} from '../lib/merge';
4343

44+
/** State root: honor GSTACK_HOME like every other gstack state tool. */
45+
function gstackHome(): string {
46+
return process.env.GSTACK_HOME || join(homedir(), '.gstack');
47+
}
48+
4449
// --- arg parsing --------------------------------------------------------
4550

4651
function parseArgs(argv: string[]): { cmd: string; flags: Record<string, string>; bools: Set<string> } {
@@ -249,7 +254,7 @@ function submitViaRest(pr: number, base: string, priority?: string): { code: num
249254

250255
/** Write last-enqueue.json — a queued marker (no merge SHA yet). */
251256
function writeEnqueueState(slug: string, pr: number, repo: string, regime: Regime, base: string): string {
252-
const dir = join(homedir(), '.gstack', 'projects', slug);
257+
const dir = join(gstackHome(), 'projects', slug);
253258
mkdirSync(dir, { recursive: true });
254259
const dest = join(dir, 'last-enqueue.json');
255260
const tmp = `${dest}.${process.pid}.tmp`;
@@ -408,6 +413,30 @@ async function cmdWait(flags: Record<string, string>): Promise<number> {
408413
return 1;
409414
}
410415

416+
/**
417+
* Rebase merges rewrite the PR head commit onto base with the same patch
418+
* content but a new SHA. Find that commit among the newest base commits by
419+
* patch-id. Returns null unless exactly one recent commit matches.
420+
*/
421+
function resolveRebasedSha(base: string, headRefOid: string): string | null {
422+
if (!/^[0-9a-f]{7,40}$/i.test(headRefOid)) return null;
423+
if (!/^[\w./-]+$/.test(base)) return null;
424+
const patchIdOf = (rev: string): string => {
425+
const r = spawnSync('bash', ['-c', `git show "$1" | git patch-id --stable`, '--', rev], {
426+
encoding: 'utf-8',
427+
timeout: 60000,
428+
});
429+
return ((r.stdout || '').trim().split(' ')[0]) || '';
430+
};
431+
const want = patchIdOf(headRefOid);
432+
if (!want) return null;
433+
const commits = run('git', ['rev-list', '--max-count=50', `origin/${base}`]).stdout
434+
.split('\n')
435+
.filter(Boolean);
436+
const matches = commits.filter((c) => patchIdOf(c) === want);
437+
return matches.length === 1 ? matches[0] : null;
438+
}
439+
411440
async function cmdWriteState(flags: Record<string, string>): Promise<number> {
412441
const pr = flags.pr;
413442
const base = flags.base;
@@ -442,16 +471,26 @@ async function cmdWriteState(flags: Record<string, string>): Promise<number> {
442471

443472
let sha = oid || '';
444473
let shaNote = '';
474+
let shaExact = true;
445475
if (!sha) {
446-
// rebase-null edge: use the base tip as the best available revert target.
447-
const tip = run('git', ['rev-parse', `origin/${base}`]).stdout;
448-
sha = tip;
449-
shaNote = ' (no mergeCommit.oid — using base tip; rebase-merge repo)';
476+
// rebase-null edge: the PR's commits were rewritten onto base. Resolve the
477+
// rebased commit by patch-id; the base tip is NOT this PR's commit (later
478+
// pushes may already be on base) so it is only an inexact last resort.
479+
const resolved = resolveRebasedSha(base, headRefOid);
480+
if (resolved) {
481+
sha = resolved;
482+
shaNote = ' (no mergeCommit.oid — rebased commit resolved by patch-id)';
483+
} else {
484+
sha = run('git', ['rev-parse', `origin/${base}`]).stdout;
485+
shaExact = false;
486+
shaNote = ' (no mergeCommit.oid — base tip, INEXACT: do not deploy-match or revert by this SHA)';
487+
}
450488
}
451489

452490
const state2 = buildLandState({
453491
pr: Number(pr),
454492
sha,
493+
sha_exact: shaExact,
455494
headRefOid,
456495
base,
457496
head_branch: v.headRefName || '',
@@ -460,7 +499,7 @@ async function cmdWriteState(flags: Record<string, string>): Promise<number> {
460499
ts: new Date().toISOString(),
461500
});
462501

463-
const dir = join(homedir(), '.gstack', 'projects', slug);
502+
const dir = join(gstackHome(), 'projects', slug);
464503
mkdirSync(dir, { recursive: true });
465504
const dest = join(dir, 'last-land.json');
466505
const tmp = `${dest}.${process.pid}.tmp`;
@@ -477,7 +516,7 @@ function cmdReadState(flags: Record<string, string>, bools: Set<string>): number
477516
const pr = flags.pr;
478517
const repo = flags.repo;
479518
if (!slug || !pr || !repo) { process.stderr.write('read-state: --slug, --pr, --repo are required\n'); return 2; }
480-
const dest = join(homedir(), '.gstack', 'projects', slug, 'last-land.json');
519+
const dest = join(gstackHome(), 'projects', slug, 'last-land.json');
481520
let state: any = null;
482521
if (existsSync(dest)) {
483522
try { state = JSON.parse(readFileSync(dest, 'utf-8')); } catch { state = null; }
@@ -494,7 +533,7 @@ function cmdReadState(flags: Record<string, string>, bools: Set<string>): number
494533
if (bools.has('json')) {
495534
process.stdout.write(JSON.stringify(state) + '\n');
496535
} else {
497-
process.stdout.write(`LAND_SHA=${state.sha}\nLAND_BASE=${state.base}\nLAND_REGIME=${state.regime}\nLAND_HEAD=${state.head_branch}\n`);
536+
process.stdout.write(`LAND_SHA=${state.sha}\nLAND_SHA_EXACT=${state.sha_exact === false ? 0 : 1}\nLAND_BASE=${state.base}\nLAND_REGIME=${state.regime}\nLAND_HEAD=${state.head_branch}\n`);
498537
}
499538
return 0;
500539
}

‎land-and-deploy/SKILL.md‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,9 +1237,12 @@ REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)
12371237
- **`READ_STATE_INVALID=...`** (no / stale / wrong-PR / wrong-repo handoff): **STOP.**
12381238
"I couldn't confirm this PR actually landed (no valid landing record). I won't deploy
12391239
off an unconfirmed merge. Re-run `/land` and check why it didn't complete."
1240-
- **`LAND_SHA=... LAND_BASE=... LAND_REGIME=... LAND_HEAD=...`**: capture these. `LAND_SHA`
1241-
is the merge commit on the base branch — every deploy-workflow match and any revert
1242-
uses it.
1240+
- **`LAND_SHA=... LAND_SHA_EXACT=... LAND_BASE=... LAND_REGIME=... LAND_HEAD=...`**: capture
1241+
these. `LAND_SHA` is the merge commit on the base branch — every deploy-workflow match and
1242+
any revert uses it. **If `LAND_SHA_EXACT=0`** the SHA is only the base tip at landing time
1243+
(rebase merge whose exact commit couldn't be resolved): still verify ancestry in 2.2, but do
1244+
NOT match deploy workflows by this SHA (match by workflow name + timing instead) and do NOT
1245+
offer a SHA-based revert — tell the user to identify the PR's commit manually first.
12431246

12441247
### 2.2: Verify the SHA is really on the base branch (H2)
12451248

@@ -1513,6 +1516,9 @@ If the user chose to revert at any point:
15131516
Tell the user: "Reverting the merge now. This will create a new commit that undoes all the changes from this PR. The previous version of your site will be restored once the revert deploys."
15141517

15151518
Use `LAND_SHA` (the confirmed merge commit from Step 2) as the revert target.
1519+
**If `LAND_SHA_EXACT=0`: STOP first** — the recorded SHA is only the base tip at landing
1520+
time and may belong to a different change. Show the user `git log origin/<base> --oneline -10`,
1521+
have them confirm which commit is this PR's, and revert that commit instead.
15161522

15171523
**Merge-queue / protected branches first (H8).** If `LAND_REGIME` is `github` or `trunk`,
15181524
a direct push to the base branch is almost always blocked by branch protection, so go

‎land-and-deploy/SKILL.md.tmpl‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -338,9 +338,12 @@ REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)
338338
- **`READ_STATE_INVALID=...`** (no / stale / wrong-PR / wrong-repo handoff): **STOP.**
339339
"I couldn't confirm this PR actually landed (no valid landing record). I won't deploy
340340
off an unconfirmed merge. Re-run `/land` and check why it didn't complete."
341-
- **`LAND_SHA=... LAND_BASE=... LAND_REGIME=... LAND_HEAD=...`**: capture these. `LAND_SHA`
342-
is the merge commit on the base branch — every deploy-workflow match and any revert
343-
uses it.
341+
- **`LAND_SHA=... LAND_SHA_EXACT=... LAND_BASE=... LAND_REGIME=... LAND_HEAD=...`**: capture
342+
these. `LAND_SHA` is the merge commit on the base branch — every deploy-workflow match and
343+
any revert uses it. **If `LAND_SHA_EXACT=0`** the SHA is only the base tip at landing time
344+
(rebase merge whose exact commit couldn't be resolved): still verify ancestry in 2.2, but do
345+
NOT match deploy workflows by this SHA (match by workflow name + timing instead) and do NOT
346+
offer a SHA-based revert — tell the user to identify the PR's commit manually first.
344347

345348
### 2.2: Verify the SHA is really on the base branch (H2)
346349

@@ -581,6 +584,9 @@ If the user chose to revert at any point:
581584
Tell the user: "Reverting the merge now. This will create a new commit that undoes all the changes from this PR. The previous version of your site will be restored once the revert deploys."
582585

583586
Use `LAND_SHA` (the confirmed merge commit from Step 2) as the revert target.
587+
**If `LAND_SHA_EXACT=0`: STOP first** — the recorded SHA is only the base tip at landing
588+
time and may belong to a different change. Show the user `git log origin/<base> --oneline -10`,
589+
have them confirm which commit is this PR's, and revert that commit instead.
584590

585591
**Merge-queue / protected branches first (H8).** If `LAND_REGIME` is `github` or `trunk`,
586592
a direct push to the base branch is almost always blocked by branch protection, so go

‎lib/merge.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,13 @@ export interface LandState {
228228
schema_version: number;
229229
pr: number;
230230
sha: string;
231+
/**
232+
* False when the merge SHA could not be resolved exactly (rebase merge with
233+
* no mergeCommit.oid and no unique patch-id match) and `sha` is only the
234+
* base tip at landing time. Consumers must not deploy-match or revert by an
235+
* inexact SHA. Absent means exact (pre-field states).
236+
*/
237+
sha_exact?: boolean;
231238
headRefOid: string;
232239
base: string;
233240
head_branch: string;
@@ -239,6 +246,8 @@ export interface LandState {
239246
export interface BuildLandStateInput {
240247
pr: number;
241248
sha: string;
249+
/** See LandState.sha_exact. Defaults to true. */
250+
sha_exact?: boolean;
242251
headRefOid: string;
243252
base: string;
244253
head_branch: string;
@@ -257,6 +266,7 @@ export function buildLandState(input: BuildLandStateInput): LandState {
257266
schema_version: LAND_STATE_SCHEMA_VERSION,
258267
pr: input.pr,
259268
sha: input.sha,
269+
sha_exact: input.sha_exact !== false,
260270
headRefOid: input.headRefOid,
261271
base: input.base,
262272
head_branch: input.head_branch,

‎test/gstack-merge.test.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,11 @@ describe('buildLandState', () => {
208208
test('refuses to build with an empty SHA (handoff would silently kill revert)', () => {
209209
expect(() => buildLandState({ ...base, sha: '' })).toThrow(/empty merge SHA/);
210210
});
211+
212+
test('sha_exact defaults true and false survives the round trip', () => {
213+
expect(buildLandState(base).sha_exact).toBe(true);
214+
expect(buildLandState({ ...base, sha_exact: false }).sha_exact).toBe(false);
215+
});
211216
});
212217

213218
describe('validateConsume', () => {

0 commit comments

Comments
 (0)