feat(workflows): expose $LOOP_PREV_OUTPUT in loop node prompts (#1286)#1367
feat(workflows): expose $LOOP_PREV_OUTPUT in loop node prompts (#1286)#1367Wirasm merged 4 commits intocoleam00:devfrom
Conversation
…m00#1286) Adds a new substitution variable that carries the previous loop iteration's cleaned output into the next iteration's prompt. Empty on iteration 1; the prior iteration's output (after stripCompletionTags) on iteration 2+. Why: fresh_context: true loops have no way to reference what the previous pass produced or why it failed without dragging the full session forward. $LOOP_PREV_OUTPUT closes that gap with zero session-cost — same trust boundary as $nodeId.output, no new external surface. Changes: - packages/workflows/src/executor-shared.ts: substituteWorkflowVariables accepts a 10th positional loopPrevOutput arg and substitutes $LOOP_PREV_OUTPUT (defaults to ''). - packages/workflows/src/dag-executor.ts: executeLoopNode passes lastIterationOutput on iteration 2+ (and explicit '' on iteration 1 / the first iteration of an interactive resume, since lastIterationOutput is a per-call variable that does not survive resume metadata). - Unit tests: 3 new cases in executor-shared.test.ts. - Integration tests: 2 new cases in dag-executor.test.ts verifying the prompt sent to the AI on iter 1 vs iter 2, and that the value reflects cleaned output (no <promise> tags). - Docs: variables.md, loop-nodes.md (new "Retry-on-failure" pattern), CLAUDE.md variable reference. Backward compatibility: prompts that don't reference $LOOP_PREV_OUTPUT are unaffected. All 843 workflow tests + type-check + lint + format:check + bun run validate pass locally.
📝 WalkthroughWalkthroughAdded a loop-only workflow variable Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant LoopExecutor
participant Substituter
participant Model
end
LoopExecutor->>Substituter: substituteWorkflowVariables(promptTemplate, ..., rejectionReason?, loopPrevOutput)
Substituter-->>LoopExecutor: promptWithLoopPrevOutput
LoopExecutor->>Model: sendQuery(promptWithLoopPrevOutput)
Model-->>LoopExecutor: assistantOutput
LoopExecutor->>Substituter: clean assistantOutput (strip <promise>) -> lastIterationOutput
Note right of LoopExecutor: Next iteration uses lastIterationOutput as loopPrevOutput ('' on first/resumed iteration)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workflows/src/dag-executor.ts (1)
1769-1784:⚠️ Potential issue | 🟠 MajorCompute
$LOOP_PREV_OUTPUTfrom the fully stripped iteration output.Line 1985 can fall back to raw
fullOutputwhencleanOutputis empty, which reintroduces<promise>...</promise>content into the next prompt even though$LOOP_PREV_OUTPUTis documented as cleaned. Strip the accumulated output once after the iteration and use that value.🐛 Proposed fix
- lastIterationOutput = cleanOutput || fullOutput; + lastIterationOutput = stripCompletionTags(fullOutput, loop.until);Based on learnings, Variable substitution supports
$LOOP_PREV_OUTPUTas the cleaned output of the previous loop iteration, empty on the first iteration.Also applies to: 1985-1985
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 1769 - 1784, The code is passing raw fullOutput into substituteWorkflowVariables as lastIterationOutput when cleanOutput is empty, which reintroduces <promise>…</promise> fragments; compute a single stripped/cleaned iteration output (e.g., derive loopPrevOutput by preferring cleanOutput but always applying the final strip/cleanup to the accumulated output once after each iteration) and store it in lastIterationOutput (or a new variable) and pass that sanitized value into substituteWorkflowVariables (the call in dag-executor.ts using loop.prompt and lastIterationOutput) so $LOOP_PREV_OUTPUT always receives the fully stripped cleaned output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/guides/loop-nodes.md`:
- Around line 207-209: The paragraph incorrectly states that all iterations 2+
receive the previous iteration's cleaned output; update the docs to document the
interactive-resume exception: explain that when the executor resumes
interactively it passes an empty string for $LOOP_PREV_OUTPUT if i ===
startIteration, so the very first resumed iteration (even if its numeric index
is >=2) receives '' rather than the previous cleaned output; reference the
executor behavior and the startIteration check ($LOOP_PREV_OUTPUT, executor, i
=== startIteration, startIteration) so readers understand this exception.
In `@packages/docs-web/src/content/docs/reference/variables.md`:
- Line 30: You added the new variable $LOOP_PREV_OUTPUT but didn’t update the
substitution-order list and the availability table; add $LOOP_PREV_OUTPUT to the
substitution-order list (in the Variable substitution section) and to the
availability table alongside other loop variables, documenting it as "Cleaned
output of the previous loop iteration (loop nodes only) — empty string on the
first iteration; useful for fresh_context: true loops that need the prior pass
without full session history." Ensure the exact variable name $LOOP_PREV_OUTPUT
is used in both places so the reference sections stay in sync.
---
Outside diff comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1769-1784: The code is passing raw fullOutput into
substituteWorkflowVariables as lastIterationOutput when cleanOutput is empty,
which reintroduces <promise>…</promise> fragments; compute a single
stripped/cleaned iteration output (e.g., derive loopPrevOutput by preferring
cleanOutput but always applying the final strip/cleanup to the accumulated
output once after each iteration) and store it in lastIterationOutput (or a new
variable) and pass that sanitized value into substituteWorkflowVariables (the
call in dag-executor.ts using loop.prompt and lastIterationOutput) so
$LOOP_PREV_OUTPUT always receives the fully stripped cleaned output.
🪄 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: 791124e5-4828-4efa-b3ac-782dd479b704
📒 Files selected for processing (7)
CLAUDE.mdpackages/docs-web/src/content/docs/guides/loop-nodes.mdpackages/docs-web/src/content/docs/reference/variables.mdpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.test.tspackages/workflows/src/executor-shared.ts
- variables.md: include $LOOP_PREV_OUTPUT in substitution-order list and availability table to match the new variable row at line 30 - loop-nodes.md: document the interactive-resume exception where the first iteration after an approval-gate resume still receives an empty $LOOP_PREV_OUTPUT regardless of iteration number (per dag-executor.ts L1781-1783 where i === startIteration always clears prev output)
|
Thanks for the review. Addressed both doc nits in 4d0ae8b:
|
|
Hi @voidborne-d — thanks for opening this PR. This repository uses a PR template at
Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly. If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank. |
|
Thanks for the prompt @Wirasm — filled out all seven sections per the template (UX Journey, Architecture Diagram, Label Snapshot, Change Metadata, Security Impact, Rollback Plan, Risks and Mitigations). One DoD item is still unchecked and called out under Risks: |
|
Hi @Wirasm — thanks for the pass. I think the comment may have landed against a stale PR-body snapshot: all seven flagged sections are filled in the description right now. Quick anchors so you can scroll-verify without re-reading the whole body:
If any of those still reads as too thin or non-load-bearing, happy to expand whichever specifically — just call out which. |
Review SummaryVerdict: minor-fixes-needed This PR adds the Blocking issuesNone. Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, test-coverage, comment-quality, docs-impact. |
|
Thanks for the review @Wirasm 🔹
Will ping again once the test commit lands. |
…OUTPUT (coleam00#1367 review) Per maintainer-review-pr suggestion (Wirasm): two-call integration test covering the resume-from-approval scenario. - Call 1: fresh interactive loop pauses at the gate after iteration 1 and asserts $LOOP_PREV_OUTPUT substitutes to empty on iter 1 (no prior output) plus the gate pause is recorded. - Call 2: resumed run with metadata.approval populated. The first resumed iteration must substitute $LOOP_PREV_OUTPUT to '', NOT to the paused run's iter-1 output (which lived in a different process and is not persisted). $LOOP_USER_INPUT still flows through as normal. Locks the documented invariant at dag-executor.ts:1769-1772.
|
@Wirasm thanks for the structured review — both suggested fixes addressed. PR head now 1. CHANGELOG 2. Resume-from-approval integration test — added in 88a84c7 ( Wrote the two-call shape exactly as you outlined:
Locks the documented invariant at Local gates green:
(3) — frontmatter description: skipping, since you flagged it as optional and the in-body coverage already names |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.test.ts (1)
3203-3260: Tag‑stripping assertion is solid; consider also asserting iter‑2 doesn't contain the inner signal.
expect(promptIter2).not.toContain('<promise>')proves the angle‑bracketed tags are removed, but a future regression could strip only the wrapper tags while leaving the innerNOT_DONE_YETtext inlastIterationOutput. An extraexpect(promptIter2).not.toContain('NOT_DONE_YET')would fully lock in “cleaned viastripCompletionTags”.♻️ Optional tightening
expect(promptIter2).toContain('PREV=[Real work output.'); expect(promptIter2).not.toContain('<promise>'); + // Tag *contents* must also be gone — guards against a regression that + // strips only the wrapper tags. + expect(promptIter2).not.toContain('NOT_DONE_YET');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 3203 - 3260, Add a second negative assertion to ensure the inner signal text is removed as well: after the existing checks on mockSendQueryDag and promptIter2 in the test that calls executeDagWorkflow, assert that promptIter2 does not contain the inner token (e.g., expect(promptIter2).not.toContain('NOT_DONE_YET')) so the test verifies stripCompletionTags removes both wrapper tags and their inner content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 3203-3260: Add a second negative assertion to ensure the inner
signal text is removed as well: after the existing checks on mockSendQueryDag
and promptIter2 in the test that calls executeDagWorkflow, assert that
promptIter2 does not contain the inner token (e.g.,
expect(promptIter2).not.toContain('NOT_DONE_YET')) so the test verifies
stripCompletionTags removes both wrapper tags and their inner content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10f93f8a-c9e6-4b6e-b87e-156f72e471b4
📒 Files selected for processing (1)
packages/workflows/src/dag-executor.test.ts
2 participants
Summary
loop:nodes computelastIterationOutputat the end of each iteration but immediately discard it — the prompt-substitution layer never sees it. Withfresh_context: truethe next iteration starts blind, so retry-on-failure / generate-then-review patterns can't see what the previous pass produced.fresh_context: falsedefeats the point) or abusinguntil_bash/$ARTIFACTS_DIRshuffling as a data channel.$LOOP_PREV_OUTPUT— empty string on iteration 1, previous iteration's cleaned output (afterstripCompletionTags) on iteration 2+.substituteWorkflowVariablesaccepts a 10th positionalloopPrevOutput?: string;executeLoopNodepasseslastIterationOutput. Closes feat: $LOOP_PREV_OUTPUT variable for loop nodes #1286.LoopNodeYAML. Prompts that don't reference$LOOP_PREV_OUTPUTare bit-for-bit unaffected — the substitution regex is a literal no-op when the token is absent. The DoD itemarchon-compose-plan-implement-qa.yamlwas skipped (file does not exist ondev); flagged below for maintainer direction.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
lastIterationOutputas 10th arg$LOOP_PREV_OUTPUTtokenvariables.md,loop-nodes.md,CLAUDE.md)$LOOP_USER_INPUT/$REJECTION_REASONLoopNodeschemaLabel Snapshot
risk: lowsize: Mworkflowsworkflows:executorChange Metadata
featureworkflowsLinked Issue
substituteWorkflowVariablesextension pattern as$REJECTION_REASON)Validation Evidence (required)
''and iter 2 sees iter 1's verbatim output, (e)<promise>tags are stripped (uses the samecleanOutputlastIterationOutputalready stores).Security Impact (required)
NoNoNoNo$LOOP_PREV_OUTPUTcarries AI-generated text from the same workflow run — same trust boundary as$nodeId.output. No new external surface.Compatibility / Migration
Yes— new parameter is optional with''default; existing prompts without the token are no-ops.NoNoHuman Verification (required)
executeLoopNodeto confirm iter-1 vs iter-2+ values; verified thecleanOutputinvariant (the same valuelastIterationOutputstores is the value substituted).lastIterationOutput, so$LOOP_PREV_OUTPUTis''there as well (consistent with iter 1 behaviour, documented inloop-nodes.md); prompts without the token (regex no-op);<promise>/ completion tag stripping.archon-compose-*workflow — the DoD-referenced YAML doesn't exist ondev. See "Risks" below.Side Effects / Blast Radius (required)
loop:node execution only.lastIterationOutputblobs would now flow into the next iteration's prompt for users who reference$LOOP_PREV_OUTPUT. Same concern as$nodeId.output; mitigated by the existingcleanOutputstep.Rollback Plan (required)
git revert <merge-commit>— change is additive (new optional parameter + new token); revert is clean.$LOOP_PREV_OUTPUTwould see literal$LOOP_PREV_OUTPUTin the rendered prompt. Detect by greping prompt logs.Risks and Mitigations
substituteWorkflowVariablesnow takes 10 positional params; future additions risk argument-order bugs.$LOOP_USER_INPUT/$REJECTION_REASONextension pattern intentionally for consistency. Happy to refactor to an options object in a follow-up if maintainers prefer; flagged but not in scope here.archon-compose-plan-implement-qa.yaml updatedis unchecked — the referenced file does not exist ondev(find … archon-compose*returns nothing).archon-*.yamldefaults — please advise which.stripCompletionTags), same shape aslastIterationOutput. Users opt in by referencing the token; non-referencing loops pay nothing.Summary by CodeRabbit
New Features
Bug Fixes
Documentation