From b8373e2bfc66bfea7bc06486ba31bf231995bc6f Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 26 Oct 2026 16:38:08 +0000 Subject: [PATCH] fix: pre-landing review fixes (pass 2) An unclosed receipt fence makes the body from that line on one section the merge never matches, so posted leading prose cannot replace it and a closed receipt before it stays a receipt; the by-tree pregate mirror must hold the tests stage (what --stage tests --remote writes) rather than every check; empty sha/tree strings in a train note are not evidence; (v1.2.3-rc1 #301) is a version word, not a repository. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/pregate.md | 7 ++++--- lib/gstack-post.ts | 13 +++++++------ lib/issue-links.ts | 2 +- lib/ship-queue.ts | 18 ++++++++++-------- scripts/release-cut.ts | 4 ++-- test/gstack-post.test.ts | 16 +++++++++++++++- test/gstack-ship-queue.test.ts | 14 +++++++------- test/release-cut.test.ts | 2 +- test/ship-issue-intent.test.ts | 2 +- 10 files changed, 49 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6fc97f70..a1104f8d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - **`lib/issue-links.ts`**: a parenthesised named reference, `(name #N[, #M]…)`, `(name PR #N)` or `(name issue #N)`, where the name is one token that is not a closure, reference, filler or version word (fix, fixes, fixing, closes, closing, resolves, addresses, implements, supersedes, refs, see, related, part, follow-up, deferred, PR, issue, bug, upstream, cf., e.g., i.e., of, to, the, v1.2, …), is a cross-repo reference to `name` (`Refs name#N`), never a same-repo Refs line or a `fix #N` promotion candidate. `(see #12)`, `(closing #12)`, `(cf. #12)`, `(#301)` and `Fixes #12` keep their meaning; the `owner/repo#N` form is unchanged. - **`lib/gstack-post.ts`**: an 11-digit GitHub Actions run id right after a run word on the same line (run, runs, screen, screens, gate, gates, backstop, bisect, rerun, dispatched, dispatch, failed, passed, with an optional `:` `#` `=`; a generic word such as id, job, attempt or workflow exempts nothing), and every id chained after an allowlisted one through `, `, ` and `, ` & `, ` or ` or ` / ` (a `(#1234)` between allowed), is exempt from `pii.phone.e164`. `failed 40269992077 | rerun passed 40287239206`, `screen 40269992077 (#3434) and 40275443393 (#3453)` and `gate 40323989742` post without `--confirm`. A bare 11-digit number, a 10-digit national number and `+14155551234` still report. - **`lib/gstack-post.ts`** (`pr-body`): the body file is merged by `## ` section into the PR's current body (read with `gh pr view --json body`, or `glab mr view`, through the same runner) instead of replacing it. A section in the file replaces the same-titled section in place (case-insensitive), the ````gstack-ship-receipt```` block is a section of its own so a receipt rewrite replaces only the receipt, sections the file does not rewrite are kept byte for byte, new sections are appended after them, and a current body with no `## ` headings is kept as the leading text. A `## ` line inside a code fence is not a section: a fence closes only on a run of its own character at least as long as the opener with nothing after it, so a ``` line inside a ```` block or a ```js line closes nothing, and a receipt fence that never closes is an unclosed fence (the body after it stays one kept section) rather than a receipt that swallows `## Approvals`. The file's leading text replaces the current leading text; text after a receipt block never replaces an intro; whitespace-only text in the file is dropped, so reposting the body /ship composes (receipt, blank line, sections) is byte-stable. One stderr line names the replaced, kept and added sections. `--replace` keeps the plain overwrite for a caller that composed the whole body. The redaction scan and the confirmation token cover the merged bytes, which are the bytes sent. A current body that cannot be read posts nothing (exit 3) and names `--replace`. -- **`lib/pregate/index.ts`**, **`lib/ship-queue.ts`**: `gstack-pregate` mirrors every record it writes to `/pregate/by-tree/.json` (best effort; a mirror failure never fails the record). `ready` reads the checkout's `.gstack/tmp/pregate.json` (or `--pregate`) first; when it is missing, unreadable, recorded for another tree, or current but still waiting on a remote receipt, it looks the current tree up in the mirror and uses that record when it verifies, printing `PREGATE: … (from the state root's pregate/by-tree/.json, recorded by another checkout)`. A lane whose `gstack-pregate --remote` had to run in a separate clone (a worktree cannot host it) no longer gets `PREGATE_STALE`, `PREGATE_REMOTE_UNCLEARED` or `missing` from its own checkout. Only a complete run stands in: a mirror that misses a registered check (an `--only` or single-stage run), was recorded against another base, or ran under `--policy-from head` is named on the `PREGATE:` line and never certifies READY; a record whose own checks failed or stayed incomplete is never overridden. A mirror for another tree changes nothing; the mirror is written through a temp file and rename. +- **`lib/pregate/index.ts`**, **`lib/ship-queue.ts`**: `gstack-pregate` mirrors every record it writes to `/pregate/by-tree/.json` (best effort; a mirror failure never fails the record). `ready` reads the checkout's `.gstack/tmp/pregate.json` (or `--pregate`) first; when it is missing, unreadable, recorded for another tree, or current but still waiting on a remote receipt, it looks the current tree up in the mirror and uses that record when it verifies, printing `PREGATE: … (from the state root's pregate/by-tree/.json, recorded by another checkout)`. A lane whose `gstack-pregate --remote` had to run in a separate clone (a worktree cannot host it) no longer gets `PREGATE_STALE`, `PREGATE_REMOTE_UNCLEARED` or `missing` from its own checkout. Only a run that holds the tests stage stands in (what `--stage tests --remote` writes): a mirror without the `lanes` check (an `--only timing` or preflight-only run), one recorded against another base, or one run under `--policy-from head` is named on the `PREGATE:` line and never certifies READY; a record whose own checks failed or stayed incomplete is never overridden. A mirror for another tree changes nothing; the mirror is written through a temp file and rename. ### Docs - `docs/releases.md`: the release-gate paragraph describes both kinds of evidence, what train evidence does not relax, the `CANDIDATE` wording and the three flags. `docs/pregate.md`: the by-tree mirror and how `ready` reads it. diff --git a/docs/pregate.md b/docs/pregate.md index 3af4d2f24..60c4bda42 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -284,9 +284,10 @@ printing `PREGATE: … (from the state root's pregate/by-tree/.json, recorded by another checkout)`. This is for the lane whose `gstack-pregate --remote` had to run from a separate clone (a worktree cannot host it): the clone's record clears the lane's own checkout as long as both hold the same -tree. Only a complete run stands in: the mirror must name every registered -check (so a `--only` or single-stage run is "a partial run without …"), be -recorded against `origin/` and not under `--policy-from head`; +tree. Only a run that holds the tests stage stands in (what `--stage tests +--remote` writes, `checks=[lanes]`; a `--only timing` or preflight-only run +is "a run without the tests stage"), and it must be recorded against +`origin/` and not under `--policy-from head`; otherwise the `PREGATE:` line says why the mirror cannot stand in. A record whose own checks failed or stayed incomplete is final and no mirror overrides it. An explicit `--pregate ` that verifies is used as before; a mirror diff --git a/lib/gstack-post.ts b/lib/gstack-post.ts index 7e3829c84..0bd643598 100644 --- a/lib/gstack-post.ts +++ b/lib/gstack-post.ts @@ -145,10 +145,11 @@ interface Section { key: string; label: string; text: string } * a key of its own, so it is never replaced. A fence closes only on a run of * its own character at least as long as the opener with nothing after it * (CommonMark), so a ``` line inside a ```` block and a ```js line close - * nothing; a receipt fence that never closes is an ordinary unclosed fence, - * so the rest of the body stays one kept section instead of a receipt. + * nothing; a receipt fence that never closes makes the body from that line + * on one section the merge never matches (`unclosed-receipt`), so it is kept + * byte for byte instead of being read as a receipt or as replaceable text. */ -function splitSections(body: string, receipts = true): Section[] { +function splitSections(body: string): Section[] { const out: Section[] = []; let fence: string | null = null; let plain = 0; @@ -159,7 +160,7 @@ function splitSections(body: string, receipts = true): Section[] { for (const line of body.split(/(?<=\n)/)) { bare = line.replace(/\r?\n$/, ""); const fenceOpen: RegExpExecArray | null = fence === null ? /^ {0,3}(`{3,}|~{3,})/.exec(bare) : null; - if (fenceOpen && receipts && bare.trim() === "```" + RECEIPT_FENCE) { open("receipt", "ship receipt"); fence = "receipt"; } + if (fenceOpen && bare.trim() === "```" + RECEIPT_FENCE) { open("receipt", "ship receipt"); fence = "receipt"; } else if (fence === "receipt") { out.at(-1)!.text += line; if (closes("```")) { fence = null; openText(); } continue; } else if (fenceOpen) fence = fenceOpen[1]!; else if (fence && closes(fence)) fence = null; @@ -167,7 +168,7 @@ function splitSections(body: string, receipts = true): Section[] { if (out.length === 0) openText(); out.at(-1)!.text += line; } - if (fence === "receipt") return splitSections(body, false); + if (fence === "receipt") Object.assign(out.at(-1)!, { key: "unclosed-receipt", label: "unclosed receipt fence" }); return out.filter(s => s.text !== ""); } @@ -190,7 +191,7 @@ export function mergeBodySections(current: string, incoming: string): { body: st const used = new Set
(); const replaced: string[] = [], kept: string[] = [], added: string[] = []; const merged: Section[] = now.map(s => { - const match = next.find(n => !used.has(n) && n.key === s.key); + const match = s.key === "unclosed-receipt" ? undefined : next.find(n => !used.has(n) && n.key === s.key); if (!match) { if (s.text.trim()) kept.push(s.label); return s; } used.add(match); replaced.push(s.label); return match; }); diff --git a/lib/issue-links.ts b/lib/issue-links.ts index fcc7e3572..f167f9bf4 100644 --- a/lib/issue-links.ts +++ b/lib/issue-links.ts @@ -48,7 +48,7 @@ const BARE = /(? !file.checks.some(c => c.id === id)); - return missing.length ? `a partial run without ${missing.join(', ')}` : null; + const missing = CHECKS.filter(c => c.stage === 'tests' && !file.checks.some(k => k.id === c.id)).map(c => c.id); + return missing.length ? `a run without the tests stage (${missing.join(', ')})` : null; } function gateLine(policy: LoadedPolicy, attempt: AttemptRecord | null): string { diff --git a/scripts/release-cut.ts b/scripts/release-cut.ts index 055dec0a1..763c49713 100755 --- a/scripts/release-cut.ts +++ b/scripts/release-cut.ts @@ -139,8 +139,8 @@ export function readTrainNotes(git: Git, notesRef = NOTES_REF): TrainNotes { if (r.kind !== 'train' || typeof r.nonce !== 'string' || !r.nonce || !['green', 'merging', 'merged'].includes(r.state ?? '')) continue; for (const m of Array.isArray(r.merged) ? r.merged : []) { if (!m || typeof m !== 'object') continue; - if (typeof m.sha === 'string') notes.evidence.bySha.set(m.sha, r.nonce); - if (typeof m.tree === 'string') notes.evidence.byTree.set(m.tree, r.nonce); + if (typeof m.sha === 'string' && m.sha) notes.evidence.bySha.set(m.sha, r.nonce); + if (typeof m.tree === 'string' && m.tree) notes.evidence.byTree.set(m.tree, r.nonce); } } return notes; diff --git a/test/gstack-post.test.ts b/test/gstack-post.test.ts index 9ad6c8506..6dff8463f 100644 --- a/test/gstack-post.test.ts +++ b/test/gstack-post.test.ts @@ -361,7 +361,21 @@ describe("gstack-post pr-body merges the PR body by section", () => { const open = withBody(unclosed, { "/b": RECEIPT("bbbb") + "\n## Summary\n\nnew\n" }); expect(run(["pr-body", "5", "--body-file", "/b"], open)).toBe(EXIT.posted); expect(sent(open).input).toBe(RECEIPT("bbbb") + "\n" + unclosed + "\n## Summary\n\nnew\n"); - expect(open.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept leading text; added ship receipt, Summary\n"); + expect(open.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept unclosed receipt fence; added ship receipt, Summary\n"); + + const prose = withBody(unclosed, { "/b": "Updated introduction\n\n## Summary\n\nnew\n" }); + expect(run(["pr-body", "5", "--body-file", "/b"], prose)).toBe(EXIT.posted); + expect(sent(prose).input).toBe("Updated introduction\n\n" + unclosed + "\n## Summary\n\nnew\n"); + + const inside = withBody("Intro\n\n## Notes\n\n" + unclosed, { "/b": "New intro\n\n## Notes\n\nnew notes\n\n## Approvals\n\n- owner: still\n" }); + expect(run(["pr-body", "5", "--body-file", "/b"], inside)).toBe(EXIT.posted); + expect(sent(inside).input).toBe("New intro\n\n## Notes\n\nnew notes\n\n" + unclosed + "\n## Approvals\n\n- owner: still\n"); + expect(inside.stderr()).toBe("gstack-post: pr-body merged by section: replaced leading text, Notes; kept unclosed receipt fence; added Approvals\n"); + + const after = withBody(RECEIPT("aaaa") + "\n## Why\n\nold\n\n" + unclosed, { "/b": RECEIPT("bbbb") + "\n## Why\n\nnew\n" }); + expect(run(["pr-body", "5", "--body-file", "/b"], after)).toBe(EXIT.posted); + expect(sent(after).input).toBe(RECEIPT("bbbb") + "\n## Why\n\nnew\n\n" + unclosed); + expect(after.stderr()).toBe("gstack-post: pr-body merged by section: replaced ship receipt, Why; kept unclosed receipt fence; added none\n"); }); // Value: protects=--replace keeps the plain overwrite for a caller that composed the whole body, a current body with no H2 headings is kept as the leading diff --git a/test/gstack-ship-queue.test.ts b/test/gstack-ship-queue.test.ts index bd7120e7b..0aa071f76 100644 --- a/test/gstack-ship-queue.test.ts +++ b/test/gstack-ship-queue.test.ts @@ -293,11 +293,11 @@ describe('ready', () => { expect(stalePregate.status).toBe(EXIT.refused); expect(stalePregate.out).toMatch(/^PREGATE: pass .* \(PREGATE_STALE\)$/m); // A lane that cleared its pregate from a separate clone (gstack-pregate --remote cannot run in a worktree) holds the verdict under - // /pregate/by-tree/.json; ready reads that mirror for the current tree when its own record is stale or missing, and only a - // complete run (every registered check) against origin/main under a reviewed policy stands in: a --only run, another base or a head policy is named. + // /pregate/by-tree/.json; ready reads that mirror for the current tree when its own record is stale or missing, and only a run + // that holds the tests stage (what `--stage tests --remote` writes: checks=[lanes]) against origin/main under a reviewed policy stands in: a + // --only timing run, another base or a head policy is named on the PREGATE line instead. const stateRoot = path.join(repo.root, 'state-root'); - const CHECK_IDS = ['regen', 'secrets', 'strays', 'literals', 'timing', 'guards', 'lanes']; - const complete = { base: `origin/main@${repo.git('rev-parse', 'origin/main').slice(0, 12)}`, policy: 'origin/main@abc (no .gstack/pregate.json)', checks: CHECK_IDS.map(id => ({ id, status: 'pass' })) }; + const complete = { base: `origin/main@${repo.git('rev-parse', 'origin/main').slice(0, 12)}`, policy: 'origin/main@abc (no .gstack/pregate.json)', checks: [{ id: 'lanes', status: 'pass' }] }; const fromSuffix = `(from the state root's pregate/by-tree/${'TREE'}.json, recorded by another checkout)`; const movedShim = ghQueueShim(repo, { prs: { '1': { headRefOid: moved, body: shippedBody('1', moved) } }, checks: { [moved]: green } }); const movedTree = runBin(repo, 'gstack-wtree', []).out.trim(); @@ -312,7 +312,7 @@ describe('ready', () => { expect(missing.out).toMatch(/^PREGATE: missing \(\.gstack\/tmp\/pregate\.json; run gstack-pregate run --allow-repo-commands\)$/m); const suffix = fromSuffix.replace('TREE', movedTree.slice(0, 12)); const partial = { ...complete, checks: [{ id: 'timing', status: 'pass' }] }; - for (const [bad, why] of [[partial, 'a partial run without regen, secrets, strays, literals, guards, lanes'], [{ ...complete, base: 'origin/develop@0123456789ab' }, 'recorded against origin/develop@0123456789ab, not origin/main'], [{ ...complete, policy: 'head (unreviewed) .gstack/pregate.json' }, 'recorded under the unreviewed head policy']] as const) { + for (const [bad, why] of [[partial, 'a run without the tests stage (lanes)'], [{ ...complete, base: 'origin/develop@0123456789ab' }, 'recorded against origin/develop@0123456789ab, not origin/main'], [{ ...complete, policy: 'head (unreviewed) .gstack/pregate.json' }, 'recorded under the unreviewed head policy']] as const) { fs.writeFileSync(mirror, JSON.stringify({ schema_version: 1, tree: movedTree, head: moved, verdict: 'pass', ...bad })); const ignored = queue(repo, ['ready', '--pr', '1'], { ...movedShim, GSTACK_STATE_ROOT: stateRoot }); expect(ignored.status).toBe(EXIT.refused); @@ -321,7 +321,7 @@ describe('ready', () => { fs.writeFileSync(mirror, JSON.stringify({ schema_version: 1, tree: movedTree, head: moved, verdict: 'pass', ...complete })); const fromMirror = queue(repo, ['ready', '--pr', '1', '--pregate', pregate], { ...movedShim, GSTACK_STATE_ROOT: stateRoot }); expect([fromMirror.status, fromMirror.err]).toEqual([EXIT.ok, '']); - expect(line(fromMirror.out, 'PREGATE:')).toBe(`PREGATE: pass tree=${movedTree.slice(0, 12)} head=${moved.slice(0, 12)} checks=7 ${suffix}`); + expect(line(fromMirror.out, 'PREGATE:')).toBe(`PREGATE: pass tree=${movedTree.slice(0, 12)} head=${moved.slice(0, 12)} checks=1 ${suffix}`); const missingFromMirror = queue(repo, ['ready', '--pr', '1'], { ...movedShim, GSTACK_STATE_ROOT: stateRoot }); expect([missingFromMirror.status, missingFromMirror.err]).toEqual([EXIT.ok, '']); expect(line(missingFromMirror.out, 'PREGATE:')).toContain(suffix); @@ -337,7 +337,7 @@ describe('ready', () => { fs.writeFileSync(local, JSON.stringify({ schema_version: 1, tree: movedTree, head: moved, verdict: 'requires-remote', checks: [{ id: 'lanes', status: 'requires-remote' }], requires_remote: [obligation] })); const uncleared = queue(repo, ['ready', '--pr', '1', '--pregate', local], { ...movedShim, GSTACK_STATE_ROOT: stateRoot }); expect([uncleared.status, uncleared.err]).toEqual([EXIT.ok, '']); - expect(line(uncleared.out, 'PREGATE:')).toBe(`PREGATE: pass tree=${movedTree.slice(0, 12)} head=${moved.slice(0, 12)} checks=7 ${suffix}`); + expect(line(uncleared.out, 'PREGATE:')).toBe(`PREGATE: pass tree=${movedTree.slice(0, 12)} head=${moved.slice(0, 12)} checks=1 ${suffix}`); fs.writeFileSync(local, '{ not json'); const unreadable = queue(repo, ['ready', '--pr', '1', '--pregate', local], { ...movedShim, GSTACK_STATE_ROOT: stateRoot }); expect([unreadable.status, unreadable.err]).toEqual([EXIT.ok, '']); diff --git a/test/release-cut.test.ts b/test/release-cut.test.ts index 2d0a199d1..d7b3fd110 100644 --- a/test/release-cut.test.ts +++ b/test/release-cut.test.ts @@ -193,7 +193,7 @@ describe('release-cut: planning', () => { const add = (record: string, on: string) => git(f.seed, 'notes', '--ref=' + NOTES_REF, 'add', '-f', '-m', record, on); add('not json {', f.shas['1.0.0']!); - for (const junk of ['null', '[]', '"train"', '7', JSON.stringify({ kind: 'train', nonce: 'bad1', state: 'green', merged: 'x' }), JSON.stringify({ kind: 'train', nonce: 'bad2', state: 'green', merged: [null, 'y', { sha: 3, tree: ['t'] }] }), JSON.stringify({ kind: 'train', nonce: 9, state: 'green', merged: [{ sha: f.shas['1.0.0'] }] })]) { + for (const junk of ['null', '[]', '"train"', '7', JSON.stringify({ kind: 'train', nonce: 'bad1', state: 'green', merged: 'x' }), JSON.stringify({ kind: 'train', nonce: 'bad2', state: 'green', merged: [null, 'y', { sha: 3, tree: ['t'] }, { sha: '', tree: '' }] }), JSON.stringify({ kind: 'train', nonce: 9, state: 'green', merged: [{ sha: f.shas['1.0.0'] }] })]) { add(junk, f.shas['1.0.0']!); git(f.seed, 'push', '-q', '-f', 'origin', NOTES_REF); const read = readTrainNotes(f.git); diff --git a/test/ship-issue-intent.test.ts b/test/ship-issue-intent.test.ts index 7263993bd..85755b2de 100644 --- a/test/ship-issue-intent.test.ts +++ b/test/ship-issue-intent.test.ts @@ -38,7 +38,7 @@ describe('issue intent (pure)', () => { expect(links).toEqual({ fixes: [], refs: [], flakes: [], cross: ['acme/tools#4', 'gbrain#7606', 'gbrain#7625', 'gbrain#7630'], unreadable: [] }); expect(renderIssueLines(links)).toEqual(['Refs acme/tools#4', 'Refs gbrain#7606', 'Refs gbrain#7625', 'Refs gbrain#7630']); expect(promotionCandidates(named, links)).toEqual([]); - for (const word of ['see', 'fixes', 'refs', 'related', 'part of', 'follow-up', 'closes', 'PR', 'issue', 'closing', 'addresses', 'fixing', 'implements', 'cf.', 'e.g.', 'i.e.', 'bug', 'upstream PR', 'v1.2', 'supersedes', 'duplicate of']) { + for (const word of ['see', 'fixes', 'refs', 'related', 'part of', 'follow-up', 'closes', 'PR', 'issue', 'closing', 'addresses', 'fixing', 'implements', 'cf.', 'e.g.', 'i.e.', 'bug', 'upstream PR', 'v1.2', 'v1.2.3-rc1', 'supersedes', 'duplicate of']) { expect([word, mentionsIn(`retry (${word} #12)`).map(x => [x.repo ?? '', x.issue, x.intent])]).toEqual([word, [['', 12, 'refs']]]); } expect(mentionsIn('v1.2.3 (#301) release notes').map(x => [x.repo ?? '', x.issue])).toEqual([['', 301]]);