feat: per-column agent assignment for workflow columns - #1432
Merged
Conversation
…esolver U1+U2 of the column-agent plan: WorkflowColumnAgent on WorkflowIrColumn (defer/override), template-subgraph column validation, v2-only-feature registration, plugin-sdk type parity, and the shared core resolver with instanceNodeId format ownership moved to core.
U3: per-run IR resolution feeds the core column-agent resolver at the runCustomNode seam; override supersedes node agent/model/persona wholesale, defer fills bare nodes only; adoption and fallback are audited via logEntry; raw-CLI nodes log a skip. Also fixes the customInstructions persona drift — node-level executor:"agent" persona injection now uses the typed soul/instructionsText fields (KTD-6).
… validation U6: WorkflowColumnPanel agent picker + defer/override toggle with specified interaction states (flags-off hint, loading, fetch-error, stale-agent warning, bound-column badge); WorkflowNodeEditor overridden-by-column-agent note + stale-id treatment; assertColumnAgentsExist + confirmPolicyEscalation gate (R13) on workflow save routes; flowToIr now preserves column agent bindings through the editor round-trip.
U4: seams stamp the governing node id into run context; a per-seam resolveSeamColumnAgent feeds the core resolver and threads the effective agent through resolveExecutorSessionModel, runtime hints, persona, memory tools, and StepSessionExecutor attribution. Characterization tests pin the no-binding path byte-identical. Gating/deferral principal moves in U5.
U5: action-gating contexts, the heartbeat deferral gate, a two-pass resumeTaskForAgent, and the reverse-direction agent.taskId guards (via a new isAgentEffectivelyExecuting callback wired at the in-process runtime) all consult the column-effective agent; the restart watcher re-resolves column bindings per tick for bound graph sessions, hot-swapping on agent-changed and falling back without restart on agent-deleted.
… docs U7: full mode × surface × own-settings matrix ledger with 5 gap-filling tests, default-workflow zero-binding parity assertions, changeset covering the complete feature, and a workflow-steps.md authoring section for column agents.
Contributor
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds per-column permanent agent bindings (modes: defer|override), centralizes effective-agent resolution in ChangesPer-column agent assignment — IR, core resolver, engine integration, dashboard authoring, and validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Greptile SummaryThis PR introduces per-column permanent agent assignment for workflow columns, with
Confidence Score: 5/5Safe to merge — the previously-flagged kill-switch gap, agent-picker error state, and project-scoped fetch issues have all been addressed; no blocking defects were found in this pass. The execution path is correctly double-gated behind both feature flags. Forward heartbeat deferral correctly identifies the column agent at seam re-entry (governing node id is stamped before the inner execute() call). Reverse heartbeat guards use the new effectiveColumnAgentByTask map, which is cleaned up atomically with the session. Write-time validation is shared across both write surfaces with a typed error and explicit confirmation handshake. The one finding is a missing cancellation guard on the executor-agent fetch in WorkflowNodeEditor, which is transient and caught by backend validation at save time. packages/dashboard/app/components/WorkflowNodeEditor.tsx — executor-agent fetch lacks a cancellation guard on project switch (stale-response race). All engine and core files look solid. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Dashboard Editor
participant Route as Workflow Routes
participant Core as @fusion/core (Resolver)
participant Engine as TaskExecutor
participant HB as HeartbeatScheduler
UI->>Route: "PATCH /workflows/:id {ir, confirmPolicyEscalation?}"
Route->>Core: validateColumnAgentBindings(ir, ...)
alt policy-escalation without confirmation
Core-->>Route: throws ColumnAgentBindingError (policy-escalation)
Route-->>UI: "400 {policyEscalation: true}"
UI->>UI: window.confirm(...)
UI->>Route: retry with confirmPolicyEscalation: true
end
Route-->>UI: 200 workflow saved
Note over Engine: task enters in-progress
Engine->>Engine: maybeExecuteWorkflowGraph(task)
Engine->>Core: resolveColumnAgentBinding(ir, nodeId)
Core-->>Engine: "WorkflowColumnAgent {agentId, mode}"
Engine->>Engine: graphColumnAgentResolver.set(taskId, resolver)
Note over Engine: seam node reached
Engine->>Engine: graphSeamGoverningNodeId.set(taskId, governingNodeId)
Engine->>Core: "resolveEffectiveAgent({binding, ownAgentId, ...})"
Core-->>Engine: "{source: column-agent, agentId}"
Engine->>HB: register isAgentEffectivelyExecuting(agentId)
HB->>HB: "suppress heartbeat tick (allowParallelExecution=false guard)"
Engine->>Engine: coding session runs as column agent
Engine->>Engine: effectiveColumnAgentByTask.set(taskId, agentId)
Engine->>Engine: deleteActiveSession(taskId)
Engine->>Engine: effectiveColumnAgentByTask.delete(taskId)
Reviews (4): Last reviewed commit: "Address PR review feedback round 2 (#143..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
docs/workflow-steps.md (1)
68-71: ⚡ Quick winAdd language identifier to fenced code block.
The code block should specify a language identifier for proper syntax highlighting and rendering.
📝 Proposed fix
-``` +```typescript { id: "review", name: "Review", traits: [], agent: { agentId: "agent-001", mode: "defer" | "override" } }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/workflow-steps.mdaround lines 68 - 71, The fenced code block showing
the workflow step object ({ id: "review", name: "Review", traits: [], agent: {
agentId: "agent-001", mode: "defer" | "override" } }) should include a language
identifier for proper syntax highlighting—update the opening backticks to
include "typescript" (i.e., changetotypescript) so the snippet is
rendered as TypeScript.</details> </blockquote></details> <details> <summary>packages/dashboard/src/__tests__/workflow-routes.test.ts (1)</summary><blockquote> `549-565`: _⚡ Quick win_ **Cover the PATCH policy-escalation path too.** These tests assert `confirmPolicyEscalation` on `POST /api/workflows`, but the route adds the same contract to `PATCH /api/workflows/:id`. A regression in the update path would currently slip through untested. As per coding guidelines, "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)." Also applies to: 623-629 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/__tests__/workflow-routes.test.ts` around lines 549 - 565, Add a test that mirrors the POST assertions for the PATCH path: after setting project policy via store.updateSettings and creating an agent with makeAgent and boundIr, attempt to create a workflow that is rejected for being broader (POST /api/workflows) to verify details.policyEscalation is true, then create a workflow resource (or use the created ID) and call PATCH /api/workflows/:id with the same ir and confirmPolicyEscalation true and assert the PATCH returns 200/201 (accepted) — reference the existing helpers and symbols used in the file (post(), boundIr(), makeAgent(), store.updateSettings()) and reuse the same expectations for error message matching and details.policyEscalation on the initial rejection, then assert success when confirmPolicyEscalation is provided on the PATCH request. ``` </details> </blockquote></details> <details> <summary>packages/engine/src/__tests__/agent-tools.test.ts (1)</summary><blockquote> `575-625`: _⚡ Quick win_ **Broaden the regression to all workflow write surfaces, not only create.** This locks the invariant for `createWorkflowCreateTool`, but the same policy-escalation behavior should also be asserted for other known write surfaces (notably update) to prevent split-surface regressions. As per coding guidelines, "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/agent-tools.test.ts` around lines 575 - 625, Add the same policy-escalation regression checks for all workflow write surfaces, not just create: replicate the test logic that uses createWorkflowCreateTool and tool.execute (checking denial when a bound agent elevates permissions and that denied.content mentions the column and confirm_policy_escalation) for the update path and any other write surfaces (e.g., the workflow update tool/handler you have in the codebase), asserting that attempts without confirm_policy_escalation are rejected with details { columnId, agentId, reason: "policy-escalation" } and that supplying confirm_policy_escalation: true allows the write to proceed and returns a Created/OK message; mirror the same setup (store.init, agent with unrestricted preset, IR with bound column) so the invariant is enforced across all write surfaces. ``` </details> </blockquote></details> <details> <summary>packages/engine/src/__tests__/executor-column-agent-principal.test.ts (1)</summary><blockquote> `457-457`: _⚡ Quick win_ **Replace `setTimeout(0)` waits with deterministic test synchronization.** Line 457, Line 489, and Line 523 rely on real timers to “let async handlers run”, which is more brittle than `vi.waitFor(...)` (or fake timers where applicable). Based on learnings, "Prefer fake timers over real polling/time waits." Also applies to: 489-489, 523-523 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/executor-column-agent-principal.test.ts` at line 457, Replace the brittle "await new Promise((r) => setTimeout(r, 0))" pauses (the occurrences of that exact expression in the test file) with deterministic test synchronization: use vi.waitFor(() => <assertion>) to wait for the specific observable effect (e.g., expect(mockFn).toHaveBeenCalled(), expect(store.value).toBe(...), or expect(container.querySelector(...)).not.toBeNull()) or switch the test to fake timers with vi.useFakeTimers()/vi.runAllTimers() and then restore; update each instance (lines with the setTimeout usage) to wait on a concrete condition relevant to that test rather than sleeping. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@packages/core/src/__tests__/column-agent-resolver.test.ts:
- Around line 74-80: Add a new assertion mirroring the existing
"ownModelProvider-only" test to cover the symmetric "ownModelId-only"
incomplete-model surface: call resolveEffectiveAgent with binding: deferBinding
and ownModelId set (e.g., ownModelId: "some-model") but without
ownModelProvider, and assert it equals { source: "column-agent", agentId:
"col-agent" } so both incomplete-pair paths (ownModelProvider-only and
ownModelId-only) are tested; update the test case around the existing expect
that uses deferBinding and ownModelProvider to include this second expect.In
@packages/core/src/column-agent-resolver.ts:
- Around line 53-65: The parser in column-agent-resolver.ts incorrectly uses
nodeId.indexOf("#") so foreachNodeId is cut at the first '#' and valid IDs
containing '#' are misparsed; change the split to use the last '#' (use
lastIndexOf) so hashIndex is the position of the final delimiter, then proceed
with the same remainder/colon/stepIndexRaw/templateNodeId validation (variables:
nodeId, hashIndex, foreachNodeId, remainder, colonIndex, stepIndexRaw,
templateNodeId) so resolveColumnAgentBinding and instanceNodeId-produced IDs are
parsed correctly.In
@packages/dashboard/app/components/WorkflowNodeEditor.tsx:
- Around line 566-576: overrideColumnBinding only checks
selectedNode.data.column so children of a foreach (which don't carry column in
irToFlow) miss inherited override bindings; update the useMemo that defines
overrideColumnBinding to walk up the node ancestry from selectedNode to find the
nearest ancestor with a data.column (or a foreach parent that defines a column),
then locate that column in columns and return its agent if mode === "override"
(same checks as currently done). Ensure you reference selectedNode (and its
parent/ancestors), columns, and reuse the existing agent mode check so
step-execute prompts inside override-bound foreach groups inherit the binding.- Around line 154-157: The editor currently allows creating column-agent
bindings (enabled by experimentalFeatures.workflowColumns and
workflowGraphExecutor in useAppSettings -> columnAgentsEnabled) but handleSave
only posts { ir, layout } and doesn't perform the confirmPolicyEscalation
handshake required by register-workflow-routes.ts; update WorkflowNodeEditor.tsx
so saving detects any column-agent with broader-than-default policy, prompt the
user (confirm/retry modal) and call the confirmPolicyEscalation endpoint before
the final save, then include the escalation confirmation (or retry the post
after confirmation) as part of the save flow; modify handleSave to sequence:
validate IR/layout for escalations, open confirmation UI if needed, call
confirmPolicyEscalation (or pass its result) and only then POST the full payload
to the existing save endpoint so register-workflow-routes.ts will accept the
binding.In
@packages/engine/src/__tests__/agent-tools.test.ts:
- Around line 576-625: The test can leak resources if assertions throw before
cleanup; wrap the setup and assertions in a try/finally (or add a finally block)
that always calls await store.close() and awaits rm(rootDir, { recursive: true,
force: true }) and rm(globalDir, { recursive: true, force: true }); ensure
agentStore (if created) is also closed/cleaned in the same finally so
rootDir/globalDir removal always runs even on failure.In
@packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts:
- Around line 153-199: The test currently hard-codes interpreterObs
stageTransitions instead of using the actual run-derived stages array, so update
the call to buildWorkflowObservation to pass stageTransitions: stages (the array
collected in the prompt handler) instead of the literal
["triage","execute","review","merge"]; keep the other fields (terminalColumn,
terminalStatus, reviewVerdict, mergeOutcome) the same so
compareWorkflowRunObservations compares the real executor behavior (symbols:
stages, interpreterObs, buildWorkflowObservation,
compareWorkflowRunObservations).In
@packages/engine/src/executor.ts:
- Around line 2099-2173: The watcher only handles the "column-agent" branch and
never clears state when the binding is removed or re-resolves away from a column
agent, leaving activeEntry.lastEffectiveColumnAgentId and the
effectiveColumnAgentByTask map stale (so isAgentEffectivelyExecuting() still
blocks the old agent); update the logic in the block that reads
graphColumnAgentResolver/resolveEffectiveAgent (around activeSessions handling)
to detect when binding is falsy or effective.source !== "column-agent" and then
clear activeEntry.lastEffectiveColumnAgentId, remove or update the
effectiveColumnAgentByTask entry for this task id, and emit the same audit/log
path used for deletions/hot-swaps so the runtime releases the old agent and can
pick up new resolutions on subsequent ticks (use resolveEffectiveAgent,
activeEntry.lastEffectiveColumnAgentId, effective.agentId,
effectiveColumnAgentByTask, and isAgentEffectivelyExecuting to locate where to
change).- Around line 3478-3485: The map graphSeamGoverningNodeId is currently keyed
only by task id and must be changed to use the per-foreach-instance keying used
by TaskExecutor.graphStepActiveContext (i.e. the graphActiveContextKey format
${task.id}:${instanceId}) so each foreach instance has its own governing node
entry; update all accesses in runGraphTaskStep(), stepExecute(), execute(), and
resolveSeamColumnAgent() to read/write using the instance-scoped key, and ensure
cleanup uses the task-id prefix removal logic consistent with
graphStepActiveContext (also apply the same change to the other region mentioned
around lines 4473-4525).
Nitpick comments:
In@docs/workflow-steps.md:
- Around line 68-71: The fenced code block showing the workflow step object ({
id: "review", name: "Review", traits: [], agent: { agentId: "agent-001", mode:
"defer" | "override" } }) should include a language identifier for proper syntax
highlighting—update the opening backticks to include "typescript" (i.e., change
totypescript) so the snippet is rendered as TypeScript.In
@packages/dashboard/src/__tests__/workflow-routes.test.ts:
- Around line 549-565: Add a test that mirrors the POST assertions for the PATCH
path: after setting project policy via store.updateSettings and creating an
agent with makeAgent and boundIr, attempt to create a workflow that is rejected
for being broader (POST /api/workflows) to verify details.policyEscalation is
true, then create a workflow resource (or use the created ID) and call PATCH
/api/workflows/:id with the same ir and confirmPolicyEscalation true and assert
the PATCH returns 200/201 (accepted) — reference the existing helpers and
symbols used in the file (post(), boundIr(), makeAgent(),
store.updateSettings()) and reuse the same expectations for error message
matching and details.policyEscalation on the initial rejection, then assert
success when confirmPolicyEscalation is provided on the PATCH request.In
@packages/engine/src/__tests__/agent-tools.test.ts:
- Around line 575-625: Add the same policy-escalation regression checks for all
workflow write surfaces, not just create: replicate the test logic that uses
createWorkflowCreateTool and tool.execute (checking denial when a bound agent
elevates permissions and that denied.content mentions the column and
confirm_policy_escalation) for the update path and any other write surfaces
(e.g., the workflow update tool/handler you have in the codebase), asserting
that attempts without confirm_policy_escalation are rejected with details {
columnId, agentId, reason: "policy-escalation" } and that supplying
confirm_policy_escalation: true allows the write to proceed and returns a
Created/OK message; mirror the same setup (store.init, agent with unrestricted
preset, IR with bound column) so the invariant is enforced across all write
surfaces.In
@packages/engine/src/__tests__/executor-column-agent-principal.test.ts:
- Line 457: Replace the brittle "await new Promise((r) => setTimeout(r, 0))"
pauses (the occurrences of that exact expression in the test file) with
deterministic test synchronization: use vi.waitFor(() => ) to wait
for the specific observable effect (e.g., expect(mockFn).toHaveBeenCalled(),
expect(store.value).toBe(...), or
expect(container.querySelector(...)).not.toBeNull()) or switch the test to fake
timers with vi.useFakeTimers()/vi.runAllTimers() and then restore; update each
instance (lines with the setTimeout usage) to wait on a concrete condition
relevant to that test rather than sleeping.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `97d0d8a6-7074-4143-9646-e6d1213a8a2d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7b961d63f5736296c24569e3ea16dd007ae824a9 and 2ed71d34a05ae198786d7211e2e4d4745b5abd6b. </details> <details> <summary>📒 Files selected for processing (32)</summary> * `.changeset/workflow-column-agent-assignment.md` * `docs/plans/2026-06-04-002-feat-column-agent-assignment-plan.md` * `docs/residual-review-findings/feat-column-agent-assignment.md` * `docs/workflow-steps.md` * `packages/cli/src/commands/dashboard.ts` * `packages/core/src/__tests__/column-agent-resolver.test.ts` * `packages/core/src/__tests__/workflow-ir-column-agent.test.ts` * `packages/core/src/agent-permission-policy.ts` * `packages/core/src/column-agent-binding-validation.ts` * `packages/core/src/column-agent-resolver.ts` * `packages/core/src/index.ts` * `packages/core/src/workflow-ir-types.ts` * `packages/core/src/workflow-ir.ts` * `packages/dashboard/app/components/WorkflowColumnPanel.tsx` * `packages/dashboard/app/components/WorkflowNodeEditor.tsx` * `packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx` * `packages/dashboard/app/components/workflow-flow-mapping.ts` * `packages/dashboard/src/__tests__/workflow-routes.test.ts` * `packages/dashboard/src/routes/register-workflow-routes.ts` * `packages/engine/src/__tests__/agent-tools.test.ts` * `packages/engine/src/__tests__/executor-column-agent-custom-node.test.ts` * `packages/engine/src/__tests__/executor-column-agent-principal.test.ts` * `packages/engine/src/__tests__/executor-column-agent-seams.test.ts` * `packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts` * `packages/engine/src/agent-heartbeat.ts` * `packages/engine/src/agent-tools.ts` * `packages/engine/src/executor.ts` * `packages/engine/src/runtimes/in-process-runtime.ts` * `packages/engine/src/step-session-executor.ts` * `packages/engine/src/workflow-graph-foreach.ts` * `packages/engine/src/workflow-node-handlers.ts` * `packages/plugin-sdk/src/index.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- delimiter-safe foreach instance-id resolution via IR-validated candidates - restart watcher handles binding removal/defer-flip and re-keys the reverse heartbeat guard on agent change/delete - governing-node stamp owned by the pass-initiating foreach instance (race fix) - editor: policy-escalation confirm/retry handshake on save; foreach children inherit the override note; project-scoped agent fetches; picker disabled on registry fetch error; column-agent strings in the canonical i18n catalog - tests: update-tool escalation surface, PATCH escalation route, parity bound to run-derived stages, try/finally cleanup, deterministic event waits, symmetric defer surfaces, binding-release regression
Collaborator
Author
|
Addressing the CodeRabbit review-body nitpicks (commit 2a02234):
Addressed: the binding-shape snippet now uses a
Addressed: added
Addressed: added
Addressed: the mock store gained |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/WorkflowNodeEditor.tsx (1)
633-673:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate the agent cache when
projectIdchanges.These fetches are now project-scoped, but
agentsis still a shared cache that survives project switches. Once any project has loaded agents, both paths short-circuit onagents.length > 0, so moving to a differentprojectIdcan keep showing the previous project's registry and let the editor save stale agent ids into the new project.Suggested fix
const [models, setModels] = useState<ModelInfo[]>([]); const [agents, setAgents] = useState<Agent[]>([]); const [skills, setSkills] = useState<DiscoveredSkill[]>([]); + + useEffect(() => { + setAgents([]); + }, [projectId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/WorkflowNodeEditor.tsx` around lines 633 - 673, The agents cache is surviving project switches because both useEffect hooks short-circuit on agents.length > 0 and never reset when projectId changes; update the logic around the agents state used in WorkflowNodeEditor: ensure agents is cleared (setAgents([])) whenever projectId changes (or include projectId in the condition/deps) so fetchAgents(undefined, projectId) always runs for the current project; specifically modify the primary effect that checks currentExecutor/agents.length and the overrideColumnBinding effect to depend on projectId and to reset agents on projectId change before deciding to skip fetching.
🧹 Nitpick comments (2)
packages/engine/src/__tests__/agent-tools.test.ts (1)
669-691: ⚡ Quick winCheck persisted state after the denied update-tool call.
The new test only asserts the error-shaped response. If
createWorkflowUpdateTool()mutates the definition before surfacing the escalation error, this still passes. Read the workflow back afterdeniedand asserttriage.agentis still missing so the update-tool surface carries the full FN-5893 invariant too.Suggested assertion
const denied = await tool.execute( "c", { workflow_id: created.id, ir: boundIr("bound") } as any, undefined, undefined, {} as any, ); expect((denied as { isError?: boolean }).isError).toBe(true); const text = denied.content[0]?.type === "text" ? denied.content[0].text : ""; expect(text).toMatch(/triage/); expect(text).toMatch(/confirm_policy_escalation: true/); expect(denied.details).toMatchObject({ columnId: "triage", agentId: agent.id, reason: "policy-escalation" }); + + const persisted = await store.getWorkflowDefinition(created.id); + const triage = (persisted?.ir as { columns?: Array<{ id: string; agent?: unknown }> }) + .columns?.find((c) => c.id === "triage"); + expect("agent" in (triage ?? {})).toBe(false);As per coding guidelines, "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/agent-tools.test.ts` around lines 669 - 691, After the denied tool.execute call, read the persisted workflow for created.id and assert the triage column still has no agent assigned so the persisted state wasn't mutated by createWorkflowUpdateTool(); specifically, after the denied variable is produced, fetch the workflow (use your repo’s existing read method for workflows) and assert that workflow.columns.triage.agent (or equivalent triage.agent field) is undefined/absent and that denied.details remains { columnId: "triage", agentId: agent.id, reason: "policy-escalation" } to ensure the FN-5893 invariant holds.packages/dashboard/src/__tests__/workflow-routes.test.ts (1)
631-649: ⚡ Quick winAssert that the denied PATCH leaves the stored workflow unchanged.
This test proves the route rejects the escalation, but it does not prove the rejected PATCH was side-effect free. Add a read-back after
deniedand asserttriage.agentis still absent so the update surface pins the same invariant as the POST coverage.Suggested assertion
const denied = await patch(`/api/workflows/${id}`, { ir: boundIr({ agentId, mode: "override" }) }); expect(denied.status).toBe(400); expect(denied.body.error).toMatch(/broader/i); expect((denied.body as { details?: { policyEscalation?: boolean } }).details?.policyEscalation).toBe(true); + + const persisted = await get(`/api/workflows/${id}`); + const triage = ( + (persisted.body as { ir: { columns: Array<{ id: string; agent?: unknown }> } }).ir.columns + ).find((c) => c.id === "triage"); + expect("agent" in (triage ?? {})).toBe(false);As per coding guidelines, "Regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/__tests__/workflow-routes.test.ts` around lines 631 - 649, After the denied PATCH, perform a read-back of the stored workflow (e.g. call the GET /api/workflows/:id for the same id used above) and assert the workflow's triage.agent is still absent/undefined so the rejected update had no side effects; reference the existing test variables id and denied and assert on the returned body's triage.agent field to match the POST invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/column-agent-resolver.ts`:
- Around line 137-149: The loop in parseInstanceNodeIdCandidates can accept a
candidate whose foreachNode exists but whose parsed.templateNodeId does not
actually resolve, causing incorrect column selection; update the loop in
resolveColumnAgentBinding (the block using parseInstanceNodeIdCandidates,
nodesById, foreachNode, parsed.templateNodeId, templateNodes, templateNode) to
skip/continue when parsed.templateNodeId is present but templateNode is
undefined, only falling back to foreachNode.column when there is no
parsed.templateNodeId; keep the existing logic that returns templateNode.column
when templateNode exists and has a column, otherwise return foreachNode.column
only for candidates that legitimately have no templateNodeId.
In `@packages/engine/src/__tests__/executor-column-agent-principal.test.ts`:
- Around line 500-528: Add a second assertion path to the test that simulates
the defer→own-settings transition (instead of the binding being removed) by
seeding the resolver to still return a defer binding that now resolves to the
task's own model pair and then triggering the same update flow; specifically,
after creating the executor with makeExecutor, seeding via seedSeam(executor,
task.id, "exec-node", /* defer that yields own settings */) and ensuring
activeGraphSession(...).setModel is called with the assigned agent's model, that
(executor as any).activeSessions.get(task.id).lastEffectiveColumnAgentId becomes
null, and that executor.isAgentEffectivelyExecuting("agent-X") returns false so
the test covers the defer→own-settings release path as well.
---
Outside diff comments:
In `@packages/dashboard/app/components/WorkflowNodeEditor.tsx`:
- Around line 633-673: The agents cache is surviving project switches because
both useEffect hooks short-circuit on agents.length > 0 and never reset when
projectId changes; update the logic around the agents state used in
WorkflowNodeEditor: ensure agents is cleared (setAgents([])) whenever projectId
changes (or include projectId in the condition/deps) so fetchAgents(undefined,
projectId) always runs for the current project; specifically modify the primary
effect that checks currentExecutor/agents.length and the overrideColumnBinding
effect to depend on projectId and to reset agents on projectId change before
deciding to skip fetching.
---
Nitpick comments:
In `@packages/dashboard/src/__tests__/workflow-routes.test.ts`:
- Around line 631-649: After the denied PATCH, perform a read-back of the stored
workflow (e.g. call the GET /api/workflows/:id for the same id used above) and
assert the workflow's triage.agent is still absent/undefined so the rejected
update had no side effects; reference the existing test variables id and denied
and assert on the returned body's triage.agent field to match the POST
invariant.
In `@packages/engine/src/__tests__/agent-tools.test.ts`:
- Around line 669-691: After the denied tool.execute call, read the persisted
workflow for created.id and assert the triage column still has no agent assigned
so the persisted state wasn't mutated by createWorkflowUpdateTool();
specifically, after the denied variable is produced, fetch the workflow (use
your repo’s existing read method for workflows) and assert that
workflow.columns.triage.agent (or equivalent triage.agent field) is
undefined/absent and that denied.details remains { columnId: "triage", agentId:
agent.id, reason: "policy-escalation" } to ensure the FN-5893 invariant holds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc946845-102c-4132-8146-e394d0cf6cf8
📒 Files selected for processing (13)
docs/workflow-steps.mdpackages/core/src/__tests__/column-agent-resolver.test.tspackages/core/src/column-agent-resolver.tspackages/core/src/workflow-definition-types.tspackages/dashboard/app/components/WorkflowColumnPanel.tsxpackages/dashboard/app/components/WorkflowNodeEditor.tsxpackages/dashboard/src/__tests__/workflow-routes.test.tspackages/engine/src/__tests__/agent-tools.test.tspackages/engine/src/__tests__/executor-column-agent-principal.test.tspackages/engine/src/__tests__/executor-test-helpers.tspackages/engine/src/__tests__/workflow-graph-executor-parity.test.tspackages/engine/src/executor.tspackages/i18n/locales/en/app.json
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/workflow-definition-types.ts
- packages/engine/src/tests/executor-test-helpers.ts
- docs/workflow-steps.md
- packages/i18n/locales/en/app.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/engine/src/tests/workflow-graph-executor-parity.test.ts
- packages/core/src/tests/column-agent-resolver.test.ts
- packages/engine/src/executor.ts
…re the palette click, extend node waitFor timeout
- R10 kill-switch: workflowColumns flag now gates the execution path (resolver installation + resume pass 2), making the documented rollback real - resolver: skip parse candidates whose templateNodeId doesn't exist under the foreach (disambiguation guard) - editor: reset the project-scoped agents cache on projectId change - tests: kill-switch inertness, defer→own-settings release, candidate skip
Collaborator
Author
|
Addressing the CodeRabbit outside-diff comment (commit 59364e7):
Addressed: added a |
gsxdsm
added a commit
that referenced
this pull request
Jun 5, 2026
…union executor/agent-tools/routes imports, both tool-description texts, both route helpers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Workflow columns can now be staffed with a permanent agent. A column binds an agent from the registry with one of two modes, and every piece of session-running work attributable to that column — custom prompt/gate/script nodes, the execute seam's coding session, and per-step
step-executesessions (foreach templates inherit the enclosing foreach node's column) — runs as that agent:deferoverrideThe binding is not just a model source — the column-effective agent becomes the execution principal: action-permission gates are computed for the agent actually running, heartbeat serialization (
allowParallelExecution=false) holds in both directions (deferral gate, two-passresumeTaskForAgent, and the scheduler's reverseagent.taskIdguards), and workflow/agent-config edits hot-swap running sessions the way a task model change does today. Everything is gated behindexperimentalFeatures.workflowColumns+workflowGraphExecutor; the built-in default workflow carries no bindings and is proven byte-identical (parity suite extended).Plan:
docs/plans/2026-06-04-002-feat-column-agent-assignment-plan.md(R1-R13, U1-U7 all shipped).Key design decisions
agent?: { agentId, mode }field onWorkflowIrColumn, not a trait — traits are board-transition policy; this is execution identity consumed by the executor. Additive/optional, omitted when unset (neveragent: null), registered in v2-only-feature detection so default-workflow parity holds.resolveColumnAgentBinding/resolveEffectiveAgentin@fusion/corewith explicit named defer/override branches (no??collapse), consumed by all three engine resolution sites and the dashboard save route.instanceNodeId/parseInstanceNodeIdmoved to core so the foreach instance-id format has exactly one owner.validateColumnAgentBindingsin core enforces agent existence +confirmPolicyEscalationfor agents whose permission policy is broader than the project default, on both the dashboard routes and thefn_workflow_create/fn_workflow_updateagent tools (agent-native parity).executor:"agent"node persona injection read a nonexistentcustomInstructionsfield and had never actually fired; it now uses the typedsoul/instructionsTextfields.Review
Multi-persona review (9 reviewers + per-finding validators) ran in-pipeline; all validated findings were fixed on-branch before this PR — including two P1s (legacy model hot-swap clobbering override sessions;
resumeTaskForAgentmissing foreach-template step-execute nodes) and the R13 agent-tool bypass.Residual Review Findings
packages/dashboard/src/routes/register-workflow-routes.ts:119— policy-escalation gate is save-time-only (TOCTOU): broadening a bound agent's policy after save escalates without re-confirmation. Filed: Column-agent policy-escalation gate is save-time-only (TOCTOU): later policy broadening bypasses confirmation #1431docs/residual-review-findings/feat-column-agent-assignment.md):confirmPolicyEscalationis transient per-request; step-session tasks are not hot-swapped mid-flight (documented limitation); resume pass-2 does sequential IR resolution; the heartbeat reverse guard is per-executor-instance.Testing
~120 new/extended tests: core IR + resolver (30), engine custom-node/seams/principal (36, incl. characterization tests pinning the no-binding path byte-identical and a full mode × surface × own-settings matrix ledger), parity invisibility proof (default workflow with zero bindings produces identical observations), dashboard routes + components (35+, incl. policy-escalation directions and stale-agent states), agent-tools gate. Browser-verified the editor surface from a fresh bundle on an isolated port: picker renders per column, flags-off disabled-with-tooltip state correct, agents eagerly loaded, graph renders, no console errors.
Post-Deploy Monitoring & Validation
running as column agent(adoption),not found — falling back(fallback), and restart-watchercolumn agenthot-swap/deleted lines.experimentalFeatures.workflowColumns(bindings become inert; no data migration involved).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests