feat(flows): NF-6 default flow parity (redo) — token + skill render + guard #377

Merged

claude-desktop merged 3 commits from feat/flows-default-parity-redo into main 2026-04-26 11:40:48 +00:00

Conversation 0 Commits 3 Files changed 12 +845 -131

claude-desktop commented 2026-04-26 10:58:16 +00:00

Collaborator

Copy link

Phase 1 redo of NF-6 default flow parity. Closes the three blockers + one major from the review of the original Phase 1 attempt (reverted in #375).

What this PR fixes vs. the reverted attempt

1. Real forgejo_token / model / system_prompt make it into the dispatched TaskRequest. The previous attempt's agent.resolve_by_login returned only { type, instance }, so agent.dispatch built a TaskRequest with forgejo_token: "pending". That works in unit tests but breaks every real dispatch. This PR returns a full ResolvedAgentEnvelope (type, instance, forgejo_user, git_name, git_email, branch_prefix, forgejo_token, model, system_prompt) on individual output ports plus an agent envelope port for downstream nodes that prefer to thread the whole object via path shorthand. agent.dispatch now consumes the envelope and the literal "pending" fallback only fires for tests that don't wire either.

2. Skill template rendered before dispatch. Previously agent.dispatch shipped the literal "implement" string as the SDK prompt. Legacy dispatchIssueForAgent (webhook-handlers.ts:567-655) loads the skill via skillForEvent, runs interpolate, then layers applyAppendix + maybeApplyCavemanAppendix + applyArtifactStyleAppendix. New agent.render_skill async node does exactly that — output task is the rendered prompt body; agent.dispatch consumes it via inputs.task.

3. Silent-drop guard at config load. Previously node_flows: { mode: "off", suppress_legacy: ["issue.assigned"] } skipped the legacy switch AND no-op'd dispatchToFlows, dropping every event on the floor. loadWebhookConfig now rejects this combo with a clear error message. 4 new tests cover the guard + the three legal combinations.

4. Restored the domain/dispatch/assign-dedup.ts lift (re-exported from webhook-handlers.ts for back-compat) and the argDefaults plumbing on executor.ts that was reverted alongside the previous attempt — both required so agent.dispatch can reach the real worker pool, dedup state, and resolveAgentByUser injection in production.

What this PR does NOT fix yet (deliberately scoped follow-ups)

• Multi-assignee fan-out. Legacy webhook.ts:184 iterates for (const assignee of payload.issue.assignees) and dispatches per-assignee. The trigger normaliser surfaces only the last. Multi-assign tickets still silently lose every dispatch except the trailing login. Cutover for issue.assigned should still wait for this — Phase 1B will either change the normaliser or add a router.fan_out node.
• Outstanding-changes_requested → address-review pivot. Legacy dispatchIssueForAgent:583-602 probes for an open PR + recent REQUEST_CHANGES review against the current head SHA and pivots skill: "implement" → skill: "address-review". Without this, dep-closure cascades wedge the review loop. Cutover for issue.assigned should still wait for this too — Phase 1C.

New default flow pipeline (v3)

src.assignee → resolve_agent (full envelope; FILTER_DROP on miss / host-mode / no-instance)
            → render_implement (skill template + appendices, async)
            → dispatch_resolved (enable_dedup + cancel_stale on, args+envelope-driven)

The previous draft's type_switch label-routing branches (designer / design-reviewer with skill review) are gone — legacy handleIssueAssigned always uses implement regardless of who the assignee is. The review skill comes from PR review_requested, which is a separate trigger and a separate flow.

Tests

• 19 default-graph tests pass (was 14), including 5 new e2e assertions that pin forgejo_token === "tok-dev" (not "pending"), the rendered task is >100 chars + contains the issue number + title (not the literal "implement" string), the unknown-login soft-skip, the dedup-hit dropped path, and the cancel-stale legacy parity.
• 26 agent-nodes tests pass (was 23), with the new agent.resolve_by_login envelope shape + agent.render_skill registration covered.
• 4 new flow-mode tests cover the silent-drop guard.
• 1820 server tests pass total; 4 pre-existing failures unchanged (sweeper JSONL × 3, foreman session CRUD).
• Typecheck clean; biome clean.

Cutover readiness for issue.assigned

After this PR + #376 (forge-mutation divergence tooling) + Phase 1B (fan-out) + Phase 1C (address-review pivot), the cutover protocol becomes:

1. Set node_flows.mode: "dry-run" (no suppress_legacy).
2. Watch GET /flows/divergence/summary?since=... for flow_only and legacy_only to drop to zero across the soak window.
3. Add "issue.assigned" to suppress_legacy. Watch task_history for 24 h.
4. Delete the legacy webhook arm + handleIssueAssigned body. Keep the function — propagateDependencyClosure still calls it via onAssignedReady.

🤖 Generated with Claude Code

Phase 1 redo of NF-6 default flow parity. Closes the three blockers + one major from the review of the original Phase 1 attempt (reverted in #375). ## What this PR fixes vs. the reverted attempt 1. **Real `forgejo_token` / `model` / `system_prompt` make it into the dispatched `TaskRequest`.** The previous attempt's `agent.resolve_by_login` returned only `{ type, instance }`, so `agent.dispatch` built a `TaskRequest` with `forgejo_token: "pending"`. That works in unit tests but breaks every real dispatch. This PR returns a full `ResolvedAgentEnvelope` (`type`, `instance`, `forgejo_user`, `git_name`, `git_email`, `branch_prefix`, `forgejo_token`, `model`, `system_prompt`) on individual output ports plus an `agent` envelope port for downstream nodes that prefer to thread the whole object via path shorthand. `agent.dispatch` now consumes the envelope and the literal `"pending"` fallback only fires for tests that don't wire either. 2. **Skill template rendered before dispatch.** Previously `agent.dispatch` shipped the literal `"implement"` string as the SDK prompt. Legacy `dispatchIssueForAgent` (`webhook-handlers.ts:567-655`) loads the skill via `skillForEvent`, runs `interpolate`, then layers `applyAppendix` + `maybeApplyCavemanAppendix` + `applyArtifactStyleAppendix`. New `agent.render_skill` async node does exactly that — output `task` is the rendered prompt body; `agent.dispatch` consumes it via `inputs.task`. 3. **Silent-drop guard at config load.** Previously `node_flows: { mode: "off", suppress_legacy: ["issue.assigned"] }` skipped the legacy switch AND no-op'd `dispatchToFlows`, dropping every event on the floor. `loadWebhookConfig` now rejects this combo with a clear error message. 4 new tests cover the guard + the three legal combinations. 4. **Restored** the `domain/dispatch/assign-dedup.ts` lift (re-exported from `webhook-handlers.ts` for back-compat) and the `argDefaults` plumbing on `executor.ts` that was reverted alongside the previous attempt — both required so `agent.dispatch` can reach the real worker pool, dedup state, and `resolveAgentByUser` injection in production. ## What this PR does NOT fix yet (deliberately scoped follow-ups) - **Multi-assignee fan-out.** Legacy `webhook.ts:184` iterates `for (const assignee of payload.issue.assignees)` and dispatches per-assignee. The trigger normaliser surfaces only the last. Multi-assign tickets still silently lose every dispatch except the trailing login. **Cutover for `issue.assigned` should still wait for this** — Phase 1B will either change the normaliser or add a `router.fan_out` node. - **Outstanding-`changes_requested` → `address-review` pivot.** Legacy `dispatchIssueForAgent:583-602` probes for an open PR + recent `REQUEST_CHANGES` review against the current head SHA and pivots `skill: "implement"` → `skill: "address-review"`. Without this, dep-closure cascades wedge the review loop. **Cutover for `issue.assigned` should still wait for this too** — Phase 1C. ## New default flow pipeline (v3) ``` src.assignee → resolve_agent (full envelope; FILTER_DROP on miss / host-mode / no-instance) → render_implement (skill template + appendices, async) → dispatch_resolved (enable_dedup + cancel_stale on, args+envelope-driven) ``` The previous draft's `type_switch` label-routing branches (designer / design-reviewer with skill `review`) are gone — legacy `handleIssueAssigned` always uses `implement` regardless of who the assignee is. The `review` skill comes from PR `review_requested`, which is a separate trigger and a separate flow. ## Tests - 19 default-graph tests pass (was 14), including 5 new e2e assertions that pin `forgejo_token === "tok-dev"` (not `"pending"`), the rendered task is >100 chars + contains the issue number + title (not the literal `"implement"` string), the unknown-login soft-skip, the dedup-hit dropped path, and the cancel-stale legacy parity. - 26 agent-nodes tests pass (was 23), with the new `agent.resolve_by_login` envelope shape + `agent.render_skill` registration covered. - 4 new flow-mode tests cover the silent-drop guard. - 1820 server tests pass total; 4 pre-existing failures unchanged (sweeper JSONL × 3, foreman session CRUD). - Typecheck clean; biome clean. ## Cutover readiness for `issue.assigned` After this PR + #376 (forge-mutation divergence tooling) + Phase 1B (fan-out) + Phase 1C (address-review pivot), the cutover protocol becomes: 1. Set `node_flows.mode: "dry-run"` (no `suppress_legacy`). 2. Watch `GET /flows/divergence/summary?since=...` for `flow_only` and `legacy_only` to drop to zero across the soak window. 3. Add `"issue.assigned"` to `suppress_legacy`. Watch `task_history` for 24 h. 4. Delete the legacy webhook arm + `handleIssueAssigned` body. Keep the function — `propagateDependencyClosure` still calls it via `onAssignedReady`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-desktop added 1 commit 2026-04-26 10:58:16 +00:00

feat(flows): NF-6 default flow parity (redo) — token + skill render + guard ... 2733e25b41

Closes the three blockers + one major from the previous review (#375):

1. **Real token / model / system_prompt threaded into TaskRequest.**
   `agent.resolve_by_login` now returns a full `ResolvedAgentEnvelope`
   (instance, forgejo_user, git_name/email, branch_prefix, forgejo_token,
   model, system_prompt) instead of just `{ type, instance }`. Each field
   is exposed as its own output port plus a single `agent` envelope port
   for downstream nodes that prefer to thread the whole object via path
   shorthand. `agent.dispatch` consumes the envelope (via either the
   `agent` input or the individual fields) and falls back to args /
   `"pending"` only when neither is wired — production always wires
   the envelope.

2. **Skill template rendered before dispatch.** New `agent.render_skill`
   async node loads the per-event variant via `skillForEvent`, runs
   `interpolate` over `{ repo, issue_number, branch, title, ... }` (plus
   author-supplied `vars` merged last), and layers `applyAppendix` +
   `maybeApplyCavemanAppendix` + `applyArtifactStyleAppendix` on top.
   Mirrors what legacy `dispatchIssueForAgent` does between picking an
   agent and calling the worker pool. Output `task` is the rendered
   prompt body; `agent.dispatch` consumes it via `inputs.task`.

3. **Silent-drop guard at config load.** `mode === "off"` combined with
   non-empty `suppress_legacy` is now rejected by `loadWebhookConfig`
   with a clear error message — that combo skipped the legacy switch
   AND no-op'd `dispatchToFlows`, dropping every matching event on the
   floor. 4 new tests cover the guard + the three legal combinations.

Also brings back the `domain/dispatch/assign-dedup.ts` lift (re-exported
from `webhook-handlers.ts` for back-compat) and the `argDefaults`
plumbing on the executor that was reverted alongside the previous
attempt — both required for `agent.dispatch` to reach the real worker
pool, dedup state, and `resolveAgentByUser` injection.

`default-graph.json` rewritten (v3) with the new pipeline:
  src.assignee → resolve_agent (full envelope; FILTER_DROP on miss)
              → render_implement (skill template + appendices)
              → dispatch_resolved (enable_dedup + cancel_stale on)

Removed the type_switch label-routing branches from the previous draft —
legacy `handleIssueAssigned` always uses `implement` regardless of who
the assignee is (the `review` skill comes from PR review_requested,
which is a separate trigger and a separate flow). The label-routing
divergence in v2 was a bug, not a feature.

Multi-assignee fan-out and the outstanding-`changes_requested` →
`address-review` pivot remain follow-ups (Phase 1B / 1C). Cutover for
`issue.assigned` should still wait until those land — the Phase 2
divergence tooling (PR #376) is the gate that proves it.

Tests: 19 default-graph tests pass (was 14), including 5 new e2e
assertions that pin `forgejo_token === "tok-dev"` (not `"pending"`),
the rendered task is >100 chars + contains the issue number + title
(not the literal `"implement"` string), the unknown-login soft-skip,
and the dedup-hit dropped path. 1820 server tests pass total; 4
pre-existing failures unchanged.

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

charles added 1 commit 2026-04-26 11:11:46 +00:00

fix(flows): NF-6 default flow parity — restore prompt_appendix + token_economy ... 67591d4320

Code review of the previous redo flagged two silent regressions in
`renderSkillHandler`'s `fakeAgent` cast: `prompt_appendix` and
`token_economy` were dropped on the way from `ResolvedAgentEnvelope`
into the `applyAppendix` / `maybeApplyCavemanAppendix` helpers.

Result: every flow-path dispatch shipped without the per-instance
`## Agent-specific guidance` block, and caveman mode (issue #231) was
dead under cutover regardless of the type's config. Both no-ops were
silent — the existing `task.length > 100` assertion passed for the
wrong reasons because the base skill template alone clears 100 chars.

Fixes:

- Extend `ResolvedAgentEnvelope` with `prompt_appendix: string | null`
  + `token_economy: TokenEconomyConfig`. Both copied into `fakeAgent`
  so the appendix layering matches legacy `dispatchIssueForAgent`.
- `flow-dispatch.ts::defaultArgInjections` populates both fields from
  the real `ResolvedAgent`.
- `agent.resolve_by_login` outputs the new fields on dedicated ports
  + emits FILTER_DROP for them on the unknown-login path so downstream
  `agent.dispatch` skips cleanly.
- New e2e tests pin `prompt_appendix` makes it into the rendered task
  AND `token_economy.caveman: true` wires the caveman appendix in.
  Tightened the loose `length > 100` test to assert the issue title +
  number are interpolated verbatim.

Also adds the live-mode guard the review flagged: `agent.dispatch`
throws when `request.forgejo_token === "pending"` (the literal fallback
that fires when neither an envelope nor `args.forgejo_token` was
wired). Without it, a misconfigured operator flow would silently ship
the string `pending` to the SDK and fail opaquely.

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

charles added 1 commit 2026-04-26 11:28:15 +00:00

fix(flows): NF-6 — extend pending guard to forgejo_user, tighten test asserts ... 4c4b247338

Round 3 review feedback:

- `agent.dispatch`'s pending-guard only covered `forgejo_token`. A flow
  that wires the token but drops `forgejo_user` would commit as
  `pending @ pending@claude-hooks.local` silently. Guard now covers
  both fields with a clear error pointing at which one is missing.
- New e2e test for the `forgejo_user`-pending case mirrors the existing
  token-pending test.
- Tightened the caveman appendix assertion from `/caveman/i` (matches
  any future skill that mentions caveman in passing) to
  `Caveman mode — terse shorthand` literal heading.
- Dropped dead `| undefined` widening on `prompt_appendix as string |
  null | undefined` — interface declares the field as `string | null`
  non-optional.

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

claude-desktop merged commit dd313f5b19 into main 2026-04-26 11:40:48 +00:00

claude-desktop deleted branch feat/flows-default-parity-redo 2026-04-26 11:40:49 +00:00

claude-desktop referenced this pull request from a commit 2026-04-26 11:40:49 +00:00

feat(flows): NF-6 default flow parity (redo) — token + skill render + guard (#377)

claude-desktop referenced this pull request 2026-04-26 11:45:08 +00:00

feat(flows): NF-6 multi-assignee fan-out for issue.assigned #378

claude-desktop referenced this pull request 2026-04-26 12:05:48 +00:00

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

charles referenced this pull request from a commit 2026-04-26 12:20:48 +00:00

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

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

Subscribe

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!377

No description provided.