mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-01 06:24:41 +01:00
fix(gstack-post): pr-body with sections replaces stale text above the first heading (GST-412)
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
bed1ff0245
commit
0dc729486d
5 files changed
+137
-28
No files matched your search
@@ -0,0 +1,14 @@
|
||||
**A passing test lane that leaves Bun's `.bun/` folder behind now grades FRESH, `--check-prose` no longer reads the Approvals table as a claim, and `gstack-post pr-body` drops a stale intro.** Three /ship false alarms from GST-421 and GST-412 (HARN-1, HARN-12, GSTA-2, GEVA-69, gstack-evals#90).
|
||||
|
||||
### Fixed
|
||||
- **GST-421: `gstack-wtree` leaves an untracked `.bun/` folder at the repo root out of the fingerprint.** A test that created it made `gstack-evidence` treat the run as a mid-run edit, so `check` graded the lane STALE with "record has no content fingerprint" and named no file. Tracked files under `.bun/` still count, and a tree without that folder hashes exactly as before.
|
||||
- **GST-421: `gstack-evidence` names what changed during a run.** When the fingerprint before and after a run differ, the record stores up to five changed paths as `wtree_changed`, the stderr warning lists them, and `check` grades STALE with `content changed during the run: <paths>`. A record without them still says "record has no content fingerprint".
|
||||
- **GST-412: `gstack-ship-receipt read --check-prose` skips the `## Approvals` block `gstack-approvals` renders** (the heading, the table and its `Source: gstack-approvals record` line), so "one Step 11 pass" in the owner's quoted words is no longer read as `gate.<lane>.pass body=11`. Text after that block, a hand-written Approvals list and an example inside a code fence are still checked, and CRLF bodies work.
|
||||
|
||||
### Changed
|
||||
- **GST-412: `gstack-post pr-body` manages the text above the first heading when the file has a `## ` section.** The current body's opening text, any text between the receipt and the first heading, or a whole body with no `## ` heading, is replaced by the file's own text above its first heading, or dropped when the file has none. The stderr line names each dropped piece with its first line (`; dropped leading text ("…")`). A receipt-only or text-only file merges as before, and reposting the same body gives the same bytes.
|
||||
|
||||
### Tests
|
||||
- `test/evidence.test.ts`: a lane that writes `.bun/x` grades FRESH; one that writes `src/new.ts` grades STALE naming it; a tracked file under `.bun/` still moves the fingerprint; a tree without `.bun/` hashes like a plain `git add -A`.
|
||||
- `test/ship-receipt-prose.test.ts`: the rendered Approvals table, text after its footer, a fenced example, a hand-written list and a CRLF body.
|
||||
- `test/gstack-post.test.ts`: stale lead text and stale text under the receipt are dropped and named; receipt-only and text-only files keep the old merge; reposts are byte-identical; an attribution callout is never doubled.
|
||||
+6
-1
@@ -970,7 +970,12 @@ not a claim; a number that is the value of a `key=` or `key:` pair is that
|
||||
key's claim alone, so `passes=2 blocking_found=1` is two claims, never a
|
||||
`2 blocking_found` one, #3513). A body that quotes a different number is
|
||||
`RECEIPT_PROSE_MISMATCH` (exit 1) and the `PROSE:` line names each field with
|
||||
both values; prose that quotes no counter is not a claim. Data PRs (evals,
|
||||
both values; prose that quotes no counter is not a claim. The `## Approvals`
|
||||
block `gstack-approvals` renders (its heading, the Date | Asked | Exact words
|
||||
table and the `Source: gstack-approvals record` line) quotes the owner's
|
||||
words, not this PR's numbers, so it is skipped: "one Step 11 pass" in that table
|
||||
is not a `pass` claim. Text you write under the heading after that block, and
|
||||
a `## Approvals` example inside a code fence, are still checked. Data PRs (evals,
|
||||
measurements, sweeps) run it before READY so the body's numbers are the run's.
|
||||
|
||||
<a id="review-scope"></a>
|
||||
|
||||
+14
-6
@@ -2510,11 +2510,18 @@ section instead of overwriting it: a section in the file replaces the
|
||||
same-titled section (title compared case-insensitively), 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 stay in
|
||||
place, new sections are appended after them, and a current body with no
|
||||
`## ` headings is kept as the leading text. One stderr line names what was
|
||||
replaced, kept and added (`gstack-post: pr-body merged by section: replaced
|
||||
What changed; kept ship receipt, Why, Approvals; added Tests`). A body file
|
||||
holding only its first section therefore no longer wipes the rest of the PR.
|
||||
place, and new sections are appended after them. The text above the first
|
||||
`## ` heading belongs to the file whenever the file has a `## ` section: the
|
||||
current body's text there (the opening lines, and any lines between the receipt
|
||||
and the first heading, or the whole body when it has no `## ` heading) is
|
||||
replaced by the file's own text above its first heading, or dropped when the
|
||||
file has none. A file with no `## ` section (a receipt-only or text-only post)
|
||||
leaves that text alone. One stderr line names what was replaced, kept, added
|
||||
and dropped, a dropped piece with its first line (`gstack-post: pr-body merged
|
||||
by section: replaced What changed; kept ship receipt, Why, Approvals; added
|
||||
Tests; dropped leading text ("Draft from gh pr create")`). A body file holding
|
||||
only its first section therefore no longer wipes the rest of the PR, and an
|
||||
old intro no longer sits above a fresh /ship body.
|
||||
The redaction scan runs on the merged bytes, so a MEDIUM or HIGH finding can
|
||||
come from a kept section; the `RULE:` line's `LINE:` is in the merged text.
|
||||
When the current body cannot be read (`gh pr view --json body` fails) nothing
|
||||
@@ -5059,7 +5066,8 @@ body that names a counter the receipt records (`passes=3` against
|
||||
`review_loop.passes: 2`, `1 deferred` against `deferred: 2`, `passed: 40`
|
||||
against `gate.passed: 41`) with a different value. The receipt is written from
|
||||
the run; the prose was typed, so a reader of the body would learn a number the
|
||||
run did not produce. The `PROSE:` line names each field with both values.
|
||||
run did not produce. The `PROSE:` line names each field with both values. The
|
||||
`## Approvals` table `gstack-approvals` renders is never read as a claim.
|
||||
|
||||
**Fix.** Correct the body to the receipt's number. When the receipt is the
|
||||
stale one (the run was repeated), rerun `/ship` so the receipt is rewritten
|
||||
|
||||
+42
-13
@@ -9,7 +9,8 @@
|
||||
* `pr-body` merges by `## ` section into the PR's current body (read through
|
||||
* the same runner): a section in the file replaces the same-titled one, the
|
||||
* ship-receipt block is a section of its own, sections the file does not
|
||||
* rewrite are kept, new ones are appended; `--replace` overwrites instead.
|
||||
* rewrite are kept, new ones are appended, and a file with a `## ` section
|
||||
* replaces the text above the first heading; `--replace` overwrites instead.
|
||||
* The scan runs on the merged bytes, which are the bytes sent.
|
||||
*
|
||||
* Exit codes: 0 posted; 1 HIGH finding (or oversize input) refused;
|
||||
@@ -211,24 +212,52 @@ function splitSections(body: string): Section[] {
|
||||
* A file section whose title the current body holds more than once (an old
|
||||
* description pasted under the new one) matches no single section, so the
|
||||
* merge refuses instead of replacing one copy and keeping the stale other.
|
||||
* A file with at least one `## ` section manages the preamble (GST-412): every
|
||||
* non-blank text section before the current body's first heading (the leading
|
||||
* text, text after a receipt block) is dropped, the file's own text before its
|
||||
* first heading takes its place (leading text first, text after the file's
|
||||
* receipt right after the receipt), a dropped piece the file brings no text for
|
||||
* is listed in `dropped` with its first line, and the receipt keeps the rules
|
||||
* above. A file without a `## ` section merges as described above.
|
||||
*/
|
||||
export function mergeBodySections(current: string, incoming: string): { body: string; replaced: string[]; kept: string[]; added: string[] } {
|
||||
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 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 used = new Set<Section>();
|
||||
const replaced: string[] = [], kept: string[] = [], added: string[] = [];
|
||||
const merged: Section[] = now.map(s => {
|
||||
const managed = next.some(n => n.key.startsWith("h2:"));
|
||||
const preamble = (list: Section[]) => {
|
||||
const end = list.findIndex(s => s.key.startsWith("h2:"));
|
||||
return list.slice(0, end < 0 ? list.length : end).filter(s => s.key.startsWith("text:") && s.text.trim() !== "");
|
||||
};
|
||||
const stale = managed ? preamble(now) : [];
|
||||
const ours = managed ? preamble(next) : [];
|
||||
const role = (s: Section) => (s.key === "text:lead" ? "lead" : "trailing");
|
||||
const paired = new Set<Section>();
|
||||
for (const r of ["lead", "trailing"]) {
|
||||
const theirs = ours.filter(n => role(n) === r);
|
||||
stale.filter(s => role(s) === r).slice(0, theirs.length).forEach((s, i) => { paired.add(s); paired.add(theirs[i]!); });
|
||||
}
|
||||
const used = new Set<Section>(ours);
|
||||
const replaced: string[] = [], kept: string[] = [], added: string[] = [], dropped: string[] = [];
|
||||
const merged: Section[] = [];
|
||||
for (const s of now) {
|
||||
if (stale.includes(s)) {
|
||||
const line = s.text.trim().split(/\r?\n/)[0]!;
|
||||
if (paired.has(s)) replaced.push(s.label);
|
||||
else dropped.push(`${s.label} ("${line.length > 60 ? line.slice(0, 59).trimEnd() + "…" : line}")`);
|
||||
continue;
|
||||
}
|
||||
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;
|
||||
});
|
||||
if (!match) { if (s.text.trim()) kept.push(s.label); merged.push(s); continue; }
|
||||
used.add(match); replaced.push(s.label); merged.push(match);
|
||||
}
|
||||
const fresh = next.filter(n => !used.has(n));
|
||||
for (const n of fresh) added.push(n.label);
|
||||
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:"));
|
||||
if (now.length === 0) return { body: incoming, replaced, kept, added };
|
||||
const all = [...first, ...merged, ...fresh.filter(n => n.key.startsWith("h2:"))];
|
||||
if (now.length === 0) return { body: incoming, replaced, kept, added, dropped };
|
||||
const all = [...ours.filter(n => role(n) === "lead"), ...first, ...merged, ...fresh.filter(n => n.key.startsWith("h2:"))];
|
||||
for (const t of ours.filter(n => role(n) === "trailing").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) => {
|
||||
const after = all[i + 1];
|
||||
@@ -236,7 +265,7 @@ export function mergeBodySections(current: string, incoming: string): { body: st
|
||||
const seam = (fromFile(s) || fromFile(after)) && s.text.trim() !== "" && !/^\r?\n/.test(after.text);
|
||||
return seam ? s.text.replace(/\n*$/, "\n\n") : s.text.endsWith("\n") ? s.text : s.text + "\n";
|
||||
}).join("");
|
||||
return { body, replaced, kept, added };
|
||||
return { body, replaced, kept, added, dropped };
|
||||
}
|
||||
|
||||
/** The PR's current body for a merge; GitHub's `--jq .body` prints it raw, GitLab's JSON carries `description`. */
|
||||
@@ -508,7 +537,7 @@ export function runPost(argv: string[], env: PostEnv, configBin = "gstack-config
|
||||
const m = mergeBodySections(current.body, body!);
|
||||
body = m.body;
|
||||
const list = (names: string[]) => (names.length ? names.join(", ") : "none");
|
||||
env.err(`gstack-post: pr-body merged by section: replaced ${list(m.replaced)}; kept ${list(m.kept)}; added ${list(m.added)}\n`);
|
||||
env.err(`gstack-post: pr-body merged by section: replaced ${list(m.replaced)}; kept ${list(m.kept)}; added ${list(m.added)}${m.dropped.length ? `; dropped ${m.dropped.join(", ")}` : ""}\n`);
|
||||
}
|
||||
if (body !== undefined && req.attribute) body = attributionCallout(req.attribute) + body;
|
||||
const parts: Array<[Part, string]> = [];
|
||||
|
||||
@@ -446,11 +446,60 @@ describe("gstack-post pr-body merges the PR body by section", () => {
|
||||
expect(after.stderr()).toBe("gstack-post: pr-body merged by section: replaced ship receipt, Why; kept unclosed receipt fence; added none\n");
|
||||
});
|
||||
|
||||
// Value: protects=a file with sections owns the text above the first heading (GST-412): stale lead text and stale text between the receipt and the
|
||||
// first heading are dropped and named on stderr with their first line, the file's own preamble takes their place, a receipt-only or text-only file
|
||||
// keeps the older merge, a repost is byte-identical and an attribution callout is never doubled; fails_when=pr-body keeps a stale intro above the
|
||||
// receipt or a stale note under it after /ship reposts the sections; why_new=lead text was kept unless the file brought its own; seam=fake run;
|
||||
// no_claim=real gh
|
||||
test("a file with sections replaces every text section above the current first heading and names each dropped piece", () => {
|
||||
const stale = withBody("Draft intro from gh pr create, long enough to need truncation in the summary line\nsecond line\n\n" + CURRENT, { "/b": RECEIPT("bbbb") + "\n## What changed\n\n- new\n" });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], stale)).toBe(EXIT.posted);
|
||||
expect(sent(stale).input).toBe(RECEIPT("bbbb") + "\n## What changed\n\n- new\n\n## Why\n\nBecause.\n\n## Approvals\n\n- owner: do it\n");
|
||||
expect(stale.stderr()).toBe("gstack-post: pr-body merged by section: replaced ship receipt, What changed; kept Why, Approvals; added none; dropped leading text (\"Draft intro from gh pr create, long enough to need truncati…\")\n");
|
||||
|
||||
const between = withBody(RECEIPT("aaaa") + "Old note under the receipt\n\n## What changed\n\n- old\n", { "/b": "## What changed\n\n- new\n" });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], between)).toBe(EXIT.posted);
|
||||
expect(sent(between).input).toBe(RECEIPT("aaaa") + "\n## What changed\n\n- new\n");
|
||||
expect(between.stderr()).toBe("gstack-post: pr-body merged by section: replaced What changed; kept ship receipt; added none; dropped trailing text (\"Old note under the receipt\")\n");
|
||||
|
||||
const own = withBody("Stale intro\n\n" + RECEIPT("aaaa") + "Stale note\n\n## Why\n\nold\n", { "/b": "Fresh intro\n\n" + RECEIPT("bbbb") + "Fresh note\n\n## Why\n\nnew\n" });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], own)).toBe(EXIT.posted);
|
||||
expect(sent(own).input).toBe("Fresh intro\n\n" + RECEIPT("bbbb") + "\nFresh note\n\n## Why\n\nnew\n");
|
||||
expect(mergeBodySections(sent(own).input!, "Fresh intro\n\n" + RECEIPT("bbbb") + "Fresh note\n\n## Why\n\nnew\n").body).toBe(sent(own).input!);
|
||||
expect(own.stderr()).toBe("gstack-post: pr-body merged by section: replaced leading text, ship receipt, trailing text, Why; kept none; added none\n");
|
||||
});
|
||||
|
||||
test("receipt-only and text-only files keep the older merge; a repost is byte-identical; a callout in the preamble is never doubled", () => {
|
||||
const lead = "Human intro\n\n" + CURRENT;
|
||||
const receiptOnly = withBody(lead, { "/b": RECEIPT("bbbb") });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], receiptOnly)).toBe(EXIT.posted);
|
||||
expect(sent(receiptOnly).input).toBe(lead.replace(RECEIPT("aaaa"), RECEIPT("bbbb")));
|
||||
expect(receiptOnly.stderr()).toBe("gstack-post: pr-body merged by section: replaced ship receipt; kept leading text, What changed, Why, Approvals; added none\n");
|
||||
|
||||
const textOnly = withBody(RECEIPT("aaaa") + "Old note\n\n## Why\n\nx\n", { "/b": "New intro\n" });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], textOnly)).toBe(EXIT.posted);
|
||||
expect(sent(textOnly).input).toBe("New intro\n\n" + RECEIPT("aaaa") + "Old note\n\n## Why\n\nx\n");
|
||||
expect(textOnly.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept ship receipt, trailing text, Why; added leading text\n");
|
||||
|
||||
const composed = "Intro\n\n" + RECEIPT("bbbb") + "\n## Summary\n\nnew\n\n## Test plan\n\n- [x] bun test\n";
|
||||
const again = mergeBodySections(composed, composed);
|
||||
expect(again.body).toBe(composed);
|
||||
expect([again.added, again.dropped]).toEqual([[], []]);
|
||||
|
||||
const callout = attributionCallout("Claude Opus on behalf of octocat");
|
||||
const withCallout = callout + RECEIPT("bbbb") + "\n## Summary\n\nnew\n";
|
||||
const reposted = mergeBodySections(callout + RECEIPT("aaaa") + "\n## Summary\n\nold\n", withCallout);
|
||||
expect(reposted.body).toBe(withCallout);
|
||||
expect(reposted.body.split("[!NOTE]")).toHaveLength(2);
|
||||
expect(mergeBodySections(reposted.body, withCallout).body).toBe(withCallout);
|
||||
});
|
||||
|
||||
// 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
|
||||
// section, an empty current body posts the new body byte for byte, the scan runs on the merged bytes, and an unreadable current body is exit 3 with
|
||||
// the --replace hint rather than an overwrite; fails_when=--replace merges, headingless text is dropped, the merge changes a lone new body, a secret in a
|
||||
// kept section passes the scan, or a failed read overwrites; why_new=first pr-body merge tests; seam=fake run; no_claim=real gh
|
||||
test("--replace overwrites; a body without H2 headings is kept as a leading section; the merged bytes are what is scanned; a failed read refuses", () => {
|
||||
// section by a file without sections and dropped (and named) by a file with sections (GST-412), an empty current body posts the new body byte for byte,
|
||||
// the scan runs on the merged bytes, and an unreadable current body is exit 3 with the --replace hint rather than an overwrite; fails_when=--replace
|
||||
// merges, headingless text is dropped by a receipt-only post or kept under new sections, the merge changes a lone new body, a secret in a kept section
|
||||
// passes the scan, or a failed read overwrites; why_new=first pr-body merge tests; seam=fake run; no_claim=real gh
|
||||
test("--replace overwrites; a body without H2 headings is kept by a file without sections and dropped by one with sections; the merged bytes are what is scanned; a failed read refuses", () => {
|
||||
const replaced = withBody(CURRENT, { "/b": "## What changed\n\nonly this\n" });
|
||||
expect(run(["pr-body", "5", "--replace", "--body-file", "/b"], replaced)).toBe(EXIT.posted);
|
||||
expect(sent(replaced).input).toBe("## What changed\n\nonly this\n");
|
||||
@@ -459,8 +508,12 @@ describe("gstack-post pr-body merges the PR body by section", () => {
|
||||
|
||||
const plain = withBody("Just a paragraph.\nAnother line.", { "/b": "## Tests\n\n- one\n" });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], plain)).toBe(EXIT.posted);
|
||||
expect(sent(plain).input).toBe("Just a paragraph.\nAnother line.\n\n## Tests\n\n- one\n");
|
||||
expect(plain.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept leading text; added Tests\n");
|
||||
expect(sent(plain).input).toBe("## Tests\n\n- one\n");
|
||||
expect(plain.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept none; added Tests; dropped leading text (\"Just a paragraph.\")\n");
|
||||
const plainText = withBody("Just a paragraph.\nAnother line.", { "/b": RECEIPT("bbbb") });
|
||||
expect(run(["pr-body", "5", "--body-file", "/b"], plainText)).toBe(EXIT.posted);
|
||||
expect(sent(plainText).input).toBe(RECEIPT("bbbb") + "\nJust a paragraph.\nAnother line.");
|
||||
expect(plainText.stderr()).toBe("gstack-post: pr-body merged by section: replaced none; kept leading text; added ship receipt\n");
|
||||
|
||||
const empty = withBody("", { "/b": "new body" });
|
||||
expect(run(["pr-body", "6", "--body-file", "/b"], empty)).toBe(EXIT.posted);
|
||||
@@ -593,8 +646,8 @@ describe("gstack-post pr-body merges the PR body by section", () => {
|
||||
expect(fresh.added).toEqual(["leading text"]);
|
||||
const receipt = "```gstack-ship-receipt\n{\n \"head\": \"a\"\n}\n```\n";
|
||||
const afterReceipt = mergeBodySections("Intro line\n\n## Summary\n\nold\n", receipt + "note\n\n## Summary\n\nnew\n");
|
||||
expect(afterReceipt.body).toContain("Intro line\n");
|
||||
expect(afterReceipt.body).toContain("note\n");
|
||||
expect(afterReceipt.body).toBe(receipt + "\nnote\n\n## Summary\n\nnew\n");
|
||||
expect(afterReceipt.replaced).toEqual(["Summary"]);
|
||||
expect(afterReceipt.dropped).toEqual(['leading text ("Intro line")']);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user