fix(workflows): restore interactive-loop cross-gate session continuity (reverts #1923)#2046
Conversation
Reverts the change from #1923, which forced a fresh Claude session on the first iteration after every interactive-loop approval gate by adding `(isLoopResume && i === startIteration)` to `needsFreshSession` in dag-executor.ts. That broke cross-gate conversation continuity: every turn after a human gate began a context-free session carrying only $LOOP_USER_INPUT (with $LOOP_PREV_OUTPUT empty), making multi-turn interactive loops (interviews, iterative refinement) impossible. Restores the released behavior `needsFreshSession = loop.fresh_context || i === 1`, reinstates the test assertion guarding cross-gate continuity (sessionArg === 'loop-session-1'), and removes #1923's regression test that encoded the fresh-session behavior. #1291's fail-loud isError handling is left untouched. Confirming and fixing the original #1208 crash is separate work. Closes #2004
📝 WalkthroughWalkthroughThe loop executor now reuses the stored gate session ID on the first resumed interactive iteration unless ChangesInteractive loop resume session reuse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/docs-web/src/content/docs/guides/loop-nodes.md`:
- Line 135: Clarify the wording in the loop-nodes guide so “first iteration” is
explicitly the loop’s initial iteration, not the first run after a resume.
Update the relevant sentence in the docs content to reference the initial loop
iteration (for example, by naming the first pass or `i === 1`) so readers don’t
misinterpret the behavior of resumed interactive loops.
🪄 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
Run ID: 16eed41d-8341-4092-824f-230cbf38d376
📒 Files selected for processing (3)
packages/docs-web/src/content/docs/guides/loop-nodes.mdpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
| iteration executed after resuming from an interactive loop approval gate is | ||
| also always fresh — the stored session id may have expired during the human | ||
| review wait, and user feedback is carried via `$LOOP_USER_INPUT` instead. | ||
| The first iteration is always fresh regardless of this setting. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that this means the loop’s initial iteration.
“First iteration” is a bit ambiguous now that resumed interactive loops reuse the stored session. Calling out the initial loop iteration (or i === 1) would avoid readers interpreting this as “first iteration after resume.”
Suggested wording
-The first iteration is always fresh regardless of this setting.
+The loop's initial iteration (`i === 1`) is always fresh regardless of this setting.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The first iteration is always fresh regardless of this setting. | |
| The loop's initial iteration (`i === 1`) is always fresh regardless of this setting. |
🤖 Prompt for 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.
In `@packages/docs-web/src/content/docs/guides/loop-nodes.md` at line 135, Clarify
the wording in the loop-nodes guide so “first iteration” is explicitly the
loop’s initial iteration, not the first run after a resume. Update the relevant
sentence in the docs content to reference the initial loop iteration (for
example, by naming the first pass or `i === 1`) so readers don’t misinterpret
the behavior of resumed interactive loops.
Summary
(isLoopResume && i === startIteration)term fromneedsFreshSessionindag-executor.ts, restoringloop.fresh_context || i === 1, so a resumed iteration re-threads the stored gate session id instead of forcingundefined. Restores the resume test assertion (sessionArg === 'loop-session-1'), drops fix(workflows): interactive loop resume uses fresh session on first iteration #1923's regression test, and reverts theloop-nodes.mdnote.isErrorhandling,fresh_context: trueloops, non-interactive loops, andpersist_sessionloops are untouched. Does not attempt to fix the original fix: interactive loop resume crashes with error_during_execution (stale session) #1208 crash (separate work).Provenance
Takeover of #2005 by @ianstantiate (commit authorship preserved), rebased onto current
devand re-validated. Supersedes #2005. Thanks @ianstantiate.Validation Evidence
Rebased onto current
dev(dag-executor.tshas changed since the original PR — verified the revert targetneedsFreshSessionis intact and the change still correct):The reverted
dag-executor.test.tspasses alongside the resume/resumedtests added by #1842 (no interaction). Net diff: +5/−93 across 3 files.Security Impact
Compatibility / Migration
dev-only/unreleased. No config/env/DB changes.Side Effects / Blast Radius
executeLoopNodeonly — anyinteractive: trueloop that resumes after a gate. Environments that genuinely hit the unconfirmed fix: interactive loop resume crashes with error_during_execution (stale session) #1208error_during_executionwill again attempt a resume on the first post-gate iteration; with fix(workflows): fail loudly on SDK isError results in DAG and loop nodes #1291 in place this now fails loudly with the real SDKerrors[]rather than masking it.Rollback Plan
fresh_context: true.Linked Issues
Closes #2004
Supersedes #2005
Related #1208, #1291, #1923
Summary by CodeRabbit
Bug Fixes
Documentation