KeeperHub / KeeperHub/keeperhub

Nested-loop boundaries are re-derived independently per call instead of resolved once per workflow

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

Description

Architectural follow-up from #2049 / PR #2065 and #2157. Not urgent, not blocking anything currently merged -- filed to track the decision deliberately rather than let it drop.

## Reason

There's no single bug reproduction here -- this is a "we cannot currently do X safely" issue, per the change-not-bug shape in ISSUES.md, so the reason is the task and the workaround cost.

**The task:** every `For Each` loop in a workflow, at every nesting level, independently calls `identifyLoopBody` to derive its own body boundaries -- seed nodes, Collect node, done-handle targets -- from scratch, at execution time, with zero knowledge of any sibling or ancestor loop in the same workflow. That pattern has now produced two separate, real bugs from the same root shape:

- #2049: a nested loop's recursive call was handed the wrong edge map (the enclosing loop's partial one instead of the workflow-global one), so the inner scan silently found nothing past its seed.
- #2157: a nested loop's scan, now correctly using the global map, has no way to tell "a Collect that belongs to me" from "a Collect that belongs to my ancestor," because nothing tells it where its ancestor's scope ends.

Both were fixed (#2049) or proposed to be fixed (#2157) as targeted patches on the specific symptom. Neither one closes the pattern itself. The cost of leaving the pattern in place is that it can produce a *third*, differently-shaped bug later that neither patch's specific check would catch, and each one currently requires the same kind of careful, line-by-line tracing through `executor.workflow.ts` that #2049's original report and #2157 both required to even find.

## Scope

**Covered:** the loop-boundary-resolution responsibility currently split across every recursive `identifyLoopBody` call inside `handleForEachExecution`, for both the top-level and nested call sites.

**Not covered:** this issue does not itself fix #2157's specific Collect-ownership gap -- it proposes to make that whole category of gap structurally harder to introduce, which is a different (larger) piece of work. #2157 can and should ship independently regardless of what happens here: it is correct and complete on its own if this issue is never picked up.

**One issue or two:** per the "can piece A ship without piece B" test in ISSUES.md -- yes in both directions. #2157's validation check is correct and useful whether or not this refactor ever happens; this refactor, if it happens, makes #2157's specific check unnecessary as a separate bolt-on (the ownership conflict falls out of the single resolution pass naturally) but doesn't depend on #2157 existing first. Filed separately on purpose.

## Plan

Compute the entire nested-loop structure once per workflow, up front, instead of at execution time per-loop:

1. In one pass over every `For Each` node, processed outer-to-inner, build a single `Map` covering every loop in the workflow, with each loop's resolution having full visibility of every loop already resolved before it.
2. Collect-ownership conflicts (#2157) are caught naturally at this single computation site -- no separate cross-check needed.
3. `handleForEachExecution`, at both the top-level and nested call sites, looks up its precomputed `LoopBodyInfo` instead of calling `identifyLoopBody` itself. There is no per-call re-scan left to hand the wrong map or the wrong scope to.

**Why I think this is worth its own issue rather than folding into #2157:** it touches the loop-execution path used by every `For Each` workflow on the platform, not only nested ones, so it needs a wider regression pass (single loop, 2-level, 3-level, done-handle-only, in-body-Collect-only) than a validation-only addition does. It also introduces a new correctness requirement of its own -- loops must be resolved strictly outer-before-inner -- that doesn't exist in the current code and would need its own careful testing to get right.

Not committing to this as the final shape -- open to triage replacing the plan, per policy. If there's a reason to believe this is not worth the churn relative to #2157's cheaper fix alone, that's a legitimate answer too; filing this mainly so that judgment call gets made on purpose rather than by default.

Contributor guide

Open the contributing guide

Research direction

Read ISSUES.md and trace identifyLoopBody and handleForEachExecution in executor.workflow.ts, including both top-level and nested call sites. Review #2049, #2157, and PR #2065 for the existing boundary behavior. Done means one outer-before-inner resolution pass supplies every loop execution path and regression coverage addresses single, nested, done-handle-only, and in-body-Collect cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.