feat(workflows): structural workflow signature (inputs/returns/with) - #2523
Conversation
…on workflow: nodes (#2470) Adds a declarative signature surface to the workflow language (Signature Phase 2): a workflow declares `inputs:` (what it takes) and `returns:` (which node's output is its result); callers supply values via `with:` — now accepted on `workflow:` sub-run nodes, not just `include:`. This coordinates, it does not compute (Workflow Language Constitution — cited). Changes: - Schema: `inputs:`/`returns:` on workflowBaseSchema; `with:` on workflow nodes with include-style shape validation; reject `with`+`input`; `fan_out.as`↔`with` collision check; delete the dead `fan_out.as` placeholder rejection (#2224 merged); `inputEnvKey` - Loader: parse/validate inputs (warn-and-drop, reject required+default), returns top-level-id existence check, env-key mangling collision reject, scan `workflow:` `with:` values for dangling refs, four-surface KEEP-IN-SYNC comment - Include-expander: `returns:` drives primarySink only (sinks/depends_on unchanged, may be a non-sink); validate `with:` against declared `inputs:` (defaults/missing-required/ undeclared-key) only when inputs declared — undeclared blocks byte-for-byte unchanged - Runtime: strict `$INPUTS.<name>` substitution in the `!shellSafe` branch (did-you-mean hint, throws on unknown); `INPUTS_<UPPER_SNAKE>` env for bash/script sub-run nodes; persist `metadata.inputs` at spawn so `$INPUTS` reconstitutes on cold resume; fan-out static `with:` + per-item `$INPUTS.<as>` channel - `returns:` rebinds a child run's terminal output (parent_run_id-gated; blank → '' + WARN, no sink fallthrough); loop_group per-iteration scan deliberately unchanged - Bare-run guard: a required-input block still lists/loads but a top-level run fails before any worktree/clone/AI cost (CLI + orchestrator) - Validator: bundled-set-only `workflow:` target check via the runtime fuzzy resolver - Docs: authoring-guide Workflow Signature section + binding-time table + `INPUTS_*` env mangling; constitution admits inputs/returns and records cross-file schema checking rejected - Tests: signature parse/validate, returns→primarySink, with vs inputs, $INPUTS runtime, bundled-target check, bare-run guard, env-key mangling; #2459 parity ratchet fixtures added Closes #2470 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughWorkflow signatures now support declared inputs, defaults, required validation, named ChangesWorkflow signatures and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WorkflowLoader
participant InputResolver
participant ChildWorkflow
participant CommandOrPrompt
Caller->>WorkflowLoader: load inputs, returns, and with
WorkflowLoader->>InputResolver: validate supplied inputs
InputResolver->>ChildWorkflow: start with resolved inputs
ChildWorkflow->>CommandOrPrompt: substitute $INPUTS or inject INPUTS_* environment
ChildWorkflow-->>Caller: return declared node output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
🔍 Comprehensive PR ReviewPR: #2523 — feat(workflows): structural workflow signature (inputs/returns/with)
SummaryA declarative workflow signature surface — workflow-level Verdict:
🟠 High Issues1. Runtime
|
| Issue | Location | Agent | Suggestion |
|---|---|---|---|
$INPUTS.<name> now throws on a top-level run that never declared inputs (compat change) |
executor-shared.ts:642-654 |
code-review | Accept fail-fast strictness (matches $node.output.field) + changelog note, OR scope $INPUTS substitution to runs carrying inputs |
workflow: node input: surface omits the inputs bag — $INPUTS.<name> throws instead of resolving |
dag-executor.ts:5435-5448 |
error-handling | One-line fix: pass inputs: resolveRunInputs(parentRun) on the input: resolution |
readSubrunMetadata drops the whole inputs map on any non-string value, no log |
schemas/workflow-run.ts:172-188 |
error-handling | Defensive-only; optional warn breadcrumb, else accept as-is |
validator.ts bundled-target check — ambiguity branch untested |
validator.ts:407-418 |
test-coverage | Add a bundled ref matching ≥2 names; assert "ambiguous within the bundled set" |
✅ What's Good
- Constitution compliance clean — declarative coordinate data;
when:/with:grew no operators; constitution + authoring guide updated in the PR and matching the code. - Shell-safety preserved —
$INPUTSonly in the non-shell branch; bash/script readINPUTS_<UPPER_SNAKE>env vars (security(workflows): script nodes raw-splice user-controlled text into executable source — harden like bash nodes #2115); spread order correct and commented. - No swallowed errors on hot paths — unknown
$INPUTSTHROWS with a did-you-mean hint; fan-out resolution emits bothnotify()andfailResult();returns:-blank WARNs without sink fall-through. - Thorough behavioral load-time tests — mutual exclusion, collision, non-existent
returns:, dangling ref, passthrough parity; field-parity test extended toinputs/returns. - Zod conventions honored —
workflowInputSpecSchema,z.infertypes,zfrom@hono/zod-openapi, two-argz.record, re-exported; package boundaries intact.
📋 Suggested Follow-up Issues
| Title | Priority | Finding |
|---|---|---|
Sub-run with: parity: apply default:, validate missing-required / undeclared keys |
P1 | #4 |
Guard env-key collisions on caller with: / fan_out.as |
P2 | #5 |
Add runtime + cold-resume tests for $INPUTS delivery and returns: rebinding |
P2 | #1, #6, #7 |
Next Steps
- ⚡ Auto-fix will address the 2 CLAUDE.md corrections (updated code to use locally hosted llama LLM, nomic-embed-text model. #2, doesn't know what o1-mini is, or how to route to openrouter.ai #3, folding in Refactor Archon V2 Into a More Modular “Master Orchestrator” Syste #8) + the one-line
input:parity fix. - 📝 Decide fix-now vs. follow-up for the sub-run parity ([FEATURE] How about bootstrapping the agent builder? #4) and env-key collision (feat: add docling as alternative parsing strategy #5) gaps.
- 🧪 Add at least one runtime/resume test (Model stucked at response stream text #1, Coder agent does not invoke tools to fetch documentation #6, feat: enhance coder system prompt for improved agent behavior #7) before merge.
- 🔁 Re-run the comment-quality pass (artifact missing) if it gates this merge.
- ✅ Confirm pending
test (ubuntu/windows)+docker-buildCI pass.
Reviewed by Archon comprehensive-pr-review workflow — 4 of 5 dimensions
Artifacts: ~/.archon/.../runs/47eb748d-8c22-400b-98ea-6391303cc8a8/review/
Hoist resolveRunInputs(parentRun) out of the per-entry with: resolution loops in executeWorkflowNode and executeFanOutWorkflowNode — it re-parsed run metadata on every iteration. Computed once into a named local now. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Wirasm
left a comment
Wirasm
left a comment
There was a problem hiding this comment.
The workflow-signature direction looks useful, but this needs a few corrections before merge.
-
Remove the unrelated HyperFrames workflow. Commit
ba329f6adds.archon/workflows/defaults/archon-hyperframes-video.yaml. It is outside this PR's stated scope and is the direct cause of both Ubuntu and Windows CI failures:check:bundledreportsbundled-defaults.generated.ts is stale. Please remove the file rather than regenerating the bundle for it. -
Handle
workflow.within include expansion.rewriteNodeOutputRefs()andapplyInputsMacro()walkworkflow.inputandfan_out.items, but neither walks the values innode.with. Consequently, an included reusable block that calls a child workflow cannot forward its input or reference its own nodes correctly. A focused reproduction fails with:Node 'outer__call' references unknown node '$local.output'Please apply both
$INPUTS.<name>substitution and included-node ref namespacing to everywith:value. -
Enforce the resolved child workflow's declared input contract before spawning it. The runtime
workflow:path resolves the child definition but then persists the caller's map directly. It currently does not apply declared defaults, reject missing required inputs, or reject undeclared keys, despite the docs promising parity withinclude:. Resolve and validate inputs after child discovery and before isolation/worktree or run-row creation. Please also account for defaulted inputs on bare top-level runs, since runtime$INPUTScurrently comes only from sub-run metadata. -
Update
CLAUDE.md. It still sayswith:is rejected onworkflow:nodes andfan_out.asis reserved/rejected, which is no longer true.
Please add regression coverage for:
- an included block containing a
workflow:node whosewith:values use both$INPUTS.fooand a child-local$node.output; - sub-run defaults, missing-required inputs, and undeclared inputs;
- runtime
$INPUTSdelivery,returns:rebinding, and cold-resume metadata reconstitution.
Local validation on this head: type-check, lint, and format pass; existing signature/schema/validator/sub-run tests pass; the workflow package fails on the accidental bundled-default file.
Was accidentally included in this PR and caused check:bundled to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4KmwzY3iYy4ZBUw7RkwGY
# Conflicts: # packages/workflows/src/executor-shared.ts
…act, docs Addresses the changes requested on #2523. - include-expander: rewriteNodeOutputRefs() and applyInputsMacro() now walk every value in a workflow: node's with: map, so an included block that calls a child workflow can forward $INPUTS and reference its own nodes. Previously an included block's with: value referencing a child-local node failed with "references unknown node '$local.output'". - Extract the declared-input contract into workflow-inputs.ts and enforce it on the runtime workflow: path as well: declared defaults applied, unsupplied required inputs and undeclared keys rejected, resolved after child discovery and before isolation/worktree or run-row creation. Both call surfaces now go through one implementation, so include: and workflow: accept identical maps by construction. - Apply declared defaults to bare top-level runs, which have no parent to stamp metadata.inputs. - CLAUDE.md: with: is no longer rejected on workflow: nodes and fan_out.as is no longer reserved; document the workflow-level inputs:/returns: signature. Regression coverage: included block with a workflow: node whose with: values use both $INPUTS and a child-local node ref; sub-run defaults, missing-required and undeclared inputs; runtime $INPUTS delivery into AI surfaces; returns: rebinding on a sub-run; cold-resume metadata reconstitution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4KmwzY3iYy4ZBUw7RkwGY
|
Thanks — all four addressed, plus the merge conflict with 1. Removed the unrelated HyperFrames workflow. 2. 3. Child input contract enforced at runtime. The contract is extracted into 4. Coverage added:
4 of the 5 new sub-run contract tests fail against the previous head, so they pin the fix rather than the current behaviour. Validation:
|
![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
🧹 Nitpick comments (4)
packages/workflows/src/loader.test.ts (1)
5127-5165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the two remaining silent-drop branches.
parseWorkflowalso drops an input whose name failsINPUT_NAME_PATTERNand ignores aninputs:block that is not a plain object. Both paths only emit a warning, so a regression there is invisible. One test per branch keeps the whole warn-and-drop set pinned.🤖 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/loader.test.ts` around lines 5127 - 5165, Add two tests alongside the existing warn-and-drop coverage: one verifying parseWorkflow drops an input whose name fails INPUT_NAME_PATTERN while still parsing successfully, and another verifying a non-plain-object inputs block is ignored with no inputs produced. Assert the relevant warning behavior and preserve successful workflow parsing for both cases.packages/workflows/src/schemas/dag-node.ts (1)
570-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the stale
dagNodeFlatSchema.withcomment.The flat-field comment for
with(around lines 715-724) still states that workflow mode "rejects it outright as unsupported (phase 2,#2470) and never retains it in any form". This PR accepts and retainswithon workflow nodes (lines 570-572 and 1160-1163). A future reader will trust the wrong rule.📝 Suggested comment rewrite
- // Raw (not `z.record(z.string(), z.string())`) because the shape is only settled for - // ONE of the two modes that care. Include mode validates it in superRefine below and - // retains it on the parsed node; workflow mode rejects it outright as unsupported - // (phase 2, `#2470`) and never retains it in any form. Typing the shared flat field to - // the include shape now would commit `workflow.with` to a mapping whose phase-2 shape - // is still undecided, making a later widening a breaking change. (Note this is NOT the - // same situation as `isolation`/`fan_out`, which are typed at the flat level and - // rejected per-mode — their shape is settled.) Other node modes strip it with the rest - // of their unsupported surface. + // Raw (not `z.record(z.string(), z.string())`) so each mode validates it contextually. + // Include AND workflow modes both validate the identifier-keyed string-map shape in + // superRefine below (`validateWithShape`) and retain it on the parsed node (`#2470`). + // Other node modes strip it with the rest of their unsupported surface. with: z.unknown().optional(),🤖 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 570 - 572, Update the stale dagNodeFlatSchema.with comment to describe the current behavior: workflow nodes accept and retain with, while preserving its named-input semantics and mutual exclusivity with input. Remove the outdated claim that workflow mode rejects or discards the field, using the with schema comments near the child sub-run handling as the source of truth.packages/workflows/src/validator.test.ts (1)
213-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path test for a real bundled target.
Both tests assert the negative case. Neither asserts that a bundled workflow whose
workflow:node names an existing bundled workflow produces noworkflowissue. That missing case is the one that would catch an empty or incomplete bundled set, which is the failure mode flagged ongetBundledWorkflowDefs()inpackages/workflows/src/validator.ts.🤖 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/validator.test.ts` around lines 213 - 235, Add a positive-path test alongside the existing validateWorkflowResources bundled-target tests using a workflow node whose workflow value matches an actual bundled workflow returned by getBundledWorkflowDefs(). Assert validation produces no issue with field 'workflow', while preserving the existing negative and project-source tests.packages/docs-web/src/content/docs/reference/workflow-language-constitution.md (1)
92-94: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify when
with:contract violations are detected forworkflow:nodes.Row 93 states missing-required and undeclared-key violations are "load errors." This is accurate for
include:. For aworkflow:node, the target resolves at spawn time (row 86 in this same table), so thewith:contract check also runs at spawn/run time, not at load time. The current wording does not distinguish the two surfaces and could mislead a reader who takes row 86's late-resolution guarantee at face value.Consider splitting the enforcement-timing clause so it explicitly says "load errors for
include:; run-time errors at spawn forworkflow:nodes."🤖 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/reference/workflow-language-constitution.md` around lines 92 - 94, Clarify the workflow signature row’s enforcement timing for with: contract violations: state that include: missing-required or undeclared-key violations are load errors, while workflow: node violations are runtime errors detected at spawn. Preserve the existing descriptions of inputs:, returns:, and runtime value delivery.
🤖 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/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 749-752: Update the logging call in the workflow required-input
failure path to remove conversationId and userId from its structured metadata.
Keep only non-identifying diagnostics such as workflowName and err.missing,
while preserving the existing event name and log level.
In `@packages/workflows/src/include-expander.ts`:
- Around line 409-415: Update include expansion to rewrite the workflow-level
returns reference when it targets an included block: use primarySinkByIncludeId
to map result.returns from the include ID to the expanded primary sink, while
preserving returns for ordinary nodes. Add a regression test covering
$blk.output and execution of the declared return value after expansion.
In `@packages/workflows/src/loader.ts`:
- Around line 971-992: Update the returns parsing in the workflow loader so any
present but invalid value, including non-string or empty/whitespace-only values,
causes a validation error instead of being warned about and dropped. Preserve
trimming for valid non-empty strings, and ensure invalid values cannot fall back
to positional sink selection; align the failure behavior with the existing
evidence_policy handling.
---
Nitpick comments:
In
`@packages/docs-web/src/content/docs/reference/workflow-language-constitution.md`:
- Around line 92-94: Clarify the workflow signature row’s enforcement timing for
with: contract violations: state that include: missing-required or
undeclared-key violations are load errors, while workflow: node violations are
runtime errors detected at spawn. Preserve the existing descriptions of inputs:,
returns:, and runtime value delivery.
In `@packages/workflows/src/loader.test.ts`:
- Around line 5127-5165: Add two tests alongside the existing warn-and-drop
coverage: one verifying parseWorkflow drops an input whose name fails
INPUT_NAME_PATTERN while still parsing successfully, and another verifying a
non-plain-object inputs block is ignored with no inputs produced. Assert the
relevant warning behavior and preserve successful workflow parsing for both
cases.
In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 570-572: Update the stale dagNodeFlatSchema.with comment to
describe the current behavior: workflow nodes accept and retain with, while
preserving its named-input semantics and mutual exclusivity with input. Remove
the outdated claim that workflow mode rejects or discards the field, using the
with schema comments near the child sub-run handling as the source of truth.
In `@packages/workflows/src/validator.test.ts`:
- Around line 213-235: Add a positive-path test alongside the existing
validateWorkflowResources bundled-target tests using a workflow node whose
workflow value matches an actual bundled workflow returned by
getBundledWorkflowDefs(). Assert validation produces no issue with field
'workflow', while preserving the existing negative and project-source tests.
🪄 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: 76a6fd4d-5964-4da0-8629-674792449acf
📒 Files selected for processing (24)
CLAUDE.mdpackages/cli/src/commands/workflow.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/workflow-language-constitution.mdpackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.test.tspackages/workflows/src/executor-shared.tspackages/workflows/src/executor.tspackages/workflows/src/include-expander.test.tspackages/workflows/src/include-expander.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/schemas.test.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/schemas/index.tspackages/workflows/src/schemas/workflow-run.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/subrun.test.tspackages/workflows/src/utils/workflow-requirements.test.tspackages/workflows/src/utils/workflow-requirements.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.tspackages/workflows/src/workflow-inputs.ts
Wirasm
left a comment
Wirasm
left a comment
There was a problem hiding this comment.
pr: 2523
title: "feat(workflows): structural workflow signature (inputs/returns/with)"
author: "leex279"
reviewed: 2026-08-12T12:32:00Z
recommendation: request-changes
PR Re-review: #2523 — Structural workflow signature
Author: @leex279
Branch: archon/thread-3318ed4f → dev
Head reviewed: dc560035fd746db41908a1e454d9a085f570353c
Files changed: 24 (+1826/-93)
Summary
The follow-up substantially addresses the previous review: the unrelated HyperFrames workflow is removed, workflow.with now participates in include macro/ref rewriting, child input contracts are enforced before isolation or run creation, CLAUDE.md describes the new behavior, and focused coverage now exercises runtime delivery, defaults, required/undeclared inputs, returns, and cold resume.
Two remaining correctness gaps can silently change the declared result contract, and the branch now conflicts with current dev. These should be resolved before merge.
Implementation Context
No local implementation report or completed plan for this PR branch was found in the PRP store. The PR description and linked issue #2470 were used as the implementation contract.
Important Findings
-
packages/workflows/src/include-expander.ts:626— rewrite workflow-levelreturnswhen it names an included block.The second pass rewrites node dependencies and node output references through
primarySinkByIncludeId, butraw.returnsis copied unchanged at line 644. A workflow such asreturns: blkexpands theblkinclude intoblk__result/blk__cleanupwhile retainingreturns: blk; there is no longer a node with that id. The child-run completion path consequently cannot read the declared return output and emits a blank result.Reproduction on this head:
{"returns":"blk","nodeIds":["blk__result","blk__cleanup"]}Build the expanded result with
returns: renameIncludeRef(raw.returns)when present, and add coverage where an outer workflow declares an include node itself asreturns. -
packages/workflows/src/loader.ts:971— reject present-but-invalidreturnsinstead of dropping the contract.Whitespace and non-string values currently log
invalid_workflow_returns_value_ignoredand still load the workflow withreturnsunset. This silently falls back to positional sink selection, so malformed contract syntax can return a different node while appearing successful. Bothreturns: " "andreturns: { bad: true }reproduced asloaded: truewith no parse error.Treat every present invalid
returnsvalue as avalidation_error; only trim and retain valid non-empty strings. Add tests for blank and non-string values. -
Refresh onto current
devand resolve the two real merge conflicts.GitHub reports
CONFLICTING/DIRTY. A read-onlygit merge-treeidentifies conflicts in:CLAUDE.md: #2497 made this a one-line pointer; this PR's instruction update must move into authoritativeAGENTS.md.packages/cli/src/commands/workflow.ts: #2530 added deterministic dry-run behavior on the same command path; retain both dry-run and required-input guards.
Re-run validation after resolving against current
dev. -
packages/core/src/orchestrator/orchestrator-agent.ts:749— do not add identifiers to the required-input informational log.The project instructions say not to log PII. This outcome needs only
workflowNameandmissing;conversationIdanduserIddo not help diagnose the contract failure and unnecessarily identify the caller. Remove both from this new log record.
Suggestions
- Update the stale
dagNodeFlatSchema.withcomment aroundpackages/workflows/src/schemas/dag-node.ts:715; it still says workflow nodes reject and discardwith. - Clarify the constitution row:
include.withcontract violations are load errors, whileworkflow.withviolations are detected at runtime before child spawn. - Add the small coverage gaps identified in the latest review: invalid input-name/non-object
inputswarn-and-drop behavior and a positive bundled-workflow target case.
Validation Results
| Check | Status | Details |
|---|---|---|
| Full pre-PR validation | PASS | bun run validate completed successfully, including bundled checks, schemas, capability matrices, type-check, lint, format, install test, and isolated package tests |
| Build | PASS | bun run build; all workspace builds completed successfully |
| GitHub Actions | PASS | Docs, Ubuntu, Windows, Docker, and CodeRabbit are green on submitted head |
| Current-dev integration | FAIL | Conflicts in CLAUDE.md and packages/cli/src/commands/workflow.ts |
Strengths
- The previous runtime-contract blocker is fixed before any child worktree or run row can be created.
- A shared
resolveDeclaredInputsimplementation keeps include and runtime sub-run contracts aligned. - Persisting resolved child inputs makes cold resume behavior explicit and testable.
- Focused sub-run tests cover defaults, missing/undeclared values, prompt delivery, environment delivery, declared returns, and cold resume.
Recommendation
REQUEST CHANGES
The main direction remains sound and the previous review was handled well. Fix the two silent returns contract failures, remove identifying fields from the new log, refresh onto current dev, and rerun validation. After that, this should be ready for a short confirmation pass.
Report: /Users/rasmus/.prp/archon-75601ef6/reviews/pr-2523-review.md
2 participants
Summary
Describe this PR in 2-5 bullets:
inputs:) or which node's output is its result (returns:), and callers could only pass a single opaqueinput:string to aworkflow:sub-run node — no named, validated arguments.returns:gives a stable, author-chosen result channel instead of guessing the terminal sink.inputs:/returns:toworkflowBaseSchema, acceptswith:onworkflow:sub-run nodes (mirroringinclude:), and introduces the strict$INPUTS.<name>runtime variable plusINPUTS_<UPPER_SNAKE>env for bash/script nodes. This coordinates, it does not compute (Workflow Language Constitution — cited): the fields are declarative signature data the engine needs to govern binding.include:undeclared-key behavior wheninputs:is absent (byte-for-byte). No cross-file schema type checking (explicitly rejected in the constitution).loop_groupper-iteration scan deliberately unchanged.until_bashenv delivery unchanged.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
inputs:/returns:returns:drives primarySink;with:vsinputs:$INPUTS.<name>strict substitutionmetadata.inputsLabel Snapshot
risk: mediumsize: Lworkflowsworkflows:loader,workflows:executor,workflows:schemasChange Metadata
featureworkflowsLinked Issue
fan_out.asplaceholder rejection removed here), feat(workflows): per-child worktree isolation for workflow: sub-run nodes (slice-2 PR-A) #2223 (per-child worktree isolation)Validation Evidence (required)
@archon/workflowstest suite passes (loader, include-expander, executor-shared, validator, workflow-requirements, schemas — new fixtures incl. the test(workflows): guard against workflow-level schema fields being silently dropped at parse #2459 parity ratchet).@archon/cliworkflow.test.tsand@archon/coreorchestrator-agent.test.tspass per-file (as CI runs them).check:capability-matrixOK.check:bundledfails ONLY due to a pre-existing untracked operator file (.archon/workflows/defaults/archon-hyperframes-video.yaml) that is not part of this change — no default files were added or modified by this feature. Suite-wide pre-existing/environmental failures (load-command-prompt.test.tshome-scope resolution,codebases.test.ts, CLI env/pi-config temp-dir tests, cross-file orchestrator mock pollution) also fail on the clean tree and are unrelated; each passes per-file.Security Impact (required)
NoNoNoNoYes, describe risk and mitigation: N/A. Note:$INPUTS.<name>substitution runs in the!shellSafebranch only and is strict (throws on unknown name with a did-you-mean hint) — it does not broaden shell-injection surface beyond existing$node.outputsubstitution.Compatibility / Migration
Yes— all new fields are optional; workflows withoutinputs:/returns:/with:behave byte-for-byte as before.include:undeclared-key behavior is unchanged wheninputs:is absent.NoNo— reuses the existing additiveworkflow_runs.metadataJSON column (newinputskey); no schema change.Human Verification (required)
returns:→primarySink in include-expander;with:vs declaredinputs:(defaults / missing-required / undeclared-key);$INPUTSruntime substitution; bundled-target check; bare-run guard for unsatisfiable required inputs; env-key mangling collision.with+inputtogether;fan_out.as↔withcollision;returns:blank child output →''+ WARN (no sink fallthrough); cold-resume$INPUTSreconstitution frommetadata.inputs; required-input block still lists/loads but a top-level bare run fails early.bun run testparallel suite andbun run test:install(per implementation note — pre-existing environmental failures on the clean tree); live multi-run fan-out$INPUTS.<as>channel exercised via unit fixtures rather than a real detached run.Side Effects / Blast Radius (required)
inputs:/returns:/with:(absent → unchanged path). The four ref-surfaces (schema / loader / include-expander / runtime) carry a KEEP-IN-SYNC comment to prevent drift.Rollback Plan (required)
55f0e7b2. All fields are additive and optional, so revert is clean and low-risk.inputs:/returns:/with:.$INPUTSsubstitution throwing at runtime; unexpected bare-run guard rejections.Risks and Mitigations
$INPUTSstrict substitution in the!shellSafebranch could surprise authors with a hard throw.fan_out.asplaceholder rejection could leaveasinert ("silently does nothing" trap).executeFanOutWorkflowNodenow resolves the staticwith:map and adds the per-item$INPUTS.<as>channel, soasis functional rather than accepted-and-ignored.Summary by CodeRabbit
with:inputs, including fan-out item values.$INPUTSvariables and environment variables.