diff --git a/.github/workflows/plain-title.yml b/.github/workflows/plain-title.yml index 0807fb9f3..8736d39c0 100644 --- a/.github/workflows/plain-title.yml +++ b/.github/workflows/plain-title.yml @@ -54,14 +54,14 @@ jobs: env: # Attacker-controlled on fork PRs: env only, never inlined into run:. PR_TITLE: ${{ github.event.pull_request.title }} - PR_BODY: ${{ github.event.pull_request.body }} GH_TOKEN: ${{ github.token }} PR_NUM: ${{ github.event.pull_request.number }} REPO: ${{ github.repository }} run: | set -euo pipefail printf '%s' "$PR_TITLE" > "$RUNNER_TEMP/pr-title.txt" - printf '%s' "$PR_BODY" > "$RUNNER_TEMP/pr-body.txt" + # The body comes from the event file: a large body overflows one environment string (E2BIG). + jq -j '.pull_request.body // ""' "$GITHUB_EVENT_PATH" > "$RUNNER_TEMP/pr-body.txt" # Commit subjects are a warning only: skip quietly when the API read fails. if ! gh api --paginate "repos/$REPO/pulls/$PR_NUM/commits" \ --jq '.[] | select((.parents | length) < 2) | .commit.message | split("\n")[0]' \ diff --git a/.gstack/ship-policy.json b/.gstack/ship-policy.json index e8d58459c..0209af766 100644 --- a/.gstack/ship-policy.json +++ b/.gstack/ship-policy.json @@ -14,7 +14,7 @@ "pr_count": { "default_pr_count": 1, "split_requires_owner": true }, "maintainers": ["garrytan"], "train": { "gate_workflow": "evals.yml", "free_workflow": "free-tests.yml", "full_gate_check": "gstack/full-gate", "record_artifact": "gstack-train-record", "max_size": 5, "fill_hours": 1, "screen_selection": "gstack-paid", "trial_reservation_usd": 1.5 }, - "changelog_fragments": { "dir": "changelog.d", "required_checks": ["free-tests", "windows-smoke", "Check VERSION is not stale vs queue", "quality", "check-freshness", "actionlint"] }, + "changelog_fragments": { "dir": "changelog.d", "required_checks": ["free-tests", "windows-smoke", "Check VERSION is not stale vs queue", "quality", "check-freshness", "actionlint", "Plain title"] }, "merge": "direct", "land": { "merged_checks": [ diff --git a/changelog.d/capy-plain-title-required-check-gst-1825.md b/changelog.d/capy-plain-title-required-check-gst-1825.md new file mode 100644 index 000000000..6b4e583f3 --- /dev/null +++ b/changelog.d/capy-plain-title-required-check-gst-1825.md @@ -0,0 +1,4 @@ +- **gstack now waits for the "Plain title" check to pass before it lands a pull request.** The check reads the pull request's description from the event file GitHub hands the workflow, so a very long description no longer breaks it. A pull request that has never had the check run gets one when its title or description is edited, or when new commits are pushed. +- **A ship receipt posted on its own now goes into the pull request's Agent notes section and keeps the opening paragraph where it was.** The new receipt replaces the one in that section, so it is the one gstack reads. +- **An older receipt at the very top of a pull request description is still read after an Agent notes section without a receipt is added below it.** Receipt examples quoted in the readable part of a description stay where they are. +- **Correction to the earlier entry about plain titles:** a repository without `.gstack/plain-titles.json` gets no title checks at all, not warnings. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index af8a29f67..fb0b922ca 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -113,7 +113,7 @@ hard-code: "changelog": "CHANGELOG.md", "history": "squash-ok", "stamp_sensitive_gates": ["tests"], - "changelog_fragments": { "dir": "changelog.d", "required_checks": ["free-tests", "windows-smoke", "Check VERSION is not stale vs queue", "quality", "check-freshness", "actionlint"] }, + "changelog_fragments": { "dir": "changelog.d", "required_checks": ["free-tests", "windows-smoke", "Check VERSION is not stale vs queue", "quality", "check-freshness", "actionlint", "Plain title"] }, "min_gstack_version": "1.91.340.0" } ``` @@ -1257,8 +1257,13 @@ which carry the workflow consequences the skill prose would otherwise spell out: | `ship: changelog_fragments : …` | Step 12 writes no version; Step 13 writes the entry as `/.md` (entry text only, `bump:` front matter only when the change warrants more than the policy bump), never an `[Unreleased]` section, linted with `gstack-prose-lint --gate`; `gstack-ship-queue land` assigns the version; see [changelog fragments](#changelog-fragments) | The ship receipt (`gstack-ship-receipt write … --body`) goes in the PR body's closing Agent notes -section, written by the pr-body section of `/ship` after the body is composed; a body from before that -layout keeps its receipt at the top and is still read. +section, written by the pr-body section of `/ship` after the body is composed. A body from before that +layout keeps its receipt at the top. That receipt is still read while it opens the body, also after the +body gains an Agent notes section with no receipt in it; once an edit or a post puts an opening paragraph +above it, it is no longer read, and the pull request needs a fresh receipt. A receipt posted on its own +with `gstack-post pr-body` to a body that has the section replaces the receipt inside it. In a body with +the section, a receipt block quoted in the readable part stays where it is and is never read as the +receipt; a body without the section is read whole, so there a quoted block can be read as the receipt. ## The ship receipt @@ -2768,9 +2773,10 @@ Codes and fixes: [troubleshooting.md](troubleshooting.md#plain-title-refused). ### Making it a required check -The rollout takes two merges. The first ships the checker, the writers and the -workflow. Once every open PR has a "Plain title" run (an edit to its title or -body starts one), a one-line follow-up adds `"Plain title"` to -`changelog_fragments.required_checks`, and `gstack-ship-queue land` then -refuses a PR whose check is red or missing. Adding it in the first merge would -hold every open PR that has never had a run. +The rollout took two merges. The first shipped the checker, the writers and the +workflow. The second added `"Plain title"` to gstack's +`changelog_fragments.required_checks`, so `gstack-ship-queue land` refuses a +PR whose check is red or missing. A PR opened before the workflow existed has no +run until its title or body is edited or a commit is pushed; do one of those +before landing it. The workflow reads the PR body from the event file, not an +environment variable, so a very long body still reaches the checker. diff --git a/lib/gstack-post.ts b/lib/gstack-post.ts index 02ed12502..507d52b7c 100644 --- a/lib/gstack-post.ts +++ b/lib/gstack-post.ts @@ -31,7 +31,7 @@ import { scan, type Finding, type RepoVisibility } from "./redact-engine"; import { CI_RUN_WORD_BEFORE } from "./redact-patterns"; import { agentNotesLines } from "./agent-notes"; import { checkTitle, loadPlainTitlePolicy, PLAIN_TITLE_POLICY_PATH } from "./plain-title"; -import { RECEIPT_FENCE, extractReceipt } from "./ship-receipt"; +import { RECEIPT_FENCE, RUNS_FENCE, extractReceipt, placeReceipt } from "./ship-receipt"; import { canonicalRemote } from "./remote-identity"; import { parseChangelog } from "./changelog-check"; import { versionWidth } from "./version-source"; @@ -271,11 +271,26 @@ function splitSections(body: string): Section[] { * The Agent notes section, the current one or the file's, always ends the * merged body, above a Capy badge that ends it. Any other file (`## ` * sections with no receipt above them, or no `## ` section) merges as - * described above, so the text under the receipt is kept too. + * described above, so the text under the receipt is kept too. A receipt the + * file brings outside an Agent notes section, posted to a body that has one, + * goes into that section through placeReceipt, so it replaces the receipt + * there instead of landing beside it while the old one still binds. */ export function mergeBodySections(current: string, incoming: string): { body: string; replaced: string[]; kept: string[]; added: string[]; dropped: string[] } { const now = splitSections(current); const next = splitSections(incoming).filter(n => !(n.key.startsWith("text:") && n.text.trim() === "")); + const lone = next.find(n => n.key === "receipt"); + const intoNotes = lone && now.some(s => s.key === NOTES_KEY) && !next.some(n => n.key === NOTES_KEY) ? extractReceipt(lone.text).receipt : undefined; + if (!intoNotes) return mergeSplit(now, next, incoming); + const after = next[next.indexOf(lone!) + 1]; + const rest = next.filter(n => n !== lone).map(n => n !== after || !n.key.startsWith("text:") ? n : { ...n, key: "text:after-receipt", text: n.text.replace(RUNS_BLOCK_RE, "") }); + const m = mergeSplit(now, rest.filter(n => !(n.key.startsWith("text:") && n.text.trim() === "")), incoming); + return { ...m, body: placeReceipt(m.body, intoNotes), replaced: [...m.replaced, "ship receipt"] }; +} + +const RUNS_BLOCK_RE = new RegExp("^```" + RUNS_FENCE + "[ \\t]*\\r?\\n[\\s\\S]*?\\r?\\n```[ \\t]*(?:\\r?\\n|$)", "m"); + +function mergeSplit(now: Section[], next: Section[], incoming: string): { body: string; replaced: string[]; kept: string[]; added: string[]; dropped: string[] } { const ambiguous = next.filter(n => now.filter(s => s.key === n.key).length > 1).map(n => `"${n.label}"`); if (ambiguous.length) throw new UsageError(`the PR's current body holds ${[...new Set(ambiguous)].join(", ")} more than once, so the file's copy matches no single section; nothing was posted. Rerun with --replace and the whole body`); const preamble = (list: Section[]) => { diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index aae90c13e..54ff08945 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -45,14 +45,23 @@ export function renderReceiptBlock(receipt: ShipReceipt): string { } const BLOCK_RE = /```gstack-ship-receipt[ \t]*\r?\n([\s\S]*?)\r?\n```/; +/** A legacy receipt that opens the body, with the runs block right after it, as /ship posted before the Agent notes section. */ +const LEGACY_TOP_RE = /^\s*```gstack-ship-receipt[ \t]*\r?\n[\s\S]*?\r?\n```[ \t]*(?:\r?\n|$)(?:\s*```gstack-receipt-runs[ \t]*\r?\n[\s\S]*?\r?\n```[ \t]*(?:\r?\n|$))?/; /** - * The receipt block a PR body carries, parsed; `found: false` when absent. A body with an Agent notes section is read - * there only, so a receipt fence quoted in the readable part never binds; a body without one (posted before the - * section existed) gives its first block anywhere. + * Where the receipt and its runs are read: the Agent notes section when it holds a receipt, else a legacy receipt that + * opens the body (a section added later without one never hides it), else the section; a body without a section + * gives the whole body. A receipt fence quoted in the readable part never binds. */ +function receiptSource(body: string): string { + const notes = splitAgentNotes(body).notes; + if (notes === null || BLOCK_RE.test(notes)) return agentNotesOrBody(body); + return LEGACY_TOP_RE.exec(body)?.[0] ?? notes; +} + +/** The receipt block a PR body carries (receiptSource), parsed; `found: false` when absent. */ export function extractReceipt(body: string): { found: boolean; receipt?: ShipReceipt; raw?: string; errors: ValidationError[] } { - const m = BLOCK_RE.exec(agentNotesOrBody(body)); + const m = BLOCK_RE.exec(receiptSource(body)); if (!m) return { found: false, errors: [] }; try { const parsed = JSON.parse(m[1]!); @@ -110,7 +119,7 @@ export const RECEIPT_RUNS_KEPT = 30; const RUNS_RE = /```gstack-receipt-runs[ \t]*\r?\n([\s\S]*?)\r?\n```[ \t]*(?:\r?\n|$)/; export type ReceiptRun = Pick & { depth?: string; carried_from?: string }; export function receiptRuns(body: string): ReceiptRun[] { - const m = RUNS_RE.exec(agentNotesOrBody(body)); + const m = RUNS_RE.exec(receiptSource(body)); return m ? m[1]!.split('\n').flatMap(l => { try { const o = JSON.parse(l); return o && typeof o.head === 'string' ? [o as ReceiptRun] : []; } catch { return []; } }) : []; } export function receiptRun(r: ShipReceipt): ReceiptRun { @@ -118,13 +127,14 @@ export function receiptRun(r: ShipReceipt): ReceiptRun { return JSON.parse(JSON.stringify({ head, version, gstack_version, run, review_loop, review_loop_exception, depth: r.review_depth?.depth, carried_from: r.carry?.from })); } -/** Every receipt and runs block with the blank lines after it, wherever it sits. */ +/** Every receipt and runs block with the blank lines after it. */ const STRIP_RE = /```gstack-(?:ship-receipt|receipt-runs)[ \t]*\r?\n[\s\S]*?\r?\n```[ \t]*(?:\r?\n|$)(?:[ \t]*\r?\n)*/g; /** * The receipt and its runs block go at the end of the body's Agent notes section, which is created when the body has - * none; the section's other content stays above them. Every receipt and runs block elsewhere in the body (a quoted - * example, a legacy block) is removed, so one receipt remains. A replaced receipt joins the runs block. + * none; the section's other content stays above them. The section's old receipt and runs blocks and a legacy block + * that opens the body are removed, so one receipt remains; a fence quoted in the readable part stays, since it never + * binds. A replaced receipt joins the runs block. * A legacy body (a receipt and no section, as /ship posted before the section existed) keeps its layout: the block is * replaced in place at the top, because its Plan and issue lines sit in the readable part and a new section holding * only the receipt would hide them from the readers that prefer the section (a carry on an open PR, GST-1825). @@ -140,9 +150,9 @@ export function placeReceipt(body: string, receipt: ShipReceipt): string { const rest = body.replace(BLOCK_RE, '').replace(RUNS_RE, '').replace(/^\s+/, ''); return block + (rest ? '\n' + rest : ''); } - const stripped = body.replace(STRIP_RE, ''); - const notes = (splitAgentNotes(stripped).notes ?? '').trim(); - return withAgentNotes(stripped.replace(/^\s+/, ''), (notes ? notes + '\n\n' : '') + block); + const rest = body.replace(LEGACY_TOP_RE, ''); + const notes = (splitAgentNotes(rest).notes ?? '').replace(STRIP_RE, '').trim(); + return withAgentNotes(rest.replace(/^\s+/, ''), (notes ? notes + '\n\n' : '') + block); } /** The receipt's `issues` object from a gstack-issue-links --out or --json file (its other keys are preview-only). */ diff --git a/test/agent-notes.test.ts b/test/agent-notes.test.ts index 2d093c2e3..29c084f5d 100644 --- a/test/agent-notes.test.ts +++ b/test/agent-notes.test.ts @@ -70,12 +70,13 @@ describe('placeReceipt: the receipt lives in the Agent notes section', () => { expect(placeReceipt(withLines, receipt(HEAD))).toBe(withLines); }); - test('a new receipt replaces the one in the notes, the old one joins the runs block there, and a quoted example fence in the readable part is removed', () => { + test('a new receipt replaces the one in the notes, the old one joins the runs block there, and a quoted example fence in the readable part stays', () => { const example = 'Here is what the record looks like:\n\n' + renderReceiptBlock(receipt(OTHER, 1)) + '\nMore prose.\n'; const first = placeReceipt(example + '\n' + notes('Fixes #14'), receipt(HEAD)); const second = placeReceipt(first, receipt(OTHER)); - expect(second.match(/```gstack-ship-receipt/g)).toHaveLength(1); - expect(splitAgentNotes(second).readable).toBe('Here is what the record looks like:\n\nMore prose.\n\n'); + expect(second.match(/```gstack-ship-receipt/g)).toHaveLength(2); + expect(splitAgentNotes(second).readable).toBe(example.trimEnd() + '\n\n'); + expect(splitAgentNotes(second).notes!.match(/```gstack-ship-receipt/g)).toHaveLength(1); expect(extractReceipt(second).receipt!.head).toBe(OTHER); expect(receiptRuns(second).map(r => r.head)).toEqual([HEAD]); expect(splitAgentNotes(second).notes).toStartWith('Fixes #14\n\n```gstack-ship-receipt'); @@ -93,6 +94,18 @@ describe('placeReceipt: the receipt lives in the Agent notes section', () => { }); describe('which receipt binds', () => { + test('a legacy receipt at the top still binds when the body gained an Agent notes section without one, and placing a new receipt moves it and its runs into the notes', () => { + const legacy = renderReceiptBlock(receipt(OTHER)) + '\n```gstack-receipt-runs\n' + JSON.stringify({ head: 'c'.repeat(40) }) + '\n```\n\nOpening.\n\n## Summary\n\nx\n\n' + notes('Fixes #14'); + expect(bindReceipt(legacy, '7', [OTHER])).toMatchObject({ ok: true, head: OTHER }); + expect(receiptRuns(legacy).map(r => r.head)).toEqual(['c'.repeat(40)]); + const placed = placeReceipt(legacy, receipt(HEAD)); + expect(placed).toStartWith('Opening.'); + expect(placed.match(/```gstack-ship-receipt/g)).toHaveLength(1); + expect(extractReceipt(placed).receipt!.head).toBe(HEAD); + expect(receiptRuns(placed).map(r => r.head)).toEqual(['c'.repeat(40), OTHER]); + expect(splitAgentNotes(placed).notes).toStartWith('Fixes #14\n\n```gstack-ship-receipt'); + }); + test('(a) a legacy receipt at the top of a body with no section binds', () => { const body = renderReceiptBlock(receipt(HEAD)) + '\nOpening.\n\n## Summary\n\nx\n'; expect(bindReceipt(body, '7', [HEAD])).toMatchObject({ ok: true, head: HEAD }); diff --git a/test/gstack-post.test.ts b/test/gstack-post.test.ts index 1878b470b..4c048dc2c 100644 --- a/test/gstack-post.test.ts +++ b/test/gstack-post.test.ts @@ -11,6 +11,7 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import { EXIT, attributionCallout, mergeBodySections, runPost, type PostEnv } from "../lib/gstack-post"; +import { extractReceipt, receiptRuns } from "../lib/ship-receipt"; interface Call { cmd: string; args: string[]; input?: string } @@ -807,6 +808,32 @@ describe("gstack-post keeps the Agent notes section last", () => { // Value: protects=a PR posted before the notes section (receipt at the top) still merges: a partial post keeps the legacy receipt where it is, and a // full post with notes last replaces it, so one receipt remains and it is the notes' one; fails_when=the legacy receipt survives a full post (two // receipts, the stale one first) or a partial post drops it; why_new=the legacy layout meets the new one; seam=none; no_claim=the gh round trip + // Value: protects=a receipt posted on its own to a body whose Agent notes hold one replaces that receipt, so the new one binds and the old one + // joins the runs block; fails_when=the posted receipt lands outside the notes while the old one stays authoritative; why_new=review finding + // gstack-post.ts:212; seam=none (mergeBodySections); no_claim=the gh round trip + test("a receipt posted on its own replaces the one in the notes, so the new receipt binds", () => { + const m = mergeBodySections(composed("aaaa", "old"), RECEIPT("bbbb")); + expect(extractReceipt(m.body).receipt!.head).toBe("bbbb"); + expect(m.body.match(/```gstack-ship-receipt/g)).toHaveLength(1); + expect(receiptRuns(m.body).map(r => r.head)).toEqual(["aaaa"]); + expect(m.body).toStartWith("Opening sentence.\n\n## Summary\n\nold\n\n## Test plan\n\n- [x] bun test\n\n"); + expect(m.replaced).toContain("ship receipt"); + }); + + // Value: protects=the PR's own opening paragraph and run history when a receipt posted with text or a runs block after it goes into the notes; + // fails_when=the text after the receipt replaces the opening, or the file's runs block lands in the readable part; why_new=review finding + // gstack-post.ts:283 (adversarial-native#1, outside-voice#1); seam=none (mergeBodySections); no_claim=the gh round trip + test("a receipt posted with text and a runs block after it keeps the opening paragraph and the notes' run history", () => { + const runs = "\`\`\`gstack-receipt-runs\n" + JSON.stringify({ head: "cccc" }) + "\n\`\`\`\n"; + const m = mergeBodySections(composed("aaaa", "old"), RECEIPT("bbbb") + runs + "\nNote under receipt.\n"); + expect(m.body).toStartWith("Opening sentence.\n\n"); + expect(m.replaced).not.toContain("leading text"); + expect(m.body).toContain("Note under receipt."); + expect(extractReceipt(m.body).receipt!.head).toBe("bbbb"); + expect(m.body.match(/```gstack-receipt-runs/g)).toHaveLength(1); + expect(receiptRuns(m.body).map(r => r.head)).toEqual(["aaaa"]); + }); + test("a legacy receipt-at-top body still merges: a partial post keeps it, a full post with notes last replaces it", () => { const legacy = RECEIPT("aaaa") + "\n## Summary\n\nold\n\n## Approvals\n\nPlan: exempt: docs-only\n"; const partial = mergeBodySections(legacy, "## Summary\n\nnew\n"); diff --git a/test/gstack-ship-policy.test.ts b/test/gstack-ship-policy.test.ts index 8bfa80740..9d3bce7af 100644 --- a/test/gstack-ship-policy.test.ts +++ b/test/gstack-ship-policy.test.ts @@ -183,7 +183,7 @@ describe("gstack's own .gstack/ship-policy.json", () => { for (const f of fs.readdirSync(path.join(root, '.github/workflows')).filter(f => f.endsWith('.yml'))) { const text = fs.readFileSync(path.join(root, '.github/workflows', f), 'utf8'); const on = text.slice(text.search(/^on:/m), text.search(/^jobs:/m)); - if (!/^\s+pull_request:/m.test(on) || /^\s+paths(-ignore)?:/m.test(on)) continue; + if (!/^\s+pull_request(?:_target)?:/m.test(on) || /^\s+paths(-ignore)?:/m.test(on)) continue; const body = text.slice(text.search(/^jobs:/m)); for (const m of body.matchAll(/^ ([A-Za-z0-9_-]+):\n((?: .*\n|\s*\n)*)/gm)) { const name = /^ name:\s*(.+)$/m.exec(m[2]!)?.[1]?.trim(); diff --git a/test/plain-title-workflow-safety.test.ts b/test/plain-title-workflow-safety.test.ts index 94cc0aa37..e307699e9 100644 --- a/test/plain-title-workflow-safety.test.ts +++ b/test/plain-title-workflow-safety.test.ts @@ -57,16 +57,22 @@ describe('plain-title.yml pull_request_target safety', () => { expect(runBlockLines(planted).filter(l => /\$\{\{\s*github\.event\.pull_request/.test(l.text)).length).toBe(2); }); - test('PR text reaches files through env: and printf, and is never echoed', () => { + test('PR text reaches files through env: and printf, or straight from the event file, and is never echoed', () => { expect(content).toMatch(/PR_TITLE: \$\{\{ github\.event\.pull_request\.title \}\}/); - expect(content).toMatch(/PR_BODY: \$\{\{ github\.event\.pull_request\.body \}\}/); expect(content).toMatch(/printf '%s' "\$PR_TITLE" >/); - expect(content).toMatch(/printf '%s' "\$PR_BODY" >/); const run = runBlockLines(content).map(l => l.text).join('\n'); expect(run).not.toMatch(/\becho\b[^\n]*\$\{?PR_/); expect(run).not.toMatch(/\bcat\b/); }); + // Value: protects=a PR body larger than one environment string (about 128 KiB) still reaches the checker, so a good + // title never fails on E2BIG; fails_when=the body travels through env:; why_new=review finding plain-title.yml:58; + // seam=the workflow text; no_claim=the runner's exact limit + test('the body is read from the event file, never through env:', () => { + expect(content).not.toMatch(/github\.event\.pull_request\.body/); + expect(content).toMatch(/jq -j '\.pull_request\.body \/\/ ""' "\$GITHUB_EVENT_PATH" > "\$RUNNER_TEMP\/pr-body\.txt"/); + }); + test('permissions are read-only', () => { const perms = content.match(/^permissions:\n((?:\s+.+\n)+)/m)?.[1] ?? ''; expect(perms).toMatch(/contents: read/);