refactor(workflow): extract node runner boundaries#1842
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR splits workflow execution into runner modules and runtime services, adds a runner registry, rewires executor dispatch and handler assembly, updates fast-mode behavior, and refreshes related docs and tests. ChangesWorkflow Node Runner Architecture
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extracts explicit engine-owned runner and service boundaries from monolithic
Confidence Score: 5/5Safe to merge; all behavioral extraction is adapter-first with compatibility re-exports preserved and focused tests covering runner dispatch, fast-mode fallbacks, and new service boundaries. Every changed code path either delegates to the existing executor internals without changing outcomes, or moves handler logic into runner modules with byte-equivalent behavior guarded by 166 passing focused tests. No safety invariants (worktree, semaphore, pause/abort, merge-proof) are relocated. The two findings are both design-quality observations with no current failure condition. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Graph as WorkflowGraphExecutor
participant Registry as WorkflowNodeRunnerRegistry
participant Handler as WorkflowNodeHandler
participant Runner as WorkflowNodeRunner (gate/notify/code/…)
participant Service as Runtime Service (Custom/Review/Planning)
participant Exec as TaskExecutor (substrate)
Graph->>Registry: toHandlers() at construction
Registry-->>Graph: merged handler map
Graph->>Handler: handler(node, context)
Handler->>Runner: runner.run(node, context)
alt primitive-backed node (merge-attempt, step-review…)
Runner->>Exec: createAuthoritativeWorkflowPrimitivesFromExecutor
Exec-->>Runner: WorkflowRuntimePrimitives
Runner->>Runner: runWorkflowMergeAttemptNode / runReview
end
alt custom-node (prompt/script/gate with executable config)
Runner->>Service: WorkflowCustomNodeExecutionService.runner(settings)
Service->>Exec: this.runGraphCustomNode(…)
Exec-->>Service: WorkflowNodeResult
Service-->>Runner: result
end
alt review (step-review seam)
Runner->>Service: WorkflowReviewService.reviewStep(input)
Service->>Exec: reviewStep(cwd, taskId, …)
Exec-->>Service: ReviewResult
Service-->>Runner: result
end
Runner-->>Handler: WorkflowNodeResult
Handler-->>Graph: outcome / value / contextPatch
Graph->>Graph: choose next edge
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Graph as WorkflowGraphExecutor
participant Registry as WorkflowNodeRunnerRegistry
participant Handler as WorkflowNodeHandler
participant Runner as WorkflowNodeRunner (gate/notify/code/…)
participant Service as Runtime Service (Custom/Review/Planning)
participant Exec as TaskExecutor (substrate)
Graph->>Registry: toHandlers() at construction
Registry-->>Graph: merged handler map
Graph->>Handler: handler(node, context)
Handler->>Runner: runner.run(node, context)
alt primitive-backed node (merge-attempt, step-review…)
Runner->>Exec: createAuthoritativeWorkflowPrimitivesFromExecutor
Exec-->>Runner: WorkflowRuntimePrimitives
Runner->>Runner: runWorkflowMergeAttemptNode / runReview
end
alt custom-node (prompt/script/gate with executable config)
Runner->>Service: WorkflowCustomNodeExecutionService.runner(settings)
Service->>Exec: this.runGraphCustomNode(…)
Exec-->>Service: WorkflowNodeResult
Service-->>Runner: result
end
alt review (step-review seam)
Runner->>Service: WorkflowReviewService.reviewStep(input)
Service->>Exec: reviewStep(cwd, taskId, …)
Exec-->>Service: ReviewResult
Service-->>Runner: result
end
Runner-->>Handler: WorkflowNodeResult
Handler-->>Graph: outcome / value / contextPatch
Graph->>Graph: choose next edge
Reviews (6): Last reviewed commit: "Merge branch 'main' into feature/workflo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/src/workflow-graph-task-runner.ts (1)
251-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
skillNamehere, notskill.executor: "skill"nodes throughout the workflow IR useconfig.skillName, so this guard misses skill executors and lets them fall through torunCustomNode(...)instead of taking the fast-mode skip.🤖 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/workflow-graph-task-runner.ts` around lines 251 - 268, The fast-mode skip guard in wrappedRunCustomNode is checking the wrong skill field, so executor: "skill" nodes are not being skipped. Update the hasExecutableConfig check to use node.config.skillName (alongside prompt and scriptName) instead of node.config.skill so skill executors are recognized and return the fast-mode-skipped result before calling this.deps.runCustomNode.
🧹 Nitpick comments (2)
packages/engine/src/executor.ts (1)
5579-5583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProvider boundary is created and immediately discarded — pointless indirection.
createAuthoritativeWorkflowPrimitivesconstructs aWorkflowRuntimePrimitiveProviderviacreateWorkflowRuntimePrimitiveProvider(...)only to call.create(settings)on it in the very next line and return the result. This is equivalent toreturn this.createAuthoritativeWorkflowPrimitivesFromExecutor(settings);— the provider object serves no purpose here since nothing holds onto it or reuses it across multiplesettingsvalues. As the reference implementation of the new "provider boundary" pattern introduced by this PR, this shape may get copy-pasted at future call sites, propagating the same needless allocation/indirection.♻️ Simplify by calling the factory directly
public createAuthoritativeWorkflowPrimitives(settings: Settings): WorkflowRuntimePrimitives { - return createWorkflowRuntimePrimitiveProvider((providerSettings) => - this.createAuthoritativeWorkflowPrimitivesFromExecutor(providerSettings), - ).create(settings); + return this.createAuthoritativeWorkflowPrimitivesFromExecutor(settings); }If the intent is for callers to eventually hold a reusable provider (rather than one-shot primitives), consider exposing
createWorkflowRuntimePrimitiveProvider((s) => this.createAuthoritativeWorkflowPrimitivesFromExecutor(s))itself as the public surface instead of pre-resolving it with.create(settings).🤖 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/executor.ts` around lines 5579 - 5583, The createAuthoritativeWorkflowPrimitives method is creating a WorkflowRuntimePrimitiveProvider and immediately discarding it by calling create(settings) right away. Simplify this by removing the unnecessary provider indirection and returning the result of createAuthoritativeWorkflowPrimitivesFromExecutor(settings) directly, or, if the provider boundary is meant to be reusable, expose createWorkflowRuntimePrimitiveProvider(...) itself instead of resolving it here. Use createAuthoritativeWorkflowPrimitives and createWorkflowRuntimePrimitiveProvider to locate the change.packages/engine/src/workflow-graph-task-runner.ts (1)
284-300: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback
writeStepsmutates the caller'staskobject in place.
task.steps = stepsdirectly mutates theTaskDetailpassed intorun(). This is scoped to the documented no-store raw-compatibility fallback (no persistence layer to write through), so it's likely intentional, but it's worth flagging since mutating an input parameter is easy to overlook if this runner is ever reused with a caller that expectstaskto remain untouched across the call.🤖 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/workflow-graph-task-runner.ts` around lines 284 - 300, The fast-mode fallback in workflow-graph-task-runner mutates the incoming task via writeSteps, which can surprise callers that expect run() not to alter its input. Update the fastModeFallbackParseSteps branch in WorkflowGraphTaskRunner.run so it stores parsed steps without modifying the original TaskDetail object in place, or clearly route the projection through an internal runner-owned copy/state instead of assigning directly to task.steps.
🤖 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.
Outside diff comments:
In `@packages/engine/src/workflow-graph-task-runner.ts`:
- Around line 251-268: The fast-mode skip guard in wrappedRunCustomNode is
checking the wrong skill field, so executor: "skill" nodes are not being
skipped. Update the hasExecutableConfig check to use node.config.skillName
(alongside prompt and scriptName) instead of node.config.skill so skill
executors are recognized and return the fast-mode-skipped result before calling
this.deps.runCustomNode.
---
Nitpick comments:
In `@packages/engine/src/executor.ts`:
- Around line 5579-5583: The createAuthoritativeWorkflowPrimitives method is
creating a WorkflowRuntimePrimitiveProvider and immediately discarding it by
calling create(settings) right away. Simplify this by removing the unnecessary
provider indirection and returning the result of
createAuthoritativeWorkflowPrimitivesFromExecutor(settings) directly, or, if the
provider boundary is meant to be reusable, expose
createWorkflowRuntimePrimitiveProvider(...) itself instead of resolving it here.
Use createAuthoritativeWorkflowPrimitives and
createWorkflowRuntimePrimitiveProvider to locate the change.
In `@packages/engine/src/workflow-graph-task-runner.ts`:
- Around line 284-300: The fast-mode fallback in workflow-graph-task-runner
mutates the incoming task via writeSteps, which can surprise callers that expect
run() not to alter its input. Update the fastModeFallbackParseSteps branch in
WorkflowGraphTaskRunner.run so it stores parsed steps without modifying the
original TaskDetail object in place, or clearly route the projection through an
internal runner-owned copy/state instead of assigning directly to task.steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd0d977-b7ad-46ca-afc2-c76c0d4e7714
📒 Files selected for processing (24)
CONCEPTS.mddocs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.mddocs/workflow-steps.mdpackages/engine/src/__tests__/executor-fast-mode-workflows.test.tspackages/engine/src/__tests__/runtime-primitives.test.tspackages/engine/src/__tests__/workflow-custom-node-execution.test.tspackages/engine/src/__tests__/workflow-node-runner.test.tspackages/engine/src/__tests__/workflow-planning-service.test.tspackages/engine/src/__tests__/workflow-review-service.test.tspackages/engine/src/executor.tspackages/engine/src/index.tspackages/engine/src/workflow-custom-node-execution.tspackages/engine/src/workflow-graph-executor.tspackages/engine/src/workflow-graph-task-runner.tspackages/engine/src/workflow-node-handlers.tspackages/engine/src/workflow-node-runner.tspackages/engine/src/workflow-node-runners/code-runner.tspackages/engine/src/workflow-node-runners/gate-runner.tspackages/engine/src/workflow-node-runners/merge-runner.tspackages/engine/src/workflow-node-runners/notify-runner.tspackages/engine/src/workflow-node-runners/parse-steps-runner.tspackages/engine/src/workflow-planning-service.tspackages/engine/src/workflow-review-service.tspackages/engine/src/workflow-runtime-primitive-provider.ts
6e4b159 to
5d8587f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md (1)
19-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGeneralize the Authority field for repo durability.
The Authority field currently reads "User request in this session: assess and plan whether workflow logic can be moved..." This retains ephemeral session context. For an implementation-ready plan committed to the repo, replace with a durable authority reference (e.g., "Architecture decision: decouple workflow node behavior from monolithic executor" or link to the adjacent lifecycle-authority plan).
🤖 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 `@docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md` around lines 19 - 20, The Authority field in this plan still references a transient user session, so update the document’s authority text to a durable repo-owned source instead. Replace the session-specific wording in the plan header with a stable architecture decision or a cross-reference to the adjacent lifecycle-authority plan, keeping the rest of the refactor scope intact. Use the existing plan header fields (especially Authority and Execution profile) to keep the document implementation-ready and durable.
🤖 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.
Nitpick comments:
In `@docs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.md`:
- Around line 19-20: The Authority field in this plan still references a
transient user session, so update the document’s authority text to a durable
repo-owned source instead. Replace the session-specific wording in the plan
header with a stable architecture decision or a cross-reference to the adjacent
lifecycle-authority plan, keeping the rest of the refactor scope intact. Use the
existing plan header fields (especially Authority and Execution profile) to keep
the document implementation-ready and durable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39d4c91c-7b3f-4a6f-820b-7a6f684ea420
📒 Files selected for processing (8)
CONCEPTS.mddocs/plans/2026-06-30-001-refactor-workflow-node-runner-classes-plan.mddocs/workflow-steps.mdpackages/engine/src/__tests__/executor-fast-mode-workflows.test.tspackages/engine/src/__tests__/runtime-primitives.test.tspackages/engine/src/__tests__/workflow-custom-node-execution.test.tspackages/engine/src/__tests__/workflow-node-runner.test.tspackages/engine/src/__tests__/workflow-planning-service.test.ts
💤 Files with no reviewable changes (5)
- packages/engine/src/tests/workflow-planning-service.test.ts
- packages/engine/src/tests/workflow-custom-node-execution.test.ts
- packages/engine/src/tests/runtime-primitives.test.ts
- packages/engine/src/tests/workflow-node-runner.test.ts
- packages/engine/src/tests/executor-fast-mode-workflows.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/workflow-steps.md
- CONCEPTS.md
ba60625 to
dcb759e
Compare
|
Addressing CodeRabbit review body
Addressed: the raw fast-mode executable-node guard now checks
Addressed differently: the raw no-store fallback still intentionally projects parsed steps onto the runner task so parse→foreach compatibility works without persistence, but it now writes through the task object passed by
Declined: |
|
Addressing CodeRabbit review body
Addressed: replaced the session-specific authority wording with a durable architecture-decision authority for decoupling workflow node behavior from monolithic triage/executor/reviewer files into node runner and runtime primitive/service boundaries. |
dcb759e to
b07105d
Compare
1 participant
Summary
Workflow node behavior now has explicit engine-owned runner and service boundaries instead of being concentrated in the default handler factory and private executor seams. Workflow IR remains declarative: the graph executor still walks and routes the graph, while node runners and runtime services own node-kind behavior, primitive construction, custom-node execution, review invocation, and planning dispatch.
Design Notes
WorkflowNodeRunnerregistry that adapts runners to the existing handler contract while preserving explicit handler override precedence.workflow-node-handlers.ts.PROMPT.mdin memory when no executor primitive deps are wired.Validation
pnpm --filter @fusion/engine exec vitest run ... --silent=passed-only --reporter=dotacross 20 focused workflow/runtime test files: 166 tests passed.pnpm --filter @fusion/engine exec tsc --noEmit --pretty falsepassed.git diff --checkpassed.Post-Deploy Monitoring & Validation
No additional operational monitoring required. This is an internal engine refactor with focused tests covering runner dispatch, primitive-provider compatibility, custom-node service delegation, review/planning service boundaries, merge behavior, fast-mode raw runner compatibility, and docs alignment.
Healthy signals: workflow graph runs continue through planning, parse-steps, optional groups, custom nodes, review, and merge with existing outcome values. Failure signals: new
parse-steps-unwired, custom-node, review, or merge-attempt regressions in engine logs for previously working built-in workflows. Rollback trigger: repeated workflow graph task failures attributable to runner/service dispatch rather than task content or provider errors.Summary by CodeRabbit