Skip to content

feat(2449): a closed citation in an exemption reason is a finding (backend#2449) - #329

Open
LukasWodka wants to merge 1 commit into
developfrom
feat/2449-closed-citation-guard
Open

feat(2449): a closed citation in an exemption reason is a finding (backend#2449)#329
LukasWodka wants to merge 1 commit into
developfrom
feat/2449-closed-citation-guard

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#2449

The gap

repo-inventory.yml exemption entries carry a free-prose reason. caller-drift.py
enforces staleness on the entry (caller-drift.py:2309 — an exemption with a caller
present is a finding) and nothing reads the reason. So a reason can become false and
stay false indefinitely while the audit stays green, and that sentence is what the next
person reads to decide whether the exemption still applies.

Measured false twice on 2026-08-24: customer_priority_bump_caller_missing said the wiring
was "sequenced behind" backend#1408, closed COMPLETED on 2026-08-06 — three repos sat behind
it for eighteen days; and rfcs' advance-deploy-env.yml reason said the repo has no
develop, which it has.

What this adds — mechanism (1) only, deliberately

scripts/reason-citations.py: where a reason cites a ticket, fail when that ticket is
dead.
The reason may still be valid — a closed citation is not proof of anything — but it
is exactly the case a human has to re-read, and it is checkable from the citation alone.
That alone would have caught case 1 the day #1408 closed.

The ticket's other two sketches are not built here, on purpose:

  • (2) date expiry needs a measurement date on every reason first — an edit to
    repo-inventory.yml and a schema decision, not a checker.
  • (3) assert the falsifiable half is a much larger design problem: it needs prose
    understood well enough to know which half is falsifiable, and the wrong answer either
    invents findings or teaches people to phrase reasons so the check cannot see them.

Splitting them keeps this one small enough to be obviously correct.

Design

Derived, never restated. The citations come from parsing the inventory — every exempt:
string, both divergent: spellings, and every anchor body in shared_reasons: (an anchor
defined and not yet aliased is scanned too, which is the state a new reason is in on the PR
that introduces it). There is no hand-written list of which reason cites which ticket.
Comments are out of scope by construction: the scan runs over parsed YAML, so the ~40 ticket
numbers in the file's header are not treated as justifications.

Issue vs pull request is read, not assumed. GitHub answers both in one field:

verdict
Issue OPEN / PR OPEN / PR MERGED live
Issue CLOSED finding — re-read the reason (this is case 1)
PR CLOSED without merging finding — the reason leans on something that never landed

A merged PR is the one closed thing whose terminal state is success; reasons cite PRs as
provenance ("remediated under model-zoo#115") and that stays true forever. Flagging them
would have made 10 of the 25 live citations findings for describing history correctly.

Which repo a citation means — stated, not assumed. owner/repo#N as written; repo#N
takes its owner from the inventory's own org:; a bare #N resolves against
<org>/<source_repo> — the repo the inventory lives in, which is what GitHub itself renders
a bare #N as. Both values are read from the inventory; neither is typed into the script.
It does not assume backend — that guess was the defect fixed in .github#314 — and it
does not stay quiet either: when GITHUB_REPOSITORY contradicts source_repo the premise of
the bare rule is false and a bare citation becomes "cannot tell" rather than a guess.

Fail closed, and "cannot tell" is a finding about the check (exit 2, never a pass):
unreadable or unparseable inventory, missing org:/source_repo:, a failing gh call, a
non-JSON response, a citation the API will not resolve (404/403/rate limit), an unrecognised
state, and zero citations found. Zero matters most: the premise is that reasons cite
tickets, so finding none means the matcher broke. Exit 1 is a real finding about the
inventory; the two are separated so a red run says whether to fix a reason or fix the script.

One GraphQL call covers every citation — this runs in a required context on every PR, and N
sequential API calls is N chances for a blip to fail a merge closed. A partial read (null
node + errors[] at exit 0, which is what GitHub actually returns) is refused, not scored.

What it found against the live inventory

reason-citations: 25 distinct citation(s) across 126 written reason(s) in repo-inventory.yml
  12 dead, 0 malformed, 12 exempted, 0 finding(s)

Twelve dead citations, all closed issues. They are seeded into EXEMPT — the same shape
and the same reason as mint-scope.py's: this arms green rather than landing a red gate
in a required context that nobody can merge past. Per backend#2449 they are not fixed
here
— re-reading twelve reasons is its own work and would bury the mechanism in a prose
diff. repo-inventory.yml is untouched by this PR.

Two of the twelve are the ticket's own case 1, found on the first run, and are marked
UNREMEDIATED rather than explained away — both lean forward on a ticket that is shut:

  • backend#1415wip_limit_check_has_no_callers: "the decision to wire it up or delete
    it is backend#1415 follow-up work"
    . Closed COMPLETED.
  • backend#1729blocked_gate_rollout_pending: "the callers follow in the rollout PR
    (backend#1729)"
    . Closed COMPLETED, so the rollout it is waiting on is tracked by nothing
    open.

The other ten (backend#1276, #1408, #1420, #1563, #1752, #1816, #1975, #1976, #1979, #2243)
are cited in the past tense, as the ticket under which something already landed or was
decided. That distinction is a judgement about prose, which is why it is a note to whoever
burns these down and not something the script tries to infer.

The other half of the map is what stops it becoming cover: a row that is no longer a finding
— reopened, or edited out of the inventory — is itself reported.

Wiring

A step in the existing selftests.yml, beside mint-scope, not a new workflow.
selftests is already a required context on develop/staging/main, so this arms with no
branch-protection edit; a new context would sit unrequired until somebody armed it. A new
reusable would also need its own repo-inventory.yml row in every repo, serialising against
every other guarded-file PR in this single-writer repo.

The token is minted read-only (permission-issues: read, permission-pull-requests: read)
and is org-wide on purpose — the opposite of add-to-kanban.yml's narrowing, because the
reasons cite tickets in seven different repos and a repo-scoped token would resolve none of
them. It fails closed on a missing secret, which also means a fork PR fails rather than
reporting clean from a read it could not make.

Verification

  • scripts/tests/reason-citations-selftest.py33 cases, all green. Hermetic: fixture
    inventories plus a throwaway gh first on PATH, so the subprocess call, the exit-code
    path and the JSON decoding are covered rather than being the part nobody tests. Covers open
    issue (pass), closed issue (finding), unreadable issue (finding), malformed citation, zero
    citations (finding), unreadable inventory (finding), plus merged/open/abandoned PRs, all
    three repo-resolution rules, both divergent: spellings, an unaliased anchor, a ticket
    number in a comment, and both halves of the exemption map.
  • scripts/tests/reason-citations-mutations.py23 mutations, 0 stale, 0 uncaught.
    Breaking the closed-issue detection (("Issue", "CLOSED") added to LIVE) reddens three
    cases; restoring it greens them. Every anchor is asserted to match exactly once, so an
    inert mutation is a failure rather than a silent pass — and that caught a real one: the
    first draft of the "cannot tell reads as OPEN" mutation inserted its line above the
    raise instead of replacing it, and the harness reported UNCAUGHT, correctly.
  • make check green; make mutations green (58 s in CI today, +~15 s for this runner);
    actionlint and yaml.safe_load clean on selftests.yml; ruff clean via make lint.
  • make selftests-cover green — the new suite and the new mutation runner are both wired.

🤖 Generated with Claude Code


Note

Medium Risk
Adds a required selftests step that mints an org-wide (read-only issues/PRs) App token and can fail merges on citation/API errors. Existing dead citations are seeded into EXEMPT so the gate lands green.

Overview
Makes stale ticket citations in repo-inventory.yml exemption reasons a required finding (backend#2449). Closed issues and PRs closed without merging fail; open issues and merged PRs do not.

scripts/reason-citations.py parses reasons from exempt, both divergent spellings, and shared_reasons (including unaliased anchors). Citations are resolved from the inventory’s org/source_repo (not hardcoded backend). Fail-closed on unreadable inventory, unresolvable API reads, unknown states, and zero citations found. Twelve already-dead citations are seeded in EXEMPT (stale rows are also findings); the inventory file is unchanged.

Wired into the existing selftests job: fixture suite in make check, mutations in make mutations, live audit via a read-only org-wide App token (forks fail closed). Hermetic selftest plus 23 mutation cases pin the matcher and fail-closed paths.

Reviewed by Cursor Bugbot for commit e226259. Bugbot is set up for automated code reviews on this repo. Configure here.

…ckend#2449)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Aug 24, 2026
@LukasWodka
LukasWodka requested a review from saadqbal August 24, 2026 14:54

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, careful PR 👍 — approving. The scoping decision is the right one: build mechanism (1) because it's checkable from the citation alone, and say plainly why (2) needs a schema change and (3) needs prose understood well enough to know which half is falsifiable. A checker that only does the tractable third of a ticket, and names the other two thirds, beats one that guesses at all three.

The distinctions I went looking for are all made. A merged PR isn't a finding — "the one closed thing whose terminal state is SUCCESS" — while a closed-unmerged one is, because the reason is leaning on something that never landed. Cannot-tell is a finding about the check rather than a pass. And zero citations is itself a finding, which is the guard I'd have asked for: it's the only thing standing between a silently-broken regex and a permanently green audit.

I also checked whether the new EXEMPT list reintroduces the problem one level up — a list of grandfathered rows whose reasons can go stale is exactly what this PR exists to catch. stale_exemptions() covers it: a row that reopened or was edited out of the inventory reddens. And the two rows marked UNREMEDIATED as the ticket's own case 1, with "do not let the row read as permission", keep the decision-versus-to-do distinction that #328 established rather than blurring back into it.

Small thing I appreciated: no tally in the list comment, for the stated reason that mint-scope.py shipped one that drifted. That's the right lesson from it — the run prints len(_exempt()) and nothing else claims a number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants