frankenbit/release-toolkit

Watch

Fork 0

bug(gates): ac-closure-check does not see PR comments, inline review comments or the PR title, and Forgejo closes from all three #1506

New issue

Closed

opened 2026-10-02 11:51:24 +02:00 by claude · 2 comments

claude commented

2026-10-02 11:51:24 +02:00

Member

rt ac-closure-check grades closing keywords in two places: the PR body and the commit messages. Forgejo also closes issues from PR comments, inline review comments, and the PR title. The gate reads none of those three. So a keyword typed into a comment closes its target on merge, unfinished acceptance criteria and all, and the check stays green.

Measured

The incident: PR #1489 and issue #1485, 2026-10-02. The PR body said Refs #1485. The commit message had no keyword. Comment 113711 said close #1485 by hand, as plain text in the comment itself. Here is #1485's timeline (GET /repos/frankenbit/release-toolkit/issues/1485/timeline), read back:

pull_ref     09:57:21  ref_action=neutered  ref_issue=1489              (the earlier body keyword, edited away)
pull_ref     11:07:02  ref_action=none      ref_issue=1489              (body: Refs)
comment_ref  11:07:14  ref_action=none      ref_issue=1489  comment=113708   (keyword inside inline code)
comment_ref  11:07:47  ref_action=closes    ref_issue=1489  comment=113711   (plain-text keyword)
close        11:11:59                                                   (same second as the rebase-merge)

The last pull_request event (the body edit, 11:07:02) came before both comments. So even a gate that read comments on pull_request events would have graded this PR without seeing comment 113711.

Re-verified on throwaway repos. claude/ac-xref-probe-a-20261002 and -b- are private, carry no workflows (.forgejo, .gitea and .github/workflows all returned 404 before use), and have has_actions=false (read back). Forgejo 15.0.7+gitea-1.22.0. Each target issue had one - [ ] box. The PR was rebase-merged by its author, who has write access. Every row below is the target's timeline, read back before and after the merge:

Surface on the PR	Text	xref row recorded	State after merge
PR comment	`closes #1` (plain)	`comment_ref`, `closes`	closed
PR comment	the keyword+number inside inline code	no row	open
PR comment	the keyword+number inside a fenced block	no row	open
inline review (code) comment	`Closes #4`	`comment_ref`, `closes`	closed
review body	`Closes #5`	no row	open
PR comment, then deleted	`Closes #6`	timeline returns a literal `null` row	open
PR comment, then edited to `Refs #7`		`neutered`, then `none`	open
PR comment, self-qualified	`closes claude/ac-xref-probe-a-20261002#8`	`comment_ref`, `closes`	closed
PR title	`probe: fix #9`	`pull_ref`, `closes`	closed
PR comment, full issue URL	`closes https://.../issues/10`	`comment_ref`, `none`	open
PR body, self-qualified	`Closes claude/ac-xref-probe-a-20261002#11`	`pull_ref`, `closes`	closed
PR comment, other repo	`closes claude/ac-xref-probe-b-20261002#1`	`comment_ref`, `closes` on repo B	closed (repo B)

Forgejo source at the deployed tag (v15.0.7).

models/issues/issue_xref.go, Comment.AddCrossReferences: only CommentTypeCode and CommentTypeComment create cross-references. The comment text goes through the Markdown parser only, which strips code spans.
The PR's own references are built from its title (plain parser) and its body (Markdown parser).
ResolveCrossReferences selects every row whose ref_issue is the PR and whose ref_action is closes or reopens. Rows are ordered by id, and the latest row per target wins. handleCloseCrossReferences in services/pull/merge.go applies the result at merge.
DeleteComment and UpdateComment both neuter the comment's old references.
services/convert/issue_comment.go, ToTimelineComment: returns nil (a JSON null) when any load fails, including when the referencing comment no longer exists.

Two things this contradicts in the gate as it stands:

The disclosure says "A qualified owner/repo#N is NOT a target", and the refusal tells authors that a qualified reference "does not fire a local close". A qualified reference to the same repo does fire a local close, from the body and from comments.
The title is a closing surface. The gate does not read it.

Live replay of the proposed reader against the merged #1489: the candidate #1485 has an effective closes row from comment_ref 113711. It is a target, and it is undeclared. Cost on the three most-commented recent PRs (#1410, #1416, #1432): 22, 27 and 34 API calls, each under 1 s.

Why it matters

The gate exists so that merging never closes an issue whose acceptance criteria are still open. A comment is the easiest place for a closing keyword to land by accident. Reviewers write "this fixes #N" in inline comments, and mergers write "close #N by hand". Neither is visible to the gate. #1485's boxes happened to be ticked. An issue with open boxes would have been closed the same way, behind a green check.

Acceptance criteria

Forge arm: for every same-repo issue number mentioned in the PR title, body, PR comments, or inline review comments, the gate reads that issue's timeline. It treats the issue as a target when the effective row from this PR (same repo, latest by id among closes/reopens) is closes. The body and commit regex arms are unchanged, and the result is the union of all arms. — DONE @ #1560 (6a36090, reviewed as 6a36090). readForgeArm, gates.ForgeCloseRef (TestForgeCloseRef); live replay of #1489 refuses #1485 via comment 113711.
A keyword inside inline code or a fenced block in a comment is not a target. The forge records no closes row for it, and the gate defers to that. — DONE @ #1560 (6a36090, reviewed as 6a36090). TestRunACClosureCheck_1506_InlineCodeInACommentIsNotATarget, with the keyword regex asserted to match as a precondition.
The #1489 shape is refused: body Refs, plain keyword in a comment, ACs ticked. It fails as undeclared, and with an unticked box it also fails as unfinished. The refusal names the comment and the remedy: edit or delete it (measured to neuter), or wrap the keyword in inline code, or declare it in Intended-targets:. — DONE @ #1560 (6a36090, reviewed as 6a36090). …_CommentReferenceIsATarget, plus toolkit-self fixture control PR 9: main's rt exits 0 on it and this rt exits 1.
null timeline rows are skipped and counted in the output. They are not a decode error and not could-not-grade, because the one measured producer is a deleted comment, whose reference Forgejo neuters. — DONE @ #1560 (6a36090, reviewed as 6a36090). …_NullRowsAreDisclosedNotFatal and TestListIssueTimeline_NullPageEndsTheWalkAndNullRowsAreCounted. The timeline's empty page is also null (measured), which the reader handles.
Inline review comments are read with one GET per review. That endpoint ignores page: measured, page 1, 2 and 3 return the same rows. A non-array response fails loud. — DONE @ #1560 (6a36090, reviewed as 6a36090). One request per review, in the same shape #1496 gave ListIssueComments; TestListPRReviewComments_UnpaginatedRouteIsReadOnceAndTeamRequestsAreFine.
Every run discloses the new surfaces, and also discloses what the gate still does not grade: cross-repo targets (measured to close the other repo's issue), review bodies (measured inert), and any comment posted after this run. — DONE @ #1560 (6a36090, reviewed as 6a36090). Review bodies were added to the disclosure in 6a36090, after review 7272; pinned by TestRunACClosureCheck_ScopeDisclosed.
The refusal no longer offers a qualified owner/repo#N as a non-closing form. — DONE @ #1560 (6a36090, reviewed as 6a36090). …_RemedyTextIsNotItselfAClosingReference.
Tests cover each surface above, and the #1489-replay arm is mutation-verified: disabling the forge arm turns it red. — DONE @ #1560 (6a36090, reviewed as 6a36090). 12 of 12 mutations were killed, including removing the forge arm (#1489 replay red). The reviewer killed 6 more of their own.
Both reusable workflow headers (the .forgejo source and the regenerated .gitea twin) describe the new scope. No trigger change. The PR body records the trigger decision as a decision tree. — DONE @ #1560 (6a36090, reviewed as 6a36090) for the trigger half: no trigger change, and the decision tree is in the PR body. The workflow-header half is RETIRED: 0de3876 removed reusable-ac-closure-check.yml and its twin before this landed. The scope text now lives in docs/integration.md's ac_closure bullet, and reusable-pr-gates.yml's one-line description stays true.
changelog.d fragment. — DONE @ #1560 (6a36090, reviewed as 6a36090). changelog.d/1506-ac-closure-forge-xrefs.fixed.md.
AGENTS.md section 15a (added by #1498) states which surfaces ac-closure-check grades. If it is on main when this lands, flip its "Graded" column to yes for each surface this fix reads (PR title, PR comments, inline review comments), and keep review bodies at no. — DONE @ #1560 (6a36090, reviewed as 6a36090). #1498 was on main, so the Graded column now says yes for the title, PR comments and inline review comments, and no for review bodies.

Out of scope

Enforcing against a comment posted after the last pull_request event, which is the exact #1489 timing. Per services/actions/commit_status.go at v15.0.7, a run triggered by issue_comment creates no commit status, so its verdict cannot reach the PR's check. This is a source read, not a live probe: no runner serves the throwaway owner. Options and their trade-offs belong in the PR's decision tree and a follow-up.
Grading cross-repo targets.
The body regex arm's over-match inside code spans. It is the safe direction, and it is unchanged here.
ListIssueComments re-reading the unpaginated /issues/{n}/comments until MaxPages. This is a separate defect, found while probing.

`rt ac-closure-check` grades closing keywords in two places: the PR body and the commit messages. Forgejo also closes issues from PR comments, inline review comments, and the PR title. The gate reads none of those three. So a keyword typed into a comment closes its target on merge, unfinished acceptance criteria and all, and the check stays green. ## Measured **The incident: PR #1489 and issue #1485, 2026-10-02.** The PR body said `Refs #1485`. The commit message had no keyword. Comment 113711 said `close #1485 by hand`, as plain text in the comment itself. Here is #1485's timeline (`GET /repos/frankenbit/release-toolkit/issues/1485/timeline`), read back: ``` pull_ref 09:57:21 ref_action=neutered ref_issue=1489 (the earlier body keyword, edited away) pull_ref 11:07:02 ref_action=none ref_issue=1489 (body: Refs) comment_ref 11:07:14 ref_action=none ref_issue=1489 comment=113708 (keyword inside inline code) comment_ref 11:07:47 ref_action=closes ref_issue=1489 comment=113711 (plain-text keyword) close 11:11:59 (same second as the rebase-merge) ``` The last `pull_request` event (the body edit, 11:07:02) came before both comments. So even a gate that read comments on `pull_request` events would have graded this PR without seeing comment 113711. **Re-verified on throwaway repos.** `claude/ac-xref-probe-a-20261002` and `-b-` are private, carry no workflows (`.forgejo`, `.gitea` and `.github/workflows` all returned 404 before use), and have `has_actions=false` (read back). Forgejo 15.0.7+gitea-1.22.0. Each target issue had one `- [ ]` box. The PR was rebase-merged by its author, who has write access. Every row below is the target's timeline, read back before and after the merge: | Surface on the PR | Text | xref row recorded | State after merge | |---|---|---|---| | PR comment | `closes #1` (plain) | `comment_ref`, `closes` | **closed** | | PR comment | the keyword+number inside inline code | no row | open | | PR comment | the keyword+number inside a fenced block | no row | open | | inline review (code) comment | `Closes #4` | `comment_ref`, `closes` | **closed** | | review body | `Closes #5` | no row | open | | PR comment, then deleted | `Closes #6` | timeline returns a literal `null` row | open | | PR comment, then edited to `Refs #7` | | `neutered`, then `none` | open | | PR comment, self-qualified | `closes claude/ac-xref-probe-a-20261002#8` | `comment_ref`, `closes` | **closed** | | PR **title** | `probe: fix #9` | `pull_ref`, `closes` | **closed** | | PR comment, full issue URL | `closes https://.../issues/10` | `comment_ref`, `none` | open | | PR **body**, self-qualified | `Closes claude/ac-xref-probe-a-20261002#11` | `pull_ref`, `closes` | **closed** | | PR comment, other repo | `closes claude/ac-xref-probe-b-20261002#1` | `comment_ref`, `closes` on repo B | **closed (repo B)** | **Forgejo source at the deployed tag (`v15.0.7`).** - `models/issues/issue_xref.go`, `Comment.AddCrossReferences`: only `CommentTypeCode` and `CommentTypeComment` create cross-references. The comment text goes through the Markdown parser only, which strips code spans. - The PR's own references are built from its title (plain parser) and its body (Markdown parser). - `ResolveCrossReferences` selects every row whose `ref_issue` is the PR and whose `ref_action` is `closes` or `reopens`. Rows are ordered by `id`, and the latest row per target wins. `handleCloseCrossReferences` in `services/pull/merge.go` applies the result at merge. - `DeleteComment` and `UpdateComment` both neuter the comment's old references. - `services/convert/issue_comment.go`, `ToTimelineComment`: returns `nil` (a JSON `null`) when any load fails, including when the referencing comment no longer exists. Two things this contradicts in the gate as it stands: 1. The disclosure says "A qualified owner/repo#N is NOT a target", and the refusal tells authors that a qualified reference "does not fire a local close". A qualified reference to the **same** repo does fire a local close, from the body and from comments. 2. The title is a closing surface. The gate does not read it. **Live replay of the proposed reader** against the merged #1489: the candidate `#1485` has an effective `closes` row from `comment_ref` 113711. It is a target, and it is undeclared. Cost on the three most-commented recent PRs (#1410, #1416, #1432): 22, 27 and 34 API calls, each under 1 s. ## Why it matters The gate exists so that merging never closes an issue whose acceptance criteria are still open. A comment is the easiest place for a closing keyword to land by accident. Reviewers write "this fixes #N" in inline comments, and mergers write "close #N by hand". Neither is visible to the gate. #1485's boxes happened to be ticked. An issue with open boxes would have been closed the same way, behind a green check. ## Acceptance criteria - [x] Forge arm: for every same-repo issue number mentioned in the PR title, body, PR comments, or inline review comments, the gate reads that issue's timeline. It treats the issue as a target when the effective row from **this** PR (same repo, latest by `id` among `closes`/`reopens`) is `closes`. The body and commit regex arms are unchanged, and the result is the union of all arms. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `readForgeArm`, `gates.ForgeCloseRef` (`TestForgeCloseRef`); live replay of #1489 refuses #1485 via comment 113711. - [x] A keyword inside inline code or a fenced block in a comment is not a target. The forge records no `closes` row for it, and the gate defers to that. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `TestRunACClosureCheck_1506_InlineCodeInACommentIsNotATarget`, with the keyword regex asserted to match as a precondition. - [x] The #1489 shape is refused: body `Refs`, plain keyword in a comment, ACs ticked. It fails as undeclared, and with an unticked box it also fails as unfinished. The refusal names the comment and the remedy: edit or delete it (measured to neuter), or wrap the keyword in inline code, or declare it in `Intended-targets:`. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `…_CommentReferenceIsATarget`, plus toolkit-self fixture control PR 9: `main`'s rt exits 0 on it and this rt exits 1. - [x] `null` timeline rows are skipped and counted in the output. They are not a decode error and not could-not-grade, because the one measured producer is a deleted comment, whose reference Forgejo neuters. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `…_NullRowsAreDisclosedNotFatal` and `TestListIssueTimeline_NullPageEndsTheWalkAndNullRowsAreCounted`. The timeline's empty page is also `null` (measured), which the reader handles. - [x] Inline review comments are read with one GET per review. That endpoint ignores `page`: measured, page 1, 2 and 3 return the same rows. A non-array response fails loud. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). One request per review, in the same shape #1496 gave `ListIssueComments`; `TestListPRReviewComments_UnpaginatedRouteIsReadOnceAndTeamRequestsAreFine`. - [x] Every run discloses the new surfaces, and also discloses what the gate still does not grade: cross-repo targets (measured to close the other repo's issue), review bodies (measured inert), and any comment posted after this run. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). Review bodies were added to the disclosure in `6a36090`, after review 7272; pinned by `TestRunACClosureCheck_ScopeDisclosed`. - [x] The refusal no longer offers a qualified `owner/repo#N` as a non-closing form. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `…_RemedyTextIsNotItselfAClosingReference`. - [x] Tests cover each surface above, and the #1489-replay arm is mutation-verified: disabling the forge arm turns it red. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). 12 of 12 mutations were killed, including removing the forge arm (#1489 replay red). The reviewer killed 6 more of their own. - [x] Both reusable workflow headers (the `.forgejo` source and the regenerated `.gitea` twin) describe the new scope. No trigger change. The PR body records the trigger decision as a decision tree. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`) for the trigger half: no trigger change, and the decision tree is in the PR body. The workflow-header half is RETIRED: `0de3876` removed `reusable-ac-closure-check.yml` and its twin before this landed. The scope text now lives in `docs/integration.md`'s `ac_closure` bullet, and `reusable-pr-gates.yml`'s one-line description stays true. - [x] `changelog.d` fragment. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). `changelog.d/1506-ac-closure-forge-xrefs.fixed.md`. - [x] AGENTS.md section 15a (added by #1498) states which surfaces `ac-closure-check` grades. If it is on `main` when this lands, flip its "Graded" column to yes for each surface this fix reads (PR title, PR comments, inline review comments), and keep review bodies at no. — DONE @ #1560 (`6a36090`, reviewed as `6a36090`). #1498 was on `main`, so the Graded column now says yes for the title, PR comments and inline review comments, and no for review bodies. ## Out of scope - **Enforcing against a comment posted after the last `pull_request` event**, which is the exact #1489 timing. Per `services/actions/commit_status.go` at `v15.0.7`, a run triggered by `issue_comment` creates no commit status, so its verdict cannot reach the PR's check. This is a source read, not a live probe: no runner serves the throwaway owner. Options and their trade-offs belong in the PR's decision tree and a follow-up. - Grading cross-repo targets. - The body regex arm's over-match inside code spans. It is the safe direction, and it is unchanged here. - `ListIssueComments` re-reading the unpaginated `/issues/{n}/comments` until `MaxPages`. This is a separate defect, found while probing.

claude added the

labels

2026-10-02 11:51:24 +02:00

claude referenced this issue

2026-10-02 14:32:29 +02:00

docs(agents): how a fix lands - issue, reviewed PR, Refs, close by hand (#1497) #1498

claude referenced this issue from a commit

2026-10-02 14:33:54 +02:00

docs(agents): name every close surface #1506 measured in section 15a

claude commented

2026-10-02 14:34:14 +02:00

Author

Member

Added one acceptance criterion to this issue's body, at the request of review 7175 on #1498.

#1498 adds AGENTS.md section 15a, whose table says which close surfaces ac-closure-check grades. It cites this issue's throwaway-repo rows for the PR title, inline review comments, review bodies and qualified references. When this fix lands, that table's "Graded" column has to flip for the surfaces the gate starts reading, and the new last box tracks that. No other text in the issue changed.

Added one acceptance criterion to this issue's body, at the request of review 7175 on #1498. #1498 adds AGENTS.md section 15a, whose table says which close surfaces `ac-closure-check` grades. It cites this issue's throwaway-repo rows for the PR title, inline review comments, review bodies and qualified references. When this fix lands, that table's "Graded" column has to flip for the surfaces the gate starts reading, and the new last box tracks that. No other text in the issue changed.

claude referenced this issue

2026-10-02 14:34:24 +02:00

docs(agents): how a fix lands - issue, reviewed PR, Refs, close by hand (#1497) #1498

claude referenced this issue

2026-10-02 14:34:44 +02:00

docs(agents): how a fix lands - issue, reviewed PR, Refs, close by hand (#1497) #1498

claude referenced this issue from a commit

2026-10-02 14:39:51 +02:00

docs(agents): name every close surface #1506 measured in section 15a

claude referenced this issue from a commit

2026-10-06 01:31:13 +02:00

fix(ac-closure): grade the forge's own closing references from comments, review comments and the title (#1506)

claude referenced this issue

2026-10-06 01:31:21 +02:00

fix(ac-closure): grade closing references in PR comments, inline review comments and the title (#1506) #1560

claude referenced this issue from a commit

2026-10-06 09:20:24 +02:00

fix(ac-closure): disclose review bodies and refuse a reference row without its repository (#1506)

release-bot referenced this issue

2026-10-06 11:08:03 +02:00

chore(release): v0.68.0 #1559

claude commented

2026-10-06 11:08:12 +02:00

Author

Member

Landed through #1560: 6a360903dc559026d5aa9a28282dfc513778c71f, fast-forward-merged by @alex on 2026-10-06. The landed commit is the reviewed head: merge_commit_sha equals the PR head and main's tip.

Approval: review 7275 by alex, APPROVED at 6a36090. Reviewer verdicts are reviews 7272 and 7273 (the latter at 6a36090).
Checks: at the PR head, 18 of 18 were green. At closing, the push-event checks on main had 2 green and 11 still running. They were not awaited.
All acceptance criteria are ticked above. Box 9 is split: the trigger half is done, and the workflow-header half is retired, because 0de3876 removed that workflow.
Out of scope here, now handled elsewhere: the ListIssueComments paging defect was fixed by #1495/#1496.
Still open as a known limit: a comment posted after the last pull-request event is graded only if the PR is touched or the gate is re-run before merging. The gate says this on every run.

Closing by hand, because the PR used Refs.

Landed through #1560: `6a360903dc559026d5aa9a28282dfc513778c71f`, fast-forward-merged by @alex on 2026-10-06. The landed commit is the reviewed head: `merge_commit_sha` equals the PR head and `main`'s tip. - **Approval:** review 7275 by alex, APPROVED at `6a36090`. Reviewer verdicts are reviews 7272 and 7273 (the latter at `6a36090`). - **Checks:** at the PR head, 18 of 18 were green. At closing, the push-event checks on `main` had 2 green and 11 still running. They were not awaited. - **All acceptance criteria are ticked** above. Box 9 is split: the trigger half is done, and the workflow-header half is retired, because `0de3876` removed that workflow. - **Out of scope here, now handled elsewhere:** the `ListIssueComments` paging defect was fixed by #1495/#1496. - **Still open as a known limit:** a comment posted after the last pull-request event is graded only if the PR is touched or the gate is re-run before merging. The gate says this on every run. Closing by hand, because the PR used `Refs`.

claude closed this issue

2026-10-06 11:08:12 +02:00