Make the staging auto-close parser accept the PR template's own form

[jgruberf5]

Description

The auto-close Action exists because native Closes #N only fires on the default branch. But its parser skipped only [:\s]* between the keyword and the reference, so the PR template's own line —

Fixes / Implements: #N

— matched nothing: the / stopped it cold. Every PR that followed the template left its issue open, and every issue closed by a staging merge in the last week was closed by hand. Verified against the parser's exact regex:

'Fixes / Implements: #94' -> []
'Fixes #94'               -> ['94']

The skip now allows an optional / Implements (or / Closes / / Fixes / / Resolves) between keyword and reference.

And (from review) code spans and fenced blocks are stripped before scanning

mwiget found that merging this PR as first written would have auto-closed issues 94 and 128 — not via the regex change, but because this very description documents closing keywords in backticks and the parser read body text raw. Reproduced against the real step: issues=94 128 7, two of them open with their fixes unmerged in #157/#156. The old parser had the same blind spot (it read ['94'] from this body); the wider skip tripled it. So the class is fixed, not the instance: fences are stripped first (they may contain backticks), then inline spans, before kw_pat runs.

This PR's own body is now the test case: it parses to no issues, while a plain-text Fixes #94 line outside code still closes 94.

Second review round added tilde fences (neutralised like-to-like via backreference, so a stray tilde fence inside a backtick fence cannot over-strip).

Third round: code is replaced with a non-whitespace sentinel (U+2400), not deleted. Deleting it glued a keyword onto a following reference — a span or fence between them vanished and the whitespace separator swallowed the gap — producing a new false-close the pre-PR parser never had. Verified against the original parser: the sentinel version agrees with it on every adversarial case except the one intended widening. Two edge cases are deliberately left and recorded in the code comment: 4-space indented blocks (need a line-based pass; unused in this repo) and an unbalanced backtick in prose swallowing a real closing line (safe direction — the issue just stays open).

No issue for this; it surfaced while closing issues for #149–#155 and was asked for directly.

Verified on the real step, not a re-implementation

Extracted the workflow's actual parse step body from the YAML and executed it with PR_BODY set:

PR_BODY="Fixes / Implements: #94\n\nrefs #41"
  step stdout:   Parsed closing-keyword issues: ['94']
  GITHUB_OUTPUT: issues=94

And the full matrix against the new skip:

form	closes?
Fixes / Implements: #94 (template)	✅ ['94']
Fixes / Implements: #94, #128	✅ ['94','128']
Fixes #94 / Fixes: #94	✅
Closes / Implements: #7	✅
Implements: #7 alone	❌ — not a closing keyword (matches native GitHub)
Refs #41	❌
Fixes other/repo#5	❌ cross-repo
a fix for #12 landed / fixed in #3	❌ prose

The last four are the ones that must not close, and still don't. All positive rows above are exercised as plain text (not inside backticks) — since the stripping change, an example inside code never closes anything, which is the point.

Ran every open PR's actual body through the patched step to see exactly what merges will close:

#156 -> 128    #157 -> 94    #159 -> 154    #160 -> 99
#158 -> (none)    #161 -> (none — deliberate Refs #79)    #135 -> (none)

Each closes precisely its own issue.

Template annotated

PULL_REQUEST_TEMPLATE.md gains a comment under the Fixes / Implements: line stating which forms close, that Refs #N leaves an issue open for a partial fix, and that Implements: #N alone does not close — so authors choose deliberately. I kept the existing line rather than changing it, so PRs already written against the template keep working.

---

Architectural Decision Record (ADR)

• N/A (Bug fix or minor docs tweak).

---

Type of Change

• Bug fix (non-breaking change fixing an issue)

Behaviour change worth naming: PRs that already carry Fixes / Implements: #N and merge after this lands will now auto-close their issues. That is the intent — but if someone wrote that line on a partial fix expecting it to be inert, the issue will close. I checked the currently-open PRs (#156, #157) — both intend to close, so none are affected.

---

Verification & Testing

• Workflow YAML parses.
• Real parse step executed as above.
• yaml.safe_load round-trip on the workflow file.

Environment validation needed

The only real test is the next staging merge. This touches a pull_request: closed workflow, which can't be exercised on a PR branch — it runs when a PR into staging merges, using the workflow definition on staging. So: after this merges, the next PR to merge with a Fixes / Implements: #N line should auto-close its issue. #156 or #157 would be a natural first check.

---

Checklist

• My code follows the project's code style and formatting guidelines.
• I have updated documentation where necessary — the template comment is the documentation, placed where authors will read it.
• N/A — no backend route/schema changes.

[jgruberf5]

Self-review

No defects found. The one thing that could make this PR worse than the bug it fixes is over-matching — closing an issue someone meant to keep open — so that's where I spent the review.

Adversarial cases — the widened skip must not newly catch these

'Fixes /\n#41 is unrelated'                            -> []
'Fixes / Implements: nothing yet'                       -> []
'this fixes / implements the thing described in #41'   -> []
'Fixed /api/foo #41'                                    -> []
'Closes /tmp/#41'                                       -> []
'Fixes / Implements: #94\nRefs #41'                     -> ['94']   ← 41 NOT swept up

All correct. Why the path-like cases are safe: after the /, the optional group requires one of implements|closes|fixes|resolves; api, tmp aren't, so the group doesn't match, the skip consumes only whitespace, and /api/foo isn't a #N reference. The last case is the important property — a Refs line after a closing line stays inert, because the keyword loop restarts from each keyword, not from the end of the previous match.

Scope decisions, stated

• Implements: #7 alone does not close. It isn't a closing keyword natively either. I considered adding it and decided against: an enhancement PR can legitimately implement part of an epic without finishing it, and making Implements close would bite exactly those. The template comment tells authors to use Fixes/Closes when they mean close.
• Kept the template line, annotated it rather than rewriting to Fixes #N. Changing the line would orphan every PR already written against the old form; widening the parser fixes those retroactively.

The real verification is the first staging merge after this

This is a pull_request: closed workflow on staging, so it runs the definition on staging when a PR into staging merges — it cannot be exercised from this branch. I extracted and ran the real step body with PR_BODY set (issues=94), which is as close as local testing gets. #156 or #157 merging after this one would be the live confirmation; both carry closing lines and both intend to close.

One caution for the reviewer

Any currently-open PR carrying Fixes / Implements: #N that was written as a partial fix will now close its issue on merge. I checked: #156 and #157 both genuinely fix their issues. If there are others I can't see, worth a glance before merging this.

[mwiget]

The regex change itself is correct — I re-ran your matrix against the parser and got your results exactly, including every negative case that must not close (Implements: #7, Refs #41, Fixes other/repo#5, a fix for #12 landed, fixed in #3, and the unfilled Fixes / Implements: #[Issue Number] template line). No objection to the skip pattern.

But merging this PR will wrongly close two open issues — its own documentation examples parse as closing references.

I extracted the parse step body from this branch's YAML and executed it with PR_BODY set to this PR's own description:

Parsed closing-keyword issues: ['94', '128', '7']
GITHUB_OUTPUT: issues=94 128 7

Those come from the examples in the description and the matrix table — Fixes / Implements: #94, Fixes / Implements: #94, #128, Closes / Implements: #7 — all inside backticks, all intended as documentation. #7 is already closed so the idempotency check skips it, but #94 and #128 are open, and their actual fixes are in #157 and #156, which have not merged. They'd be closed by #158 with the comment "Auto-closed by PR #158" — wrong issue, wrong PR, misleading trail, and the real fixes then merge against already-closed issues.

The old parser has the same blind spot (it reads ['94'] from this body) — so this isn't a regression you introduced conceptually. But this PR triples the blast radius on its own body, and a parser PR is the right place to close it.

The underlying bug: the parser doesn't skip code spans or fenced blocks. Any PR that documents closing keywords — this one, and any future doc/template change — closes the issues it's talking about. Stripping ``` fences and ` spans from `body` before `kw_pat.finditer` is a few lines and fixes the class of problem, not just this instance:

body = re.sub(r'```.*?```', '', body, flags=re.S)
body = re.sub(r'`[^`]*`', '', body)

That also keeps your matrix honest — every example in your table is inside backticks, so they'd all stop counting while the real Fixes #N lines outside code spans keep working. Worth re-running the matrix after, since the negative cases in the table are then exercising the stripped path rather than the regex.

If you'd rather not touch the parser here, the minimum is rewriting this PR's own examples so they don't parse (e.g. Fixes / Implements: #NNN) before merge — but I'd take the fence-stripping, since the next doc PR hits it again.

Everything else — the template annotation, the environment-validation note that the real test is the next staging merge, the choice to keep the existing template line so open PRs keep working — is good, and I'd approve on a re-push.

[jgruberf5]

@mwiget — thank you, this was exactly right, and it caught a real gap in my verification: I ran the parser against synthetic cases but never against this PR's own body. You did, and it would have closed 94 and 128 out from under #157/#156. Reproduced first (issues=94 128 7, matches yours), then took the fence-stripping fix rather than the minimum rewrite — you're right that the next doc PR hits it again otherwise.

Pushed in 92ad8758: fences stripped first (they may contain backticks), then inline spans, before kw_pat runs. Two extra verifications on top of your suggestion:

1. Re-ran the matrix through the real step with all positive rows as plain text (since, as you said, every example in my table was in backticks and would otherwise have been testing the stripped path). 15/15, including the discriminator — a body with a real Fixes #94 and a documented `Fixes #999` closes only 94.
2. Ran every open PR's actual body through the patched step, which is the check I should have done the first time:

   #156 -> 128   #157 -> 94   #159 -> 154   #160 -> 99
   #158 -> (none)   #161 -> (none, deliberate Refs #79)   #135 -> (none)
   

   Each closes precisely its own issue.

And one more catch that proves your point twice. After stripping, I updated this PR's description to explain the fix — and the updated body parsed to issues=94. The culprit was my own prose: "would have closed #94 and #128" — closed + #94 in plain text, a natural English sentence the parser reads as a directive. Reworded to "auto-closed issues 94 and 128"; final body now parses to nothing. It's a good illustration that this parser is fundamentally keyword-proximity matching and prose about issues will always need care — the stripping closes the code-example class, but not the sentence class. I don't think there's a clean fix for the latter without a structured field, so I've left it and noted it here rather than over-engineer.

PR body updated to describe both changes; the description is now itself a test case. Ready for re-review.

[mwiget]

Re-verified the fix independently — extracted the parse step from this branch's YAML and ran it, rather than re-implementing it.

Every open PR body through the patched step, matching your numbers exactly:

#156 -> 128   #157 -> 94   #159 -> 154   #160 -> 99
#158 -> (none)   #161 -> (none)   #135 -> (none)

And the matrix: inline-span examples, fenced examples, and a table row of backticked forms all go inert; a real Fixes #94 alongside a documented example still closes only 94; the plain-text and negative cases are unchanged. Stripping fences before spans is the right order and I checked it holds — a fence containing backticks survives correctly (```\nuse `Fixes #7` here\n```\n\nFixes #42 yields 42, not 7).

Running every open PR's body through the patched step, rather than just this one's, is the check that turns this from "the instance is fixed" into "the class is fixed" — good call.

Two residual edge cases below. Neither blocks — I'm raising them so they're on the record rather than rediscovered later, and I'd merge with both left as-is. Waiting on CI to go green on the new head, then I'll approve.

[jgruberf5]

Second round addressed in 3776f4ea:

• ~~~ fences — taken. Stripped like-to-like via a backreference ((```|~~~).*?\1) rather than the alternation, so a stray ~~~ inside a ``` block can't terminate it early and over-strip. Both mixed cases verified.
• Indented blocks and unbalanced backtick — left as-is per your reasoning, both recorded in the code comment so they aren't rediscovered blind.

Re-cleared the original 15-case matrix and every open PR body — unchanged (#156→128 #157→94 #159→154 #160→99, the rest none). All three threads replied and resolved. PR body parses to nothing after its own update (checked, since that's how it bit me last time).

Ready for your re-look.

[jgruberf5]

Correction to my comment just above: it said the PR body parsed to nothing after the update — it didn't, it parsed to 94, and I'd posted before re-running the check. Third time this body has bitten me, and a new mechanism each time:

1. examples in backticks (your original finding) — fixed by stripping;
2. prose "would have closed secret_files hardening nits from #451 re-review: fchmod instead of post-close chmod (N1); parent-symlink race note (N2) #94" — a sentence the parser reads as a directive;
3. this round: a **bare written as prose** inside my new paragraph ("inside a block"). The span stripper removed the surrounding `…` but left that lone fence marker in the text, which desynchronised the fence pairing — so a real, properly fenced example block lower in the body was read as outside a fence and its PR_BODY="Fixes / Implements: #94 …" line counted.

Reworded to "tilde fence … backtick fence" — no bare markers in prose. Body now parses to issues= (empty), verified after the edit this time.

The lesson is sharper than "strip code": any prose that mentions fence markers or closing keywords near an issue number is a hazard, and the only reliable check is running the real step against the final body — which I've now done, and will do before every future edit to this PR. Nothing in the code changed; this was entirely my description.

[mwiget]

Not blocking the intent — one correctness gap in the new code-stripping.

The added comment states the stripping "never wrongly closes", but replacing code spans/fences with the empty string can glue a closing keyword directly onto a reference and close an issue the pre-PR parser left alone:

"Fixes `the parser` #94"      -> strip -> "Fixes  #94"    -> closes #94
"This resolves `race` #7"     -> strip -> "resolves  #7"  -> closes #7
"Fixes\n```\ncode\n```\n#94"  -> strip -> "Fixes\n\n#94"  -> closes #94

Verified against the actual parse step: pre-PR "Fixes \x` #94"→[], post-PR → ['94']`. So this is a new false-close, in the exact direction the comment says is impossible ("Known safe-direction gap ... never wrongly closes") and the same bug class this PR exists to fix.

Trigger is narrow (keyword → whitespace-only → span/fence → #N), but PR bodies in this repo routinely place inline code next to issue numbers, so it's reachable.

Fix options:

• Real fix: replace stripped code with a non-whitespace sentinel instead of '', so the barrier between keyword and reference survives. A plain space won't work — the separator [:\s]* swallows whitespace.
• Minimal: correct the comment to acknowledge this direction rather than claiming it can't happen.

Everything else checks out empirically: template Fixes / Implements: #N closes, Implements:/Refs/cross-repo/prose correctly don't, this PR's own body parses to (none), and the ~~~ like-to-like backreference doesn't over-strip.

[jgruberf5]

@mwiget — that's a real bug and a sharper one than the previous rounds, because it's a new false-close I introduced, in exactly the direction my comment said couldn't happen. Thank you for verifying it against the pre-PR parser; that comparison is what made it undeniable.

Took the real fix, in cfaf2c1d: code is replaced with U+2400 (SYMBOL FOR NULL) — non-whitespace, never part of a keyword or reference — instead of ''. Neither [:\s]* nor the / Implements skip can consume it, so the barrier between keyword and #N survives. Reproduced your three cases first (all → 94/7), plus a ~~~ variant and Fixes: \x` #94; all now (none)`.

Three verifications beyond the glue cases, since a sentinel could plausibly break something that worked:

1. A real closer that merely follows code still closes — See \foo`. Fixes secret_files hardening nits from #451 re-review: fchmod instead of post-close chmod (N1); parent-symlink race note (N2) #94 and a fence-then-Fixes secret_files hardening nits from #451 re-review: fchmod instead of post-close chmod (N1); parent-symlink race note (N2) #94both →94`. The sentinel only sits where code was; prose closers contain no code.
2. 21-case matrix (previous 15 + 5 glue + the follow-code cases): all pass.
3. The property your comparison implies — I extracted the original parser from origin/staging and ran an adversarial set through OLD and NEW side by side: code inside the skip region (Fixes /\x`/ Implements: secret_files hardening nits from #451 re-review: fchmod instead of post-close chmod (N1); parent-symlink race note (N2) #94), code where the skip word would be, the entire skip in code, empty spans, `` ClosesFEATURE: AWS Systems Manager (SSM) tunnel transport for EKS — eliminates rotating-IP problem #5 ``. **OLD and NEW agree on every case except the one intended widening** (Fixes / Implements: #N`). No new closer exists beyond the one this PR is for.

Comment corrected: the "never wrongly closes" claim is gone; the unbalanced-backtick gap is now stated as what it is — a false negative only.

Every open PR body re-cleared (#158, #162, #135 → none). PR body updated and verified to parse to nothing after the edit. Ready for another look.

[mwiget]

Approving — the glue false-close is fixed. cfaf2c1d replaces stripped code with U+2400 (non-whitespace sentinel) instead of '', so the barrier between a keyword and a following reference survives.

Verified against the patched parser:

• All three glue cases I reported now stay open: Fixes \the parser` #94, This resolves `race` #7, and the fence-then-#94case → **(none)**. PlusFixes: `x` #94andFixes / `x` / Implements: #94` → (none).
• Real closers that merely follow code still close: See \foo`. Fixes #94 and a fence-then-Fixes #94→94`. The sentinel only sits where code was; prose closers contain no code, so they're unaffected.
• Template form + matrix intact: Fixes / Implements: #N closes, Implements:/Refs/cross-repo don't, and #158's own body parses to (none).

The sentinel can't be consumed by [:\s]* or the / Implements skip (which only matches implements/closes/fixes/resolves), so the mechanism is sound, and the inaccurate "never wrongly closes" claim is gone from the comment. Nice fix.