charles/claude-hooks

Fork 0

Investigate: address-review never fired for PR #604 / issue #567 after changes_requested review #610

New issue

Closed

opened 2026-04-30 21:09:04 +00:00 by claude-desktop · 1 comment

claude-desktop commented

2026-04-30 21:09:04 +00:00

Collaborator

User story

As the operator of claude-hooks, I want the dispatch chain that turns a changes_requested PR review into a fresh address-review task to be reliable, so that I do not end up with idle board cards waiting indefinitely while CI is green and feedback is sitting on the PR.

Background

Concrete repro from 2026-04-30:

Issue: charles/claude-hooks#567 (feat(workspace): session rename / delete / export / search).
PR: #604, opened by dev, labels area:dashboard, area:sessions, type:user-story, state open / merged: false.
CI on the head SHA: succeeded.
A reviewer posted a changes_requested review.
Expected: webhook → review-graph / pr-changes-requested-graph → fresh address-review task enqueued onto the dev pool. Board card should flip to running (or queued) for ~12 min and then resolve.
Actual: task_history for (repo='charles/claude-hooks', issue_number=567) contains exactly one row — the original implement task that finished status=success ~12 min before this ticket was filed. No second row was inserted. Board card stays in IN REVIEW with stall age climbing.

This is silent — there is no error in the operator's view. The card just sits.

Investigation checklist

Confirm the Forgejo system webhook actually delivered a pull_request_review event for PR #604 (forge.jacquin.app admin → System Hooks → Recent Deliveries — there should be a 200 from claude-hooks). If missing, the bug is upstream of the service.
If the delivery is recorded, follow it through apps/server/src/http/webhook.ts → flow runner → review-requested-graph / pr-changes-requested-graph. Add temporary console.log taps at each fork if needed; reproduce with curl against 127.0.0.1:4500/webhook/forgejo using a saved payload.
Check the review-loop cap (apps/server/src/domain/workflow/review-loop.ts:17 — caps address-review dispatches per PR). Verify the cap counter for PR #604 is below the limit; rule out "we silently hit the cap and fell through".
Check the implement→address-review pivot path (apps/server/src/domain/workflow/event-handlers.ts:338-347): does it fire on this issue, or does the gate (e.g. PR head SHA mismatch, missing labels, session check) drop the request?
Cross-check session state in agents.db for <dev>:charles/claude-hooks:567. If a session exists but the resume path is broken, the dispatch may be silently filtered.
Determine which arm of the diagnostic above is the real cause and capture the trace in this ticket as a comment.

Acceptance criteria — fix

(Once the cause is known, this section gets sharpened. Initial sketch below.)

Root cause identified and documented inline (PR description + ticket comment) with the exact log line / counter / DB row that explains the silent drop.
Fix lands in the responsible module — most likely one of:
- apps/server/src/domain/flows/pr-changes-requested-graph.json / .ts
- apps/server/src/domain/workflow/event-handlers.ts (pivot path)
- apps/server/src/domain/workflow/review-loop.ts (cap accounting)
- apps/server/src/http/forge-event-broadcast.ts (envelope routing)
Regression test pinning the exact replay: a pull_request_review event with state=changes_requested for a PR whose previous implement task succeeded MUST enqueue a fresh address-review task, and a follow-up call to the same handler must respect the review-loop cap (so we don't trade one bug for an infinite loop).
If the fix introduces new logging, it goes through the existing console.warn / console.log channels — no new logger.

Acceptance criteria — observability

When the chain correctly declines to dispatch (cap hit, head SHA mismatch, etc.), the decision is logged at console.log with enough context ([review-loop] cap hit for ${repo}#${pr}: dispatched=N max=M) so a future operator does not have to git-blame their way to the answer.
When the chain incorrectly drops, the same log line should make it obvious in retrospect.

Out of scope

The operator UI escape hatch — covered in the companion ticket Force-clear / kill stuck board card from the UI.
Generalising review-loop accounting beyond what the bug demands.
Replacing the flow runner.

References

DB query that surfaced the missing row (state DB: ~/.local/state/claude-hooks/tasks.db):

SELECT id, agent_type, status,
       datetime(started_at/1000, 'unixepoch', 'localtime')  AS started,
       datetime(finished_at/1000, 'unixepoch', 'localtime') AS finished
FROM task_history
WHERE repo='charles/claude-hooks' AND issue_number=567
ORDER BY started_at DESC LIMIT 10;
-- Returns ONE row: dev | success | 2026-04-30 22:35:55 | 2026-04-30 22:48:12

apps/server/src/domain/workflow/review-loop.ts — cap mechanism and dispatch-suppression seam.
apps/server/src/domain/workflow/event-handlers.ts:338-347 — implement→address-review pivot.
apps/server/src/domain/flows/pr-changes-requested-graph.json and the matching .ts glue.
apps/server/src/http/forge-event-broadcast.ts — review event normalisation.
Forgejo system-webhook delivery log: https://forge.jacquin.app/-/admin/hooks → recent deliveries for /webhook/forgejo.
Forge state of PR #604 at the time of filing: state: open, merged: false, merged_at: null, closed_at: null.

## User story As the operator of claude-hooks, I want the dispatch chain that turns a `changes_requested` PR review into a fresh `address-review` task to be reliable, so that I do not end up with idle board cards waiting indefinitely while CI is green and feedback is sitting on the PR. ## Background Concrete repro from 2026-04-30: - Issue: `charles/claude-hooks#567` (`feat(workspace): session rename / delete / export / search`). - PR: `#604`, opened by `dev`, labels `area:dashboard`, `area:sessions`, `type:user-story`, state `open` / `merged: false`. - CI on the head SHA: succeeded. - A reviewer posted a `changes_requested` review. - Expected: webhook → review-graph / `pr-changes-requested-graph` → fresh `address-review` task enqueued onto the dev pool. Board card should flip to `running` (or `queued`) for ~12 min and then resolve. - Actual: `task_history` for `(repo='charles/claude-hooks', issue_number=567)` contains exactly **one** row — the original implement task that finished `status=success` ~12 min before this ticket was filed. No second row was inserted. Board card stays in `IN REVIEW` with stall age climbing. This is silent — there is no error in the operator's view. The card just sits. ## Investigation checklist - [ ] Confirm the Forgejo system webhook actually delivered a `pull_request_review` event for PR #604 (`forge.jacquin.app` admin → System Hooks → Recent Deliveries — there should be a 200 from claude-hooks). If missing, the bug is upstream of the service. - [ ] If the delivery is recorded, follow it through `apps/server/src/http/webhook.ts` → flow runner → `review-requested-graph` / `pr-changes-requested-graph`. Add temporary `console.log` taps at each fork if needed; reproduce with `curl` against `127.0.0.1:4500/webhook/forgejo` using a saved payload. - [ ] Check the review-loop cap (`apps/server/src/domain/workflow/review-loop.ts:17` — caps address-review dispatches per PR). Verify the cap counter for PR #604 is below the limit; rule out "we silently hit the cap and fell through". - [ ] Check the implement→address-review pivot path (`apps/server/src/domain/workflow/event-handlers.ts:338-347`): does it fire on this issue, or does the gate (e.g. PR head SHA mismatch, missing labels, session check) drop the request? - [ ] Cross-check session state in `agents.db` for `<dev>:charles/claude-hooks:567`. If a session exists but the resume path is broken, the dispatch may be silently filtered. - [ ] Determine which arm of the diagnostic above is the real cause and capture the trace in this ticket as a comment. ## Acceptance criteria — fix (Once the cause is known, this section gets sharpened. Initial sketch below.) - [ ] Root cause identified and documented inline (PR description + ticket comment) with the exact log line / counter / DB row that explains the silent drop. - [ ] Fix lands in the responsible module — most likely one of: - `apps/server/src/domain/flows/pr-changes-requested-graph.json` / `.ts` - `apps/server/src/domain/workflow/event-handlers.ts` (pivot path) - `apps/server/src/domain/workflow/review-loop.ts` (cap accounting) - `apps/server/src/http/forge-event-broadcast.ts` (envelope routing) - [ ] Regression test pinning the exact replay: a `pull_request_review` event with `state=changes_requested` for a PR whose previous `implement` task succeeded MUST enqueue a fresh `address-review` task, and a follow-up call to the same handler must respect the review-loop cap (so we don't trade one bug for an infinite loop). - [ ] If the fix introduces new logging, it goes through the existing `console.warn` / `console.log` channels — no new logger. ## Acceptance criteria — observability - [ ] When the chain *correctly* declines to dispatch (cap hit, head SHA mismatch, etc.), the decision is logged at `console.log` with enough context (`[review-loop] cap hit for ${repo}#${pr}: dispatched=N max=M`) so a future operator does not have to git-blame their way to the answer. - [ ] When the chain *incorrectly* drops, the same log line should make it obvious in retrospect. ## Out of scope - The operator UI escape hatch — covered in the companion ticket `Force-clear / kill stuck board card from the UI`. - Generalising review-loop accounting beyond what the bug demands. - Replacing the flow runner. ## References - DB query that surfaced the missing row (state DB: `~/.local/state/claude-hooks/tasks.db`): ```sql SELECT id, agent_type, status, datetime(started_at/1000, 'unixepoch', 'localtime') AS started, datetime(finished_at/1000, 'unixepoch', 'localtime') AS finished FROM task_history WHERE repo='charles/claude-hooks' AND issue_number=567 ORDER BY started_at DESC LIMIT 10; -- Returns ONE row: dev | success | 2026-04-30 22:35:55 | 2026-04-30 22:48:12 ``` - `apps/server/src/domain/workflow/review-loop.ts` — cap mechanism and dispatch-suppression seam. - `apps/server/src/domain/workflow/event-handlers.ts:338-347` — implement→address-review pivot. - `apps/server/src/domain/flows/pr-changes-requested-graph.json` and the matching `.ts` glue. - `apps/server/src/http/forge-event-broadcast.ts` — review event normalisation. - Forgejo system-webhook delivery log: `https://forge.jacquin.app/-/admin/hooks` → recent deliveries for `/webhook/forgejo`. - Forge state of PR #604 at the time of filing: `state: open`, `merged: false`, `merged_at: null`, `closed_at: null`.

claude-desktop added the

area:webhook

type:bug

labels

2026-04-30 21:09:12 +00:00

dev was assigned by code-lead

2026-05-01 11:12:17 +00:00

dev referenced this issue from a commit

2026-05-01 11:47:35 +00:00

fix(flows): add observability logging and integration test for PR review dispatch (#610)

dev referenced this issue from a pull request that will close it,

2026-05-01 11:48:57 +00:00

fix(flows): add observability logging and integration test for PR review dispatch #637

dev commented

2026-05-01 11:49:28 +00:00

Collaborator

Root cause analysis

What happened

The webhook did fire (pull_request_review_rejected Forgejo header -> pull_request_review.submitted trigger) and the pr-changes-requested flow DID execute. The investigation found three compounding issues that made it appear as if no dispatch happened:

Issue 1 — `task_history` query used the wrong number

dispatch_address stores the task with issue_number = src.out.pr.number (the PR number, 604), not the linked issue number (567). A query like:

SELECT * FROM task_history WHERE repo='charles/claude-hooks' AND issue_number=567

returns zero rows even when address-review was dispatched, because the row has issue_number=604. This is expected per the graph spec.

To find address-review dispatches for PR #604, query issue_number=604.

If resolve_author could not match dev (the PR's authorLogin) to an agent envelope, the entire downstream pipeline dropped with no log line. The operator would see the flow ran but zero node-level side effects, with no indication of why.

Fix: resolveAgentByLoginHandler now logs:

[agent-nodes] resolve_by_login: no agent found for login "dev" -- dropping downstream pipeline

Issue 3 — Silent drop in `guardAuthorDispatch` cap-hit and proceed paths

When the round cap was hit or the guard passed, there was no log line. The operator could not tell if the guard passed (address-review dispatched) or tripped (force-merge instead).

Fix: guardAuthorDispatch now logs on both paths:

[review-loop] cap hit for charles/claude-hooks#604: rounds=3 max=3 -- skipping address-review, force-merging
[review-loop] author guard passed for charles/claude-hooks#604: rounds=1 max=3 -- dispatching address-review

Fix PR

PR #637 adds:

The two observability log lines above
DispatchOptions.argDefaults injection hook for tests
Integration regression test: dispatchToFlows(pull_request_review_rejected event) -> real pr-changes-requested graph -> address-review dispatched to author (was completely absent before)
Routing regression test: Forgejo pull_request_review_rejected event header correctly normalises and routes to a pull_request_review.submitted flow

## Root cause analysis ### What happened The webhook did fire (`pull_request_review_rejected` Forgejo header -> `pull_request_review.submitted` trigger) and the `pr-changes-requested` flow DID execute. The investigation found **three compounding issues** that made it appear as if no dispatch happened: --- ### Issue 1 — `task_history` query used the wrong number `dispatch_address` stores the task with `issue_number = src.out.pr.number` (the **PR number**, 604), not the linked issue number (567). A query like: ```sql SELECT * FROM task_history WHERE repo='charles/claude-hooks' AND issue_number=567 ``` returns **zero rows** even when address-review was dispatched, because the row has `issue_number=604`. This is expected per the graph spec. **To find address-review dispatches for PR #604, query `issue_number=604`.** --- ### Issue 2 — Silent FILTER_DROP when author login does not resolve If `resolve_author` could not match `dev` (the PR's `authorLogin`) to an agent envelope, the entire downstream pipeline dropped with **no log line**. The operator would see the flow ran but zero node-level side effects, with no indication of why. Fix: `resolveAgentByLoginHandler` now logs: ``` [agent-nodes] resolve_by_login: no agent found for login "dev" -- dropping downstream pipeline ``` --- ### Issue 3 — Silent drop in `guardAuthorDispatch` cap-hit and proceed paths When the round cap was hit or the guard passed, there was **no log line**. The operator could not tell if the guard passed (address-review dispatched) or tripped (force-merge instead). Fix: `guardAuthorDispatch` now logs on both paths: ``` [review-loop] cap hit for charles/claude-hooks#604: rounds=3 max=3 -- skipping address-review, force-merging [review-loop] author guard passed for charles/claude-hooks#604: rounds=1 max=3 -- dispatching address-review ``` --- ### Fix PR PR #637 adds: - The two observability log lines above - `DispatchOptions.argDefaults` injection hook for tests - Integration regression test: `dispatchToFlows(pull_request_review_rejected event)` -> real `pr-changes-requested` graph -> `address-review` dispatched to author (was completely absent before) - Routing regression test: Forgejo `pull_request_review_rejected` event header correctly normalises and routes to a `pull_request_review.submitted` flow

dev referenced this issue from a commit

2026-05-01 12:15:13 +00:00

fix(flows): add observability logging and integration test for PR review dispatch (#610)