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 @coderabbitai help to get the list of available commands and usage tips.}

🔍 Comprehensive PR Review

PR: #1923 — fix(workflows): interactive loop resume uses fresh session on first iteration
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-06-08

Summary

This is a surgical, correct fix for issue #1208. The one-line addition to needsFreshSession in dag-executor.ts precisely prevents a stale/expired session ID from being passed to the SDK on the first resumed iteration of an interactive loop — eliminating the error_during_execution crash. All five agents returned APPROVE. No critical or high issues found. The fix introduces no new error paths, no silent fallbacks, no type violations, and no scope creep.

Verdict: ✅ APPROVE

🟢 Low Issues (All Optional)

LOW-1: currentSessionId initialized to stale value, then immediately discarded

📍 packages/workflows/src/dag-executor.ts:1994

currentSessionId is initialized to loopGateMeta.sessionId (the stale ID) on line 1994, but the fix correctly forces resumeSessionId = undefined for the first resumed iteration, so the stale value never reaches the SDK. After the first iteration, currentSessionId is overwritten at line 2142. Harmless; the 6-line comment explains the invariant. Recommendation: leave as-is.

Reported by: code-review + error-handling

LOW-2: Test event-filter cast doesn't guard against undefined argument

📍 packages/workflows/src/dag-executor.test.ts (new regression test)

The filter uses (call[0] as Record<string, unknown>).event_type — if call[0] were ever undefined, this would throw inside filter() obscuring the test failure. Not possible today with this mock. Recommendation: leave as-is.

Reported by: error-handling

LOW-3: No test for session threading on iteration startIteration+1 of a resume

📍 packages/workflows/src/dag-executor.test.ts:4958

Both new tests complete in one resumed iteration. The threading mechanism at line 2142 is unchanged and covered by the existing "fresh_context: false threads session" test (line 4411). Recommendation: leave as-is; optional follow-up.

it('interactive loop resume threads fresh session into iterations after startIteration', async () => {
  let callCount = 0;
  mockSendQueryDag.mockImplementation(function* () {
    callCount++;
    if (callCount >= 2) {
      yield { type: 'assistant', content: 'Done. <promise>APPROVED</promise>' };
    } else {
      yield { type: 'assistant', content: 'Still working.' };
    }
    yield { type: 'result', sessionId: `fresh-session-${String(callCount)}` };
  });
  // ... assert calls[0][2] === undefined, calls[1][2] === 'fresh-session-1'
});

Reported by: code-review + test-coverage

LOW-4: Production comment names "Claude SDK" specifically, but fix is provider-agnostic

📍 packages/workflows/src/dag-executor.ts:2053-2057

The comment says "may reference a Claude SDK session that expired." The needsFreshSession guard applies to all providers that return a sessionId. A future maintainer adding a new provider with session resume could read "Claude SDK" and conclude the guard doesn't apply to them.

Suggested one-line reword (optional, trivial):

// Session threading. Force a fresh session on the first iteration of an
// interactive loop resume: the stored session id from the gate metadata
// may be stale after a human review wait — passing it to the SDK can
// trigger errors (e.g. error_during_execution on Claude SDK).
// User feedback is carried via $LOOP_USER_INPUT, so session continuity is
// not required for the first resumed iteration.

Reported by: comment-quality

LOW-5: loop-nodes.md understates the freshness guarantee for interactive loop resumes

📍 packages/docs-web/src/content/docs/guides/loop-nodes.md:135

Line 135 says "The first iteration is always fresh regardless of this setting." After this PR the guarantee also covers the first iteration after an interactive loop resume — even if its numeric index is 2+. A user designing a fresh_context: false interactive loop expecting full session continuity across an approval gate will be surprised.

Suggested one-sentence addition (optional):

The first iteration is always fresh regardless of this setting. The first
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.

Reported by: docs-impact

✅ What's Good

• Surgical fix: Exactly one boolean subexpression added to needsFreshSession. Nothing else touched.
• Comment quality: The 6-line block explains the expiry failure mode, why session continuity is safe to skip, and links to fix: interactive loop resume crashes with error_during_execution (stale session) #1208 — exactly the kind of non-obvious invariant that warrants a comment.
• Fail-loud path preserved: The if (msg.isError) throw block from PR fix(workflows): fail loudly on SDK isError results in DAG and loop nodes #1291 is untouched — the guard that made fix: interactive loop resume crashes with error_during_execution (stale session) #1208 visible instead of silent remains intact.
• Test regression anchor: New test uses a clearly named stale ID ('stale-session-from-hours-ago') and checks for zero failure events — tests observable behavior, not internal state.
• Issue anchoring: #1208 appears in three comment sites across the test file.
• CLAUDE.md compliance: No any types, no new mock.module() calls, no scope creep, no infinite-retry risk, no silent swallowing.

📋 Suggested Follow-up Issues (if not fixing in this PR)

Next Steps

1. Optional: Apply LOW-4 comment broadening (trivial one-liner — no block on merge)
2. Optional: Apply LOW-5 docs update (one sentence in loop-nodes.md)
3. Nothing blocks merge — mark ready for review when comfortable

Reviewed by Archon prp-core:prp-review-agents workflow

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-archon-fix-github-issue-1780923940901
Commit: aea5f8f
Philosophy: Fix everything unless clearly a new concern

Fixes Applied (2 total)

Tests Added

(none)

Skipped (3)

All three were explicitly recommended as "leave as-is" or "skip" by the review agents:

Suggested Follow-up Issues

(none)

Validation

✅ Type check | ✅ Lint | ✅ Tests (283 passed)

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-archon-fix-github-issue-1780923940901