KeeperHub / KeeperHub/keeperhub

Fully legacy nested For Each still adopts the enclosing loop's Collect after #2183

Open
#2,194 0 comments 0 reactions 0 assignees View on GitHub
confirmed needs-discussion
Dominant language
TypeScript
Stars
24
Forks
93
Avg merge
1d 4h
Merged PRs (30d)
253

Description

Split out of #2157 during review of #2183. Credit to @Dami904, whose ordering analysis on #2157 is what exposed this shape.

# Reason: reproduction

A fully legacy nested pair - no `loop` or `done` sourceHandles anywhere - where the inner loop's body reaches a Collect that is meant to be the outer loop's:

```
nodes: fe-outer (For Each), fe-inner (For Each), step, shared (Collect)
edges: fe-outer -> fe-inner, fe-inner -> step, step -> shared
```

Run `identifyLoopBody("fe-outer", ...)` and `identifyLoopBody("fe-inner", ...)` from `lib/workflow/executor/executor.workflow.ts`, then the start-up ownership pass in `executeWorkflow`.

# Reason: what happened

`fe-outer` resolves no Collect at all, and `fe-inner` resolves `shared` as its own `collectNodeId`. The ownership pass added in #2183 claims nothing for `fe-outer`, so `fe-inner`'s claim of `shared` raises no conflict and the run proceeds with the aggregate attributed to the inner loop.

Mechanism, against `staging` at `119a4303` (line numbers move once #2183 lands; the functions do not):

- With no handles, `nextBodyTargets` falls through to the global edge map (`:1572`), so the outer scan descends into `fe-inner`'s body.
- `computeNextDepth` increments on the nested For Each, so by the time the outer scan reaches `shared` it is at depth 1, not 0. The depth-0 branch that records `collectNodeId` (`:1654-1663`) never fires for it, and there is no done-handle target to resolve at `:1704-1717` either. The outer ends with `collectNodeId` and `doneCollectNodeId` both undefined.
- The inner scan starts at depth 0 and reaches `shared` at depth 0, so it records it.

# Reason: what you expected, and what told you to expect it

Same outcome as the handle-aware and mixed topologies #2183 now rejects: two loops cannot resolve the same Collect, and a graph where a nested body crosses into the enclosing loop's Collect fails at start-up naming both loops. #2183's start-up pass is the source; it establishes that invariant for every shape where the outer loop resolves the contested Collect, and this is the one shape where it does not resolve it at all.

# Reason: what it costs

Identical to the silent case in #2157: the aggregate fires under the inner loop's scope once per outer iteration, the logged execution rows attribute to the wrong loop, and any outer-body node reading `shared` sees the inner loop's results. Green run, wrong data, and now inconsistent with the sibling topologies that fail loudly.

# Where you saw it

Static read of `staging`, confirmed by a transliterated run of `identifyLoopBody` against the graph above during the #2183 review. No production report.

# Scope: what this covers, and what it does not

Covers only the no-handle legacy nesting shape. Handle-aware nesting, mixed nesting and sibling loops sharing a Collect are all caught by #2183 and are not in scope here.

Not verified: whether any workflow on the platform is actually wired this way. The canvas has emitted sourceHandles on For Each edges since the handle-aware mode landed, so this shape is reachable through import, the API and MCP rather than by hand.

# Plan: what should happen next

Open. The obvious move - have the outer scan claim a Collect it reaches at any depth - is wrong as stated, because a nested loop with its own Collect legitimately sits at depth 1 inside the outer's body and must not be claimed by the outer. The claim has to distinguish "a Collect that closes a nested loop" from "a Collect the nested loop crossed into", and in legacy mode the only signal for that is the depth bookkeeping. Whether that is enough, or whether legacy nesting should instead be rejected at validation time, is the decision. Marking `confirmed`; not `accepted` until the plan is settled.

Contributor guide

Open the contributing guide

Research direction

Start in lib/workflow/executor/executor.workflow.ts by reading identifyLoopBody and the start-up ownership pass in executeWorkflow. Reproduce the fully legacy nested graph and trace nextBodyTargets and computeNextDepth for both loops. Done means the chosen validation or ownership behavior consistently prevents the inner loop from adopting the enclosing loop's Collect, without breaking the sibling topologies covered by #2183.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.