The wrapper keeps writing after the pull request head moves or the pull request is merged or closed #54

New issue

Open

opened 2026-10-06 05:35:53 +00:00 by jercik · 0 comments

jercik commented

2026-10-06 05:35:53 +00:00

Owner

What happens

headMoved compares only the head. It reads the pull request and returns on headSha alone (src/wrapper/orchestrator.ts:70-79). The forge client already parses state and merged (src/forge/types.ts:35-45), and assertDispatchable already refuses a closed or merged pull request (src/wrapper/event-context.ts:74-88), but only at the start of a dispatch run. A dispatched pull request that is merged afterwards is still reconciled. Reproduced as s8-merged-mid-run: the pull request was merged right after the one validation read, and the run made two writes while the forge reported it merged.
Event runs have no state check. The pull_request_target branch reads the payload and never looks at state or merged (src/wrapper/event-context.ts:103-118). The wrapper's own workflow triggers on edited (.forgejo/workflows/review.yml:57-58), and the upstream Forgejo source raises edited for a title or body change whatever the pull request's state. Reproduced as s8-event-merged: an edited event for a pull request that the payload and the forge both report closed and merged got a review and two forge writes.
The head is read once and the writes follow later. One read precedes classification and every reconcile write (src/wrapper/orchestrator.ts:122-138), and a second precedes a re-triage (src/wrapper/orchestrator.ts:105-107). After that come service reads that the wait budgets at several minutes, and forge writes with no timeout. Only the inline review carries commit_id (src/forge/client.ts:147-153). The summary edit, the thread replies and the resolutions carry no commit (src/forge/client.ts:154-160; s5b-inventory lists every write and which ones carry it). Reproduced as s5b-toctou: with the head moved right after the check was answered, a stand-in forge received four writes while its head differed from the asked one: the summary comment, the inline review, a reply and a resolution. This one is a limit rather than a bug. The forge offers no conditional write, so the window can be narrowed and not closed.
The diff is read after the summary write, and the review is posted under the earlier head. After the summary is written, the inline reconciler reads the pull request's diff (src/reconcile/inline.ts:53). The read takes only the pull request number, so it returns whatever diff the pull request has at that moment (src/forge/client.ts:80-83). The file text is read at the asked head (src/reconcile/inline.ts:65), and the review is posted with commit_id set to the asked head (src/reconcile/inline.ts:83). If the head moved in between, the comment positions come from the new head's diff and the commit from the old one. The positions and the commit then describe different heads, and a comment can sit on a line that means something else in the old commit. Reproduced as v-s5b-diff-order: the requests ran as head read (head 1111…), summary write, diff read, review post with commit_id 1111…, while the forge head had been 2222… since right after the first read. The stand-in serves one fixed diff, so this shows the order and the commit_id, not a shifted line.

What it should do instead

Make one check that a run must pass before writing: the head equals the asked head, state is open and merged is false. Run it on event runs as well as dispatch runs, and run it again before each of the three reconcile steps (summary, inline review, dispositions) and before the disposition loop, not once. Stop with the existing head-moved outcome, or a sibling for a closed pull request.

Read the diff before the first write, so the diff, the file text and commit_id come from the same head, and check the head again immediately before the inline review is posted.

None of this closes the window in item 3. The workflow's cancel-in-progress group (.forgejo/workflows/review.yml:67-68) is what ends a superseded run today.

Why it matters

A merged or closed pull request gets a new summary comment and inline threads that nobody will act on, and the run asks the service for a review of it first. A head that has moved gets thread replies and resolutions from a review of the old head, and inline comments whose positions and commit disagree. None of this has a security effect: every write is still a comment or a conversation resolution.

🤖 Generated with Claude Code

The wrapper checks the pull request head once before it writes, ignores whether the pull request is still open, and does not look at the state at all on event runs. It also reads the pull request diff after its first write. Comments can land on a pull request that was merged or closed, and on a head the review was not made for. ## What happens 1. **`headMoved` compares only the head.** It reads the pull request and returns on `headSha` alone ([`src/wrapper/orchestrator.ts:70-79`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/wrapper/orchestrator.ts#L70-L79)). The forge client already parses `state` and `merged` ([`src/forge/types.ts:35-45`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/forge/types.ts#L35-L45)), and `assertDispatchable` already refuses a closed or merged pull request ([`src/wrapper/event-context.ts:74-88`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/wrapper/event-context.ts#L74-L88)), but only at the start of a dispatch run. A dispatched pull request that is merged afterwards is still reconciled. Reproduced as `s8-merged-mid-run`: the pull request was merged right after the one validation read, and the run made two writes while the forge reported it merged. 2. **Event runs have no state check.** The `pull_request_target` branch reads the payload and never looks at `state` or `merged` ([`src/wrapper/event-context.ts:103-118`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/wrapper/event-context.ts#L103-L118)). The wrapper's own workflow triggers on `edited` ([`.forgejo/workflows/review.yml:57-58`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/.forgejo/workflows/review.yml#L57-L58)), and the upstream Forgejo source raises `edited` for a title or body change whatever the pull request's state. Reproduced as `s8-event-merged`: an `edited` event for a pull request that the payload and the forge both report closed and merged got a review and two forge writes. 3. **The head is read once and the writes follow later.** One read precedes classification and every reconcile write ([`src/wrapper/orchestrator.ts:122-138`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/wrapper/orchestrator.ts#L122-L138)), and a second precedes a re-triage ([`src/wrapper/orchestrator.ts:105-107`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/wrapper/orchestrator.ts#L105-L107)). After that come service reads that the wait budgets at several minutes, and forge writes with no timeout. Only the inline review carries `commit_id` ([`src/forge/client.ts:147-153`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/forge/client.ts#L147-L153)). The summary edit, the thread replies and the resolutions carry no commit ([`src/forge/client.ts:154-160`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/forge/client.ts#L154-L160); `s5b-inventory` lists every write and which ones carry it). Reproduced as `s5b-toctou`: with the head moved right after the check was answered, a stand-in forge received four writes while its head differed from the asked one: the summary comment, the inline review, a reply and a resolution. This one is a limit rather than a bug. The forge offers no conditional write, so the window can be narrowed and not closed. 4. **The diff is read after the summary write, and the review is posted under the earlier head.** After the summary is written, the inline reconciler reads the pull request's diff ([`src/reconcile/inline.ts:53`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/reconcile/inline.ts#L53)). The read takes only the pull request number, so it returns whatever diff the pull request has at that moment ([`src/forge/client.ts:80-83`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/forge/client.ts#L80-L83)). The file text is read at the asked head ([`src/reconcile/inline.ts:65`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/reconcile/inline.ts#L65)), and the review is posted with `commit_id` set to the asked head ([`src/reconcile/inline.ts:83`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/src/reconcile/inline.ts#L83)). If the head moved in between, the comment positions come from the new head's diff and the commit from the old one. The positions and the commit then describe different heads, and a comment can sit on a line that means something else in the old commit. Reproduced as `v-s5b-diff-order`: the requests ran as head read (head `1111…`), summary write, diff read, review post with `commit_id` `1111…`, while the forge head had been `2222…` since right after the first read. The stand-in serves one fixed diff, so this shows the order and the `commit_id`, not a shifted line. ## What it should do instead Make one check that a run must pass before writing: the head equals the asked head, `state` is `open` and `merged` is `false`. Run it on event runs as well as dispatch runs, and run it again before each of the three reconcile steps (summary, inline review, dispositions) and before the disposition loop, not once. Stop with the existing `head-moved` outcome, or a sibling for a closed pull request. Read the diff before the first write, so the diff, the file text and `commit_id` come from the same head, and check the head again immediately before the inline review is posted. None of this closes the window in item 3. The workflow's `cancel-in-progress` group ([`.forgejo/workflows/review.yml:67-68`](https://code.j4k.dev/j4k-oss/review-wrapper/src/commit/9f272a522e8ed6f748533761de45ee3e94fd7481/.forgejo/workflows/review.yml#L67-L68)) is what ends a superseded run today. ## Why it matters A merged or closed pull request gets a new summary comment and inline threads that nobody will act on, and the run asks the service for a review of it first. A head that has moved gets thread replies and resolutions from a review of the old head, and inline comments whose positions and commit disagree. None of this has a security effect: every write is still a comment or a conversation resolution. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

j4k-oss/review-wrapper#54

No description provided.

Rows
Columns