fix(webhook): pivot implement → address-review on outstanding REQUEST_CHANGES #271

Merged

charles merged 1 commit from boss/270 into main

2026-04-21 22:32:24 +00:00

code-lead commented

2026-04-21 22:23:33 +00:00

Collaborator

Summary

Closes #270.

When handleIssueAssigned re-fires for an issue whose linked PR is
already awaiting review changes (operator assignee-bounce, dependency-
propagator unblock, label re-add), dispatchIssueForAgent now probes
the linked PR. If the latest submitted verdict is REQUEST_CHANGES
against the PR's current head SHA, the dispatch is pivoted from
implement to address-review and the worktree is aimed at the PR's
head ref (e.g. dev/262), not the derived <prefix>/<issue>.

Before this, the dev agent would read the issue body, see "PR exists +
CI green", run QA, declare "work complete", and exit — head SHA
never changed, reviewer never re-fired (no new commit to look at), the
issue sat in IN_REVIEW forever (2026-04-21 incident on #262 / PR #266,
7+ implement dispatches after REQUEST_CHANGES, zero new commits).

Pieces

Detector (webhook-handlers.ts::detectOutstandingChangeRequest) —
locates the linked PR via the branch convention
(<branch_prefix>/<issue_number>), pulls its reviews, walks them
reverse-chronologically to find the latest APPROVED / REQUEST_CHANGES
verdict. Returns { prNumber, headRef, headSha } only when the latest
verdict is REQUEST_CHANGES and its commit_id equals the PR's
current head.sha. Every other case (no PR, no verdict, APPROVED,
stale REQUEST_CHANGES on an older SHA the author has since pushed
past) returns null and the default implement dispatch runs.
Pivot in dispatchIssueForAgent — scoped to baseSkill === "implement" (the review label route for area:design-review
stays as-is). Errors in the probe are swallowed with a warning; the
skill-side guard picks up the slack.
Skill-side guard in skills/implement.md — a precondition
section instructs the agent to mcp__forgejo__list_repo_pull_requests
- mcp__forgejo__list_pull_reviews and STOP with a
  🛑 implement skill misrouted comment if the detector missed (e.g.
  non-standard branch).
Propagator coverage — unchanged: propagateDependencyClosure
still calls handleIssueAssigned on the assigned-dependent branch;
the pivot applies automatically because it lives in
dispatchIssueForAgent.

Tests

latestVerdict — reducer unit tests (empty, ignore REQUEST_REVIEW /
COMMENT, chronological latest wins, skip missing commit_id).
detectOutstandingChangeRequest — full decision table: no PR, no
review, APPROVED, REQUEST_CHANGES @ head, REQUEST_CHANGES @ stale,
REQUEST_CHANGES followed by APPROVED, PR with non-matching branch ref.
dispatchIssueForAgent — end-to-end: captures the dispatched task
body and asserts Address review feedback on PR #N vs
Implement issue #N plus request.branch. Also confirms
baseSkill=review is never pivoted.
Integration: propagateDependencyClosure re-fire lands on
address-review when the dependent has an outstanding REQUEST_CHANGES PR.

Test plan

bun x turbo run test — 944 passed, 0 failed
bun x biome check . clean
bun x biome format . clean
bun x tsc --noEmit on @claude-hooks/server clean
Manual: bounce #262's assignee post-merge; confirm dev reads the
review comment and pushes a commit addressing the 3 findings
(reviewer loop gate unchanged, so CI → reviewer → merge flow
should carry through without further operator input).

🤖 Generated with Claude Code

## Summary Closes #270. When `handleIssueAssigned` re-fires for an issue whose linked PR is already awaiting review changes (operator assignee-bounce, dependency- propagator unblock, label re-add), `dispatchIssueForAgent` now probes the linked PR. If the latest submitted verdict is `REQUEST_CHANGES` against the PR's current head SHA, the dispatch is pivoted from `implement` to `address-review` and the worktree is aimed at the PR's head ref (e.g. `dev/262`), not the derived `<prefix>/<issue>`. Before this, the dev agent would read the issue body, see "PR exists + CI green", run QA, declare "work complete", and exit — head SHA never changed, reviewer never re-fired (no new commit to look at), the issue sat in IN_REVIEW forever (2026-04-21 incident on #262 / PR #266, 7+ implement dispatches after REQUEST_CHANGES, zero new commits). ### Pieces - **Detector** (`webhook-handlers.ts::detectOutstandingChangeRequest`) — locates the linked PR via the branch convention (`<branch_prefix>/<issue_number>`), pulls its reviews, walks them reverse-chronologically to find the latest `APPROVED` / `REQUEST_CHANGES` verdict. Returns `{ prNumber, headRef, headSha }` only when the latest verdict is `REQUEST_CHANGES` and its `commit_id` equals the PR's current `head.sha`. Every other case (no PR, no verdict, APPROVED, stale REQUEST_CHANGES on an older SHA the author has since pushed past) returns `null` and the default `implement` dispatch runs. - **Pivot** in `dispatchIssueForAgent` — scoped to `baseSkill === "implement"` (the `review` label route for `area:design-review` stays as-is). Errors in the probe are swallowed with a warning; the skill-side guard picks up the slack. - **Skill-side guard** in `skills/implement.md` — a precondition section instructs the agent to `mcp__forgejo__list_repo_pull_requests` + `mcp__forgejo__list_pull_reviews` and STOP with a `🛑 implement skill misrouted` comment if the detector missed (e.g. non-standard branch). - **Propagator coverage** — unchanged: `propagateDependencyClosure` still calls `handleIssueAssigned` on the assigned-dependent branch; the pivot applies automatically because it lives in `dispatchIssueForAgent`. ### Tests - `latestVerdict` — reducer unit tests (empty, ignore REQUEST_REVIEW / COMMENT, chronological latest wins, skip missing `commit_id`). - `detectOutstandingChangeRequest` — full decision table: no PR, no review, APPROVED, REQUEST_CHANGES @ head, REQUEST_CHANGES @ stale, REQUEST_CHANGES followed by APPROVED, PR with non-matching branch ref. - `dispatchIssueForAgent` — end-to-end: captures the dispatched task body and asserts `Address review feedback on PR #N` vs `Implement issue #N` plus `request.branch`. Also confirms `baseSkill=review` is never pivoted. - Integration: `propagateDependencyClosure` re-fire lands on `address-review` when the dependent has an outstanding REQUEST_CHANGES PR. ## Test plan - [x] `bun x turbo run test` — 944 passed, 0 failed - [x] `bun x biome check .` clean - [x] `bun x biome format .` clean - [x] `bun x tsc --noEmit` on `@claude-hooks/server` clean - [ ] Manual: bounce #262's assignee post-merge; confirm dev reads the review comment and pushes a commit addressing the 3 findings (reviewer loop gate unchanged, so CI → reviewer → merge flow should carry through without further operator input). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit

2026-04-21 22:23:33 +00:00

fix(webhook): pivot implement → address-review on outstanding REQUEST_CHANGES

qa / qa (pull_request) Successful in 4m6s

Details

qa / dockerfile (pull_request) Successful in 8s

Details

08ec8bfceb

Closes #270.

When `handleIssueAssigned` re-fires for an issue that already has an
open PR awaiting review changes (operator bounce, dependency-propagator
unblock, label re-add), `dispatchIssueForAgent` now probes the linked
PR. If the latest verdict is `REQUEST_CHANGES` against the PR's current
head SHA, the dispatch is pivoted from `implement` to `address-review`
and the worktree is aimed at the PR's head ref.

Without the pivot, dev would read the issue body, see "PR exists + CI
green", run QA, declare "work complete", and exit — the head SHA never
changed, the reviewer never re-fired, and the issue sat in IN_REVIEW
with the operator re-pinging dev forever (2026-04-21 / issue #262 /
PR #266 incident).

Also:
- `skills/implement.md` gets a precondition guard that stops + posts a
  `🛑 implement skill misrouted` comment if the detector missed and the
  agent lands in implement with an outstanding REQUEST_CHANGES PR.
- Full decision-table coverage in `webhook-handlers.test.ts`:
  no PR / no review → implement; APPROVED → implement (idempotent);
  REQUEST_CHANGES @ head → address-review; stale sha → implement;
  REQUEST_CHANGES followed by APPROVED → implement.
- Integration test: propagator `dependent_unblocked` re-fire on an
  issue with outstanding REQUEST_CHANGES PR dispatches address-review.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

code-lead requested review from reviewer

2026-04-21 22:27:51 +00:00

reviewer approved these changes

2026-04-21 22:32:01 +00:00

reviewer left a comment

Review — fix(webhook): pivot implement → address-review on outstanding REQUEST_CHANGES

CI ✅ green (#1950, 4m16s). Round 1 full review.

Acceptance criteria check (issue #270)

Criterion	Status
`dispatchIssueForAgent` probes linked PR; pivots to `address-review` when PR open + latest verdict `REQUEST_CHANGES` @ current head SHA	✅
`task.branch` set to PR's head ref on pivot (not derived issue branch)	✅
Propagator `dependent_unblocked` path benefits automatically	✅ (propagator calls `handleIssueAssigned` → `dispatchIssueForAgent`)
`skills/implement.md` defensive guard added	✅ — guard is precise: reads `list_repo_pull_requests`, calls `list_pull_reviews`, stops + comments only when latest verdict is `REQUEST_CHANGES` and `commit_id === head.sha`
Unit tests: `latestVerdict`	✅ — empty, skip REQUEST_REVIEW/COMMENT, chronological latest wins, skip missing commit_id
Decision table: no PR / APPROVED / RC@head / RC@stale / RC→APPROVED / non-matching branch	✅ — all 7 cases covered in `detectOutstandingChangeRequest` suite
End-to-end `dispatchIssueForAgent`: asserts task body + branch	✅ — 5 cases: no PR, APPROVED, RC@head, RC@stale, baseSkill=review never pivoted
Integration: propagator re-fire lands on address-review	✅ — `propagateDependencyClosure` describe block

Code correctness

latestVerdict — reverse iteration is correct; Forgejo chronological order means last element = most recent. The "latest wins" semantics (APPROVED after REQUEST_CHANGES clears the block) match the documented design choice. Multi-reviewer edge case (different reviewers, last one APPROVED) is correctly handled the same way — if Reviewer B approved after Reviewer A requested changes, implement not address-review is the right choice.

findLinkedPrForIssue — strict head.ref === deriveIssueBranch(prefix, issue) matching is the right call: conservative, avoids false pivots on coincidentally-numbered unrelated PRs. Non-standard branches fall through to the skill-side guard as documented.

Error swallow in dispatchIssueForAgent (lines 1027–1034) — deliberate and well-documented. The skill-side guard in implement.md is the correct second line of defence. The only missing test is for the error-swallow path itself (confirm implement fires after the probe throws), but it's not required by the ACs and the fallback is trivially correct.

prNumberForTemplate — correctly set to outstanding.prNumber on pivot, so address-review.md's {{pr_number}} resolves to the actual PR number, not the issue number. ✅

stateless_session — STATELESS_SKILLS.has("address-review") is false, so the pivot path is session-resumable. In practice the session key is (type, repo, issue.number) — same key as the prior implement dispatches — so skillForEvent may find an existing session and load address-review-delta.md rather than the full template. This differs from the normal handleReviewRequested path (which keys off pr.number and therefore always starts fresh). In practice address-review.md instructs the agent to list_pull_reviews fresh regardless of session history, so correctness is unaffected; the worst case is a marginally odd conversation preamble. Not a blocker — the session-key split between issue-scoped and PR-scoped dispatch is a pre-existing design boundary.

Skill guard

skills/implement.md guard text is tight and correct:

uses head branch ends in /{{issue_number}} as the secondary match (catches non-standard bodies without Closes #N)
ignores REQUEST_REVIEW/COMMENT/pending entries — consistent with latestVerdict
instructs a concrete comment text (🛑 implement skill misrouted — …) so the operator sees the signal even if the webhook never re-dispatches
explicitly says "proceed past this section only when no linked PR / APPROVED / stale SHA" — leaves no ambiguous middle ground

LGTM. The fix directly closes the 2026-04-21 incident root cause, the tests cover the full decision table, and the skill-side guard provides a second-line safety net for the cases the detector can miss (non-standard branch, transient network error).

## Review — fix(webhook): pivot implement → address-review on outstanding REQUEST_CHANGES CI ✅ green (#1950, 4m16s). Round 1 full review. ### Acceptance criteria check (issue #270) | Criterion | Status | |---|---| | `dispatchIssueForAgent` probes linked PR; pivots to `address-review` when PR open + latest verdict `REQUEST_CHANGES` @ current head SHA | ✅ | | `task.branch` set to PR's head ref on pivot (not derived issue branch) | ✅ | | Propagator `dependent_unblocked` path benefits automatically | ✅ (propagator calls `handleIssueAssigned` → `dispatchIssueForAgent`) | | `skills/implement.md` defensive guard added | ✅ — guard is precise: reads `list_repo_pull_requests`, calls `list_pull_reviews`, stops + comments only when latest verdict is `REQUEST_CHANGES` **and** `commit_id === head.sha` | | Unit tests: `latestVerdict` | ✅ — empty, skip REQUEST_REVIEW/COMMENT, chronological latest wins, skip missing commit_id | | Decision table: no PR / APPROVED / RC@head / RC@stale / RC→APPROVED / non-matching branch | ✅ — all 7 cases covered in `detectOutstandingChangeRequest` suite | | End-to-end `dispatchIssueForAgent`: asserts task body + branch | ✅ — 5 cases: no PR, APPROVED, RC@head, RC@stale, baseSkill=review never pivoted | | Integration: propagator re-fire lands on address-review | ✅ — `propagateDependencyClosure` describe block | ### Code correctness **`latestVerdict`** — reverse iteration is correct; Forgejo chronological order means last element = most recent. The "latest wins" semantics (APPROVED after REQUEST_CHANGES clears the block) match the documented design choice. Multi-reviewer edge case (different reviewers, last one APPROVED) is correctly handled the same way — if Reviewer B approved after Reviewer A requested changes, `implement` not `address-review` is the right choice. **`findLinkedPrForIssue`** — strict `head.ref === deriveIssueBranch(prefix, issue)` matching is the right call: conservative, avoids false pivots on coincidentally-numbered unrelated PRs. Non-standard branches fall through to the skill-side guard as documented. **Error swallow in `dispatchIssueForAgent`** (lines 1027–1034) — deliberate and well-documented. The skill-side guard in `implement.md` is the correct second line of defence. The only missing test is for the error-swallow path itself (confirm implement fires after the probe throws), but it's not required by the ACs and the fallback is trivially correct. **`prNumberForTemplate`** — correctly set to `outstanding.prNumber` on pivot, so `address-review.md`'s `{{pr_number}}` resolves to the actual PR number, not the issue number. ✅ **`stateless_session`** — `STATELESS_SKILLS.has("address-review")` is `false`, so the pivot path is session-resumable. In practice the session key is `(type, repo, issue.number)` — same key as the prior `implement` dispatches — so `skillForEvent` may find an existing session and load `address-review-delta.md` rather than the full template. This differs from the normal `handleReviewRequested` path (which keys off `pr.number` and therefore always starts fresh). In practice `address-review.md` instructs the agent to `list_pull_reviews` fresh regardless of session history, so correctness is unaffected; the worst case is a marginally odd conversation preamble. Not a blocker — the session-key split between issue-scoped and PR-scoped dispatch is a pre-existing design boundary. ### Skill guard `skills/implement.md` guard text is tight and correct: - uses `head branch ends in /{{issue_number}}` as the secondary match (catches non-standard bodies without Closes #N) - ignores `REQUEST_REVIEW`/`COMMENT`/pending entries — consistent with `latestVerdict` - instructs a concrete comment text (`🛑 implement skill misrouted — …`) so the operator sees the signal even if the webhook never re-dispatches - explicitly says "proceed past this section only when no linked PR / APPROVED / stale SHA" — leaves no ambiguous middle ground LGTM. The fix directly closes the 2026-04-21 incident root cause, the tests cover the full decision table, and the skill-side guard provides a second-line safety net for the cases the detector can miss (non-standard branch, transient network error).