diff --git a/changelog.d/gst-1227-post-preamble.md b/changelog.d/gst-1227-post-preamble.md index 8cdb694d3..4f50a51cf 100644 --- a/changelog.d/gst-1227-post-preamble.md +++ b/changelog.d/gst-1227-post-preamble.md @@ -2,9 +2,10 @@ ### Fixed - **A /ship post keeps the text above the PR's first heading.** Since GST-412, a body file with a receipt above its first `## ` section dropped everything above the PR's first heading, so a /ship run on a Capy-opened PR deleted the opening paragraph and the `…` badge. That text is now kept byte for byte and the new receipt goes right under it, so the next /ship post doesn't read it as a stale note under the receipt. Opening lines the file brings of its own still replace it, and a stale note between the receipt and the first heading is still dropped and named on stderr. -- **A badge at the end of the body stays there.** A `` … `` block after the first receipt or heading is now a piece of its own, so rewriting the section above it no longer deletes it, and new sections go above it. -- **Every run id in a `runs=/,/` list is exempt from `pii.phone.e164`.** Only the id right after `runs=` was exempt (and only at 11 digits), so the CI carry and run clear receipts `lib/run-evidence.ts` prints needed `--confirm` from their second run on. The scanner now exempts each id of that list (`run=` too) and an id after a bare `actions/runs/` path with no leading `/`. Each id needs its `/`, so `runs=4155550123` and `+1 415 555 0123` on the same line still report. See docs/troubleshooting.md#redact-run-id-as-phone. +- **A Capy badge at the end of the body stays there.** A `` … `` block that ends the body after its first receipt or heading is now a piece of its own, so rewriting the section above it no longer deletes it, new sections go above it, and a badge only the file brings goes last, once, so reposting the same file gives the same bytes. Every other marker block, a badge with text after it and an unclosed marker stay inside their section, as before. +- **Every run id in a `runs=/,/` list is exempt from `pii.phone.e164`.** Only the id right after `runs=` was exempt (and only at 11 digits), so the CI carry and run clear receipts `lib/run-evidence.ts` prints needed `--confirm` from their second run on. The scanner now exempts each 11- or 12-digit id of that list (`run=` too) and an id after an `actions/runs/` path with no leading `/` when it starts the line or follows whitespace, `(`, `[`, `<` or a quote. Each id needs its `/`, so `runs=4155550123`, `runs=4155550123/1`, `sales-actions/runs/4155550123` and `+1 415 555 0123` on the same line still report. See docs/troubleshooting.md#redact-run-id-as-phone. ### Tests - `test/gstack-post.test.ts`: "a full post keeps the opening text and a badge block above the first heading or at the end of the body byte for byte" and "run ids in a runs= list or an actions/runs/ path are not phone numbers; a phone number beside them still is" both fail on main. Four GST-412 tests that expected the opening text to be dropped now expect it kept. -- `test/redact-context-exemptions.test.ts`: run ids in a `runs=` list or an `actions/runs/` path stay clean (6 cases fail on main), and 7 controls, `call +1 415 555 0123` among them, still report. +- `test/gstack-post.test.ts`: "a marker block that is not a trailing Capy badge stays inside its section", "a trailing badge the file brings lands at the end once and reposts byte for byte", "a fenced heading inside a marker block and an unclosed badge marker change nothing about the sections" and "two marker blocks with the same name merge by their sections" pin the main-branch handling of every other marker block. +- `test/redact-context-exemptions.test.ts`: run ids in a `runs=` list or an `actions/runs/` path stay clean (5 cases, all failing on main), and 10 controls, `call +1 415 555 0123` and `sales-actions/runs/4155550123` among them, still report. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 8893ad8d1..44245a5d8 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -2720,12 +2720,14 @@ exact tokens it finds in these positions: - a 12-digit value right after `tree`, `gated_tree`, `head` or `predecessor` with `=` or a space (`tree=704992822996`, `tree 704992822996`); - an 11-digit id inside an `/actions/runs/` URL or as that link's text; -- any id inside an `actions/runs/` path, with or without a host or a - leading `/` (`see actions/runs/41629861820`), and every id of a - `runs=/` list a CI carry or run clear prints - (`runs=41629861819/1,41629861820/2`; `run=` too). The scanner itself - exempts both (`isCiRunIdentifier`), and only that shape counts: each id - needs its `/` of one to three digits, so `runs=4155550123`, +- any id inside an `actions/runs/` path, with or without a host, when + `actions/runs/` starts the line or follows whitespace, `/`, `(`, `[`, + `<` or a quote (`see actions/runs/41629861820`; `sales-actions/runs/…` + is not one), and every 11- or 12-digit id of a `runs=/` list + a CI carry or run clear prints (`runs=41629861819/1,41629861820/2`; + `run=` too). The scanner itself exempts both (`isCiRunIdentifier`), and + only that shape counts: each id needs its `/` of one to three + digits, so `runs=4155550123`, `runs=4155550123/1`, `runs=+14155550123/1` and a phone number elsewhere on the line still report (GST-1227); - an 11-digit id right after a run word on the same line, with an optional @@ -2778,10 +2780,13 @@ opening text above the first receipt or heading (a paragraph, a bot's badge such as `…`, or the whole body when it has no `## ` heading) is kept byte for byte, and a new receipt lands right under it; only opening lines the file brings of its own replace -it (GST-1227). A marker block (`` through -``) after the first receipt or heading is a piece of its -own, so a badge at the end of the body stays there when the section above it -is rewritten, and new sections are added above it. A partial post (`## ` sections +it (GST-1227). A Capy badge that ends the body after its first receipt or +heading (`` through ``, outside +a code fence, with only blank lines after it) is a piece of its own: it stays +at the end when the section above it is rewritten, new sections go above it, +and a badge only the file brings goes last, once. Every other marker block, a +badge with text after it and an unclosed badge marker stay inside the section +they sit in. A partial post (`## ` sections with no receipt above them, such as a lane posting `## Screenshots`) and a file with no `## ` section (a receipt-only or text-only post) leave that text alone, so a human intro or a `Review-loop exception:` line under the receipt survives. One stderr line names what was replaced, kept, added diff --git a/lib/gstack-post.ts b/lib/gstack-post.ts index bfc213ea2..25e0b9d2f 100644 --- a/lib/gstack-post.ts +++ b/lib/gstack-post.ts @@ -160,7 +160,7 @@ function parseArgs(argv: string[]): Request { return req; } -/** One piece of a PR body: the leading text, a `## ` section, the ship-receipt block, a marker block, or text after a receipt or marker block. */ +/** One piece of a PR body: the leading text, a `## ` section, the ship-receipt block, text after a receipt block, or a Capy badge that ends the body. */ interface Section { key: string; label: string; text: string } /** @@ -175,38 +175,42 @@ interface Section { key: string; label: string; text: string } * 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. - * After the first receipt or heading, a `` line outside - * a fence opens a marker block (a bot's badge, such as Capy's - * `…`) that runs through its - * `` line, or up to the next heading when it never - * closes, so a badge at the end of the body never leaves with the section - * above it (GST-1227). A marker block in the leading text stays part of it. + * A Capy badge (`` through ``, + * both outside a fence) that ends the body after its first receipt or + * heading, with only blank lines after it, is a piece of its own + * (`badge:capy-badge`), so it never leaves with the section above it + * (GST-1227). A badge anywhere else, an unclosed badge marker and every other + * marker block stay inside the section they sit in. */ function splitSections(body: string): Section[] { const out: Section[] = []; let fence: string | null = null; - let block: string | null = null; + let badgeAt = -1; + let inBadge = false; let plain = 0; - const open = (key: string, label: string) => out.push({ key, label, text: "" }); + const open = (key: string, label: string) => { badgeAt = -1; inBadge = false; out.push({ key, label, text: "" }); }; const openText = () => out.length === 0 ? open("text:lead", "leading text") : open(`text:${plain++}`, "trailing text"); const closes = (run: string) => new RegExp(`^ {0,3}${run[0] === "`" ? "`" : "~"}{${run.length},}\\s*$`).test(bare); let bare = ""; - const endsBlock = () => { if (block !== null && bare.includes(``)) { block = null; openText(); } }; for (const line of body.split(/(?<=\n)/)) { bare = line.replace(/\r?\n$/, ""); - if (block !== null && /^ {0,3}## /.test(bare)) block = null; - if (block !== null) { out.at(-1)!.text += line; endsBlock(); continue; } const fenceOpen: RegExpExecArray | null = fence === null ? /^ {0,3}(`{3,}|~{3,})/.exec(bare) : null; - const marker = fence === null ? /^ {0,3}/.exec(bare) : null; + const outside = fence === null && !fenceOpen; 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; else if (fence === null && /^ {0,3}## /.test(bare)) { const title = bare.replace(/^ {0,3}## /, "").trim(); open(`h2:${title.toLowerCase()}`, title); } - else if (marker && out.some(s => s.key === "receipt" || s.key.startsWith("h2:"))) { open(`block:${marker[1]}`, `${marker[1]} block`); block = marker[1]!; } if (out.length === 0) openText(); + if (outside && /^ {0,3}/.test(bare)) { badgeAt = out.at(-1)!.text.length; inBadge = true; } + else if (inBadge && outside && //.test(bare)) inBadge = false; + else if (!inBadge && bare.trim() !== "") badgeAt = -1; out.at(-1)!.text += line; - endsBlock(); + } + const last = out.at(-1); + if (last && badgeAt >= 0 && !inBadge && fence === null && last.key !== "text:lead") { + out.push({ key: "badge:capy-badge", label: "capy-badge block", text: last.text.slice(badgeAt) }); + last.text = last.text.slice(0, badgeAt); } if (fence === "receipt") Object.assign(out.at(-1)!, { key: "unclosed-receipt", label: "unclosed receipt fence" }); return out.filter(s => s.text !== ""); @@ -216,8 +220,8 @@ function splitSections(body: string): Section[] { * The body to send for `pr-body` without `--replace`: each section of the * file replaces the same-keyed section of the current body in place, current * sections the file does not rewrite are kept byte for byte, new `## ` - * sections are appended after them (above a marker block that ends the - * body), new leading text goes first, and a new receipt goes right after the + * sections are appended after them (above a Capy badge that ends the body; + * a badge only the file brings goes last), new leading text goes first, and a new receipt goes right after the * current leading text, or first when there is none (where * lib/ship-receipt.ts puts the receipt). Text outside sections * matches by role (the file's leading text replaces the leading text; text @@ -273,13 +277,12 @@ export function mergeBodySections(current: string, incoming: string): { body: st } const fresh = next.filter(n => !used.has(n)); for (const n of next) if (fresh.includes(n) || (ours.includes(n) && !paired.has(n))) added.push(n.label); - const first = fresh.filter(n => !n.key.startsWith("h2:")); + const badge = (s: Section) => s.key === "badge:capy-badge"; + const first = fresh.filter(n => !n.key.startsWith("h2:") && !badge(n)); if (now.length === 0) return { body: incoming, replaced, kept, added, dropped }; const lead = merged[0]?.key === "text:lead" && merged[0].text.trim() ? 1 : 0; - let tail = merged.length; - while (tail > lead && (merged[tail - 1]!.key.startsWith("block:") || !merged[tail - 1]!.text.trim())) tail--; - if (!merged.slice(tail).some(s => s.key.startsWith("block:"))) tail = merged.length; - const all = [...merged.slice(0, lead), ...first, ...merged.slice(lead, tail), ...fresh.filter(n => n.key.startsWith("h2:")), ...merged.slice(tail)]; + const tail = merged.length && badge(merged.at(-1)!) ? merged.length - 1 : merged.length; + const all = [...merged.slice(0, lead), ...first, ...merged.slice(lead, tail), ...fresh.filter(n => n.key.startsWith("h2:")), ...merged.slice(tail), ...fresh.filter(badge)]; for (const t of [...ours].reverse()) all.splice(all.indexOf(next[next.indexOf(t) - 1]!) + 1, 0, t); const fromFile = (s: Section) => next.includes(s); const body = all.map((s, i) => { diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index c6f90557a..f90de67dc 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -357,11 +357,13 @@ export function isNumericMetadataValue(span: string, match: RegExpExecArray): bo * command. Only the position decides: a run/job flag (`--run 37235771700`, * `--job=111534615007`), a `gh run view|watch|rerun|download|cancel` * argument, an `actions/runs/` or `/job/` URL segment (with or - * without a host or a leading `/`), a `run_id` / `job_id` key, or any id - * of a `runs=/[,/…]` list (`run=` too), the - * receipt a CI carry or run clear prints (lib/run-evidence.ts - * runReceiptFields); the `/` of one to three digits after every id - * is what tells that list from a number (GST-1227). An 11-digit id (every GitHub Actions run id since + * without a host: `actions/runs/` at the line start or after whitespace, + * `/`, `(`, `[`, `<` or a quote, so `sales-actions/runs/` is not one), + * a `run_id` / `job_id` key, or any 11- or 12-digit id of a + * `runs=/[,/…]` list (`run=` too), the receipt a + * CI carry or run clear prints (lib/run-evidence.ts runReceiptFields); the + * `/` of one to three digits after every id is what tells that list + * from a number (GST-1227). An 11-digit id (every GitHub Actions run id since * 2025) also counts right after a run word on the same line, the way a ship * receipt, a READY block or an approval record names one (`screen 40477638211 * GREEN`, `gate=40323989742`, `rerun passed 40287239206`, `run #19368712345`); @@ -373,8 +375,8 @@ export function isNumericMetadataValue(span: string, match: RegExpExecArray): bo * else still report. */ const CI_ID_POSITION_BEFORE = - /(?:--(?:run|job)(?:-id)?(?:[ \t]+|[ \t]*=[ \t]*)|\bgh[ \t]+run[ \t]+(?:view|watch|rerun|download|cancel)[ \t]+|\bactions\/runs\/|\/jobs?\/|(?:run|job)_id["']?[ \t]*[:=][ \t]*["']?)$/i; -const CI_RUN_LIST = /(? { test("run ids in a runs= list or an actions/runs/ path are not phone numbers; a phone number beside them still is", () => { const clean = [ "CI_CARRY: cleared pregate.json/remote_gate run tree=704992822996 tests=412 runs=41629861819/1,41629861820/2 profiles=free-tests,windows-smoke heads=2f9d11a0c3e4 verified=2026-11-03T18:00:00.000Z", - "carried run=41629861819/1,4162986182/3", + "carried run=41629861819/1,416298618201/3", "logs at garrytan/gstack actions/runs/41629861819/attempts/2", ]; for (const body of clean) { const f = fakeEnv({ files: { "/b": body + "\n" }, visibility: "public" }); expect([body, run(["pr-comment", "12", "--body-file", "/b"], f)]).toEqual([body, EXIT.posted]); } - for (const body of ["call +1 415 555 0123 about runs=41629861819/1,41629861820/2", "runs=+14155550123/1", "runs=1/4155550123", "runs=4155550123"]) { + for (const body of ["call +1 415 555 0123 about runs=41629861819/1,41629861820/2", "runs=+14155550123/1", "runs=1/4155550123", "runs=4155550123", "runs=4155550123/1", "sales-actions/runs/4155550123"]) { const f = fakeEnv({ files: { "/b": body + "\n" }, visibility: "public" }); expect([body, run(["pr-comment", "12", "--body-file", "/b"], f)]).toEqual([body, EXIT.confirm]); expect(f.stdout()).toContain("RULE: pii.phone.e164 LINE: 1 PART: body"); @@ -517,6 +517,55 @@ describe("gstack-post pr-body merges the PR body by section", () => { expect(trailing.stderr()).toBe("gstack-post: pr-body merged by section: replaced ship receipt, Approvals; kept What changed, Why, capy-badge block; added Tests\n"); }); + const BADGE = "\n[![Capy](https://capy.ai/badge.svg)](https://capy.ai)\n\n"; + const SHOTS = (alt: string) => "\n![" + alt + "](u)\n\n"; + + // Value: protects=a marker block other than a Capy badge that ends the body stays inside its section exactly as before GST-1227, so a rewrite + // replaces it with the section, a new section lands after it, text after a badge in mid-section never pairs with the note under a receipt, + // and the file's own block stays in its section; fails_when=the old block and the stale lines after it survive a rewrite, a shots block is + // split from its section, "Fixes #12" is dropped, or the file's block moves above the first heading; why_new=GST-1227 review P1; seam=none + // (mergeBodySections); no_claim=the gh round trip + test("a marker block that is not a trailing Capy badge stays inside its section", () => { + const mid = mergeBodySections(RECEIPT("aaaa") + "\n## What changed\n\n- old\n" + SHOTS("a") + "stale line\n\n## Why\n\nBecause.\n", RECEIPT("bbbb") + "\n## What changed\n\n- new\n"); + expect([mid.body, mid.kept]).toEqual([RECEIPT("bbbb") + "\n## What changed\n\n- new\n\n## Why\n\nBecause.\n", ["Why"]]); + const last = mergeBodySections(CURRENT + SHOTS("a"), RECEIPT("bbbb") + "\n## Tests\n\n- one\n"); + expect(last.body).toBe(RECEIPT("bbbb") + "\n## What changed\n\n- the old summary\n\n## Why\n\nBecause.\n\n## Approvals\n\n- owner: do it\n" + SHOTS("a") + "\n## Tests\n\n- one\n"); + const collide = mergeBodySections("Intro\n\n## What changed\n\n- x\n" + BADGE + "Fixes #12\n", RECEIPT("bbbb") + "Review-loop exception: foo\n"); + expect(collide.body).toBe("Intro\n\n" + RECEIPT("bbbb") + "\nReview-loop exception: foo\n\n## What changed\n\n- x\n" + BADGE + "Fixes #12\n"); + const own = mergeBodySections(CURRENT, RECEIPT("bbbb") + "\n## What changed\n\n" + SHOTS("n") + "after the shots\n"); + expect(own.body).toBe(RECEIPT("bbbb") + "\n## What changed\n\n" + SHOTS("n") + "after the shots\n\n## Why\n\nBecause.\n\n## Approvals\n\n- owner: do it\n"); + }); + + // Value: protects=a trailing badge the file brings into a body without one lands at the end once and a repost is byte-identical; fails_when=each + // identical post adds another badge or moves it above the sections; why_new=GST-1227 review P2; seam=none (mergeBodySections); no_claim=the gh round trip + test("a trailing badge the file brings lands at the end once and reposts byte for byte", () => { + const file = RECEIPT("bbbb") + "\n## Tests\n\n- one\n\n" + BADGE; + const once = mergeBodySections(CURRENT, file).body; + expect(once).toBe(RECEIPT("bbbb") + "\n## What changed\n\n- the old summary\n\n## Why\n\nBecause.\n\n## Approvals\n\n- owner: do it\n\n## Tests\n\n- one\n\n" + BADGE); + const twice = mergeBodySections(once, file).body; + expect(twice).toBe(once); + expect(mergeBodySections(twice, file).body).toBe(once); + }); + + // Value: protects=fences inside a marker block keep their meaning (a fenced `## x` opens no section), a badge inside a fence is text, and an + // unclosed badge marker swallows nothing; fails_when=the fenced `## x` ends the block and opens a section, or the unclosed marker keeps the + // rest of the old section after a rewrite; why_new=GST-1227 review P2; seam=none (mergeBodySections); no_claim=the gh round trip + test("a fenced heading inside a marker block and an unclosed badge marker change nothing about the sections", () => { + const fenced = mergeBodySections("## What changed\n\n\n```md\n## x\n```\n\n\n## Why\n\nold\n", "## Why\n\nnew\n"); + expect([fenced.body, fenced.replaced, fenced.kept]).toEqual(["## What changed\n\n\n```md\n## x\n```\n\n\n## Why\n\nnew\n", ["Why"], ["What changed"]]); + const example = mergeBodySections("## Docs\n\n```md\n" + BADGE + "```\n", "## Docs\n\nnew\n"); + expect([example.body, example.kept]).toEqual(["## Docs\n\nnew\n", []]); + const unclosed = mergeBodySections(RECEIPT("aaaa") + "\n## Why\n\nBecause.\n\n## Approvals\n\n- x\n\nmore approvals\n", RECEIPT("bbbb") + "\n## Approvals\n\n- y\n"); + expect([unclosed.body, unclosed.kept]).toEqual([RECEIPT("bbbb") + "\n## Why\n\nBecause.\n\n## Approvals\n\n- y\n", ["Why"]]); + }); + + // Value: protects=two marker blocks with one name merge by their sections; fails_when=the merge refuses them as a duplicated section; + // why_new=GST-1227 review; seam=none (mergeBodySections); no_claim=the gh round trip + test("two marker blocks with the same name merge by their sections", () => { + const two = mergeBodySections("## A\n\n" + SHOTS("a") + "\n## B\n\n" + SHOTS("b"), "## A\n\n" + SHOTS("a2")); + expect([two.body, two.replaced, two.kept]).toEqual(["## A\n\n" + SHOTS("a2") + "\n## B\n\n" + SHOTS("b"), ["A"], ["B"]]); + }); + // Value: protects=a partial post (no receipt above its first `## ` heading, even with a caption of its own or a receipt further down) keeps // the human intro and the note under the receipt; fails_when=a lane posting only `## Screenshots` // deletes the intro or a Review-loop exception line; why_new=GST-412 made any file with a section own the preamble (GstackMinder, diff --git a/test/redact-context-exemptions.test.ts b/test/redact-context-exemptions.test.ts index 8c635dcdd..8c8383a1b 100644 --- a/test/redact-context-exemptions.test.ts +++ b/test/redact-context-exemptions.test.ts @@ -305,8 +305,7 @@ describe("pii.phone.e164: run ids in a runs= list or an actions/runs/ path (GST- // The run list a CI carry or run clear prints (lib/run-evidence.ts runReceiptFields): every id, not only the first. "CI_CARRY: cleared pregate.json/remote_gate run tests=412 runs=41629861819/1,41629861820/2 profiles=free-tests heads=2f9d11a0c3e4", '{"remote_gate": "carried runs=41629861819/1,41629861820/1,41629861821/3 profiles=free-tests,windows-smoke"}', - "run=41629861819/1,4162986182/1", - "runs=4162986182/1", + "run=41629861819/1,416298618201/1", "see actions/runs/41629861820 for the log", "garrytan/gstack actions/runs/4162986182/attempts/2", ], @@ -318,6 +317,9 @@ describe("pii.phone.e164: run ids in a runs= list or an actions/runs/ path (GST- "runs=4155550123", "reruns=4155550123/1", "factions/runs/4155550123", + "sales-actions/runs/4155550123", + "runs=4162986182/1", + "runs=1234567890123/1", ], ); });