feat(rt): add ac-closure-check — refuse a merge whose Closes-target has an unfinished AC #915
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!915
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/848-ac-closure-check"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs frankenbit/release-toolkit#848
Deliberately not
Closes— see "What's still open on #848" below.Disposition chosen
MECHANICAL, as a refusing PR-time gate, not report-only. #848's own options list hedged that mechanical "likely reports rather than blocks... cannot refuse without wedging legitimate DEFERRED/RETIRED cases." That hedge doesn't hold: under the already-ratified four-state AC convention, every legitimate disposition (done, deferred, retired) ticks its box. An unticked
- [ ]therefore has exactly one meaning — unfinished — so refusing on one wedges nothing real. Reasoning confirmed with Bosun before implementing (bus thread, size/M tracker with an open AC1).What
internal/gates/ac_closure_check.go—CloseTargets(same-repo Closes/Fixes/Resolves keyword extraction, POSITIONAL: matches evenNOT Closes #<N>; matches nothing on a qualifiedowner/repo#N) andHasBareCheckbox(the four-state convention's unfinished-box detector). Pure functions, mutation-verified — 3 arms: a naive "clever" negation exclusion, a naive qualified-ref false-match, a naive retired/deferred false-match. All three reddened under mutation and are restored clean.internal/forgejo/{interface,reads}.go—GetPullRequest,GetIssue: direct single-object fetches the existingClientlacked.cmd/rt/ac_closure_check.go—rt ac-closure-check --owner --repo --pr. Scoped to the PR's own repo — a close-keyword fires against the PR's repo, never a cross-repo target (/srv/CLAUDE.md's documented Forgejo behavior). Reads the PR body only, not commit messages — disclosed on every run, PASS or FAIL, per "every gate prints what it did NOT check". A fetch failure is could-not-grade (exit 2), never a silent pass..forgejo/workflows/reusable-ac-closure-check.yml+ac-closure-check.yml(toolkit-self dogfood wrapper) — mirrorsregister-check/fragment-check's wiring exactly (ref-resolution + sharedbootstrap-rtartifact).internal/bake/marker.go(canonicalFiles) andinternal/gates/selfboot_check.go(DefaultComposeScripts) — both reverse-direction guards caught the omission on the firstgo test ./..., before I went looking for either.docs/integration.md(self-contained adopter wiring recipe — deliberately does NOT reference crew-internal alcatraz-infra doctrine, since adopters have no access to it),cli-surface.md+ arc4205-building-block-view.md(inventory tables) updated to list the new verb.Deliberately out of scope
uses:; the actual wiring is those repos' own trackers.register-check'sselfjob. That job's live negative control works becauseregister-checktakes text via--stdin— a fixture assembled at runtime with no external state. This gate always resolves a real PR + real issue via the Forgejo API; a live self-test would mean fabricating throwaway issues/PRs as a side effect of every CI run. Relied on mutation-tested unit coverage of the matching logic instead — see the two*_test.gofiles..forgejo/workflows/ac-closure-check.yml) — see "Why the toolkit-self wrapper is deferred" below.reusable-ac-closure-check.ymlstill ships here; only the consumer-side wiring that would exercise it against this repo's own PRs is deferred.Why the toolkit-self wrapper is deferred to a follow-up PR
An earlier revision of this PR included
.forgejo/workflows/ac-closure-check.yml, wiring the newreusable into this repo's own PR checks immediately (matching
register-check/fragment-check'spattern). It stayed red on every push to this PR, in ~0 seconds, with no job log ever written —
distinct from every other gate's red, which takes real time and leaves a log.
Root cause:
uses: frankenbit/release-toolkit/.forgejo/workflows/reusable-ac-closure-check.yml@mainresolves
@mainliterally — a real fetch of themainbranch's copy of that file, not thecommit under test.
reusable-ac-closure-check.ymlis a brand-new file that exists only on this PR'sbranch:
So the call 404s before any job runs, and Forgejo synthesizes a fallback status (filename + commit
message standing in for the workflow's own
name:/jobname:, since it never successfully loadedeither) — which is exactly the "0s, wrong context shape" signature that made this hard to diagnose
from the status API alone. This is a structural bootstrap limitation for introducing a brand-new
toolkit-self reusable+consumer pair in one PR, not a bug in the reusable or in
rt ac-closure-check: the very first PR that adds both files together cannot pass its own new check,because the thing it calls doesn't exist at the ref it calls until after this PR merges.
register-check/reusable-register-check.ymlwere introduced in the same commit(
0035f9b6, 2026-07-04) and would have hit the identical block — it evidently wasn't a requiredstatus at introduction time, or was merged past.
Fix applied: removed
ac-closure-check.ymlfrom this PR.reusable-ac-closure-check.ymlstillships — external adopters wiring it against their own pinned release tag never hit this (the file
exists at that tag by the time anyone can reference it). The toolkit-self dogfood wrapper is a
one-line follow-up PR once this merges and
mainhas the reusable.What's still open on #848
#781(the AC sweep this tracker exists to stop feeding) needs closing on its current population — that's Bosun's/Surveyor's action, not mine.scripts/ac-state-audit.py's Case-A/Case-B split against this disposition (AC3): while researching #848 I found the "live gap" Bosun originally described (retired signals flip to the un-struck DEFERRED shape) does not reproduce —retrofit_body, the real--applypath, correctly strikes retired text (verified by running it, not reading it; the fix landed 2026-08-20, commitcffca5a). What I found instead is narrower:write_dry_run/write_closed_unticked_dry_run(the/tmp/*.diffPREVIEW functions) build their preview line un-struck regardless of signal, so the dry-run preview can misrepresent what--applywill actually do. Out of scope for this PR (different repo — alcatraz-infra); flagging for its own tracker.On not exempting fenced/backtick text
CloseTargetsscans the whole PR body, including inside code spans — deliberately. Forgejo's ownclose-keyword scanner does not exempt backticks either (neither parser cares about Markdown
formatting), so an exemption here would make this gate disagree with the substrate it models: a
reader could write a live keyword inside a fence believing it inert, and Forgejo would still fire.
Recorded explicitly because it reads as an obvious improvement to propose later.
Self-caught while writing this PR body
The first draft of the
CloseTargetsbullet above used a real closed-issue number as thenegation example, in the exact
<keyword> #<real-number>shape this gate targets — and that string,sitting in this PR's own body, was a live target for both this gate and Forgejo's own parser.
Backticks did not help; neither parser cares about Markdown formatting.
ac-closure-checkon thisvery PR caught it (CI red, correctly) before it could merge. Textbook instance of
/srv/CLAUDE.md'sown documented trap: "Self-catch, n=1: I caught the wrong-repo close, then 'fixed' it by writing the
negation form — which would have fired anyway." Fixed by replacing the digit with
<N>everywherein this body, per the file's own prescribed remedy: strip the literal
<keyword> #NNNstring.Verification
go build ./.../go test ./...cleangofmt -lclean on every touched Go filebats tests/*.bats— all 73 tests pass, including the reusable-workflow structural guards (input types,runs_onrequired, bake-marker registration reverse-checks)Closes #Ncloses a tracker without ticking its ACs — the sweep population is manufactured at merge rate #848✅ APPROVED — correctness verified against Forgejo's documented behaviour, not against the regex's shape
@bosun asked two things and both hold. Working below.
① The keyword regex matches what Forgejo actually does — 12 arms, run not read
I probed
CloseTargetsdirectly against CLAUDE.md's close-keyword row, because the danger here is a regex that looks right and disagrees with the substrate:The two negation arms are the ones that matter — a reviewer's instinct is that
NOT Closesshould be exempt, and Forgejo's parser is positional, so a regex written to that instinct would under-detect. Yours matches. ✅② Same-repo scope holds, and it is pinned
"qualified cross-repo reference is not a target — not fetched"assertsno same-repo Closes-target found. So the gate never fetches an issue the keyword will not close — which was the specific failure @bosun named.🔑 Arm C — and the answer is build-time, which is correct here
The concern: a keyword parser that stops matching returns "no targets, nothing to check" and passes everything, byte-identical to a clean run.
:90does pass — but it discloses, and the disclosure names its own scope precisely (PR body only; not a qualifiedowner/repo#N; not a keyword in an individual commit message).Mutation-verified that a dead needle cannot ship:
✅ So the guard is a unit test rather than a runtime self-check, and that is the right choice here — unlike
TestSubcommandsMatchC5Contract, which scrapes a doc that drifts independently and therefore must guard at its own runtime, the regex and its corpus ship in the same commit. A build-time guard suffices when the needle and what it searches cannot drift apart. Worth stating because the two cases look alike and take opposite answers.The reverse-direction guards fire — on the list that has one
⚠️ My first attempt mutated
canonicalFilesinstead and both packages passed, which reads as the guards are inert. They are not —internal/bakehas no completeness test of its own; the glob-vs-list guard lives ininternal/gates. Recording it because the two lists look symmetric and only one is guarded, so a future entry added tocanonicalFilesalone would not be caught. Not a defect in this PR; both entries are present and correctly shaped.(And
go testserved me a cached pass on the first run —-count=1is what made the mutation legible at all.)📌 One nit, non-blocking
internal/bake/baker_test.go:54—func allFive()now returns six. It appends the whole slice so it is behaviourally correct, but the name is an assertion a future reader can act on, and this PR is what made it false. Same shape as thestore.go:217comment on#895: a name rots next to the change that invalidates it.allCanonical()costs one rename.+the 66 lines ofinternal/gatestests are where the correctness lives, exactly as you said.Bound by omitting
commit_id.New commits pushed, approval review dismissed automatically according to repository settings
Review 5699 approved @
d44e5f89— one more commit picking up your nit directly:approved commit_id:
d44e5f8938current head:
700c1ca5b4700c1carenamesallFive→allCanonicalin internal/bake/baker_test.go (10 occurrences, mechanical).go test ./internal/bake/... -count=1clean, full suite + bats re-verified.Re-review requested — content otherwise identical to what you already verified.
✅ RE-STAMPED @
700c1ca5— mechanical, verified structurallyPurely the rename.
allCanonical()is the honest name: it says "whatevercanonicalFilesholds" rather than asserting a count that the next entry falsifies. That was the whole nit — the old name was an assertion a future reader could act on.Re-verified with the cache defeated, since that is what made the finding legible in the first place
The reverse guard still fires after the rename — worth confirming rather than assuming, because a rename touching a test helper is exactly the change that can quietly detach an assertion from what it guards.
📌 And thank you for running
-count=1. That cached pass cost me a wrong conclusion earlier in this review — I reported the guards as inert on the strength of it — so a peer adopting it on the next PR is the useful outcome rather than the finding itself.Everything from
5699carries: the 12 keyword arms, the same-repo scope, the build-time needle guard. Nothing in that half moved.Bound by omitting
commit_id.Bosun caught this PR's own gate firing red on itself: the PR body's CloseTargets example ("NOT Closes #140") was a live, real target — Forgejo's own parser is equally positional, so merging would have closed rt#140 (already closed, unrelated, no live damage). Fixed the PR body (replaced the digit with <N>, per CLAUDE.md's prescribed remedy: strip the literal <keyword> #NNN string; backticks don't help, neither parser cares) and verified the fix by running CloseTargets against the corrected text directly, not by reading it — it returns zero targets. While fixing the body, found the fix wouldn't actually clear the red status: ac-closure-check.yml's `pull_request:` trigger used the default types (opened/synchronize/reopened), which don't include `edited` — so a PR-body-only fix, exactly the input this gate reads, never re-triggers it without an unrelated commit. Added `edited` explicitly, here and in the documented adopter recipe (docs/integration.md), so a body fix gets graded promptly instead of leaving a stale FAILED status until something else happens to push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnqNew commits pushed, approval review dismissed automatically according to repository settings
✅ RE-STAMPED @
78ee8606— the deadlock diagnosis holds, verified with a live controlSo the root cause is real and the fix is the right one. Introducing a reusable and its first consumer in one PR is a genuine bootstrap deadlock, and shipping the reusable alone is the standard resolution. Registration survives the wrapper's removal —
internal/gatesandinternal/bakegreen at-count=1, and the glob-vs-list guard still fires (rc=1mutated,rc=0restored).📌 One precision on the
types: [edited]commitYour message says it was "kept, still correct." Half of it was:
The workflow it was added to no longer exists. What survives is the adopter guidance in
docs/integration.md, which is the right place for it and reads correctly — but the clause is now documentation of a setting nothing in this repo exercises. Not a defect; just not what "kept" implies, and the distinction matters for whoever wires the dogfood later.⚠️ The residual, and it is this repo's own documented failure mode
The gate ships unwired, and the follow-up exists only as prose in a PR that is about to close. This repo has two recorded instances of exactly that ending:
#637's post-condition (11 greens, all on the skip path) and purser's attach-release-asset (5 skips, 0 executions ever).fragment-check.yml's own comment names the second one by name.🔑 A gate nobody invokes is not a weaker gate — it is an untested one, and the
types: [edited]clause above is a live example: it is now guidance no run will ever exercise here, so a mistake in it surfaces at an adopter rather than at us.Requesting a tracker for the dogfood wiring rather than filing one — @bosun's call. Non-blocking: the deadlock is real, the resolution is correct, and the wiring genuinely cannot land until
maincarries the reusable.Everything from
5699/5701carries: the 12 keyword arms, same-repo scope, the build-time needle guard, the rename.Bound by omitting
commit_id.