fix(journey): map session-store status into the workflow loop vocabulary (EVO-1690)#53
Merged
Conversation
…ary (EVO-1690) The session store speaks 'active'/'waiting'/'completed'; the execution loop only advances on 'running'. Copying the pre-created session's status verbatim (the norm since EVO-1644) made the loop condition false on the first iteration: every journey 'completed successfully' with zero nodes executed and no error anywhere. Only 'paused' survives the mapping - a paused session must not resume by itself. Extracted as a pure util with regression specs; validated live: the same manual trigger that produced completedNodes: 0 now executes trigger -> send-message and creates the message in the CRM conversation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideMaps persisted journey-session statuses into the workflow engine’s internal status vocabulary so pre-created sessions correctly enter the execution loop, and adds targeted tests for the new status resolution utility. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider narrowing
sessionStatus: string | undefinedinresolveInitialWorkflowStatusto the concrete session-store status type (e.g., a union or existing type alias) so that impossible or misspelled values are caught at compile time rather than defaulting silently to'running'. - Since
WorkflowExecutionStatusincludes'completed'and'failed'butresolveInitialWorkflowStatuscurrently only returns'running'or'paused', either extend the mapping logic to return all variants where appropriate or trim the union to the statuses this function can actually produce to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider narrowing `sessionStatus: string | undefined` in `resolveInitialWorkflowStatus` to the concrete session-store status type (e.g., a union or existing type alias) so that impossible or misspelled values are caught at compile time rather than defaulting silently to `'running'`.
- Since `WorkflowExecutionStatus` includes `'completed'` and `'failed'` but `resolveInitialWorkflowStatus` currently only returns `'running'` or `'paused'`, either extend the mapping logic to return all variants where appropriate or trim the union to the statuses this function can actually produce to avoid confusion.
## Individual Comments
### Comment 1
<location path="src/modules/temporal/workflows/initial-workflow-status.util.ts" line_range="16-17" />
<code_context>
+ * with zero nodes executed. Only 'paused' survives the mapping: a paused
+ * session must not resume by itself.
+ */
+export function resolveInitialWorkflowStatus(
+ sessionStatus: string | undefined,
+): WorkflowExecutionStatus {
+ return sessionStatus === 'paused' ? 'paused' : 'running';
</code_context>
<issue_to_address>
**suggestion:** Tighten the `sessionStatus` type to known session-store statuses instead of `string`.
`string | undefined` makes it easy for unsupported statuses to slip in unnoticed. If the session store exposes a finite set of statuses, model that as a string union type (e.g. `'active' | 'waiting' | 'completed' | 'paused' | undefined`) so TypeScript can enforce exhaustive handling when new statuses are introduced.
Suggested implementation:
```typescript
/**
* Maps a persisted journey-session status into the workflow loop's vocabulary
* (EVO-1690). The session store speaks 'active'/'waiting'/'completed'; the
* execution loop only advances on 'running' — copying the session status
* verbatim made `while (state.status === 'running')` false on the first
* iteration and every pre-created session (the norm since EVO-1644) completed
* with zero nodes executed. Only 'paused' survives the mapping: a paused
* session must not resume by itself.
*/
export type JourneySessionStoreStatus =
| 'active'
| 'waiting'
| 'completed'
| 'paused';
export function resolveInitialWorkflowStatus(
sessionStatus: JourneySessionStoreStatus | undefined,
): WorkflowExecutionStatus {
return sessionStatus === 'paused' ? 'paused' : 'running';
}
```
If there is already a shared type for the journey-session status (for example in a session-store module), you should reuse that instead of introducing `JourneySessionStoreStatus` here, and import it into this file. Replace the local `JourneySessionStoreStatus` definition with an import and keep the parameter typed as that union plus `undefined`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+16
to
+17
| export function resolveInitialWorkflowStatus( | ||
| sessionStatus: string | undefined, |
There was a problem hiding this comment.
suggestion: Tighten the sessionStatus type to known session-store statuses instead of string.
string | undefined makes it easy for unsupported statuses to slip in unnoticed. If the session store exposes a finite set of statuses, model that as a string union type (e.g. 'active' | 'waiting' | 'completed' | 'paused' | undefined) so TypeScript can enforce exhaustive handling when new statuses are introduced.
Suggested implementation:
/**
* Maps a persisted journey-session status into the workflow loop's vocabulary
* (EVO-1690). The session store speaks 'active'/'waiting'/'completed'; the
* execution loop only advances on 'running' — copying the session status
* verbatim made `while (state.status === 'running')` false on the first
* iteration and every pre-created session (the norm since EVO-1644) completed
* with zero nodes executed. Only 'paused' survives the mapping: a paused
* session must not resume by itself.
*/
export type JourneySessionStoreStatus =
| 'active'
| 'waiting'
| 'completed'
| 'paused';
export function resolveInitialWorkflowStatus(
sessionStatus: JourneySessionStoreStatus | undefined,
): WorkflowExecutionStatus {
return sessionStatus === 'paused' ? 'paused' : 'running';
}If there is already a shared type for the journey-session status (for example in a session-store module), you should reuse that instead of introducing JourneySessionStoreStatus here, and import it into this file. Replace the local JourneySessionStoreStatus definition with an import and keep the parameter typed as that union plus undefined.
dpaes
approved these changes
Jun 11, 2026
dpaes
left a comment
dpaes
left a comment
There was a problem hiding this comment.
✅ Code review — APPROVED (EVO-1690)
Fixes the exact root cause: the workflow copied the persisted session status (active/waiting) verbatim into state.status, so while (... state.status === 'running') never ran and every manually-triggered journey completed with 0 nodes. Fix extracts resolveInitialWorkflowStatus() — paused stays paused, everything else → running — wired into the init state. Verified the trigger path end-to-end (startJourney pre-creates ACTIVE → workflow maps ACTIVE→running → loop entered).
Acceptance criteria
- AC1 manual trigger executes nodes E2E ✓ (live-validated; see caveat)
- AC2 regression:
active/waiting→ loop ✓ (5 util specs) - AC3
pausedstays paused (resume preserved) ✓
Non-blocking
- [low] Mapper preserves only
paused; terminal statuses (completed/failed/cancelled) collapse torunning. Intentional ("failed re-runs instead of silently completing") and only reachable on a worker-crash replay; allowlist-by-exclusion — note if new statuses are added. - [low] No workflow-level integration test; AC1's "message created" is self-reported (evo-flow CI = Sourcery only, util spec not even gated). Fix is small + visible, so acceptable.
- [nit] 30-day stuck-active discovery (flow with no exit node) correctly out of scope — surface to product.
Approving.
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
2 participants
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
JourneyExecutionWorkflowcopied the pre-existing session's status verbatim into its internal state, but the session store speaks'active'/'waiting'while the execution loop only advances on'running'— sowhile (state.status === 'running')was false on the first iteration and the journey "completed successfully" withcompletedNodes: 0, no error anywhere. Since EVO-1644 pre-creates the session before starting the workflow, every manually triggered journey silently executed zero nodes.resolveInitialWorkflowStatus()(pure, regression-tested): only'paused'survives the mapping (a paused session must not resume by itself); everything else enters the loop as'running'.'failed'now also re-enters the loop on workflow (re)start — previously it silently completed with zero nodes, which was the bug's same shape, so re-running is the correct retry semantics.Test plan
npm test -- initial-workflow-status— 5 regression specs (active/waiting/completed/undefined → running; paused → paused)npm test -- src/modules/temporal— 49 specs greennpm run typecheck— cleancompletedNodes: 0executedtrigger → send-messageand created the message in the CRM conversation (journey 'E2E Smoke', conversation bb860c2f…). Discovered during validation (documented on the card): flows that don't end inexit-journey-nodedeliberately stay active for 30 days by design (condition(() => false, '30d')) — orthogonal to this fix.Changed Files
src/modules/temporal/workflows/initial-workflow-status.util.ts(+ spec)src/modules/temporal/workflows/journey-execution.workflow.ts— surgical +5/-4Linked Issue
Summary by Sourcery
Map journey session-store statuses into the workflow execution loop vocabulary so pre-created sessions enter the loop correctly and execute nodes.
Bug Fixes:
Enhancements: