fix(webhook): dispatch address-review skill when re-firing assignee on a PR with outstanding REQUEST_CHANGES #270

New issue

Closed

opened 2026-04-21 22:11:34 +00:00 by claude-desktop · 0 comments

claude-desktop commented

2026-04-21 22:11:34 +00:00

Collaborator

Bug

When an issue with an open linked PR gets re-dispatched via issues.assigned (operator bounce, dependency propagator, label re-add), handleIssueAssigned routes the task through the implement skill — even when the PR carries an outstanding REQUEST_CHANGES review that's the actual blocker.

The implement skill runs "build the feature", sees PR already exists + CI green + tests pass, concludes work complete — QA green across the board, and exits without reading the review comment. Head sha doesn't change. Reviewer doesn't re-review (no new commit to look at). The issue sits in IN_REVIEW forever while dev keeps getting pinged and saying "done already".

Repro — 2026-04-21 / 2026-04-22, issue #262 / PR #266

22:31 — dev-default dispatched via issues.assigned → implement skill → opens PR #266 at sha 21ace1df.
22:44–23:17 — three CI-failure → fix-ci cycles land a series of commits, ending at sha c07b5d7d.
23:21 — CI green → reviewer dispatched.
23:24 — reviewer posts REQUEST_CHANGES with 3 findings (missing last-modified timestamp, etc.).
23:24 — dev dispatched via pull_request_review.submitted with state=changes_requested → address-review skill → task bcedc120. Did not settle before operator intervention.
23:43 — operator closes #263 → propagator dependent_unblocked re-fires handleIssueAssigned on #262 → implement skill → task 01964658, 7 turns, $0.18. Reads issue body, lists PRs, notices PR #266 exists, runs QA, concludes "work complete".
23:49 — operator manual assignee-bounce on #262 → issues.assigned → implement skill → task d98aa129, 15 turns, $0.35. Same pattern: reads issue, runs QA, declares victory.

Net effect: PR head sha stays at c07b5d7d (the sha the reviewer already rejected). REQUEST_CHANGES stands. Dev keeps getting pinged, keeps saying "done".

Acceptance criteria

Routing fix

apps/server/src/webhook-handlers.ts::dispatchIssueForAgent (or the equivalent assignment-webhook entry point) looks up the issue's linked PR before picking a skill. If:
- the linked PR is open
- the latest review is REQUEST_CHANGES
- the PR head sha equals the sha the REQUEST_CHANGES review was submitted against
then the dispatcher picks the address-review skill and sets task.branch to the PR's head ref (so the worker checks out dev/262, not dev/N derived from issue number).
Same check applies to the propagator's dependent_unblocked re-fire path — it currently calls handleIssueAssigned unconditionally.

Skill side — defensive

skills/implement.md gets a guard at the top: "Before executing, call mcp__forgejo__list_repo_pull_requests scoped to this issue. If a PR exists with an outstanding REQUEST_CHANGES review, STOP and post a comment 🛑 implement skill misrouted — this issue has PR #N awaiting review changes; operator should re-dispatch address-review. Do not run QA and claim completion."

Verification

Unit test in apps/server/src/webhook-handlers.test.ts — synthesize an issue + linked PR with a REQUEST_CHANGES review; assert dispatchIssueForAgent picks address-review.
Table test: PR open + no review → implement; PR open + APPROVED → implement (idempotent); PR open + REQUEST_CHANGES on same sha → address-review; PR open + REQUEST_CHANGES on stale sha (author already pushed a response) → implement.
Integration test: simulate propagator dependent_unblocked on an issue with outstanding REQUEST_CHANGES PR; assert address-review dispatched, not implement.
Manual: bounce #262's assignee post-fix; confirm dev reads the review comment and pushes a new commit addressing the 3 findings.

Out of scope

Adding a new webhook event type. Use existing issues.assigned + handleIssueAssigned; only the skill pick inside that handler changes.
Re-writing address-review.md itself — the skill works when invoked; the bug is that it isn't.
The reviewer-side of the loop — reviewer behaves fine.

References

apps/server/src/webhook-handlers.ts::handleIssueAssigned + dispatchIssueForAgent.
apps/server/src/deps.ts::propagateDependencyClosure — the assigned + blockers clear branch re-fires handleIssueAssigned.
skills/implement.md vs. skills/address-review.md.
2026-04-21 incident: #262 / PR #266 — 7+ dev dispatches after REQUEST_CHANGES, zero new commits, head_sha stuck on c07b5d7d.
Related but distinct: #269 janitor stale_idle_assigned rule — would flag this, but doesn't diagnose the cause.

## Bug When an issue with an open linked PR gets re-dispatched via `issues.assigned` (operator bounce, dependency propagator, label re-add), `handleIssueAssigned` routes the task through the **`implement`** skill — even when the PR carries an outstanding `REQUEST_CHANGES` review that's the actual blocker. The implement skill runs "build the feature", sees PR already exists + CI green + tests pass, concludes `work complete — QA green across the board`, and exits without reading the review comment. Head sha doesn't change. Reviewer doesn't re-review (no new commit to look at). The issue sits in `IN_REVIEW` forever while dev keeps getting pinged and saying "done already". ## Repro — 2026-04-21 / 2026-04-22, issue #262 / PR #266 1. 22:31 — dev-default dispatched via `issues.assigned` → implement skill → opens PR #266 at sha `21ace1df`. 2. 22:44–23:17 — three CI-failure → fix-ci cycles land a series of commits, ending at sha `c07b5d7d`. 3. 23:21 — CI green → reviewer dispatched. 4. 23:24 — reviewer posts `REQUEST_CHANGES` with 3 findings (missing last-modified timestamp, etc.). 5. 23:24 — dev dispatched via `pull_request_review.submitted` with `state=changes_requested` → address-review skill → task `bcedc120`. Did not settle before operator intervention. 6. 23:43 — operator closes #263 → propagator `dependent_unblocked` re-fires `handleIssueAssigned` on #262 → **implement** skill → task `01964658`, 7 turns, $0.18. Reads issue body, lists PRs, notices PR #266 exists, runs QA, concludes "work complete". 7. 23:49 — operator manual assignee-bounce on #262 → `issues.assigned` → **implement** skill → task `d98aa129`, 15 turns, $0.35. Same pattern: reads issue, runs QA, declares victory. Net effect: PR head sha stays at `c07b5d7d` (the sha the reviewer already rejected). REQUEST_CHANGES stands. Dev keeps getting pinged, keeps saying "done". ## Acceptance criteria ### Routing fix - [ ] `apps/server/src/webhook-handlers.ts::dispatchIssueForAgent` (or the equivalent assignment-webhook entry point) looks up the issue's linked PR before picking a skill. If: - the linked PR is `open` - the latest review is `REQUEST_CHANGES` - the PR head sha equals the sha the `REQUEST_CHANGES` review was submitted against then the dispatcher picks the **`address-review`** skill and sets `task.branch` to the PR's head ref (so the worker checks out `dev/262`, not `dev/N` derived from issue number). - [ ] Same check applies to the propagator's `dependent_unblocked` re-fire path — it currently calls `handleIssueAssigned` unconditionally. ### Skill side — defensive - [ ] `skills/implement.md` gets a guard at the top: "Before executing, call `mcp__forgejo__list_repo_pull_requests` scoped to this issue. If a PR exists with an outstanding `REQUEST_CHANGES` review, STOP and post a comment `🛑 implement skill misrouted — this issue has PR #N awaiting review changes; operator should re-dispatch address-review`. Do not run QA and claim completion." ### Verification - [ ] Unit test in `apps/server/src/webhook-handlers.test.ts` — synthesize an issue + linked PR with a `REQUEST_CHANGES` review; assert `dispatchIssueForAgent` picks `address-review`. - [ ] Table test: PR `open` + no review → `implement`; PR `open` + `APPROVED` → `implement` (idempotent); PR `open` + `REQUEST_CHANGES` on same sha → `address-review`; PR `open` + `REQUEST_CHANGES` on stale sha (author already pushed a response) → `implement`. - [ ] Integration test: simulate propagator `dependent_unblocked` on an issue with outstanding REQUEST_CHANGES PR; assert `address-review` dispatched, not `implement`. - [ ] Manual: bounce #262's assignee post-fix; confirm dev reads the review comment and pushes a new commit addressing the 3 findings. ## Out of scope - Adding a new webhook event type. Use existing `issues.assigned` + `handleIssueAssigned`; only the skill pick inside that handler changes. - Re-writing `address-review.md` itself — the skill works when invoked; the bug is that it isn't. - The reviewer-side of the loop — reviewer behaves fine. ## References - `apps/server/src/webhook-handlers.ts::handleIssueAssigned` + `dispatchIssueForAgent`. - `apps/server/src/deps.ts::propagateDependencyClosure` — the `assigned + blockers clear` branch re-fires `handleIssueAssigned`. - `skills/implement.md` vs. `skills/address-review.md`. - 2026-04-21 incident: #262 / PR #266 — 7+ dev dispatches after REQUEST_CHANGES, zero new commits, head_sha stuck on `c07b5d7d`. - Related but distinct: #269 janitor `stale_idle_assigned` rule — would flag this, but doesn't diagnose the cause.

claude-desktop added the

area:webhook

type:bug

labels

2026-04-21 22:11:35 +00:00