feat: add load-time inputs for include nodes - #2467
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis change adds validated ChangesInclude Input Substitution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IncludeNode
participant IncludeExpander
participant RuntimeReferences
IncludeNode->>IncludeExpander: provide with mapping
IncludeExpander->>RuntimeReferences: namespace child output references
IncludeExpander->>IncludeNode: substitute $INPUTS values in cloned fields
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Consolidated Review: PR #2467Date: 2026-08-05T09:30:00Z Executive SummaryThis PR cleanly introduces load-time Overall Verdict: REQUEST_CHANGES Auto-fix Candidates: 1 HIGH issue can be auto-fixed Statistics
CRITICAL Issues (Must Fix)None. HIGH Issues (Should Fix)Issue 1: Included
|
| Option | Approach | Effort | Risk if Skipped |
|---|---|---|---|
| Fix Now | Throw IncludeExpansionError with include/block/command context when safety scanning cannot resolve the command. |
LOW | Unsupported tokens or stale refs can reach execution silently. |
| Create Issue | Preserve compatibility and add a structured load diagnostic later. | LOW | Same behavior remains until follow-up ships. |
| Skip | Keep the warning-only fallback. | NONE | Workflow authors receive no actionable failure. |
Recommendation: Fix now. A safety scan that cannot inspect the input is ambiguous rather than safe, and fail-fast aligns with the newly introduced command-body restriction.
throw new IncludeExpansionError(
`Node '${includeNode.id}': command file '${command}.md' in included block ` +
`'${child.name}' could not be resolved for include safety validation. ` +
'Make the command available, or inline the prompt.'
);Issue 2: Supported substitution surfaces lack behavioral coverage
Source Agent: test-coverage
Location: packages/workflows/src/include-expander.ts:212-231
Problem:
New tests exercise prompt, when, and fenced text, but not the loop, recursive loop_group, approval, bash, script, workflow.input, or cancel branches in applyInputsMacro(). A future change could leave $INPUTS.* unexpanded on one of these executable or structured surfaces without a test failure.
Options:
| Option | Approach | Effort | Risk if Skipped |
|---|---|---|---|
| Fix Now | Add one table-driven public-expander test covering every remaining supported surface. | MEDIUM | Regressions can reach scripts, loop controls, cancellation text, or child inputs. |
| Create Issue | Add focused tests in a follow-up. | LOW | Coverage hole remains temporarily. |
| Skip | Rely on current implementation and prompt-focused tests. | NONE | Broad feature contract is not guarded. |
Recommendation: Fix now. A compact table-driven test is proportionate and keeps the surface-wide contract regression-safe.
Issue 3: Authoring guide promises an unenforced with: value grammar
Source Agent: comment-quality
Location: packages/docs-web/src/content/docs/guides/authoring-workflows.md:892
Problem:
The guide says values are only string literals or $node.output references and that expressions are never allowed. The schema validates identifier keys and that values are strings, then applyInputsMacro() inserts the string verbatim. No grammar check enforces the narrower documentation claim.
Options:
| Option | Approach | Effort | Risk if Skipped |
|---|---|---|---|
| Fix Now | Document the enforced rule: values must be strings and are substituted verbatim at load time. | LOW | Authors are promised validation the product does not provide. |
| Create Issue | Add a deliberate restricted-value grammar in a later language change. | MEDIUM | Documentation remains inaccurate until fixed. |
| Skip | Keep the literal/output-reference claim. | NONE | Normative guidance misstates behavior. |
Recommendation: Fix the documentation now; consider a stricter grammar only as a separately specified feature.
Issue 4: Workflow-language constitution repeats the unsupported restriction
Source Agent: comment-quality
Location: packages/docs-web/src/content/docs/reference/workflow-language-constitution.md:59
Problem:
The normative constitution repeats that only literals or $node.output references are accepted and expressions are rejected, though the shipped validator accepts any string. This finding conflicts with the docs-impact agent's general conclusion that the documentation matches implementation; the concrete schema and macro evidence supports this more specific finding.
Options:
| Option | Approach | Effort | Risk if Skipped |
|---|---|---|---|
| Fix Now | State that identifier-keyed string values are substituted at load time; retain the enforced workflow.with boundary. |
LOW | The language reference remains misleading. |
| Create Issue | Implement the claimed grammar and tests in a dedicated PR. | MEDIUM | Current reference remains inaccurate. |
| Skip | Leave the expression-rejection claim. | NONE | Reviewers and authors infer nonexistent guardrails. |
Recommendation: Fix the wording now, consistently with the authoring guide.
LOW Issues (For Consideration)
None.
Positive Observations
- Child-internal output references are namespaced before caller values are inserted, preserving parent reference scope under node-ID collisions.
- Mapping validation is strongly typed and keeps Phase 2 extra-key validation intentionally deferred.
- Existing tests cover empty and repeated values, missing mappings, nested forwarding, fenced text, output-reference collisions, dangling injected references, and ordinary command-file rejection.
- Error handling is mostly explicit and contextual: missing macros are aggregated, unsafe
when:composition is rejected, and unexpected exceptions are rethrown. - The implementation comments accurately describe ordering and the load-time/runtime command-file boundary.
- No API, database, configuration, command, or broader architectural documentation changes are required.
Suggested Follow-up Issues
| Issue Title | Priority | Related Finding |
|---|---|---|
"Define and enforce a restricted include.with value grammar" |
P2 | MEDIUM issues 3-4, if arbitrary string values are not the intended permanent contract. |
Next Steps
- Auto-fix step should address the 1 HIGH command-surface gap.
- Review the four MEDIUM issues and decide whether to fix now or defer the optional grammar enforcement work.
- Merge after command-file validation and documentation are aligned with the intended Phase 1 contract.
Agent Artifacts
| Agent | Artifact | Findings |
|---|---|---|
| Code Review | code-review-findings.md |
1 |
| Error Handling | error-handling-findings.md |
1 |
| Test Coverage | test-coverage-findings.md |
1 |
| Comment Quality | comment-quality-findings.md |
2 |
| Docs Impact | docs-impact-findings.md |
0 |
Metadata
- Synthesized: 2026-08-05T09:30:00Z
- Artifact:
/Users/rasmus/.archon/workspaces/coleam00/Archon/artifacts/runs/166bc0584cbae0a19db002d663d28136/review/consolidated-review.md
Fix Report: PR #2467Date: 2026-08-05T09:19:42Z SummaryAll five review findings were fixed. Included command-backed loops now undergo the same load-time safety validation as ordinary command nodes, unresolved included command files fail fast, and tests and documentation cover the complete shipped behavior. Fixes Applied
Tests Added
Docs Updated
Skipped Findings(none) Blocked (Could Not Fix)(none) Suggested Follow-up Issues(none) Validation
|
Wirasm
left a comment
Wirasm
left a comment
There was a problem hiding this comment.
First review pass — one docs change before this lands. The implementation isn't reviewed yet; this is the example that jumped out on read-through.
| depends_on: [plan] | ||
| with: | ||
| plan: $plan.output | ||
| base_branch: main |
There was a problem hiding this comment.
Change this to base_branch: $BASE_BRANCH.
Two reasons, and the second is the one that matters:
1. main is the wrong branch to name in this repo. main is the release branch here; everything targets dev. A reader who copies this example inherits the wrong default. (Unrelated but adjacent: a stale local refs/remotes/origin/HEAD on this machine was also pointing at main, so the word showed up twice from two unrelated causes — that one's fixed separately.)
2. It teaches hardcoding a branch name in the same paragraph that introduces the feature. Archon already provides $BASE_BRANCH precisely so authors don't do this. Using it here demonstrates something more interesting than a literal: a workflow variable flowing through with: into a block.
Verified it works — applyInputsMacro is a pure verbatim text.replace, and this file has zero references to substituteWorkflowVariables or BASE_BRANCH. So $BASE_BRANCH lands as literal text in the block's node body and resolves through normal runtime substitution, by exactly the mechanism the prose above already describes for $node.output. No code change needed — the doc line is the whole fix.
Worth a sentence in the surrounding prose too: a with: value can carry any variable the runtime substitutes, not just $node.output.
c87c104 to
fbe9108
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/workflows/src/include-expander.ts`:
- Around line 409-424: The $INPUTS validation currently scans stripped command
content, allowing references inside fenced or inline code to bypass detection.
Keep the stripped scan for sibling output references, but run INPUTS_REF against
the original content so all unsupported include-input references are rejected;
add regression cases covering fenced and inline-code $INPUTS references.
- Around line 204-211: Update the substitute callback in the include expansion
logic to require that name is an own property of args using Object.hasOwn before
reading its value. Treat inherited or absent mappings as missing by adding name
to missing and returning the original match; preserve substitution for
explicitly supplied mappings, including values that are undefined if the
existing contract requires them.
In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 701-703: Update the node validation logic near the raw with field
to reject data.with when neither hasInclude nor hasWorkflow is enabled. Preserve
the existing workflow-specific error and ensure unsupported prompt, command, and
bash modes fail validation instead of silently dropping with.
🪄 Autofix
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 Plus
Run ID: ebc32889-2968-42e1-9ae6-af0570830b6c
📒 Files selected for processing (9)
packages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/workflow-language-constitution.mdpackages/workflows/src/command-file.tspackages/workflows/src/include-expander.test.tspackages/workflows/src/include-expander.tspackages/workflows/src/loader.test.tspackages/workflows/src/schemas.test.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/workflow-discovery.ts
| // Raw here because include/workflow modes validate and retain it differently. | ||
| // Other node modes strip it with the rest of their unsupported surface. | ||
| with: z.unknown().optional(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject with on every unsupported node mode.
A prompt, command, or bash node can pass this raw field through validation. The transform then drops it without an error. This makes a workflow configuration silently ineffective.
Add a !hasInclude && !hasWorkflow && data.with !== undefined validation branch. Keep the existing workflow-specific error.
Proposed fix
if (hasWorkflow && data.with !== undefined) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message:
"'with:' named-parameter mapping is not yet supported on workflow nodes (slice 2). Use 'input:' instead.",
path: ['with'],
});
}
+ if (!hasInclude && !hasWorkflow && data.with !== undefined) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
+ message: "'with' is only supported on include nodes.",
+ path: ['with'],
+ });
+ }As per coding guidelines, “Fail fast with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions.”
🤖 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/workflows/src/schemas/dag-node.ts` around lines 701 - 703, Update
the node validation logic near the raw with field to reject data.with when
neither hasInclude nor hasWorkflow is enabled. Preserve the existing
workflow-specific error and ensure unsupported prompt, command, and bash modes
fail validation instead of silently dropping with.
Source: Coding guidelines
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/workflows/src/include-expander.ts`:
- Around line 232-234: Update the isWorkflowNode branch in the node substitution
logic to also apply substitute to workflow.fan_out.items when that property is
defined, preserving the existing node.input substitution and leaving other node
types unchanged.
🪄 Autofix
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 Plus
Run ID: 516d5467-6fdf-4a53-930e-d6c38ffd2d51
📒 Files selected for processing (9)
packages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/workflow-language-constitution.mdpackages/workflows/src/command-file.tspackages/workflows/src/include-expander.test.tspackages/workflows/src/include-expander.tspackages/workflows/src/loader.test.tspackages/workflows/src/schemas.test.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/workflow-discovery.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/docs-web/src/content/docs/reference/workflow-language-constitution.md
- packages/workflows/src/command-file.ts
- packages/workflows/src/include-expander.test.ts
- packages/workflows/src/schemas/dag-node.ts
- packages/workflows/src/workflow-discovery.ts
- packages/workflows/src/loader.test.ts
- packages/docs-web/src/content/docs/guides/authoring-workflows.md
- packages/workflows/src/schemas.test.ts
PR Review: #2467 — load-time inputs for
|
| Check | Result |
|---|---|
| Type check | PASS |
| Lint | PASS (0 warnings) |
| Format | PASS |
| Tests | PASS — 0 fail in every package; packages/workflows 1641 pass |
| Docs build | PASS (85 pages) |
Critical
C1 · $INPUTS in workflow.fan_out.items is silently dropped
packages/workflows/src/include-expander.ts — applyInputsMacro's isWorkflowNode branch substitutes node.input but never node.fan_out.items. rewriteNodeOutputRefs, the function it is required to mirror, walks both:
// rewriteNodeOutputRefs — both surfaces
if (node.input !== undefined) node.input = code(node.input);
if (node.fan_out !== undefined) node.fan_out.items = code(node.fan_out.items);
// applyInputsMacro — fan_out.items missing
if (node.input !== undefined) node.input = substitute(node.input);This violates Task 4 verbatim: "It must walk exactly the field set rewriteNodeOutputRefs walks... copy that branch structure field-for-field."
Confirmed by repro, not inference:
fan_out.items ACTUAL = "$INPUTS.itemsref" errors: []
workflow.input ACTUAL = "scope=$gather.output" (control — correct)
Both failure halves fire at once. The literal $INPUTS.<name> reaches the executor, and missingInputs never sees the name — so a caller who forgot to pass that input gets no load error either. That directly contradicts the PR's own Acceptance criterion: "An unsupplied $INPUTS.<name> fails the load, naming the include node, the block and the parameter."
There is no safety net. validateDagStructure does scan fan_out.items (loader.ts:265) but only for the \$id\.output grammar, so $INPUTS.tasks matches nothing. The guard fires only when the input is coincidentally named output:
input named 'tasks': errors=0 (SILENT)
input named 'scope': errors=0 (SILENT)
input named 'output': errors=1 Node 'inc__fan' references unknown node '$INPUTS.output'
Fix — one line in applyInputsMacro's workflow branch:
if (node.fan_out !== undefined) node.fan_out.items = substitute(node.fan_out.items);Plus a case in substitutes inputs across every other supported inline node surface — that test enumerates every surface except this one, so its title currently asserts a completeness it doesn't have.
Important
I1 · WARN → hard error on unresolvable command files (explicitly forbidden; needs your call)
include-expander.ts scanBlockCommandRefs now throws where it previously warned and continued. Task 6 says: "Do not upgrade that warn to an error — it would break blocks whose commands live outside the scanned set." It also breaks the must-survive constraint "Undeclared includes keep working byte-for-byte."
This was deliberate, not an oversight — commit fbe9108f lists "fail fast when included command files cannot be validated", and the loader test was renamed should warn (not fail) … → should fail … with its mockLogger.warn assertion deleted.
Confirmed by repro — a workflow with no with: and no $INPUTS anywhere is dropped entirely from discovery:
errors: [{ error: "…could not be resolved for include safety validation…" }]
workflow 'p' still discoverable? NO -- DROPPED
Blast radius (resolveCommandContentForScan returns null): defaults.loadDefaultCommands: false; a command name containing / (rejected by isValidCommandName before resolution); cwd === null global/non-repo discovery. Under loadDefaultCommands: false this drops archon-issue-review-full, archon-plan-to-pr and archon-idea-to-pr — all three include archon-review-block, whose 9 command nodes are bundled defaults. A stock install is unaffected today.
This is a judgment call, not a clear-cut bug. Fail-fast is a stated CLAUDE.md principle and the author defended it in-code; the issue author explicitly forbade it. Recommended split: keep the hard fail for a resolved file containing $INPUTS.<name> (that half is correct and valuable), revert the unresolved-file branch to WARN + continue. If you'd rather keep the strict behavior, it should be owned explicitly in the PR description and #2466 amended — it shouldn't land silently against a written constraint.
I2 · loop_group body command files escape the scan on both sides
A command:/loop.command node nested in a loop_group body inside an included block is invisible to the safety scan — resolveIncludeBlockCommandContents (workflow-discovery.ts) iterates workflow.nodes top-level only, so its content never enters the map, and scanBlockCommandRefs iterates child.nodes top-level only. Confirmed by repro: errors: [] where the identical command at top level is correctly rejected.
applyInputsMacro does recurse into loop_group.nodes; the scan does not. The top-level loop structure is pre-existing (not a regression from this PR), but the new $INPUTS check inherits the blind spot, and it makes a docs claim untrue — see S1.
I3 · The "KEEP IN SYNC" comment now undercounts, and it already drifted
rewriteNodeOutputRefs's block comment still reads "KEEP IN SYNC (three ref-surface enumerations must agree)". applyInputsMacro is a fourth enumeration of the identical surface, and it has already drifted — that drift is exactly C1. Suggested: "four ref-surface enumerations must agree: this rewrite, applyInputsMacro below, the loader's validateDagStructure scan, and the substituteNodeOutputRefs call sites in dag-executor.ts."
I4 · Misleading comment on the flat dagNodeSchema.with field
Current: "Raw here because include/workflow modes validate and retain it differently." Workflow mode never retains with in any form — it unconditionally rejects it as unsupported (phase 2). A maintainer could read this as "both branches keep it somehow." Suggested: "Raw here because include mode validates its shape and retains it on the parsed node; workflow mode still rejects it outright as unsupported (phase 2). Other node modes strip it with the rest of their unsupported surface."
Suggestions
- S1 · Docs overclaim.
authoring-workflows.mdstates unconditionally that loading fails if a command file "cannot be resolved for safety validation or contains$INPUTS.<name>". Only true for command-backed nodes at the block's top level (see I2). Scope the sentence or close the recursion gap. - S2 · Reserved
INPUTSnode id is undocumented anywhere — a user who names a nodeINPUTSgets a load error with no doc explaining why. - S3 ·
CLAUDE.md:453— theinclude:node-type description wasn't updated withwith:/$INPUTS. The same line already documentsworkflow:'s "with:andretry:rejected" at comparable density. - S4 · Task 6's "best-effort" caveat was required next to the message and isn't there. One line above the new
$INPUTSthrow would satisfy it. - S5 · Docs never say "never expressions" explicitly, which Task 8 asked for. One clause closes it.
- S6 · Duplicated identifier grammar —
/^[a-zA-Z_][a-zA-Z0-9_-]*$/indag-node.tsand the capture group inINPUTS_REF. They agree today; a shared constant would prevent a key that validates but never substitutes. - S7 · No test combines the unresolvable-command throw with
loadDefaultCommands: false(low value — the branch itself is pre-existing). - S8 ·
command-file.tsis scope creep beyond feat(workflows): with: parameters on include: nodes — $INPUTS as a load-time macro (signature phase 1) #2466 (addsloop.command). Correct on its own and closes a real gap, but it widens I1's blast radius to files that previously weren't scanned at all.
Strengths
- The load-bearing ordering is right and genuinely protected.
rewriteNodeOutputRefs→applyInputsMacro→ id rename, with a comment explaining why. Hand-traced: reversing it would rename an injected$gather.outputinto$review__gather.output, and the collision test's exact-match assertion would fail. That test earns its keep. - Test quality is high. Every Task 7 case and Acceptance item is covered with regression-catching assertions, not placeholders. The old
"with: is rejected"test was genuinely replaced per the⚠️ , not deleted. Loader tests exercise the real discovery path with files on disk, not just unit-level calls. - Nested-include forwarding is correct by design — memoized single expansion means an intermediate block's
$INPUTSpassthrough survives to be resolved by the outer caller. superRefine→transformis safe. Verified against zod 4.4.3: a non-fatalctx.addIssue()short-circuitssafeParsebefore the transform runs, so theas Record<string, string>cast is never observably reached.INPUTS_REF/gstatefulness is handled —String.replaceresetslastIndexper spec, and the.exec()site resets defensively.- Constitution treatment is careful and honest — data-only mapping, present tense for the shipped
include:half, explicit thatworkflow.withhas not shipped. No overclaiming of parity with the unbuilt phase-2 half. - CLAUDE.md zod conventions respected — two-arg
z.record, no hand-rolled parallel interface, correct schema naming.
Bottom line
C1 is a one-line fix plus a test case and should land before merge — it's a silent failure in exactly the mode the feature exists to prevent, and it's untested in both directions. I1 is a real decision for the maintainer rather than a defect to fix on autopilot: fail-fast is defensible, but it contradicts a written constraint and changes load behavior for include: users who never opted into this feature. I2–I4 are cheap. Everything else is polish.
The implementation is otherwise careful, well-tested, and faithful to a demanding spec.
Addendum — type-design pass + independent confirmationThe type-design and error-handling passes landed after the main review. Net effect: no change to the recommendation or to C1/I1/I2 — all three were independently re-confirmed with live repros. Below is what's genuinely new, plus one correction to my own report. Correction to S6 (identifier-regex duplication) — downgradeI flagged the duplicated New — S9 · The schema that types
|
| Dimension | Rating | Driver |
|---|---|---|
| Encapsulation | 6/10 | Type and enforcement live in two objects that agree only by authorship |
| Invariant expression | 7/10 | Fully correct with good messages, but hand-rolled where zod expresses it natively |
| Usefulness | 8/10 | Both types solve real narrow problems; correct absence signal on the helper |
| Enforcement | 6/10 | Solid tests, no exploitable gap found against the real parser; docked for the hand-written/structural split |
No blocking issues from this pass. C1 and I1 remain the only items I'd hold the merge for.
Addendum 2 — silent-failure pass (final)The error-handling pass landed last and is the most consequential of the six. It raises one new finding at Critical severity and closes the two questions I'd flagged as unverified. Recommendation is unchanged (REQUEST CHANGES) but the fix list grows. New — C2 ·
|
| Surface | Where | Runtime behavior |
|---|---|---|
agents[].prompt |
dag-node.ts:123 |
Passed straight to the Claude SDK (dag-executor.ts:1122 → claude/provider.ts:567-571). No substitution, ever. |
systemPrompt |
dag-node.ts:219 |
Wired directly to baseOptions.systemPrompt (dag-executor.ts:1108). No substitution, ever. |
approval.on_reject.prompt |
dag-node.ts:395 |
Runs through prompt substitution on a rejection retry (dag-executor.ts:5202-5231) — but that path only understands $id.output, not $INPUTS. |
Confirmed by repro — caller supplies detail, control substitutes, the other three don't, and errors: 0:
errors: 0
prompt (control): "main CLEAN-TEMP-FILES"
agents.helper.prompt : "Sub-task: $INPUTS.detail"
systemPrompt : "You handle $INPUTS.detail"
on_reject.prompt : "Retry with $INPUTS.detail"
Why this is worse than the equivalent $node.output gap, and why I'm rating it Critical rather than "pre-existing." These three are pre-existing blind spots in rewriteNodeOutputRefs, so applyInputsMacro technically did match it field-for-field here — no spec violation. But the two macros have different fallbacks. A surface rewriteNodeOutputRefs misses is only a namespacing miss; the executor's runtime substituteNodeOutputRefs pass still resolves the ref. $INPUTS has no such second chance — I verified there is no $INPUTS handling anywhere outside include-expander.ts and the schemas, so load-time expansion is the sole resolution path. A missed surface is therefore permanent literal $INPUTS.<name> text delivered to the model, and because the field is never visited it is never recorded in missingInputs either — no load error whether the caller supplies the value or forgets it entirely.
That is precisely the failure #2466's "Read this first" section says must never happen: "an author should learn this from the PR, not from $INPUTS.scope silently reaching the model as literal text."
Recommended: wire all three into applyInputsMacro (cheap — they're plain string fields). If you'd rather scope this PR tightly, the minimum acceptable alternative is an explicit documented reserved-surface list plus a tracked follow-up, so it's a stated boundary rather than an undocumented hole. Silently shipping it is the one option I'd argue against.
Partial restore of S6 — my downgrade was too broad
In Addendum 1 I downgraded the duplicated identifier regex to "not a finding" on the grounds that the same char class is already unshared across five other files. That reasoning holds for the family, but I over-corrected on the specific pair. The drift is one-directional, and the dangerous direction is silent:
- Loosen the
with:-key validator (dag-node.ts) withoutINPUTS_REF→ e.g.with: {my.key: "v"}validates, but$INPUTS.my.keymatches only$INPUTS.my, leaving.keyas trailing literal text. Silent wrong substitution, no error. - Loosen
INPUTS_REFwithout the key validator → the matchingwith:key fails schema validation and the node fails to load. Loud and safe.
Unlike the other five sites (which encode node-id output refs — a related but distinct concept), these two were introduced by the same PR, same author, same feature, and encode the identical semantic concept "a valid include-input name." Narrow recommendation: one shared INPUT_NAME_PATTERN constant for this pair only. Explicitly not the broader five-site refactor — that's older, cross-cutting, and out of scope here.
Closed — the two questions I'd flagged as unverified
- Expansion errors are surfaced correctly, not silently dropped.
expandWorkflowIncludescatchesIncludeExpansionErrorper top-level workflow, pushes{filename, error, errorType}intoerrors[], and excludes that workflow from the returned map. Two confirmed surfacing points:/workflow listrenders a "N workflow(s) failed to load:" section (command-handler.ts:612-653, capped at 10), andGET /api/workflowsreturns it in its documentederrors?field. The resilient drop-one-report-it-keep-the-rest mechanism is sound — which is exactly what makes I1 user-visible as a load failure rather than a background warning. - The prototype check is correct, with no false accepts or rejects. Tested against
Bun.YAML.parse(the real parser,loader.ts:85): bare timestamps parse as strings notDate; mappings never yieldMap;null/arrays are caught by earlier clauses. So the clause is defensive redundancy, not live logic. No prototype-pollution vector —__proto__arrives as an ordinary own key withObject.prototypeintact. Nothing to change.
Also confirmed clean
missingInputsthrow placement —.map()runs its callback unconditionally, and theloop_grouprecursion passes the sameSetby reference, so nested-body misses land before the post-map throw. No interaction bug with the entry-nodewhen:combining logic.- The dangling-ref safety net genuinely works for
$node.output—validateDagStructurereally does re-validate the flattened DAG in the right order (after macro substitution), so a caller's dangling$typo.outputis caught. It provides zero protection for$INPUTS, per C1/C2. - The zod
.transform()question that split two passes is settled by execution:transformCalls: 0,success: false. The transform does not run after a non-fatalctx.addIssue(), so the cast is unreachable — as originally reported.
Revised fix list
Before merge: C1 (fan_out.items — one line), C2 (three surfaces, or an explicit documented boundary + follow-up), I1 (revert the unresolved-file branch to WARN, or sign off on the deviation explicitly).
Cheap follow-ups: I2, I3, I4, S6-narrowed, S10 (isCommandNode).
The underlying implementation remains careful and well-tested; the gaps are all in surface coverage, and C1/C2 share one root cause — the ref-surface enumerations were never mechanically tied together, which is exactly what I3's stale "KEEP IN SYNC" comment was supposed to prevent.
…t command scanning Addresses the blocking findings from the review of #2467. C1 — $INPUTS in workflow.fan_out.items was dropped. applyInputsMacro walked node.input but not node.fan_out.items, so the literal reached the executor, which JSON.parses it and spawns one child per unsubstituted placeholder. The name never reached missingInputs either, so a caller who forgot the value got no load error. Added to the workflow branch and to the "every other supported inline node surface" test, whose title claimed a completeness it lacked. C2 — three AI-turn surfaces were never walked: systemPrompt, agents.*.prompt (plus .description, same delivery path) and approval.on_reject.prompt. These are also blind spots in rewriteNodeOutputRefs, so the macro did mirror it field-for-field — but the two have different fallbacks. A surface the rewrite misses is only a namespacing miss; the executor's runtime pass still resolves the ref. $INPUTS has no runtime pass at all, so a missed surface is permanent literal text delivered to the model, and silent in both directions. Walked here only; the rewrite half is a pre-existing gap left to its own issue. F1 — args[name] read inherited Object.prototype members, so an unsupplied $INPUTS.toString spliced a native function body into the prompt instead of failing the load. Now Object.hasOwn, with anything not supplied as an own key treated as missing. F2 — the $INPUTS command-body guard tested fence-stripped content, so a fenced or inline-code $INPUTS.scope passed validation and shipped unsubstituted. It now tests raw content. This is the behaviour the macro's own docblock and the shipped guide already claimed. The sibling-ref scan keeps stripping — it is wrong there too, but that is pre-existing and goes with the rewrite gap. I1 — reverted the unresolvable-command-file throw to WARN + continue. An unreadable file is an incomplete-information state, not an unsafe one, and failing it dropped workflows with no with: and no $INPUTS anywhere from discovery entirely, breaking "undeclared includes keep working byte-for-byte". The hard fail is kept for a file that was actually read and contains $INPUTS. Documented the scan as best-effort by construction next to the message (S4). Also folded in: the KEEP IN SYNC comment now counts four enumerations and records that the drift it was meant to prevent had already happened (I3); the flat dagNodeSchema.with comment now states the rationale that survives scrutiny — workflow.with's phase-2 shape is undecided, so committing the shared field to Record<string, string> risks a breaking change (I4); one shared INPUT_NAME_SOURCE for the with:-key validator and INPUTS_REF, whose drift is silent in one direction only (S6, narrowed to that pair); isCommandNode added to the guard catalog now the rule of three is met (S10); docs scoped to what the scan actually covers and given the explicit "never expressions" clause (S1, S5); CLAUDE.md include: line updated (S3). Deferred deliberately, each filed rather than fixed here: the loop_group body command scan hole (I2 — pre-existing on both sides, wants its own coverage); the rewriteNodeOutputRefs half of the three unwalked surfaces; with: accepted and silently stripped on six non-include node modes; and the schema/enforcement split on includeNodeSchema (S9 — the implementer followed Task 1 faithfully).
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/workflows/src/include-expander.ts`:
- Around line 440-442: The command-reference scan currently ignores commands
nested in loop_group bodies, allowing unresolved $INPUTS macros through. Update
command-content discovery and scanBlockCommandRefs to recursively traverse each
loop_group.nodes collection alongside top-level child.nodes, and add a
regression test covering a nested command file containing $INPUTS.scope.
🪄 Autofix
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 Plus
Run ID: cdbea9b2-8a4a-447f-9efc-c36a32d08854
📒 Files selected for processing (8)
CLAUDE.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/workflows/src/command-file.tspackages/workflows/src/include-expander.test.tspackages/workflows/src/include-expander.tspackages/workflows/src/loader.test.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workflows/src/command-file.ts
- packages/docs-web/src/content/docs/guides/authoring-workflows.md
| * BEST-EFFORT BY CONSTRUCTION. This scan sees only what discovery could resolve, and only | ||
| * the block's TOP-LEVEL command nodes — a command nested in a `loop_group` body is not | ||
| * reached. So a clean scan is "nothing found in what we could read", never a proof of |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scan command files inside loop_group bodies.
A loop_group body can contain command nodes. This scan only visits child.nodes, so it does not reject $INPUTS.<name> in a nested command file. The command runs after include expansion and receives the unresolved macro.
Recursively walk loop_group.nodes in both command-content discovery and scanBlockCommandRefs. Add a regression test for a nested command file containing $INPUTS.scope.
🤖 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/workflows/src/include-expander.ts` around lines 440 - 442, The
command-reference scan currently ignores commands nested in loop_group bodies,
allowing unresolved $INPUTS macros through. Update command-content discovery and
scanBlockCommandRefs to recursively traverse each loop_group.nodes collection
alongside top-level child.nodes, and add a regression test covering a nested
command file containing $INPUTS.scope.
|
Follow-ups from this PR are now filed, so the deferrals are tracked rather than implied:
One thing worth folding in only if you are already pushing: the two |
Included blocks had no safe parameter surface, forcing authors to duplicate reusable inline DAGs. Changes: - validate and retain include with mappings and reserve INPUTS - substitute inline INPUTS macros without capturing parent refs - reject unsupported command-body parameters with actionable errors - add schema, expansion, discovery, and documentation coverage Fixes #2466
Fixed: - validate command-backed loop prompts during include expansion - fail fast when included command files cannot be validated - align include input documentation with the accepted string mapping Tests added: - cover every inline input substitution surface and loop command discovery
…t command scanning Addresses the blocking findings from the review of #2467. C1 — $INPUTS in workflow.fan_out.items was dropped. applyInputsMacro walked node.input but not node.fan_out.items, so the literal reached the executor, which JSON.parses it and spawns one child per unsubstituted placeholder. The name never reached missingInputs either, so a caller who forgot the value got no load error. Added to the workflow branch and to the "every other supported inline node surface" test, whose title claimed a completeness it lacked. C2 — three AI-turn surfaces were never walked: systemPrompt, agents.*.prompt (plus .description, same delivery path) and approval.on_reject.prompt. These are also blind spots in rewriteNodeOutputRefs, so the macro did mirror it field-for-field — but the two have different fallbacks. A surface the rewrite misses is only a namespacing miss; the executor's runtime pass still resolves the ref. $INPUTS has no runtime pass at all, so a missed surface is permanent literal text delivered to the model, and silent in both directions. Walked here only; the rewrite half is a pre-existing gap left to its own issue. F1 — args[name] read inherited Object.prototype members, so an unsupplied $INPUTS.toString spliced a native function body into the prompt instead of failing the load. Now Object.hasOwn, with anything not supplied as an own key treated as missing. F2 — the $INPUTS command-body guard tested fence-stripped content, so a fenced or inline-code $INPUTS.scope passed validation and shipped unsubstituted. It now tests raw content. This is the behaviour the macro's own docblock and the shipped guide already claimed. The sibling-ref scan keeps stripping — it is wrong there too, but that is pre-existing and goes with the rewrite gap. I1 — reverted the unresolvable-command-file throw to WARN + continue. An unreadable file is an incomplete-information state, not an unsafe one, and failing it dropped workflows with no with: and no $INPUTS anywhere from discovery entirely, breaking "undeclared includes keep working byte-for-byte". The hard fail is kept for a file that was actually read and contains $INPUTS. Documented the scan as best-effort by construction next to the message (S4). Also folded in: the KEEP IN SYNC comment now counts four enumerations and records that the drift it was meant to prevent had already happened (I3); the flat dagNodeSchema.with comment now states the rationale that survives scrutiny — workflow.with's phase-2 shape is undecided, so committing the shared field to Record<string, string> risks a breaking change (I4); one shared INPUT_NAME_SOURCE for the with:-key validator and INPUTS_REF, whose drift is silent in one direction only (S6, narrowed to that pair); isCommandNode added to the guard catalog now the rule of three is met (S10); docs scoped to what the scan actually covers and given the explicit "never expressions" clause (S1, S5); CLAUDE.md include: line updated (S3). Deferred deliberately, each filed rather than fixed here: the loop_group body command scan hole (I2 — pre-existing on both sides, wants its own coverage); the rewriteNodeOutputRefs half of the three unwalked surfaces; with: accepted and silently stripped on six non-include node modes; and the schema/enforcement split on includeNodeSchema (S9 — the implementer followed Task 1 faithfully).
eb95862 to
d0fd551
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The deterministic tier covered bash/script/bun/uv/timeout and two trigger rules. It did not cover join semantics against a skipped upstream, until_bash termination, output_type, or fan-out at all — so the composition primitives merged in #2223/#2224/#2467 had no unattended regression test. Four workflows, no AI nodes, all assertions in bash: e2e-joins trigger_rule all_done + none_failed_min_one_success against a SKIPPED upstream, output_type, and a loop_group terminating on until_bash e2e-echo-child one bash node; the cheapest possible fan-out child e2e-fanout-alldone fan_out over a literal list with one failing child; all_done must aggregate it as {error,status} e2e-fanout-allsuccess same list, join: all_success — must FAIL the node. Wired as a negative test, so a zero exit is the regression and CI inverts the assertion Each runs in seconds and costs nothing, which is what makes them viable on every push rather than as a deliberate exercise. Verified with --no-worktree, the form CI uses: the first two exit 0, the third exits 1. Also adds rasmus-tests/ for the AI-driven probes these were derived from. Those need a funded provider and minutes per run, so they stay out of CI — t8-cascade additionally needs a concurrent abandon to observe cascade-cancel and cannot run unattended without a driver script. The split is now readable from either side: unattended work lives in test-workflows/, subscription work in rasmus-tests/. Two engine findings came out of running them: #2494 and #2495.
1 participant
Summary
include.withwas rejected at load time.include.withnow accepts an identifier-keyed string map and expands$INPUTS.<name>in included inline node fields while flattening; unsupported$INPUTS.inside command files fails clearly.workflow.with, input/return declarations, runtime$INPUTSdelivery, command-file parameterization, database/API/UI, or dependency changes.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
dag-nodeschemainclude.with; reservesINPUTSas a node ID.dag-nodeschemainclude-expanderinclude-expander$INPUTS.<key>after child-reference namespacing.include-expander$INPUTS.because command bodies load at execution time.Label Snapshot
risk: mediumsize: Mworkflows,docs,testsworkflows:include-expanderChange Metadata
featureworkflowsLinked Issue
Validation Evidence (required)
Commands and result summary:
scope=THE-PLAN.Security Impact (required)
NoNoNoNoYes, describe risk and mitigation: not applicable.Compatibility / Migration
YesNoNoHuman Verification (required)
What was personally validated beyond CI:
$node.outputreferences expand correctly; the isolated end-to-end workflow printedscope=THE-PLAN.INPUTSnode ID, invalid mapping values/keys, namespace-safe child references, dangling injected output references, and$INPUTS.inside resolved command files.Side Effects / Blast Radius (required)
@archon/workflowsschema validation, include flattening, workflow discovery command checks, and workflow authoring documentation.$INPUTS.<name>tokens.Rollback Plan (required)
b091f6fbfromdevif the feature must be removed.Risks and Mitigations
$INPUTS.in command files could reach an agent unexpanded.Summary by CodeRabbit
with:string mappings to pass$INPUTS.<name>into inline prompts, including code fences and inline code.$INPUTSvalues now fail workflow loading with clear error details.$INPUTSreferences in command files, includingloop.command, are rejected; unresolved command files produce warnings.