refactor(workflow): S05 runtime work-item driver#1578
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 implements the S05 "runtime work-item driver" slice by adding a ChangesWorkflow work-item runtime execution
Sequence Diagram(s)sequenceDiagram
participant API as runWorkItem
participant Store as WorkflowStore
participant Loader as TaskLoader
participant IR as WorkflowIRResolver
participant Handler as NodeHandler
API->>Store: getTask (optional)
Store-->>API: TaskDetail
API->>IR: resolve workflow IR for task
IR-->>API: workflow IR + target node
API->>Handler: execute handler at node with work-item context
Handler-->>API: outcome, value, contextPatch
API->>Store: transitionWorkflowWorkItem to terminal state (optional)
Store-->>API: updated work item
API-->>API: emit terminal event
API-->>API: return WorkflowTaskRuntimeResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s04-builtin-ir-regions
branch
from
2c9d2b6 to
069dcb8
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
5ed4024 to
c4f0d29
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s04-builtin-ir-regions
branch
from
069dcb8 to
e1b826b
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
c4f0d29 to
dfd8196
Compare
Greptile SummaryThis PR implements the S05 runtime work-item driver slice, adding
Confidence Score: 3/5The runtime's core execution path has a real contract gap: the final store write after handler execution is unguarded, so any DB-level error produces an unhandled rejection rather than the structured result the caller expects. The packages/engine/src/workflow-task-runtime.ts — the happy-path Important Files Changed
Sequence DiagramsequenceDiagram
participant Scheduler
participant runWorkItem
participant Store
participant Handler
participant IR
Scheduler->>runWorkItem: runWorkItem(workItem, settings)
runWorkItem->>Store: check getTask + transitionWorkflowWorkItem wired
alt store unwired
runWorkItem-->>Scheduler: "{disposition:"failed", reason:"store-unwired"}"
end
runWorkItem->>runWorkItem: "guard workItem.state === "running""
alt "state !== "running""
runWorkItem->>Store: transitionWorkflowWorkItem(id, "failed") no try-catch
runWorkItem-->>Scheduler: "{disposition:"failed"}"
end
runWorkItem->>Store: getTask(taskId)
runWorkItem->>IR: resolveRuntimeTarget(taskId)
IR-->>runWorkItem: "{workflowId, ir}"
runWorkItem->>IR: ir.nodes.find(nodeId)
IR-->>runWorkItem: "node {id, kind}"
runWorkItem->>Handler: "handler(node, {task, settings, context})"
note over Handler: merge-gate / merge-attempt / manual-merge-hold / retry-backoff / recovery-router / branch-group
Handler-->>runWorkItem: "{outcome, value, contextPatch}"
runWorkItem->>runWorkItem: resolve disposition + terminalState
runWorkItem->>Store: transitionWorkflowWorkItem(id, terminalState) no try-catch
Store-->>runWorkItem: WorkflowWorkItem
runWorkItem-->>Scheduler: "{disposition, outcome, visitedNodeIds, context}"
Reviews (8): Last reviewed commit: "Merge origin/main: align merge-region vi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s04-builtin-ir-regions
branch
from
e1b826b to
a41dcad
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
dfd8196 to
1f576b9
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s04-builtin-ir-regions
branch
from
a41dcad to
f59ea05
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
1f576b9 to
6aac550
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s04-builtin-ir-regions
branch
from
f59ea05 to
f8b5768
Compare
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
6aac550 to
de10d18
Compare
Fusion-Task-Id: FN-000
gsxdsm
force-pushed
the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
from
de10d18 to
fa44451
Compare
Address PR #1578 feedback by deriving merge-gate routing from task/settings auto-merge policy and persisting manual-hold work items as manual-required instead of failed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/engine/src/workflow-task-runtime.ts`:
- Around line 128-133: The runtime currently calls failWorkItem before ensuring
the store is wired, so failWorkItem's optional transitionWorkflowWorkItem call
can be a no-op and leave items stuck; fix by moving the store wiring guard
(checks for this.deps.store.getTask and
this.deps.store.transitionWorkflowWorkItem) to run before any early returns that
call failWorkItem in runWorkItem, and then remove the optional chaining when
invoking transitionWorkflowWorkItem inside failWorkItem (call
this.deps.store.transitionWorkflowWorkItem(...) directly). Apply the same
reorder-and-remove-optional-chaining change to the other similar block that
validates and fails work items (the block around the later failure path that
also calls failWorkItem).
- Around line 163-179: runWorkItem in workflow-task-runtime.ts builds the
handler context without including workItem.attempt, so handlers (notably the
"merge-attempt" handler in workflow-node-handlers.ts) never forward the retry
attempt into primitiveContextForNode; update the call site in runWorkItem to
include attempt: workItem.attempt in the context passed to handler (the object
currently containing task, settings, context), and then update the
"merge-attempt" handler to pass that attempt value through when calling
primitiveContextForNode(_node, ctx.task, ctx.context, attempt) or the
appropriate parameter position so requestMerge and underlying primitives receive
the attempt number.
🪄 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: b66c2d4b-a2d2-4f77-b5a2-963b4af0f366
📒 Files selected for processing (4)
docs/plans/workflow-owned-merge-stack/s05-runtime-work-item-driver.mdpackages/engine/src/__tests__/workflow-task-runtime.test.tspackages/engine/src/workflow-node-handlers.tspackages/engine/src/workflow-task-runtime.ts
Address PR #1578 feedback by failing unwired work item dispatch without a no-op persistence path and forwarding work item attempts into merge primitive context.
Address PR #1578 feedback by seeding run and workflow identifiers into work-item handler context so merge primitives receive the leased work identity.
Addressed: |
| if (workItem.state !== "running") { | ||
| return this.failWorkItem(workItem, `workflow-work-item-not-running:${workItem.state}`); | ||
| } |
There was a problem hiding this comment.
failWorkItem on non-running items causes uncaught exceptions and incorrect state mutations
When runWorkItem is handed a work item whose state is NOT "running", it calls failWorkItem, which unconditionally calls transitionWorkflowWorkItem. Two failure modes follow from that:
-
Terminal states (succeeded, failed, cancelled, exhausted): The production store (
packages/core/src/store.ts:9083) throws"Workflow work item X is terminal (succeeded) and cannot transition to failed". That exception escapesrunWorkItemas an unhandled rejection — callers expectingPromise<WorkflowTaskRuntimeResult>see a crash instead. -
Non-terminal non-running states (runnable, held, retrying, manual-required): The store accepts the transition. A
"manual-required"item waiting for human approval would be silently moved to"failed", erasing the hold.
The correct behaviour for this guard is to return a structured error without touching the store — only items that were legitimately claimed (and are therefore in "running") should be transitioned by this executor. No existing test exercises this path with a real store (all tests use state: "running"), so the bug is undetected today.
…actor main's workflow-graph-executor now collapses the merge-policy region (merge-gate, branch-group-*, merge-attempt) into a single synthetic `merge` node. Reconcile the S05 work-item-driver test's visitedNodeIds assertion to match, and union the `observed` recording-primitive fields from both branches (executedTasks + merge attempt/run/workflow ids). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| this.deps.store.transitionWorkflowWorkItem(workItem.id, terminalState, { | ||
| leaseOwner: null, | ||
| leaseExpiresAt: null, | ||
| lastError: reason ?? null, | ||
| }); |
There was a problem hiding this comment.
transitionWorkflowWorkItem is synchronous but CAN throw: the production store raises "Workflow work item X not found" (item deleted between lease check and write) and "Workflow work item X is terminal (…) and cannot transition to …" (concurrent lease race). Neither error is caught here, so any such exception escapes as an unhandled rejection — callers expecting Promise<WorkflowTaskRuntimeResult> receive a crashed promise instead of a structured { disposition: "failed" } result. Wrapping this call in a try-catch and returning a structured failure preserves the function's contract for all DB-error scenarios.
gsxdsm
deleted the
feature/workflow-owned-merge-s05-runtime-work-item-driver
branch
1 participant
Stack Slice
feature/workflow-owned-merge-s04-builtin-ir-regionsdocs/plans/2026-06-09-003-refactor-workflow-owned-merge-full-migration-slices-plan.mdGoal
Let WorkflowTaskRuntime start from a workflow work item and persist node/work-item outcomes.
Dependency
S1 workflow work items, S3 generic scheduler claim path, and S4 built-in IR regions.
Expected Scope
packages/engine/src/workflow-task-runtime.ts; workflow graph executor and node handler files; runtime tests.
Expected Tests
Runnable completion, retrying work creation, manual hold creation, restart resume, and duplicate lease refusal.
Exit Gate
Runtime can progress workflow work without old merge queue callbacks.
Status
Draft stack placeholder. This PR reserves ordering and review context; implementation should replace or extend the handoff artifact before this slice is marked ready.
Implementation Added
Summary by CodeRabbit