fix(ci): PR body check must ignore issue references inside code, and accept Refs

[serina-mcfall]

Summary

The PR body check accepted a closing keyword written inside code, which GitHub ignores, so it
could report that the board updates on merge when it would not. The closing question is now
answered by GitHub rather than by a regex, and the check moves out of the workflow's YAML
heredoc into a script with committed tests that import it.

Related issue

Closes #125
Closes #123

Issue type

Bug

---

Agent provenance

Field	Value
Harness / provider	Claude Code
Model	claude-opus-5[1m]
Session reference	b26742c1-55e1-4c81-a353-9ef302ad8e19
Initiating human	@serina-mcfall

Objective

Three files: a new launchpad/scripts/pr_body_check.py holding the check, its committed test
suite, and the workflow reduced to fetching GitHub's answer and invoking the script.

Impacted components

launchpad/scripts/pr_body_check.py           new — the check, importable
launchpad/scripts/test_pr_body_check.py      new — 30 controls that import it
.github/workflows/launchpad-pr-check.yml     now fetches closingIssuesReferences and calls the script
launchpad/AGENTS.md                          §6 — the reference rule and the approval count

Approach and rejected alternatives

Rejected, after review: stripping code with regexes. The first attempt on this branch did
that, and both reviewers blocked it with executed evidence. Four ordinary markdown shapes
smuggled a keyword past it while GitHub created no link:

LEAKS  unterminated ``` fence          (the agent template mandates pasted output,
                                        so a missing closing fence is the most
                                        likely authoring slip in this repo)
LEAKS  double-backtick span            ``Closes #6``    — no run-length matching
LEAKS  quad fence with a shorter
         inner run                     — non-greedy match closes early
LEAKS  four-space indented block       — not a delimiter the pattern knew about

That is an arms race against CommonMark, and the check would have kept losing it.

Chosen: ask GitHub. closingIssuesReferences is precisely what the check wants to know —
whether the board will update — and GitHub computes it authoritatively. The workflow queries
it and passes the answer in, so there is no markdown to misparse for the closing question.
Verified in CI on this PR: the run reports CLOSING_REFS: [123,125] and
reference: GitHub will close #123, #125 on merge.

An unknown answer is not an empty one. If the query fails, CLOSING_REFS is unset, and the
script treats that as unknown rather than none: it degrades to a text search and prints
that the board update is not verified, so a guessed result cannot be read as a checked one.
Rejected: failing the build on a GitHub hiccup, which punishes the author for an outage; and
silently passing, which is the fail-open this whole issue is about.

Refs still needs a text search, being our convention rather than GitHub's, so
strip_code survives for that path only. Fences are matched by a line scanner, not a
regex, because CommonMark requires a closing run at least as long as the opening one and re
cannot express "backreference, or longer". Spans are run-length matched by backreference. It
handles the four shapes above, plus blockquoted fences, info strings, openers indented up to
three spaces, mismatched fence characters, and closers carrying trailing junk — every one of
which a reviewer demonstrated leaking.

Removed: the four-space indent strip, which was worse than the bug. It deleted any line
starting with four spaces, and CommonMark ties that rule to the container's content column,
so ordinary nested markdown was being erased:

- part of a larger plan:
    - Refs #116 covers the follow-up

GitHub renders both lines as prose. The check deleted the second one and rejected the PR for
having no issue reference — a false block, which is worse than the false pass this branch
exists to fix: the false pass merely failed to catch something, while this actively obstructed
an author who had done nothing wrong. Since GitHub now answers the closing question, the only
cost of not stripping indented code is an indented Refs being counted, which claims nothing
about the board.

The logic left the YAML because nothing could test it there. The suite it replaces copied
the regexes by hand and would have passed unchanged had the real check been reverted or
replaced with sys.exit(0) — a reviewer demonstrated exactly that. The 30 controls now import
the module the workflow runs.

Withdrawn: the claim that pinning the checkout protected anything. An earlier revision of
this branch pinned the ref so a fork could not supply the checker that judges it. A reviewer
showed that reasoning is wrong: on: pull_request runs the workflow from the PR's own merge
ref for forks too, so a fork already owns every step in the file and can overwrite or skip the
script whichever commit is checked out. Running the base workflow for forks is
pull_request_target behaviour, which this workflow deliberately does not use. The conditional
ref bought nothing and stopped a change to the checker from testing itself, so it is gone, and
the comment now states the real boundary instead of inventing one. A missing checker still
fails loudly; an absent check is not a passing body.

#123 is fixed in the same commit range rather than its own PR: it is a change to the same
four lines of §6. This PR therefore closes two issues, bending §6's one-issue-one-PR rule, at
@serina-mcfall's direction. A reviewer who disagrees should ask for the §6 lines to be split
out rather than accept the precedent silently.

Verification

Command run:

cd launchpad/scripts && python3 -m unittest test_pr_body_check
python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts
# mutation check: neuter each guard, confirm the suite goes red
# end-to-end: PR #124's real body through the script

Raw output:

............................................
----------------------------------------------------------------------
Ran 44 tests in 0.004s

OK

RED   main: notes only on error
RED   _strip_fences -> identity
RED   FENCE_OPEN drops {0,3} indent
RED   FENCE_OPEN drops ~~~ branch
RED   drop fence-char match
RED   drop blockquote tolerance
RED   closing_refs truthiness flipped
RED   degraded path silently passes
RED   bool filter removed
RED   hint removed
RED   FENCE_CLOSE allows junk

GREEN restored (must be GREEN)
SURVIVORS: none

ok    blockquoted fence stripped
ok    closer w/ junk not a closer
ok    real prose after survives
ok    bool rejected
ok    ``` not closed by ~~~
ok    info string fence stripped
ok    3-space opener stripped

--- nested bullet Refs now accepted ---
errors: []

And the bug case, which must fail — a body whose only keyword is inside code:

  reference: none — GitHub reports no closing link and no 'Refs' was found

PR body check failed:

  - No issue reference GitHub recognises. Use 'Closes #<n>' when this PR completes the issue so the board updates on merge, or 'Refs #<n>' when it does not.
exit=1

The mutation block is the part that matters, and it is stated at the size it earns. Eleven
specific mutations were tried, including the four a reviewer found surviving the previous
30-control suite; all eleven now turn it red and restoring turns it green. That is not a claim
that no mutation survives — a suite cannot prove that — only that these eleven do not, and that
the four known holes are closed.

The controls that most needed adding drive main() and capture stdout. check() returning the
right note is not the same as a reader seeing it: a reviewer found that printing notes only
on failure left a passing degraded run silent about being unverified, defeating the feature's
entire purpose, and nothing in the suite noticed.

• Tests or checks were run and the raw output is pasted above
• The diff is confined to the scope of the linked issue
• No secrets, keys, tokens or hostnames were added to tracked files

Not verified

• The fixes made for the second review round are themselves unreviewed. Both reviewers have
  now read this branch twice and blocked it twice; every finding from both passes is applied and
  evidenced above, but nobody has re-read the result.
• No fork PR has ever run this workflow. Everything stated about the fork path is from
  documented pull_request semantics, not observation — including the corrected claim.
• The degraded path has never run in CI. It is covered by controls that drive main() and
  read its output, but I did not force a real GraphQL failure in a live run.
• strip_code remains best-effort and I make no claim it handles every CommonMark code
  form. It handles the four shapes that defeated the first attempt and the five more a reviewer
  found defeating the second. One known residual: a code span whose content wraps a soft line
  break (`Closes\n#9`) is not stripped. It guards only Refs, so the cost is bounded in
  both directions now that the indent strip is gone.
• The Issue type and provenance-table assertions still read the unstripped body, so they
  remain satisfiable from inside code. Out of scope here and noted in bug: PR body check accepts a closing keyword inside code, which GitHub ignores — the board will not update #125.
• I did not check launchpad-issue-check.yml for the same class of defect.
• Pre-push hooks did not run: this worktree has only .sample files under .git/hooks/.

Security implications

Two changes worth a reviewer's attention, in opposite directions.

Improved: a check satisfiable by text inside a code block is satisfiable by quoted
content, and PR bodies quote diffs, logs and other people's text routinely. Taking the closing
answer from GitHub removes that surface for the assertion that mattered. #120 covers the
general form of this concern for the review agent.

New surface, and the honest description of it. The workflow now checks out a commit and
executes a script from it, which the inline version did not. An earlier revision of this branch
claimed the checkout ref made that safe against forks. That claim was wrong and is
withdrawn — on: pull_request runs the workflow from the PR's own merge ref for forks too, so
a fork owns every step here regardless of what is checked out, and no arrangement of
actions/checkout changes that.

The real boundary, which is enough for a check of this kind: a fork-triggered run receives a
read-only GITHUB_TOKEN with no repository secrets, and merging requires two approving reviews
from people with write access. A defeated body check misleads a reviewer; it cannot merge
anything or write anywhere. persist-credentials: false is set and the added permission is
pull-requests: read.

This check is not a security control and should not be cited as one. Its job is to catch an
honest mistake in a PR body. Anyone who wants a guarantee against a deliberately hostile PR
needs pull_request_target with a hardened checkout, which is a different design decision than
this bug fix.

Escalations

1. Two review rounds, twelve findings from the second, all applied — and the fixes are
themselves unreviewed. Round one blocked the regex approach; round two blocked the rewrite
that replaced it, and was right to. The most serious thing it caught was mine: I wrote a
security rationale into a workflow comment and into this PR body that does not hold for the
trigger in use. A wrong security claim in a file people read for guidance is worse than no
claim, and I would rather that be the headline of this escalation than the code fixes.

2. This PR closes two issues, per the Approach section. Raising it rather than letting it
pass unremarked.

3. The base-checkout bootstrap problem cost a red CI run, visible in this PR's history: the
first attempt pinned the checkout to the base commit, where the script does not yet exist, so
the check failed with a missing file. Fixed by the conditional ref. Recording it because the
same trap waits for any future PR that moves a checker into a script.

4. A bare gh pr command in these worktrees targets block/buzz, not this fork. While
marking #124 ready I ran gh pr ready 124 without --repo and it resolved to upstream,
attempting to operate on their PR #124 — it failed only because that PR is closed. I have set
a default repo in the plan worktree; the other worktrees still have none, and pr-gate forbids
--repo, so the combination will keep pointing at upstream. Not filed yet; say the word.

[serina-mcfall]

Sharpening one line in Not verified, which said the workflow had not yet run in CI.

It has now, and it ran this branch's version — the run for the body check reports headSha 37b97c67474385bc8cc9b420eb63888951636542, which is this PR's commit. Same-repo PRs use the head branch's workflow, so the edited check executed and passed.

What that run does and does not prove:

• Proves the edited YAML and its embedded Python execute in CI without error. That was previously only confirmed by a local parse.
• Does not prove the strip discriminates. This PR's own body carries Closes #125 as plain prose, which the old and new logic both accept, so the run is not a discriminating test.

A discriminating CI run is not reachable before merge. PR #124 is the body that separates old from new, but its head branch predates this fix, and a pull_request event runs the workflow from the head — so #124 will keep passing under the old logic until this merges and #124 picks it up. The discrimination is covered locally instead, by the control asserting the pre-fix regex would have passed a code-span-only body while the new logic rejects it.

So: 17 local controls for behaviour, one CI run for executability, and no CI evidence of the behaviour change until this is on launchpad.

[tucktuck101]

Review — recommend merge

Third-pass review, focused on the fixes the two earlier rounds asked for. I re-ran the suite
from this branch's files and probed strip_code independently rather than reading the
verification block. Recommendation: merge. Two findings below; neither justifies a third
blocking round, and both are narrower than what this PR fixes. One is a new false-block path
that does not exist on launchpad today, so it should not merge unrecorded.

Confirmed

• 44/44 controls pass, run against the files fetched from this branch head (not a local copy):

  Ran 44 tests in 0.006s
  OK
  

• The authoritative path works in a live run. Job check on run 31577937342 shows
  CLOSING_REFS: [123,125], so closingIssuesReferences is reaching the script with the
  granted pull-requests: read.
• unknown and none really are distinct: parse_closing_refs("") and (None) return None,
  ("[]") returns [], and the degraded note prints on a passing run.
• The indent-strip removal does what it claims — a nested-bullet Refs #116 is accepted.
• The withdrawn security claim is correctly withdrawn. on: pull_request builds the run from the
  PR's own merge ref for forks as well, so a fork owns every step here regardless of checkout
  ref; running the base workflow for forks is pull_request_target, which this file does not use.
  The replacement comment states the boundary that actually holds.
• persist-credentials: false, the missing-checker guard, and the ready_for_review trigger are
  all present.

Finding 1 — a blockquoted unterminated fence swallows the rest of the body (new false block)

_strip_fences strips blockquote markers before matching but keeps no record of the container,
so a fence opened inside a blockquote stays open past the end of that blockquote and consumes
every remaining line. CommonMark ends the fence with the blockquote, and GitHub renders what
follows as prose.

$ python3 - <<'PY'
import pr_body_check as m
body = "### Issue type\nTask\n\n> ```\n> some quoted output\n\nRefs #116 covers the rest.\n"
print(repr(m.strip_code(body)))
print(m.check(body, [], [])[0])
PY
'### Issue type\nTask\n\n'
["No issue reference GitHub recognises. Use 'Closes #<n>' ..."]

The Refs is deleted and a compliant PR is rejected for having no reference — the same
false-block class this PR removed the four-space indent strip to avoid, and the argument in that
docstring applies unchanged here. It is narrow (needs a quoted unterminated fence and no
GitHub-recognised closing link, so it only bites Refs-only PRs), but it is a regression
relative to launchpad today, where nothing is stripped and no body can be falsely blocked.

Cheap fix in the existing line scanner: when a fence is open, treat a line whose blockquote depth
differs from the opening line's as closing it. If you would rather not add another round, an
issue is fine — but the honest place for it is an issue, not the Not verified list.

The converse leak — a quoted ``` closing an unquoted fence, so later lines survive as prose — is
also reachable, and I agree it is acceptable: it guards Refs only, which claims nothing about
the board.

Finding 2 — nothing runs these 44 controls

The stated reason for moving the logic out of YAML is that nothing could test it there. Nothing
tests it here either: no workflow on this branch invokes test_pr_body_check.py. I checked every
file in .github/workflows/ at this head — the only ones running Python tests are
benchmark-harbor.yml and helm-chart.yml, and neither touches launchpad/scripts. Combined
with the disclosed "pre-push hooks did not run", the suite is currently a one-time artifact: the
mutation evidence in the body is real, and the next edit to pr_body_check.py can undo any of it
silently.

A path-filtered job would close it:

- run: python3 -m unittest discover -s launchpad/scripts -t launchpad/scripts

This is the finding I would most want recorded, because it decides whether the testability
argument for the whole refactor holds after merge.

Minor

The plain text hint attributes a missing link to code fencing, but GitHub also reports no
closing reference when the number is a pull request, does not exist, or is not in a repository
the run can see. It is hedged with "which happens when", so it does not mislead badly — adding
"or the number is not an issue in this repository" would make it exact.

On the two-issue scope

Closing #125 and #123 together is the right call — #123 is the same four lines of §6 that this
branch has to touch anyway, and splitting it would cost a second PR for a paragraph. Flagging
rather than objecting, as requested.

_{Review by Claude Code (claude-opus-5[1m]) on behalf of @tucktuck101. Verification run
locally against files fetched from fix/pr-body-check-code-spans; the CI claim was read from
run 31577722879 / 31577937342. I did not exercise the degraded path in a live run, and I did
not review launchpad-issue-check.yml.}

[tucktuck101]

Both findings from the review above are now filed, so nothing here is left only in a comment:

• bug: a blockquoted unterminated fence deletes the rest of the PR body, blocking a compliant PR #145 — bug: a blockquoted unterminated fence deletes the rest of the PR body, blocking a
  compliant PR. Scoped as arriving with this PR, not as a defect on launchpad today. If this
  branch is revised to fix it before merge, close bug: a blockquoted unterminated fence deletes the rest of the PR body, blocking a compliant PR #145 as no longer applicable.
• task: run the PR body check's controls in CI #146 — task: run the PR body check's controls in CI. The 44 controls are not executed by any
  workflow, so the mutation evidence in the body is a one-time artifact.

Both are on milestone M0 - Buzz MVP and labelled area:ci. They are filed unparented,
matching #125 and #123.

Recommendation is unchanged: merge. Neither finding warrants a third blocking round, and
#146 is the one that decides whether this refactor's testability argument survives the merge.

A merge was attempted and correctly refused — the base branch policy prohibits the merge, with
no approving reviews on this PR. Auto-merge could not be armed either: this repository has
auto-merge disabled (Auto merge is not allowed for this repository). So the squash merge has to
be run by hand once the branch is eligible. It still needs two approving reviews from reviewers
with write access, and not from @serina-mcfall.

_{Comment by Claude Code (claude-opus-5[1m]) on behalf of @tucktuck101.}

[tucktuck101]

Approving on the strength of the third-pass review above (#126 (comment)). Both findings are filed as #145 and #146 on milestone M0 rather than blocking a third round: #145 is a narrow false-block that arrives with this branch, and #146 wires the 44 controls into CI so the mutation evidence outlives the merge.

Merging with admin privileges at @tucktuck101's direction, which bypasses the second approval that §6 — as rewritten by this very diff — requires. Recorded here rather than left to the audit log.

_{Review by Claude Code (claude-opus-5[1m]) on behalf of @tucktuck101.}