charles/claude-hooks
1
0
Fork 0
Code Issues 10 Pull requests Projects Releases Packages 1 Wiki Activity Actions

feat(flows): NF-6 Phase 1C — outstanding-changes-requested address-review pivot #379

Merged
charles merged 2 commits from feat/flows-address-review-pivot into main 2026-04-26 12:20:46 +00:00
Conversation 0 Commits 2 Files changed 6 +363 -23
claude-desktop commented 2026-04-26 12:05:48 +00:00
Collaborator
Copy link

Phase 1C — closes the second remaining cutover blocker for suppress_legacy: ["issue.assigned"]. With this PR + #378 (multi-assignee fan-out, already merged), the default flow mirrors every guard rail of legacy dispatchIssueForAgent.

Why

Legacy dispatchIssueForAgent (webhook-handlers.ts:516-525) probes the issue's linked PR for an outstanding REQUEST_CHANGES review against the current head SHA. When found, it pivots implement → address-review and aims the worktree at pr.headRef. Without this, dep-closure cascades and operator assignee-bounces would re-fire implement on a PR awaiting review comments — dev would read "PR exists, CI green", QA, exit. Head SHA never changes, review loop wedges (issue #270).

What

  • New forge.detect_outstanding_change_request node — wraps the exported detectOutstandingChangeRequest legacy fn. Two output ports: pivot carries { prNumber, headRef, headSha } envelope; no_pivot carries true. Exactly one is non-FILTER_DROP per execution.
  • New gate input added to agent.render_skill — authors wire any FILTER_DROP-bearing upstream as a binary skip signal. Handler ignores the value; executor's FILTER_DROP propagation does the work.
  • flow-dispatch.ts::defaultArgInjections wires the production injection (envelope→ResolvedAgent cast).
  • default-graph.json v4: pipeline forks after detect_pivot — no_pivot → render_implement → dispatch_implement; pivot → render_address_review(skill="address-review", branch=pr.headRef) → dispatch_address_review(branch_prefix=pivot.headRef).

Tests

  • 3 new pivot-specific e2e tests on top of the existing 23: no-pivot fires implement only; pivot fires address-review with branch_prefix === pr.headRef; detector throw aborts dispatch with status: "error" and zero side effects.
  • 1847 server tests pass (was 1844 pre-pivot); 4 pre-existing failures unchanged.
  • Typecheck + biome clean.

Cutover progress

Blocker Status
Token / model / system_prompt / appendix wiring merged (#377)
Silent-drop config guard merged (#375)
Forge-mutation divergence parity tooling merged (#376)
Multi-assignee fan-out merged (#378)
Outstanding-changes_requested → address-review pivot this PR

After this PR + a soak window of zero divergence on GET /flows/divergence/summary, node_flows.suppress_legacy: ["issue.assigned"] is finally safe to set. Phase 4 (the remaining 7 NF-6 flows for other triggers) is the last block before legacy can be deleted entirely.

🤖 Generated with Claude Code

Phase 1C — closes the second remaining cutover blocker for `suppress_legacy: ["issue.assigned"]`. With this PR + #378 (multi-assignee fan-out, already merged), the default flow mirrors every guard rail of legacy `dispatchIssueForAgent`. ## Why Legacy `dispatchIssueForAgent` (`webhook-handlers.ts:516-525`) probes the issue's linked PR for an outstanding `REQUEST_CHANGES` review against the current head SHA. When found, it pivots `implement` → `address-review` and aims the worktree at `pr.headRef`. Without this, dep-closure cascades and operator assignee-bounces would re-fire `implement` on a PR awaiting review comments — dev would read "PR exists, CI green", QA, exit. Head SHA never changes, review loop wedges (issue #270). ## What - New `forge.detect_outstanding_change_request` node — wraps the exported `detectOutstandingChangeRequest` legacy fn. Two output ports: `pivot` carries `{ prNumber, headRef, headSha }` envelope; `no_pivot` carries `true`. Exactly one is non-FILTER_DROP per execution. - New `gate` input added to `agent.render_skill` — authors wire any FILTER_DROP-bearing upstream as a binary skip signal. Handler ignores the value; executor's FILTER_DROP propagation does the work. - `flow-dispatch.ts::defaultArgInjections` wires the production injection (envelope→ResolvedAgent cast). - `default-graph.json` v4: pipeline forks after `detect_pivot` — `no_pivot` → `render_implement` → `dispatch_implement`; `pivot` → `render_address_review(skill="address-review", branch=pr.headRef)` → `dispatch_address_review(branch_prefix=pivot.headRef)`. ## Tests - 3 new pivot-specific e2e tests on top of the existing 23: no-pivot fires implement only; pivot fires address-review with `branch_prefix === pr.headRef`; detector throw aborts dispatch with `status: "error"` and zero side effects. - 1847 server tests pass (was 1844 pre-pivot); 4 pre-existing failures unchanged. - Typecheck + biome clean. ## Cutover progress | Blocker | Status | |---|---| | Token / model / system_prompt / appendix wiring | merged (#377) | | Silent-drop config guard | merged (#375) | | Forge-mutation divergence parity tooling | merged (#376) | | Multi-assignee fan-out | merged (#378) | | Outstanding-`changes_requested` → `address-review` pivot | **this PR** | After this PR + a soak window of zero divergence on `GET /flows/divergence/summary`, `node_flows.suppress_legacy: ["issue.assigned"]` is finally safe to set. Phase 4 (the remaining 7 NF-6 flows for other triggers) is the last block before legacy can be deleted entirely. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat(flows): NF-6 Phase 1C — outstanding-changes-requested address-review pivot
All checks were successful
qa / qa (pull_request) Successful in 6m16s
Details
qa / dockerfile (pull_request) Successful in 11s
Details
4a03e82b51
 
Closes the second remaining cutover blocker for `suppress_legacy:
["issue.assigned"]`. With this PR the default flow now mirrors every
guard rail of the legacy `dispatchIssueForAgent` path.

Legacy `dispatchIssueForAgent` (`webhook-handlers.ts:516-525`) probes
the issue's linked PR for an outstanding `REQUEST_CHANGES` review
against the current head SHA. When found, it pivots the dispatch
from `implement` → `address-review` and aims the worktree at the
PR's `headRef` instead of the derived `dev/<issue>` branch. Without
this pivot, dep-closure cascades and operator assignee-bounces would
re-fire `implement` against `dev/N`; dev would read "PR exists, CI
green", QA, and exit — the head SHA never changes and the review
loop wedges (issue #270).

Implementation:

- New `forge.detect_outstanding_change_request` node wraps the
  exported `detectOutstandingChangeRequest` legacy fn. Two outputs:
  `pivot` carries the `{prNumber, headRef, headSha}` envelope when a
  pivot is needed; `no_pivot` carries `true` otherwise. Exactly one
  port is non-FILTER_DROP per execution → downstream branches gate
  via input wiring without a router.switch.
- New `gate` input added to `agent.render_skill` so authors can wire
  any FILTER_DROP-bearing upstream as a binary skip signal. Handler
  ignores the value; the executor's FILTER_DROP propagation does the
  work.
- `flow-dispatch.ts::defaultArgInjections` wires the production
  `detectOutstandingChangeRequest` injection (envelope→ResolvedAgent
  cast for the legacy fn signature).
- `default-graph.json` v4: pipeline now forks after `detect_pivot` —
  `no_pivot` → `render_implement` → `dispatch_implement` (existing
  behaviour); `pivot` → `render_address_review` (skill=address-review,
  branch=pr.headRef) → `dispatch_address_review` with
  `branch_prefix=detect_pivot.pivot.headRef`.

Tests: 3 new pivot-specific e2e tests on top of the existing 23 —
no-pivot fires implement, pivot fires address-review with the PR's
headRef as branch_prefix, detector throw aborts dispatch with
`status: "error"` and zero side effects.

After this PR + a soak window of zero divergence on `GET
/flows/divergence/summary?since=...`, `node_flows.suppress_legacy:
["issue.assigned"]` is finally safe to set. Phase 4 (the remaining 7
NF-6 flows for other triggers) is the last block before legacy can
be deleted entirely.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(flows): NF-6 pivot — branch override + pr_number + graceful degrade
All checks were successful
qa / qa (pull_request) Successful in 6m15s
Details
qa / dockerfile (pull_request) Successful in 13s
Details
0e7981bc17
 
Round 1 review feedback on PR #379. Two blockers + two majors.

Blockers:
- Address-review dispatch wired `branch_prefix: pivot.headRef`. Worker
  reads explicit branch override from `TaskRequest.branch` (worker.ts:63);
  when unset it derives `<branch_prefix>/<issue>`. So a pivot for PR
  head `dev/feature-x` on issue 42 landed the worktree on
  `dev/feature-x/42` — wrong branch. Fix: new `branch` input port on
  `agent.dispatch` plumbed to `TaskRequest.branch`; default-graph.json
  swapped `branch_prefix` → `branch`. Mirrors legacy
  `webhook-handlers.ts:591` `branch = outstanding.headRef`.
- Pivot test asserted `req.branch_prefix === "dev/feature-x"` —
  pinned the broken JSON wiring instead of legacy parity. Now asserts
  `req.branch === "dev/feature-x"` AND `req.branch_prefix === "dev"`
  (the agent's own prefix stays unchanged).

Majors:
- Pivot detector throw now degrades gracefully → no_pivot (implement).
  Mirrors legacy `webhook-handlers.ts:594-601` try/catch ("a Forgejo
  blip must not block the regular dispatch path"). Test flipped from
  asserting `status: "error"` to asserting `status: "ok"` + 1 implement
  dispatch with no branch override.
- `render_skill` `pr_number` input port + matching graph wiring from
  `detect_pivot.pivot.prNumber`. Legacy `webhook-handlers.ts:613` sets
  `pr_number: outstanding.prNumber` so the address-review template's
  `{{pr_number}}` interpolates to the PR number, not the issue.
  New e2e test pins the rendered task contains "99" (PR), not just
  "42" (issue).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charles deleted branch feat/flows-address-review-pivot 2026-04-26 12:20:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area:agents
Agent types, pool scheduling, per-instance config

  
area:dashboard
Dashboard UI and observability surfaces

  
area:database
DB layer — schema, migrations, ORM, raw SQL

  
area:design
UI/UX mockup work — routes to designer agent

  
area:design-review
Design review dispatch — routes to design-reviewer agent

  
area:flows
Flow runner — YAML loader, executor, op registry, expression eval

  
area:infra
Deployment, isolation, containers, systemd units

  
area:meta
Tracking, scaffolding, project setup

  
area:security
Security — routes to reviewer-security (opus)

  
area:sessions
Session-id store, Claude SDK resume logic

  
area:webhook
Forgejo webhook routing and handlers

  
area:workdir
Clone cache, worktrees, git identity

  
security
Security-sensitive issue

  
type:bug
Bug

  
type:chore
Chore

  
type:meta
Tracking or decisions, not implementation work

  
type:user-story
User story

No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
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
charles/claude-hooks!379
No description provided.