[codex] Add workflow extension plugin seams#1498
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive workflow extension plugin system that allows plugins to register six kinds of extensions (move policies, work engines, node handlers, verdict providers, auto-merge facts, and shared board action services) via a central registry. Extensions integrate at multiple execution checkpoints: store move validation, task dispatch, workflow graph execution, verdict evaluation, and auto-merge routing. ChangesWorkflow Extension Plugin System
Sequence DiagramssequenceDiagram
participant Plugin as Plugin Registry
participant Runner as Plugin Runner
participant Registry as Extension Registry
Plugin->>Runner: register/load plugin
Runner->>Runner: call getPluginWorkflowExtensions()
Runner->>Runner: syncPluginWorkflowExtensions()
loop for each extension contribution
Runner->>Registry: register(pluginId, extension)
Registry-->>Runner: WorkflowExtensionDefinition
end
Runner->>Runner: track registered IDs per plugin
Plugin->>Runner: disable/unload plugin
Runner->>Runner: call disablePluginWorkflowExtensions(pluginId)
alt force=true
Runner->>Registry: degrade(ids, "force-disabled", message)
else
Runner->>Registry: unregister(ids)
end
sequenceDiagram
participant Client as Client/Agent
participant Store as TaskStore
participant Registry as Extension Registry
participant Policy as Move-Policy Extension
Client->>Store: moveTask(task, column, {workflowMoveActor, source})
Store->>Registry: list("move-policy")
loop for each extension
Store->>Policy: evaluate({task, columns, actor, source, metadata})
alt allowed
Policy-->>Store: {allowed: true}
else denied
Policy-->>Store: {allowed: false, reason, message}
Store-->>Client: TransitionRejectionError
end
end
Store-->>Client: moved task (if all allow)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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)
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 adds a generic workflow extension plugin layer, letting optional behaviors (move policies, work engines, node handlers, verdict providers, auto-merge fact providers) be registered by plugins rather than hardcoded in the engine. The preflight+lock split for move-policy evaluation, the upsert-based
Confidence Score: 4/5Safe to merge with fixes to the degradeToDefault fault-handling gaps in three engine handlers. Three out of five extension fault handlers — evaluateAutoMergeFactProviders, evaluateTaskVerdictProviders, and maybeDispatchWorkflowWorkEngine — skip the registry.degrade() call when fallback is degradeToDefault. Without the fix, a plugin extension that throws repeatedly will keep running on every auto-merge, every fn_task_done, and every heartbeat dispatch cycle instead of being permanently disabled after the first fault. packages/engine/src/auto-merge-fact-providers.ts and packages/engine/src/executor.ts (both evaluateTaskVerdictProviders and maybeDispatchWorkflowWorkEngine). Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant TaskStore
participant Registry as WorkflowExtensionRegistry
participant MovePolicy as Plugin: MovePolicy
participant Executor
participant WorkEngine as Plugin: WorkEngine
participant VerdictProvider as Plugin: VerdictProvider
participant Merger
participant FactProvider as Plugin: FactProvider
Caller->>TaskStore: moveTask(id, toColumn)
TaskStore->>TaskStore: prepareWorkflowMovePolicyPreflight() [outside lock]
TaskStore->>Registry: list(move-policy)
Registry-->>TaskStore: [policies]
TaskStore->>MovePolicy: evaluate(input) [5s timeout]
MovePolicy-->>TaskStore: allowed true/false
TaskStore->>TaskStore: "withTaskLock -> moveTaskInternal()"
TaskStore->>TaskStore: validate stale-preflight check
TaskStore-->>Caller: Task
Executor->>Executor: execute(task)
Executor->>Registry: get(extensionId) per column.extensions
Executor->>WorkEngine: dispatch(input)
WorkEngine-->>Executor: claimed / not-claimed / parked
Executor->>VerdictProvider: evaluate(input) at fn_task_done
VerdictProvider-->>Executor: status pass/fail/blocked
Merger->>FactProvider: collect(input)
FactProvider-->>Merger: route, facts, reason
Merger->>Merger: chooseStricterAutoMergeRoute()
Merger->>Merger: upsertMergeRequestRecord(state)
Reviews (4): Last reviewed commit: "fix(engine): tighten workflow extension ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 8
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/merger.ts (1)
8154-8175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor global
settings.autoMergebefore queueing or running the shadow merge request.This still auto-queues/runs when project-level auto-merge is off. If
settings.autoMerge === falseand the task does not explicitly opt back in withautoMerge: true,initialStatebecomes"queued"and the next block promotes it to"running", which breaks the terminal-until-human-merge contract.Suggested fix
- const providerManualRoute = - autoMergeFacts.route === "manual-required" || - autoMergeFacts.route === "blocked"; - const initialState = task.autoMerge === false || providerManualRoute ? "manual-required" : "queued"; + const providerManualRoute = + autoMergeFacts.route === "manual-required" || + autoMergeFacts.route === "blocked"; + const autoMergeManuallyGated = + task.autoMerge === false || + (settings.autoMerge === false && task.autoMerge !== true) || + providerManualRoute; + const initialState = autoMergeManuallyGated ? "manual-required" : "queued"; @@ - if (task.autoMerge !== false) { + if (!autoMergeManuallyGated) { if (currentState === "retrying") { store.transitionMergeRequestState(task.id, "queued"); store.transitionMergeRequestState(task.id, "running"); } else if (currentState === "queued") { store.transitionMergeRequestState(task.id, "running"); } }As per coding guidelines:
When settings.autoMerge: false, in-review is terminal-until-merged by a human.Based on learnings: this short-circuit is intentionally skipped only when the task has an explicit per-taskautoMerge: trueoverride.🤖 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/merger.ts` around lines 8154 - 8175, The code queues/runs shadow merges even when global settings.autoMerge is false because initialState and state transitions only consider task.autoMerge and providerManualRoute; update the logic to honor settings.autoMerge by treating the merge as manual-required unless the task explicitly opts in (task.autoMerge === true): change the initialState calculation to set "manual-required" when settings.autoMerge === false AND task.autoMerge !== true (in addition to providerManualRoute or task.autoMerge === false), and guard the transition block (the branch that calls store.transitionMergeRequestState to "queued"/"running") to skip/promote only when settings.autoMerge !== false OR task.autoMerge === true so shadow merges are not queued/run when global autoMerge is disabled.Sources: Coding guidelines, Learnings
🤖 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 @.changeset/workflow-extension-plugins.md:
- Line 2: The changeset currently sets "`@runfusion/fusion`" to "patch" but this
PR adds a new feature surface, so edit the changeset entry that mentions
"`@runfusion/fusion`" and change its version bump from "patch" to "minor" (update
the value on the line containing "`@runfusion/fusion`": patch →
"`@runfusion/fusion`": minor) so the release type matches the repo policy for new
features.
In `@packages/core/src/__tests__/workflow-extension-registry.test.ts`:
- Around line 8-16: The test helper function extension() currently hardcodes
kind: "move-policy" causing mismatches when called with other IDs (e.g.,
extension("work-engine")); change extension() to accept a kind parameter (with a
sensible default) and set the returned object's kind to that parameter (not the
hardcoded string), and then update call sites (such as the call at
extension("work-engine")) to pass the correct kind where needed so extensionId
and kind remain consistent.
In `@packages/core/src/store.ts`:
- Around line 6529-6537: The call to evaluateWorkflowMovePolicies is currently
invoked for every flagged move and can block hard-cancel and recovery rehome
moves; modify the call site so that before invoking evaluateWorkflowMovePolicies
(the call shown with task, workflowIr, fromColumn, toColumn, actor from
resolveWorkflowMoveActor, and source from options?.workflowMoveSource), you
detect and bypass policy evaluation for hard-cancel moves and recovery rehome
moves — specifically: treat a user-initiated in-progress→todo move identified as
a hard-cancel (use the same detection logic used for moveTask/user pause
semantics or the actor resolved by resolveWorkflowMoveActor) and any move whose
source equals the recoveryRehome source/flag (options?.workflowMoveSource or
moveSource) as exemptions; only call evaluateWorkflowMovePolicies when the move
is not one of these exempt cases so hard cancels and recovery rehomes are not
vetoed by plugins.
- Around line 6349-6358: The evaluateWorkflowMovePolicies call is running while
the task lock is held (moveTask -> withTaskLock), which blocks other mutations
if a plugin hangs; move the plugin execution out of the lock and add a
degradation/timeout boundary like the plugin-gate path does. Specifically,
change the flow so evaluateWorkflowMovePolicies (and any extension.evaluate
calls inside it) are invoked outside withTaskLock (or refactor
evaluateWorkflowMovePolicies to schedule/await extension.evaluate via the same
plugin-gate helper used elsewhere), enforce a hard timeout and mark the
definition.degraded on timeout/exception, and keep only non-plugin state updates
inside withTaskLock to avoid blocking other task mutations. Ensure you reference
evaluateWorkflowMovePolicies, moveTask, withTaskLock, extension.evaluate, and
getWorkflowExtensionRegistry when making the change.
In `@packages/core/src/workflow-ir.ts`:
- Around line 976-988: The enum branch in validate logic (check where field.type
=== "enum") currently accepts any string if field.enumValues is undefined;
change it to require field.enumValues to be a non-empty array: if
field.enumValues is missing or has length 0, throw a WorkflowIrError indicating
that the extension (use owner and key and field.key in the message) must declare
enumValues for enum fields, then continue to validate that the provided value
(enumValue) is included in field.enumValues as before.
In `@packages/engine/src/plugin-runner.ts`:
- Around line 578-584: The collectPluginWorkflowExtensionIds method builds
registry IDs manually; import and use the workflowExtensionRegistryId helper
from `@fusion/core` instead: add the import for workflowExtensionRegistryId and
replace the template string `plugin:${pluginId}:${entry.extension.extensionId}`
inside collectPluginWorkflowExtensionIds with a call to
workflowExtensionRegistryId(pluginId, entry.extension.extensionId) so ID
generation matches the adapter and central helper.
In `@packages/engine/src/workflow-graph-executor.ts`:
- Around line 527-528: The catch block checking extension.fallback ===
"degradeToDefault" currently just continues to the default handler but never
marks the extension as degraded; update that catch clause in
workflow-graph-executor.ts to invoke the registry's degradation API for the
failing extension (e.g., call nodeHandlerRegistry.degrade(extension.id) or the
equivalent degradeExtension/degrade method available in this module) before
falling through to the default handler, handle any errors from the degrade call
gracefully, and then continue to the default path.
- Around line 491-499: The normalizePluginNodeResult function currently discards
a plugin-provided value when normalizing custom outcomes (outcome starting with
"outcome:"); update normalizePluginNodeResult in WorkflowGraphExecutor to
preserve result.value if present (i.e., set value to result.value if defined,
otherwise fall back to result.outcome.slice("outcome:".length)), keep
contextPatch unchanged, and return a WorkflowNodeResult with outcome "success"
for custom outcomes; ensure types align with WorkflowNodeExtensionResult and
WorkflowNodeResult so custom outcome values are not lost.
---
Outside diff comments:
In `@packages/engine/src/merger.ts`:
- Around line 8154-8175: The code queues/runs shadow merges even when global
settings.autoMerge is false because initialState and state transitions only
consider task.autoMerge and providerManualRoute; update the logic to honor
settings.autoMerge by treating the merge as manual-required unless the task
explicitly opts in (task.autoMerge === true): change the initialState
calculation to set "manual-required" when settings.autoMerge === false AND
task.autoMerge !== true (in addition to providerManualRoute or task.autoMerge
=== false), and guard the transition block (the branch that calls
store.transitionMergeRequestState to "queued"/"running") to skip/promote only
when settings.autoMerge !== false OR task.autoMerge === true so shadow merges
are not queued/run when global autoMerge is disabled.
🪄 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: 3962ffc1-f89e-4a1b-a072-f9afdb65ec48
📒 Files selected for processing (31)
.changeset/workflow-extension-plugins.mdCONCEPTS.mddocs/PLUGIN_AUTHORING.mddocs/plans/2026-06-08-001-feat-workflow-extension-plugins-plan.mdpackages/core/src/__tests__/board-action-services.test.tspackages/core/src/__tests__/plugin-contribution-types.test.tspackages/core/src/__tests__/plugin-loader-contributions.test.tspackages/core/src/__tests__/transition-parity.test.tspackages/core/src/__tests__/workflow-extension-registry.test.tspackages/core/src/__tests__/workflow-ir-extension-metadata.test.tspackages/core/src/board-action-services.tspackages/core/src/index.tspackages/core/src/plugin-loader.tspackages/core/src/plugin-types.tspackages/core/src/store.tspackages/core/src/workflow-extension-registry.tspackages/core/src/workflow-extension-types.tspackages/core/src/workflow-ir-types.tspackages/core/src/workflow-ir.tspackages/engine/src/__tests__/auto-merge-fact-providers.test.tspackages/engine/src/__tests__/plugin-runner.test.tspackages/engine/src/__tests__/workflow-node-handler-extensions.test.tspackages/engine/src/__tests__/workflow-verdict-provider-extensions.test.tspackages/engine/src/__tests__/workflow-work-engine-dispatch.test.tspackages/engine/src/auto-merge-fact-providers.tspackages/engine/src/executor.tspackages/engine/src/merger.tspackages/engine/src/plugin-runner.tspackages/engine/src/plugin-workflow-extension-adapter.tspackages/engine/src/workflow-graph-executor.tspackages/plugin-sdk/src/index.ts
👮 Files not reviewed due to content moderation or server errors (1)
- packages/engine/src/executor.ts
Addressed in |
Addressed in |
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/engine/src/merger.ts (1)
8162-8179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove existing merge-request records back to
manual-requiredwhen gating flips on.This only applies
initialStatewhen the record does not exist. If a task is already inqueuedorretryingandsettings.autoMergeis later turned off, or a fact provider starts returningmanual-required/blocked, the persisted merge-request record stays runnable instead of being re-gated.💡 Suggested fix
const currentState = existingRecord?.state ?? initialState; if (!existingRecord) { store.upsertMergeRequestRecord(task.id, { state: initialState }); + } else if (autoMergeManuallyGated && currentState !== "manual-required") { + store.transitionMergeRequestState(task.id, "manual-required"); } if (!autoMergeManuallyGated) { if (currentState === "retrying") { store.transitionMergeRequestState(task.id, "queued");Based on learnings:
When settings.autoMerge: false, in-review is terminal-until-merged by a human.🤖 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/merger.ts` around lines 8162 - 8179, The code only applies initialState when no record exists, so when autoMergeManuallyGated becomes true existing merge records (queued/retrying/running) remain runnable; update logic after computing autoMergeManuallyGated/currentState to, when autoMergeManuallyGated is true and an existingRecord exists and currentState !== "manual-required", call store.transitionMergeRequestState(task.id, "manual-required") (cover queued/retrying/running -> manual-required) instead of leaving them unchanged; reference autoMergeManuallyGated, initialState, existingRecord/currentState, store.getMergeRequestRecord, store.upsertMergeRequestRecord and store.transitionMergeRequestState.Source: Learnings
🧹 Nitpick comments (1)
packages/engine/src/plugin-workflow-extension-adapter.ts (1)
7-38: 💤 Low valueConsider consistent parameter patterns across adapter functions.
registerPluginWorkflowExtensionsuses a structured params object (line 7-11), whileunregisterPluginWorkflowExtensionsanddegradePluginWorkflowExtensionsuse positional parameters (lines 21-24, 32-35). For a cohesive adapter API, consider using the same parameter style across all three functions.🤖 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/plugin-workflow-extension-adapter.ts` around lines 7 - 38, The three adapter functions use mixed parameter styles—registerPluginWorkflowExtensions(params: { registry, pluginId, contributions }) vs positional params in unregisterPluginWorkflowExtensions(registry, ids) and degradePluginWorkflowExtensions(registry, ids, message)—so make them consistent by converting unregisterPluginWorkflowExtensions and degradePluginWorkflowExtensions to accept a single params object (e.g., { registry: WorkflowExtensionRegistry; ids: readonly string[]; message?: string }) or conversely change registerPluginWorkflowExtensions to positional args; update the function bodies to use params.registry/params.ids/params.message, update any callers to the new call sites, and keep exported names (registerPluginWorkflowExtensions, unregisterPluginWorkflowExtensions, degradePluginWorkflowExtensions) unchanged.
🤖 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/store.ts`:
- Around line 1221-1224: The preflight result stored in movePolicyPreflight must
be tied to the task's workflow snapshot so it can't be replayed against a
different workflow; update the preflight to capture the workflow
identifier/version/hash (the same snapshot data produced by
selectTaskWorkflowAndReconcile()/updateWorkflowDefinition), and in moveTask()
after acquiring withTaskLock() compare that saved workflow snapshot against the
current task workflow inside moveTaskInternal(); if they differ, discard the
preflight verdict and re-run policy evaluation under the lock (or fail the move
and require the caller to retry). Ensure identifiers referenced are moveTask(),
moveTaskInternal(), movePolicyPreflight, withTaskLock(),
selectTaskWorkflowAndReconcile(), and updateWorkflowDefinition().
In `@packages/engine/src/__tests__/workflow-node-handler-extensions.test.ts`:
- Around line 69-106: The test's extension mock (registered via
getWorkflowExtensionRegistry().register using
workflowExtensionRegistryId("node-plugin","decision")) returns outcome
"outcome:custom-route" but the assertion expects routing to the "human" node;
update the mock handle to return "outcome:needs-human" so the decide node routes
to the human edge, or alternatively change the expected visitedNodeIds in the
expect(...) assertion to ["start","decide","default"]; modify the return value
in the vi.fn().mockResolvedValue(...) for the registered extension or adjust the
expect(result.visitedNodeIds) accordingly to keep outcome and routing
consistent.
---
Outside diff comments:
In `@packages/engine/src/merger.ts`:
- Around line 8162-8179: The code only applies initialState when no record
exists, so when autoMergeManuallyGated becomes true existing merge records
(queued/retrying/running) remain runnable; update logic after computing
autoMergeManuallyGated/currentState to, when autoMergeManuallyGated is true and
an existingRecord exists and currentState !== "manual-required", call
store.transitionMergeRequestState(task.id, "manual-required") (cover
queued/retrying/running -> manual-required) instead of leaving them unchanged;
reference autoMergeManuallyGated, initialState, existingRecord/currentState,
store.getMergeRequestRecord, store.upsertMergeRequestRecord and
store.transitionMergeRequestState.
---
Nitpick comments:
In `@packages/engine/src/plugin-workflow-extension-adapter.ts`:
- Around line 7-38: The three adapter functions use mixed parameter
styles—registerPluginWorkflowExtensions(params: { registry, pluginId,
contributions }) vs positional params in
unregisterPluginWorkflowExtensions(registry, ids) and
degradePluginWorkflowExtensions(registry, ids, message)—so make them consistent
by converting unregisterPluginWorkflowExtensions and
degradePluginWorkflowExtensions to accept a single params object (e.g., {
registry: WorkflowExtensionRegistry; ids: readonly string[]; message?: string })
or conversely change registerPluginWorkflowExtensions to positional args; update
the function bodies to use params.registry/params.ids/params.message, update any
callers to the new call sites, and keep exported names
(registerPluginWorkflowExtensions, unregisterPluginWorkflowExtensions,
degradePluginWorkflowExtensions) unchanged.
🪄 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: 41762d39-870b-42c2-b85f-2b93b181b14f
📒 Files selected for processing (14)
.changeset/workflow-extension-plugins.mdpackages/core/src/__tests__/transition-parity.test.tspackages/core/src/__tests__/workflow-extension-registry.test.tspackages/core/src/__tests__/workflow-ir-extension-metadata.test.tspackages/core/src/store.tspackages/core/src/workflow-extension-registry.tspackages/core/src/workflow-ir.tspackages/engine/src/__tests__/merger-merge-lifecycle.test.tspackages/engine/src/__tests__/plugin-runner.test.tspackages/engine/src/__tests__/workflow-node-handler-extensions.test.tspackages/engine/src/merger.tspackages/engine/src/plugin-runner.tspackages/engine/src/plugin-workflow-extension-adapter.tspackages/engine/src/workflow-graph-executor.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/workflow-extension-plugins.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/tests/workflow-extension-registry.test.ts
- packages/engine/src/plugin-runner.ts
- packages/engine/src/workflow-graph-executor.ts
- packages/core/src/workflow-ir.ts
|
@coderabbitai review |
|
@greptile review |
✅ Action performedReview finished.
|
Summary
Adds a generic workflow extension plugin layer so optional workflow behavior can be registered through plugins instead of hardcoded into the shared engine.
Changes
Validation
pnpm --filter @fusion/core exec vitest run src/__tests__/plugin-contribution-types.test.ts src/__tests__/workflow-extension-registry.test.ts src/__tests__/plugin-loader-contributions.test.ts src/__tests__/workflow-ir-extension-metadata.test.ts src/__tests__/transition-parity.test.ts src/__tests__/board-action-services.test.ts --silent=passed-only --reporter=dotpnpm --filter @fusion/engine exec vitest run src/__tests__/plugin-runner.test.ts src/__tests__/workflow-work-engine-dispatch.test.ts src/__tests__/workflow-node-handler-extensions.test.ts src/__tests__/workflow-verdict-provider-extensions.test.ts src/__tests__/auto-merge-fact-providers.test.ts --silent=passed-only --reporter=dotpnpm --filter @fusion/core typecheckpnpm --filter @fusion/engine typecheckpnpm --filter @fusion/plugin-sdk typecheckgit diff --checkSummary by CodeRabbit
New Features
Documentation
Tests