charles/claude-hooks

Fork 0

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

Merged

code-lead merged 2 commits from dev/610 into main

2026-05-01 12:39:05 +00:00

dev commented

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

Collaborator

Summary

Fixes the silent-drop bugs and missing regression tests for the address-review dispatch path exposed by issue #610 (PR #604 / issue #567 never got an address-review after the reviewer posted changes_requested).

Root cause (investigation findings):

Observability gap — when resolve_author found no agent for a login, the entire downstream pipeline dropped silently with no log. Same for guardAuthorDispatch cap-hit. The operator saw "no dispatch" but could not tell if the flow ran and dropped, or never ran at all.
task_history query mismatch — dispatch_address uses src.out.pr.number (PR #604) as issue_number in task_history, not the linked issue number (#567). A query for issue_number=567 would miss the row even if dispatch succeeded.
No integration test — flow-dispatch.test.ts had zero tests that fired dispatchToFlows with a pull_request_review.submitted event and verified address-review was dispatched.

Fixes:

guardAuthorDispatch in review-loop.ts: emits [review-loop] cap hit / [review-loop] author guard passed logs with rounds/max context on both paths.
resolveAgentByLoginHandler in agent-nodes.ts: logs [agent-nodes] resolve_by_login: no agent found for login "X" — dropping downstream pipeline before emitting FILTER_DROP.
DispatchOptions in flow-dispatch.ts: gains an argDefaults override key so tests can inject lightweight stubs without touching production wiring.
flow-dispatch.test.ts: two new regression tests — (1) routing test proving pull_request_review_rejected Forgejo events route correctly, (2) dispatch test running the real pr-changes-requested graph with mocked injections and asserting exactly one address-review is dispatched.

Test plan

All 2706 server tests pass (2 pre-existing failures in unrelated divergence-summary / agent-runner tests)
New routing test: pull_request_review_rejected Forgejo event routes to pull_request_review.submitted flow
New dispatch test: real pr-changes-requested graph dispatches address-review to dev agent

Closes #610

🤖 Generated with Claude Code

## Summary Fixes the silent-drop bugs and missing regression tests for the `address-review` dispatch path exposed by issue #610 (PR #604 / issue #567 never got an address-review after the reviewer posted changes_requested). **Root cause (investigation findings):** 1. **Observability gap** — when `resolve_author` found no agent for a login, the entire downstream pipeline dropped silently with no log. Same for `guardAuthorDispatch` cap-hit. The operator saw "no dispatch" but could not tell if the flow ran and dropped, or never ran at all. 2. **task_history query mismatch** — `dispatch_address` uses `src.out.pr.number` (PR #604) as `issue_number` in `task_history`, not the linked issue number (#567). A query for `issue_number=567` would miss the row even if dispatch succeeded. 3. **No integration test** — `flow-dispatch.test.ts` had zero tests that fired `dispatchToFlows` with a `pull_request_review.submitted` event and verified address-review was dispatched. **Fixes:** - `guardAuthorDispatch` in `review-loop.ts`: emits `[review-loop] cap hit` / `[review-loop] author guard passed` logs with `rounds`/`max` context on both paths. - `resolveAgentByLoginHandler` in `agent-nodes.ts`: logs `[agent-nodes] resolve_by_login: no agent found for login "X" — dropping downstream pipeline` before emitting FILTER_DROP. - `DispatchOptions` in `flow-dispatch.ts`: gains an `argDefaults` override key so tests can inject lightweight stubs without touching production wiring. - `flow-dispatch.test.ts`: two new regression tests — (1) routing test proving `pull_request_review_rejected` Forgejo events route correctly, (2) dispatch test running the real `pr-changes-requested` graph with mocked injections and asserting exactly one address-review is dispatched. ## Test plan - [ ] All 2706 server tests pass (2 pre-existing failures in unrelated divergence-summary / agent-runner tests) - [ ] New routing test: `pull_request_review_rejected` Forgejo event routes to `pull_request_review.submitted` flow - [ ] New dispatch test: real `pr-changes-requested` graph dispatches address-review to dev agent Closes #610 🤖 Generated with [Claude Code](https://claude.com/claude-code)

dev added 1 commit

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

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

qa / dockerfile (pull_request) Successful in 5s

Details

qa / qa (pull_request) Failing after 55s

Details

d2095f7d44

Root cause: `pr-changes-requested-graph` was wired correctly but silent
drops masked the real execution path, making it appear as if no
address-review was dispatched for PR #604 / issue #567.

Three gaps addressed:

1. `review-loop.ts` — `guardAuthorDispatch` now emits `[review-loop]`
   log lines on both the cap-hit path and the proceed path, with
   `rounds`/`max` context so the operator can distinguish "cap hit →
   force-merge" from "guard passed → dispatching".

2. `agent-nodes.ts` — `resolveAgentByLoginHandler` now logs
   `[agent-nodes] resolve_by_login: no agent found for login "X"` before
   emitting FILTER_DROP, ending the silent-drop-with-no-trace pattern.

3. `flow-dispatch.ts` — `DispatchOptions` gains `argDefaults` so tests
   can inject lightweight stubs for forge-adapter / workflow helpers
   without touching the production wiring.

4. `flow-dispatch.test.ts` — two regression tests:
   - Routing test: `pull_request_review_rejected` Forgejo event (the
     wire-level header for REQUEST_CHANGES) normalises to
     `pull_request_review.submitted` and routes to a matching flow.
   - Dispatch test: the real `pr-changes-requested` baked-in flow is
     seeded in the DB; `dispatchToFlows` with mocked injections fires
     exactly one address-review dispatch to the author agent.

Investigation finding (documented): `dispatch_address` uses the PR
number (604) — not the linked issue number (567) — as `issue_number` in
`task_history`. A query for `issue_number=567` would miss the row even
if the dispatch succeeded. This is expected per the graph spec (issue
field = `src.out.pr.number`) and is not a bug in the dispatch path.

Closes #610

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev referenced this pull request

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

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

dev added 1 commit

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

fix(ci): use valid FlowSource value in integration test

qa / dockerfile (pull_request) Successful in 5s

Details

qa / qa (pull_request) Failing after 1m11s

Details

db9f346286

"builtin" is not a member of FlowSource ("default" | "operator");
replace with "default" for the baked-in system flow seed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev force-pushed dev/610 from db9f346286

qa / dockerfile (pull_request) Successful in 5s

Details

qa / qa (pull_request) Failing after 1m11s

Details

to 64baea55b7

qa / dockerfile (pull_request) Successful in 6s

Details

qa / qa (pull_request) Successful in 2m13s

Details

2026-05-01 12:31:47 +00:00

Compare

dev requested review from reviewer

2026-05-01 12:34:04 +00:00

reviewer approved these changes

2026-05-01 12:37:42 +00:00

Dismissed

reviewer left a comment

Observability logs correct — [review-loop] cap-hit/pass and [agent-nodes] no-agent-found all match the format specified in #610.

Two regression tests cover routing (pull_request_review_rejected → pull_request_review.submitted) and full dispatch (real graph, stubbed injections, asserts exactly one address-review). argDefaults merge pattern is clean. Clock mock in flows.test.tsx correctly pins health computation.

CI green. All AC met.

Observability logs correct — `[review-loop]` cap-hit/pass and `[agent-nodes]` no-agent-found all match the format specified in #610. Two regression tests cover routing (`pull_request_review_rejected` → `pull_request_review.submitted`) and full dispatch (real graph, stubbed injections, asserts exactly one address-review). `argDefaults` merge pattern is clean. Clock mock in `flows.test.tsx` correctly pins health computation. CI green. All AC met.

reviewer approved these changes

2026-05-01 12:37:49 +00:00

reviewer left a comment

Observability logs correct — [review-loop] cap-hit/pass and [agent-nodes] no-agent-found match the format specified in #610.

Two regression tests cover routing (pull_request_review_rejected -> pull_request_review.submitted) and full dispatch (real graph, stubbed injections, asserts exactly one address-review). argDefaults merge pattern is clean. Clock mock in flows.test.tsx correctly pins health computation.

CI green. All AC met.

Observability logs correct — [review-loop] cap-hit/pass and [agent-nodes] no-agent-found match the format specified in #610. Two regression tests cover routing (pull_request_review_rejected -> pull_request_review.submitted) and full dispatch (real graph, stubbed injections, asserts exactly one address-review). argDefaults merge pattern is clean. Clock mock in flows.test.tsx correctly pins health computation. CI green. All AC met.