Conversation
…ssage A resume re-walks the DAG and re-emits prior-success for every node whose earlier pass already succeeded. The live emitter folded that replay into `node_skipped`, and the Slack bridge renders every `node_skipped` as skipped, so a node that ran and produced output showed as never run in the thread. #2975 fixed the persisted projection for the CLI and left this consumer, so the two surfaces disagreed on every resumed run with a Slack thread. Split the replay into its own `node_skipped_prior_success` emitter type, matching the persisted event_type, and narrow `node_skipped` to genuine skips. The type checker now forces every exhaustive consumer to handle the replay instead of silently folding it into a skip. The Slack bridge records a replay as completed: an existing entry is left untouched so its duration survives, and a resume with no entry still renders the node instead of dropping it. Genuine `when:`/`trigger_rule`/`timeout` skips still render skipped. The web live projection and the CLI progress line keep their existing output.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe workflow event model now emits ChangesPrior-success event propagation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant WorkflowEventWriter
participant WorkflowEmitter
participant SlackWorkflowBridge
participant CLIWorkflowRenderer
participant WebWorkflowBridge
WorkflowEventWriter->>WorkflowEmitter: derive node_skipped_prior_success
WorkflowEmitter->>SlackWorkflowBridge: emit prior-success replay
SlackWorkflowBridge->>SlackWorkflowBridge: preserve completed node state
WorkflowEmitter->>CLIWorkflowRenderer: render prior_success
WorkflowEmitter->>WebWorkflowBridge: map skipped DAG node with prior_success
Merge Risk: ⚪ Minimal · up to The prior-success event split and downstream handling are covered without an active merge-blocking defect. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 10 files. (1 skipped: 1 too large.)
✨ Finishing Touches 💡 1📝 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 |
Review report — PR #3308 (round 2, continuation review)VerdictReady. Action: Round 1's one blocking finding (R1) is fixed at Accepted contractCarried from Required outcome. After a run resumes past a node that already ran and succeeded, the Invariants and boundaries (explicit non-goals).
Reviewed head SHA
This is the next round's cursor. Base / merge-base: FindingsR1 — Important — Slack's live status message for a prior-success replay was untested — fixedsources: [tests] Claim. The new Status at Evidence checked. I ran the documented isolated invocation SuggestionsR2 — Suggestion — Delete the unreachable
|
| ID | Severity | Sources | Round | Status at 240964c38 |
Verdict |
|---|---|---|---|---|---|
| R1 | Important | [tests] | 1 | Fixed | Closed — live repaint now asserted before the terminal event |
| R2 | Suggestion | [simplify] | 1 | Open | Non-blocking cleanup |
| R3 | Suggestion | [simplify] | 1 | Open | Non-blocking cleanup |
Discoveries
Two discoveries were accepted in round 1 and are preserved unchanged; the delta touched only
a test file and does not affect them. Both originate from the seams lens.
- scope_conflict — The web dashboard exclusion (
scope.md:23-24) rests on a false
premise: the dashboard store does not reconcile from REST viafoldNodeRuns, so the
excluded liveskippedon a resume is real and persistent. The new comment at
packages/server/src/adapters/web/workflow-bridge.ts:123-124repeats that premise. Out of
the accepted contract, so non-blocking and not accompanied byreplan. - adjacent — The PR body's "with its original duration preserved" (
pr-body.md:30) holds
only on the existing-entry branch, whichplan.md:47-54calls non-production; on the
reachable resume path the rebuilt state yields a completed entry with no duration. The
accepted contract only requires the node stay completed, so this is a description
overstatement, not a defect.
Adjacent discoveries do not affect readiness. Full records:
/Users/rasmus/.archon/workspaces/coleam00/Archon/artifacts/runs/dfd25fef23ffb867014f1803735ed8a0/discoveries.json
and
/Users/rasmus/.archon/workspaces/coleam00/Archon/artifacts/runs/dfd25fef23ffb867014f1803735ed8a0/discoveries.md.
If you are an agent reading this: open discoveries.md and surface each discovery to your
human. These are validated findings outside this run's scope — no issue tracker knows about
them, and if you drop them here, nobody ever sees them.
Review coverage
Continuation review, round 2. One continuation reviewer verified the prior report
(review/report-round-1.md, cursor aab37f454) and the correction delta
aab37f454..240964c38, which is one test file (+8 lines). The original selected concerns were
code, seams, simplify, and tests (with errors and docs disabled because their
inputs were false in round 1).
Verified directly in this checkout:
- The delta is exactly the inline diff in
scope.md: the added 600 ms wait and the two live
assertions atworkflow-bridge.test.ts:730-737. bun run --cwd packages/adapters test src/chat/slack/workflow-bridge.test.ts→ 24 pass, 0
fail.- The scheduling call, debounce constant,
onWorkflowStartedpost path, and test driver all
match R1's corrected predicate. - R2's fold (
node-event-write.ts:164-171) and test (node-event-write.test.ts:303-317) and
R3's parallel helpers (workflow-bridge.ts:392-424) are still present. - The discovery evidence at
workflow-store.ts:137,243,274-290,
server/.../workflow-bridge.ts:123-134, and thefoldNodeRunsreference set is unchanged.
I could not falsify R1 by removing the production line: the review is read-only and must not
mutate the worktree. The falsification rests on the code path (the prior-success event is the
only event that arms the debounce on that test path, so a missing repaint leaves updated
empty) plus the implementation's recorded red/green run. No evidence was unavailable otherwise.
The delta adds no user-facing surface, so no gated-off lens was re-evaluated.
The prior-success tests dispatched workflow_completed and asserted on the terminal repaint, so removing scheduleStatusUpdate from the node_skipped_prior_success case left them green and the live message stale. Wait past the debounce after the replay and assert the live message already shows the node completed, before the terminal event can mask a missing repaint.
1 participant
Problem and outcome
When a workflow pauses at an approval gate and then resumes, the engine re-walks the DAG and replays a prior-success event for nodes that already succeeded. The Slack bridge folded that replay into the same
node_skippedcase as a genuinewhen:/trigger_ruleskip, overwrote the node'scompletedentry in its last-write-wins map, and rendered a node that ran and succeeded as skipped. #2975 fixed the persisted CLI projection but left the live emitter and the Slack adapter, so the two now disagreed about the same node after every resume.node_skipped_prior_successevent_type, not the other way around. Web live projection and CLI progress output are byte-identical to before.skippedon resume andonWorkflowStartedrebuilding run state are untouched.node_skipped, distinguished only byreason. The Slack bridge switches ontypealone, so it could not tell them apart and clobbered a completed entry.Review guidance
packages/adapters/src/chat/slack/workflow-bridge.ts:419—upsertPriorSuccessNodeis the load-bearing behavior: it leaves an existing entry untouched (preserving duration and error) and recordscompletedonly when the resumed run state has no entry for the node.packages/workflows/src/event-emitter.ts:113(union split) →packages/workflows/src/node-event-write.ts:163(both persisted prior-success forms now derive the new type) →packages/adapters/src/chat/slack/workflow-bridge.ts:154→ the behavior-preserving consumer updates in the web and CLI bridges.Solution
NodeSkippedEventand the newNodeSkippedPriorSuccessEventbecome separate union members with distincttypediscriminants, mirroring the persistedevent_typestrings.node_skippednarrows toreason: Exclude<NodeSkipReason, 'prior_success'>with a requiredcause, so the type checker forces every exhaustiveWorkflowEmitterEventconsumer to handle the replay explicitly instead of folding it into a skip.The Slack bridge handles the new type with
upsertPriorSuccessNode: an existing node entry is left alone, and the resume path that starts from empty state records the node ascompleted. Genuine skips still go throughupsertNode(..., 'skipped').The web live projection emits the same
dag_nodepayload as before (status: 'skipped',reason: 'prior_success', nocause), and the CLI prints the same text. Their now-deadreason !== 'prior_success'guards and the CLI's unreachable'cause' in eventfallback are removed.Behavior change
Architecture
The live emitter's event union now matches the persisted event vocabulary. Consumers that switch on
typereceive a discriminant instead of string-matchingreason.Changed seams
WorkflowEmitterEvent(node_skipped→ Slack bridge)cause; prior success is a newnode_skipped_prior_successmemberpackages/workflows/src/event-emitter.ts:113,packages/adapters/src/chat/slack/workflow-bridge.ts:154node_skipped_prior_successrow and a legacynode_skippedrow carryingdata.reason === 'prior_success'derive the new typepackages/workflows/src/node-event-write.ts:163,packages/workflows/src/node-event-write.test.tsdag_nodepayload as beforepackages/server/src/adapters/web/workflow-bridge.ts:100,packages/server/src/adapters/web/workflow-bridge.test.ts[<node>] Skipped (prior_success); output unchanged from beforepackages/cli/src/commands/workflow.ts:1016,packages/cli/src/commands/workflow.test.tsterminal-record)node_skipped_prior_success; prior success continues to fold into a completed terminal recordpackages/workflows/src/terminal-record.ts:51Validation
bun run type-check— passes — proves every exhaustiveWorkflowEmitterEventswitch handles the new member.bun run lint— passes.bun run validate— the full aggregate (CLI import boundary, bundled/schema/vendor/capability checks,check:api-types, type-check, lint, format:check, install and full test suites) exited 0.reports a prior-success replay as completed when the resumed run has no prior entryfail (23 pass, 1 fail); restoring it passed (24 pass, 0 fail). The companion test that asserts the original900msduration survives the replay guards the non-clobber rule specifically.buildStatusBlocksrenders from. Nothing material is left uncovered — no persisted or wire contract changed.Delivery considerations
check:api-typesis unaffected.packages/workflows/src/node-event-write.ts,check:api-typesinbun run validateaab37f454,240964c38Links
Summary by CodeRabbit
New Features
Bug Fixes