Skip to content

bug(ci): both PR-body gates know 3 of GitHub's 9 closing keywords, so "Fix #N" closes an issue while the gate reports no reference at all #17580

Closed
Closed
bug(ci): both PR-body gates know 3 of GitHub's 9 closing keywords, so "Fix #N" closes an issue while the gate reports no reference at all#17580
Labels
bugSomething isn't workingciinfrastructurepriority: medium
Milestone
v0.9.5 - CI and guards

Description

@mrveiss
mrveiss
opened on Sep 27, 2026

Both PR-body gates recognise three closing keywords. GitHub closes on nine. So six of them auto-close an issue while the gates report that the PR delivers nothing — and in one measured case, that the PR references nothing at all.

Measured

scripts/check_pr_issue_batching.py:65 and .github/workflows/pr-issue-validation.yml:88 both use:

resolves|closes|fixes

GitHub's documented set is close·closes·closed, fix·fixes·fixed, resolve·resolves·resolved. Run against scripts/validate_pr_body.py with a minimal, otherwise-valid body:

Body line Gate's verdict What GitHub does on merge
Closes #12345 "This PR delivers exactly one issue" closes it
Fix #12345 "No issue reference found" closes it

The second row is the defect in its clearest form: a body that auto-closes an issue and reads to the gate as though it links nothing.

Two consequences, both live

1. The batching rule is defeated silently. check_pr_issue_batching.py exists to reward batching and to demand a rationale from a single-issue PR. A PR whose body says Fix #A, Fix #B, Fix #C closes three issues and scores as closes-nothing, so no rationale is ever requested. The docstring is explicit that "a guard that cannot tell satisfied from not examined is worse than an absent one, because the green is read as a judgement" — this is that shape, one keyword class over.

2. A negated sentence closes the issue it says it does not close. Measured today: PR #17544 merged at 00:40:54Z and #16464 closed at 00:40:55Z with reason=COMPLETED. That PR carries Refs #16464 and its body contains

Acceptance criteria — this PR does not close #16464.

GitHub reads close #16464 and does not understand the negation. The gate, meanwhile, reported "Closes nothing; references #16464" — so the author's check agreed with the author's intent while the platform did the opposite. #16464 has been reopened with its remaining criterion restated; it needs host evidence no diff can supply, which is exactly why the sentence was written.

Why the narrow set exists, and why it is still wrong

The rationale is recorded and worth keeping: "The keyword set is the one pr-issue-validation.yml already uses ... so the two gates cannot disagree about what a reference is." That invariant holds — the two agree with each other. Both diverge from the platform, which is the one party whose behaviour is not a matter of local policy. Widening must therefore change both in step, as that comment demands.

fix and close are ordinary English words, so widening will match prose. That is not a cost — it is the point: the gate would then report what GitHub will actually do, rather than what a stricter reading wishes it would.

Acceptance criteria

  • Both gates recognise all nine documented GitHub closing keywords, changed in the same commit, with the "keep the two in step" invariant restated.
  • A body containing Fix #N is reported as delivering #N — asserted for each of the nine, since three of them pass today and six do not.
  • A closing keyword appearing mid-sentence rather than at the start of a line is flagged, with the fix(migrations): 6 tables in the orphaned database/migrations/ chain were never applied to the live Alembic chain #16464 sentence as the fixture. GitHub's negation blindness cannot be fixed here; warning the author before the merge is the only defence available.
  • The batching count is asserted for a body using a non--s keyword, so consequence 1 cannot return.

Related

#16464 (auto-closed by this and reopened), #17544 (the PR whose body did it), #16795 (split the two keyword classes for the batching question), #13327 (Refs counts, so a PR that does not close its issue can say so — the intent this defeats), #17128.

Activity

  1. mrveiss
    added this to the v0.9.0 milestone on Sep 27, 2026
  2. mrveiss
    added
    bugSomething isn't working
    infrastructure
    ci
    on Sep 27, 2026

  3. mrveiss commented on Sep 28, 2026

    @mrveiss
    mrveiss
    on Sep 28, 2026
    OwnerAuthor

    Chunking triage — a proposal, not an assignment

    Nothing was relabelled, moved or closed by this pass.

  4. mrveiss
    added
    priority: medium
    on Sep 28, 2026
  5. mrveiss
    modified the milestones: v0.9.0, v0.9.5 - CI and guards on Sep 28, 2026

  6. mrveiss commented on Sep 28, 2026

    @mrveiss
    mrveiss
    on Sep 28, 2026
    OwnerAuthor

    Branch pointer (so this work is not lost with the session that wrote it): origin/issue-17580-closing-keywords, tip 0f931aa63d (a merge of origin/main into the working branch). No PR yet; the authoring session has ended. Caretaker: the frontend session (63) opens the PR after its current priority push; Closes only where every AC is delivered, Refs #17640 otherwise.

  7. mrveiss commented on Sep 28, 2026

    @mrveiss
    mrveiss
    on Sep 28, 2026 – with Claude
    OwnerAuthor

    PR opened: #17745, from issue-17580-closing-keywords (tip 0f931aa63) into main.

    Steps reached: 1 (diff scope confirmed), 2 (PR opened), 3 (this comment).

    Generated by Claude Code

  8. mrveiss
    added 5 commits that reference this issue on Sep 29, 2026

    fix(ci): both PR-body gates knew three of GitHub's nine closing keywo…

    f4239d1

    fix(ci): warn when a closing keyword sits mid-sentence, since GitHub …

    bc10d51

    test(ci): assert the batching verdict, not only the count it derives …

    74f6df1

    fix(ci): the widened keyword set matched inside ordinary words, so bo…

    ab053c9

    fix(ci): GitHub closes on `Fixes: #123` and both gates required white…

    088e18a
  9. mrveiss
    closed this as completedin 093ac84on Sep 29, 2026

  10. mrveiss commented on Sep 29, 2026

    @mrveiss
    mrveiss
    on Sep 29, 2026
    OwnerAuthor

    Closure evidence — all four criteria verified against origin/main after #17745 merged at 093ac84989

    • Both gates recognise all nine documented GitHub closing keywords, in the same commit, with the keep-in-step property enforced. scripts/check_pr_issue_batching.py:84 _CLOSING_WORDS = "closes|closed|close|fixes|fixed|fix|resolves|resolved|resolve"; test_both_gates_know_the_same_set (:116) reads the workflow's own patterns rather than restating them. The parity helper was itself the bug for a while — it took next(...), the first grep -iqE line, so a stale three-keyword set on the fork-override branch compared green against the main branch. Fixed and mutation-proven: restoring the three-keyword set and the old helper leaves all tests passing; restoring either alone fails.
    • A body containing Fix #N is reported as delivering #N, asserted for each of the nine — test_every_github_closing_keyword_is_recognised_as_closing parametrised over the set, plus test_the_checker_knows_exactly_githubs_set (:108) so the set cannot drift silently, and test_the_longest_inflection_comes_first_in_each_verb (:136) pinning the alternation order.
    • A mid-sentence closing keyword is flagged, with the fix(migrations): 6 tables in the orphaned database/migrations/ chain were never applied to the live Alembic chain #16464 sentence as the fixture — test_the_16464_sentence_is_flagged (:181), test_a_deliberate_declaration_is_not_flagged (:209) as the contrast, and test_the_warning_reaches_the_author (:185). The warning was keyed on issue-number sets, so Closes #42 plus This does not close #42 cancelled out and the most-likely-real mistake was exempt; now keyed on match.start(1), with test_a_mixed_body_flags_only_the_mid_sentence_one (:278).
    • The batching count is asserted for a non--s keyword — test_a_body_using_only_non_s_keywords_is_reported_as_batched (:296), plus test_the_batching_count_still_includes_a_mid_sentence_closing (:267).

    Found and fixed while delivering it, both regressions this PR would otherwise have introduced: widening to bare stems without a left word boundary made This remains unresolved #1234 satisfy the link gate and count as a closing claim — test_a_keyword_inside_a_word_is_not_a_reference (:248) and test_the_boundary_does_not_cost_a_real_reference (:261) are the pair. And GitHub's documented syntax permits a colon ("Closes: #10, CLOSES #10, CLOSES: #10"), which neither gate accepted; :? is now in all five patterns inside the boundary. Closes:#10 without a space stays rejected and is asserted as a decision, since the docs do not say whether the space is required.

    Refs #17640 — the inline-code-stripping disagreement is untouched and stays open there.

  11. mrveiss commented on Oct 3, 2026

    @mrveiss
    mrveiss
    on Oct 3, 2026
    OwnerAuthor

    Widening both gates to the nine keywords is necessary but will not give complete coverage — the gates inspect the PR body, and the body is one of three surfaces that can close an issue. Two are invisible to any pre-merge check.

    Measured today, two independent cases:

    surface visible to a body-inspecting gate / closingIssuesReferences? evidence
    PR body yes PRs 17341 / 17347 keyword in body → link. PR 17352 keyword mid-sentence → link
    branch commit messages no PR 17926: keyword present in a commit message, gh pr view --json closingIssuesReferences returns []
    squash message typed at merge time no PR 17912: merge commit body carried the keyword, the field was empty, its linked issue closed 2 seconds after merge

    Surface 2 reaches main because this repo is configured:

    gh api repos/{owner}/{repo} -q .squash_merge_commit_message   ->  COMMIT_MESSAGES

    so the squash body is built from the commit messages by default — nobody has to type the keyword at merge time for it to land.

    Surface 3 cannot be checked mechanically at all: the squash message is composed at the moment of merging, after every gate has run. It does not exist when the gate runs.

    Why this belongs on this issue rather than as its own. This issue's AC is that both gates recognise all nine keywords. That is correct and should land. The risk is that a green gate then reads as "this PR closes nothing", when the PR can still close an issue through either invisible surface — which is precisely the shape this issue already names: "a guard that cannot tell satisfied from not examined is worse than an absent one, because the green is read as a judgement." Widening the keyword set fixes the keyword half and leaves the surface half, and the gate's message is what decides whether that gap is visible or silent.

    Suggested addition to the AC, not a new issue:

    • the gates' output distinguishes "no closing reference in the body" from "this PR closes nothing" — the second is not checkable from the body alone
    • a commit-message scan over <base>..HEAD using the same nine-keyword pattern, matching mid-sentence and case-insensitively, since the frames that bite are not at line start:
    git log <base>..HEAD --format=%B | grep -inE '\b(close[sd]?|fix(e[sd])?|resolve[sd]?) +#?[0-9]+'

    Validated with controls both directions — it matches Closes <ref>, This does not close <ref>, the mechanism that closed <ref>, fixes 12345 and RESOLVED <ref>; it does not match Refs <ref>, It closes the outage bypass, disclosure of <ref> or enclosed <ref>.

    Found by running that grep as a control on my own branch after recommending it elsewhere: it matched a commit message of mine, where the keyword sat inside the paragraph explaining why I had removed a closing keyword from the PR body. Caught pre-merge only because the control was run.

    Cross-ref #17361, which asks where the closing link comes from; answered there as the body, with branch-name inference falsified. The keyword-set fix is this issue's.

  12. mrveiss commented on Oct 3, 2026

    @mrveiss
    mrveiss
    on Oct 3, 2026
    OwnerAuthor

    Correcting my own comment above — it had a wrong premise. The widening already landed; I wrote as though it were outstanding.

    This issue is CLOSED as COMPLETED (2026-09-29) and both gates carry all nine keywords on current main:

    git show origin/main:scripts/check_pr_issue_batching.py
  :84  _CLOSING_WORDS = "closes|closed|close|fixes|fixed|fix|resolves|resolved|resolve"

git show origin/main:.github/workflows/pr-issue-validation.yml
  :102 grep -iqE "(^|[^[:alnum:]_-])(closes|closed|close|fixes|fixed|fix|resolves|resolved|resolve|refs|references|part of):?[[:space:]]+(#?[0-9]+|MVA-[0-9]+)"

    So the acceptance criterion is genuinely met, the left-boundary comment at :142 shows the encloses false-positive was considered, and nothing here needs reopening.

    What my comment should have said: the surface limitation is independent of the keyword set and survives this fix. Both gates inspect the PR body; branch commit messages and the merge-time squash message are invisible to them. That part is tracked on #17361, which is open, and does not belong here.

    I also mis-dated this issue's evidence elsewhere — the case it cites is PR #17544 merged 2026-09-27T00:40:54Z, and I described it as having been measured the same day as a case of mine on 2026-10-03. It was six days earlier. The substance is unchanged and in fact stronger: the mechanism was filed, fixed and shipped before my encounter with it, which makes mine a late witness rather than a discovery.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingciinfrastructurepriority: medium

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions