diff --git a/CLAUDE.md b/CLAUDE.md index 9e390cbb83..e3cd2db7ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -450,7 +450,7 @@ Structured logging uses Pino via `createLogger('')` from `@archon/paths` 2. **Workflows** (YAML-based): - Stored in `.archon/workflows/` (searched recursively) - Multi-step AI execution chains, discovered at runtime - - **`nodes:` (DAG format)**: Nodes with explicit `depends_on` edges; independent nodes in the same topological layer run concurrently. Node types: `command:` (named command file), `prompt:` (inline prompt), `bash:` (shell script, stdout captured as `$nodeId.output`, no AI, receives managed per-project env vars in its subprocess environment when configured), `loop:` (iterative AI prompt until completion signal), `loop_group:` (multi-node sub-DAG body repeated per iteration until `until` signal / `until_bash` exit 0 / `max_iterations`; body is sealed for `depends_on` but may read outer outputs via `$nodeId.output` and the previous iteration via `$LOOP_PREV..output`; a failed body node fails the group immediately; group-level `model`/`provider` become body defaults), `approval:` (human gate; pauses until user approves or rejects; `capture_response: true` stores the user's comment as `$.output` for downstream nodes, default false), `script:` (inline TypeScript/Python or named script from `.archon/scripts/`, runs via `bun` or `uv`, stdout captured as `$nodeId.output`, no AI, receives managed per-project env vars in its subprocess environment when configured, supports `deps:` for dependency installation and `timeout:` in ms, requires `runtime: bun` or `runtime: uv`), `include:` (load-time inlining of another workflow's nodes as a flattened, namespaced sub-DAG — each included node becomes `__`; the include node's `depends_on`/`when`/`trigger_rule` attach to the block's entry nodes, and `$includeId.output` resolves to the block's terminal (primary) sink; expansion happens at discovery so the executor sees ordinary nodes — see the "Reusing a Shared Sub-DAG" guide), `workflow:` (runtime sub-run — starts another workflow by static name as a separate governed CHILD run with its own `workflow_runs` row (`parent_run_id`), artifacts, gates, cost, and audit trail; `input:` forwards a data string (substituted like prompt bodies) as the child's `$ARGUMENTS`; the child's terminal output threads back as `$nodeId.output`; a child gate pauses the whole tree (approve the CHILD by run id — the parent auto-resumes on child completion); `isolation:` chooses the child's checkout — `inherit` (default; shares the parent's) or `worktree` (its own git worktree + branch, opt-in only, never inferred; requires an injected child-isolation resolver, so it fails fast on folder projects and surfaces that don't wire one), `with:` and `retry:` rejected, disallowed inside a `loop_group` body; abandon cascade-cancels descendants; `fan_out:` runs ONE CHILD PER ITEM of a runtime list — `items` (a `$node.output` ref or literal JSON array), `max_parallel` (default 5, bounds concurrency not total count or spend), `join` (default `all_done`: every terminal outcome aggregates with failures as `{error,status}`; `all_success` for the genuinely dependent case), `as` reserved and rejected at load. Children are INDEPENDENT: every index spawns, each runs to its own terminal state, and none cancels another — the sole exception is a child that pauses at a gate, which is cancelled because a parent has one approval slot (gate before/after the fan-out, never inside a child). Racing (`join: first_success`) is rejected outright, not deferred. Concurrent children on a SHARED checkout collide on the path lock, so a spawn-time preflight refuses that expansion unless the child declares `mutates_checkout: false`, the node sets `isolation: worktree`, or `max_parallel: 1`) . Supports `when:` conditions, `trigger_rule` join semantics, `$nodeId.output` substitution, `output_format` for structured JSON output (SDK-enforced on Claude/Codex/OpenCode; best-effort prompt-augmentation + repair on Pi/Copilot — the parsed output is **validated against the declared schema for every provider**, best-effort providers (Pi/Copilot) re-ask up to 3× on a validation miss, and a node that declares `output_format` but returns no schema-valid output **fails** rather than degrading silently; `$nodeId.output.field` access is strict — a field not in the producer's schema, or a schemaless node whose output isn't JSON / lacks the key, fails the consuming node, while an author-declared-optional field resolves to `''`), `allowed_tools`/`denied_tools` for per-node tool restrictions (all providers except Codex), `hooks` for per-node SDK hook callbacks (Claude only), `mcp` for per-node MCP server config files (all providers except Pi, env vars expanded at execution time), and `skills` for per-node skill preloading via AgentDefinition wrapping (per-node injection on Claude/Pi/OpenCode/Copilot; Codex instead auto-discovers skills from `.agents/skills/` on the filesystem — the `skills:` list is informational for Codex nodes), `agents` for inline sub-agent definitions invokable via the Task tool (Claude only), and `effort`/`thinking` for reasoning depth (Claude/Pi/Copilot) plus the Claude-only SDK advanced options `maxBudgetUsd`/`systemPrompt`/`fallbackModel`/`betas`/`sandbox` (also settable at workflow level), and `persist_session` for cross-run provider session continuity (node-level opt-in; workflow-level default via `persist_sessions: true`; requires a provider with the `sessionResume` capability), and `output_type` (any node type) for engine-written typed output sidecars — when set, the executor writes `$ARTIFACTS_DIR/nodes/.md` + `.meta.json` after the node completes (best-effort) so downstream nodes and later runs can locate output by type instead of guessing filenames + - **`nodes:` (DAG format)**: Nodes with explicit `depends_on` edges; independent nodes in the same topological layer run concurrently. Node types: `command:` (named command file), `prompt:` (inline prompt), `bash:` (shell script, stdout captured as `$nodeId.output`, no AI, receives managed per-project env vars in its subprocess environment when configured), `loop:` (iterative AI prompt until completion signal), `loop_group:` (multi-node sub-DAG body repeated per iteration until `until` signal / `until_bash` exit 0 / `max_iterations`; body is sealed for `depends_on` but may read outer outputs via `$nodeId.output` and the previous iteration via `$LOOP_PREV..output`; a failed body node fails the group immediately; group-level `model`/`provider` become body defaults), `approval:` (human gate; pauses until user approves or rejects; `capture_response: true` stores the user's comment as `$.output` for downstream nodes, default false), `script:` (inline TypeScript/Python or named script from `.archon/scripts/`, runs via `bun` or `uv`, stdout captured as `$nodeId.output`, no AI, receives managed per-project env vars in its subprocess environment when configured, supports `deps:` for dependency installation and `timeout:` in ms, requires `runtime: bun` or `runtime: uv`), `include:` (load-time inlining of another workflow's nodes as a flattened, namespaced sub-DAG — each included node becomes `__`; the include node's `depends_on`/`when`/`trigger_rule` attach to the block's entry nodes, and `$includeId.output` resolves to the block's terminal (primary) sink; expansion happens at discovery so the executor sees ordinary nodes; `with:` passes an identifier-keyed string map the block reads as `$INPUTS.`, substituted VERBATIM at load time (never expressions) across every inline text surface including inside code fences — an unsupplied name is a load error, and `$INPUTS` in a `command:`/`loop.command` file is rejected because a command body is read after expansion and can never be parameterized (best-effort: top-level command nodes only, unresolvable files warn and are skipped) — see the "Reusing a Shared Sub-DAG" guide), `workflow:` (runtime sub-run — starts another workflow by static name as a separate governed CHILD run with its own `workflow_runs` row (`parent_run_id`), artifacts, gates, cost, and audit trail; `input:` forwards a data string (substituted like prompt bodies) as the child's `$ARGUMENTS`; the child's terminal output threads back as `$nodeId.output`; a child gate pauses the whole tree (approve the CHILD by run id — the parent auto-resumes on child completion); `isolation:` chooses the child's checkout — `inherit` (default; shares the parent's) or `worktree` (its own git worktree + branch, opt-in only, never inferred; requires an injected child-isolation resolver, so it fails fast on folder projects and surfaces that don't wire one), `with:` and `retry:` rejected, disallowed inside a `loop_group` body; abandon cascade-cancels descendants; `fan_out:` runs ONE CHILD PER ITEM of a runtime list — `items` (a `$node.output` ref or literal JSON array), `max_parallel` (default 5, bounds concurrency not total count or spend), `join` (default `all_done`: every terminal outcome aggregates with failures as `{error,status}`; `all_success` for the genuinely dependent case), `as` reserved and rejected at load. Children are INDEPENDENT: every index spawns, each runs to its own terminal state, and none cancels another — the sole exception is a child that pauses at a gate, which is cancelled because a parent has one approval slot (gate before/after the fan-out, never inside a child). Racing (`join: first_success`) is rejected outright, not deferred. Concurrent children on a SHARED checkout collide on the path lock, so a spawn-time preflight refuses that expansion unless the child declares `mutates_checkout: false`, the node sets `isolation: worktree`, or `max_parallel: 1`) . Supports `when:` conditions, `trigger_rule` join semantics, `$nodeId.output` substitution, `output_format` for structured JSON output (SDK-enforced on Claude/Codex/OpenCode; best-effort prompt-augmentation + repair on Pi/Copilot — the parsed output is **validated against the declared schema for every provider**, best-effort providers (Pi/Copilot) re-ask up to 3× on a validation miss, and a node that declares `output_format` but returns no schema-valid output **fails** rather than degrading silently; `$nodeId.output.field` access is strict — a field not in the producer's schema, or a schemaless node whose output isn't JSON / lacks the key, fails the consuming node, while an author-declared-optional field resolves to `''`), `allowed_tools`/`denied_tools` for per-node tool restrictions (all providers except Codex), `hooks` for per-node SDK hook callbacks (Claude only), `mcp` for per-node MCP server config files (all providers except Pi, env vars expanded at execution time), and `skills` for per-node skill preloading via AgentDefinition wrapping (per-node injection on Claude/Pi/OpenCode/Copilot; Codex instead auto-discovers skills from `.agents/skills/` on the filesystem — the `skills:` list is informational for Codex nodes), `agents` for inline sub-agent definitions invokable via the Task tool (Claude only), and `effort`/`thinking` for reasoning depth (Claude/Pi/Copilot) plus the Claude-only SDK advanced options `maxBudgetUsd`/`systemPrompt`/`fallbackModel`/`betas`/`sandbox` (also settable at workflow level), and `persist_session` for cross-run provider session continuity (node-level opt-in; workflow-level default via `persist_sessions: true`; requires a provider with the `sessionResume` capability), and `output_type` (any node type) for engine-written typed output sidecars — when set, the executor writes `$ARTIFACTS_DIR/nodes/.md` + `.meta.json` after the node completes (best-effort) so downstream nodes and later runs can locate output by type instead of guessing filenames - Workflow-level `requires: [github]` hard-blocks invocation (before any worktree/clone/AI cost) when the originating user hasn't connected their GitHub identity — enforced only when per-user GitHub is enabled (GitHub App + `TOKEN_ENCRYPTION_KEY`); a no-op for solo PAT installs - Provider inherited from `.archon/config.yaml` unless explicitly set; per-node `provider` and `model` overrides supported - Model and options can be set per workflow or inherited from config defaults diff --git a/packages/docs-web/src/content/docs/guides/authoring-workflows.md b/packages/docs-web/src/content/docs/guides/authoring-workflows.md index 97a6940647..0cecb507e6 100644 --- a/packages/docs-web/src/content/docs/guides/authoring-workflows.md +++ b/packages/docs-web/src/content/docs/guides/authoring-workflows.md @@ -954,12 +954,59 @@ written the nodes by hand. There is no separate child run. - **Output.** `$.output` in another node resolves to the block's primary sink. In the example, `$review.output` is the output of the block's `implement-fixes` node. +### Passing values into an included block + +An include can pass an identifier-keyed string map through `with:`. The included block uses +those values through `$INPUTS.` in its inline text: + +```yaml +# parent workflow +nodes: + - id: plan + prompt: Plan the requested change. + + - id: review + include: reusable-review + depends_on: [plan] + with: + plan: $plan.output + base_branch: main +``` + +```yaml +# reusable-review workflow +nodes: + - id: inspect + prompt: Review $INPUTS.plan against $INPUTS.base_branch. +``` + +Input names must start with a letter or underscore and may then contain letters, numbers, +underscores, or hyphens. Values must be strings and are inserted verbatim during load-time +expansion — they are **never expressions**: nothing is evaluated, computed, or interpreted, +and the value is spliced in as text exactly as written. An inserted `$node.output` reference +remains a reference and resolves through the normal runtime output substitution. A missing +input is a load error; extra caller keys are ignored until workflow input declarations ship. + +Substitution applies everywhere the value could reach the model or the shell, including +inside Markdown code fences and inline code spans — `$INPUTS.` has no +documentation-only meaning, so a fenced occurrence is still a live parameter. + +#### Command bodies cannot use include inputs + +Phase 1 cannot parameterize a `command:` file or `loop.command` file used by an included +block. Command bodies are read at execution time, after load-time include expansion has +finished. When such a file can be read at load time and contains `$INPUTS.` anywhere — +including inside a code fence — workflow loading fails with a message directing you to inline +the prompt text. Use an inline `prompt:` when the block needs include inputs. + +This check is best-effort, so a clean load is not a guarantee. It covers the block's +top-level `command:`/`loop.command` nodes only, so a command nested inside a `loop_group` +body is not scanned; and a command file that cannot be resolved at load time is logged as a +warning and skipped rather than failing the workflow. This restriction applies to `include:`; +named `with:` mappings for `workflow:` sub-runs have not shipped. + ### Non-goals (Phase 1) -- **No `with:` input mapping yet.** Passing values into an included block is not supported; - an include node with a `with:` key is rejected with a clear error. A block reaches parent - context only through workflow variables (`$BASE_BRANCH`, `$ARTIFACTS_DIR`, …) and command - files, which is enough for the shared-review-block use case. - **No deep access.** A parent can read `$includeId.output` (the terminal) but not the output of an individual node inside the block. The block's internal node names are an implementation detail. diff --git a/packages/docs-web/src/content/docs/reference/workflow-language-constitution.md b/packages/docs-web/src/content/docs/reference/workflow-language-constitution.md index 3bbbbc8095..cab4174a76 100644 --- a/packages/docs-web/src/content/docs/reference/workflow-language-constitution.md +++ b/packages/docs-web/src/content/docs/reference/workflow-language-constitution.md @@ -89,7 +89,7 @@ The test the rule actually applies is *"does one child's outcome end another's?" | Parentheses & nested boolean grouping in `when:` | ❌ rejected (see policy below) | The first step of home-growing an expression language | | Templating (Jinja-style interpolation, computed node ids) | ❌ rejected | Evaluation inside declaration — the Helm road | | Dynamic include targets (`include: $x.output`) | ❌ rejected | Turns structure into a runtime value; the engine can no longer statically validate the graph | -| `with:` include parameters carrying expressions | ⚠️ constrained | Admissible only as **data-only** mapping (values or `$node.output` refs) — the moment values can be computed inline, it is function application | +| `with:` include parameters | ✅ shipped (data-only) | Identifier-keyed string values are substituted during load-time expansion; inserted `$node.output` values continue through normal runtime output substitution. `workflow.with` is not yet shipped | ## The five smells — and the management lever for each @@ -107,7 +107,7 @@ These are the specific mechanisms by which workflow languages rot. Each is liste **Mechanism.** Reuse primitives are the most dangerous axis because they converge on function application: includes become calls, parameters become arguments, loop-carried state becomes variables — and suddenly the config format has scoping rules, evaluation order, and abstraction. This is how Helm charts became programs. -**Archon today.** `loop_group` already carries loop-state (`$LOOP_PREV`); `include:` Phase 1 adds textual reuse. Both were held on the declarative side deliberately: `include` is load-time expansion with zero runtime semantics, `with:` was **deferred and rejects fail-fast**, deep output access across the include boundary is unsupported, and dynamic targets are out of scope. +**Archon today.** `loop_group` already carries loop-state (`$LOOP_PREV`); `include:` adds textual reuse. Both are held on the declarative side deliberately: `include` is load-time expansion with zero new runtime semantics, and its shipped `with:` surface is a data-only string mapping resolved during expansion. Expressions, deep output access across the include boundary, `workflow.with`, and dynamic targets remain unsupported. **Lever — composition must be resolvable at load time.** Any reuse feature must fully resolve before execution begins (the engine executes a flat, static DAG). Parameterization, if ever added, is data-only mapping. Anything requiring runtime resolution of *structure* is Phase-2 sub-run territory — where it becomes a governance object with its own run record, not a language feature. diff --git a/packages/workflows/src/command-file.ts b/packages/workflows/src/command-file.ts new file mode 100644 index 0000000000..5c87a0f671 --- /dev/null +++ b/packages/workflows/src/command-file.ts @@ -0,0 +1,9 @@ +import type { DagNode } from './schemas'; +import { isCommandNode, isLoopNode } from './schemas'; + +/** Return the command-file name used by a node, including deferred loop prompts. */ +export function getFileBackedCommandName(node: DagNode): string | undefined { + if (isCommandNode(node)) return node.command; + if (isLoopNode(node) && typeof node.loop.command === 'string') return node.loop.command; + return undefined; +} diff --git a/packages/workflows/src/include-expander.test.ts b/packages/workflows/src/include-expander.test.ts index af699dd390..b5db1d1b18 100644 --- a/packages/workflows/src/include-expander.test.ts +++ b/packages/workflows/src/include-expander.test.ts @@ -135,6 +135,327 @@ describe('expandWorkflowIncludes — namespacing', () => { }); }); +// --------------------------------------------------------------------------- +// Load-time include inputs +// --------------------------------------------------------------------------- + +describe('expandWorkflowIncludes — with input mapping', () => { + test('inlines literals, preserves caller refs, and still namespaces internal refs', () => { + const block = wf('parameterized', [ + { id: 'gather', bash: 'echo child' }, + { + id: 'judge', + prompt: 'Plan: $INPUTS.plan; scope: $gather.output; base: $INPUTS.base', + depends_on: ['gather'], + }, + ]); + const parent = wf('parent', [ + { id: 'plan', bash: 'echo parent plan' }, + { + id: 'review', + include: 'parameterized', + depends_on: ['plan'], + with: { plan: '$plan.output', base: 'main' }, + }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const judge = nodeById(workflows.get('parent')!, 'review__judge'); + expect(judge && 'prompt' in judge ? judge.prompt : '').toBe( + 'Plan: $plan.output; scope: $review__gather.output; base: main' + ); + }); + + test('rejects an injected dangling output ref during flattened validation', () => { + const block = wf('parameterized', [{ id: 'judge', prompt: 'Plan: $INPUTS.plan' }]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { plan: '$nosuch.output' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(workflows.has('parent')).toBe(false); + expect(errors.find(error => error.filename === 'parent')?.error).toContain( + "Node 'review__judge' references unknown node '$nosuch.output'" + ); + }); + + test('rejects missing inputs with include and block context', () => { + const block = wf('parameterized', [ + { id: 'judge', prompt: 'Use $INPUTS.scope and $INPUTS.base' }, + ]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { unused: 'allowed' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(workflows.has('parent')).toBe(false); + const message = errors.find(error => error.filename === 'parent')?.error; + expect(message).toContain("Node 'review'"); + expect(message).toContain("included block 'parameterized'"); + expect(message).toContain('$INPUTS.base, $INPUTS.scope'); + }); + + // `$INPUTS` has no runtime resolution pass — load-time expansion is the ONLY path + // that resolves it. A surface the macro skips therefore delivers literal + // `$INPUTS.` text to the model forever, and is never recorded in + // missingInputs either, so a caller who forgot the value gets no load error. + test('substitutes the AI-turn surfaces that have no runtime second chance', () => { + const block = wf('parameterized', [ + { + id: 'work', + prompt: 'Main: $INPUTS.detail', + systemPrompt: 'You handle $INPUTS.detail', + agents: { + helper: { + description: 'Handles $INPUTS.detail', + prompt: 'Sub-task: $INPUTS.detail', + }, + }, + }, + { + id: 'gate', + approval: { + message: 'Approve $INPUTS.detail?', + on_reject: { prompt: 'Retry with $INPUTS.detail' }, + }, + }, + ]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { detail: 'CLEAN-TEMP-FILES' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const expanded = workflows.get('parent')!; + expect(nodeById(expanded, 'review__work')).toMatchObject({ + prompt: 'Main: CLEAN-TEMP-FILES', + systemPrompt: 'You handle CLEAN-TEMP-FILES', + agents: { + helper: { + description: 'Handles CLEAN-TEMP-FILES', + prompt: 'Sub-task: CLEAN-TEMP-FILES', + }, + }, + }); + expect(nodeById(expanded, 'review__gate')).toMatchObject({ + approval: { + message: 'Approve CLEAN-TEMP-FILES?', + on_reject: { prompt: 'Retry with CLEAN-TEMP-FILES' }, + }, + }); + }); + + test('an unsupplied input on those same surfaces fails the load', () => { + const block = wf('parameterized', [ + { + id: 'work', + prompt: 'no refs here', + systemPrompt: 'You handle $INPUTS.fromSystem', + agents: { helper: { description: 'd', prompt: 'Sub: $INPUTS.fromAgent' } }, + }, + { + id: 'gate', + approval: { message: 'ok?', on_reject: { prompt: 'Retry $INPUTS.fromReject' } }, + }, + { id: 'fan', workflow: 'child', fan_out: { items: '$INPUTS.fromFanOut' } }, + ]); + const parent = wf('parent', [{ id: 'review', include: 'parameterized' }]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(workflows.has('parent')).toBe(false); + const message = errors.find(error => error.filename === 'parent')?.error; + expect(message).toContain('$INPUTS.fromAgent'); + expect(message).toContain('$INPUTS.fromFanOut'); + expect(message).toContain('$INPUTS.fromReject'); + expect(message).toContain('$INPUTS.fromSystem'); + }); + + // Inherited Object.prototype members are not supplied inputs. Reading them with a + // plain `args[name]` lookup substitutes a native function body into the prompt + // instead of reporting the input as missing. + test('an inherited property name is treated as missing, not as a value', () => { + const block = wf('parameterized', [ + { id: 'use', prompt: 'a=$INPUTS.toString b=$INPUTS.constructor c=$INPUTS.__proto__' }, + ]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { unrelated: 'x' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(workflows.has('parent')).toBe(false); + const message = errors.find(error => error.filename === 'parent')?.error; + expect(message).toContain('$INPUTS.__proto__'); + expect(message).toContain('$INPUTS.constructor'); + expect(message).toContain('$INPUTS.toString'); + expect(message).not.toContain('native code'); + }); + + test('an own property that shadows an inherited name still substitutes', () => { + const block = wf('parameterized', [{ id: 'use', prompt: 'v=$INPUTS.toString' }]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { toString: 'literal-value' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const use = nodeById(workflows.get('parent')!, 'review__use'); + expect(use && 'prompt' in use ? use.prompt : '').toBe('v=literal-value'); + }); + + test('two callers substitute independently', () => { + const block = wf('parameterized', [{ id: 'use', prompt: 'Use $INPUTS.value' }]); + const parent = wf('parent', [ + { id: 'first', include: 'parameterized', with: { value: 'alpha' } }, + { id: 'second', include: 'parameterized', with: { value: 'beta' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const expanded = workflows.get('parent')!; + const first = nodeById(expanded, 'first__use'); + const second = nodeById(expanded, 'second__use'); + expect(first && 'prompt' in first ? first.prompt : '').toBe('Use alpha'); + expect(second && 'prompt' in second ? second.prompt : '').toBe('Use beta'); + }); + + test('keeps an injected parent ref parent-scoped when a child id collides', () => { + const block = wf('parameterized', [ + { id: 'gather', bash: 'echo child' }, + { + id: 'use', + prompt: 'Parent: $INPUTS.plan; child: $gather.output', + depends_on: ['gather'], + }, + ]); + const parent = wf('parent', [ + { id: 'gather', bash: 'echo parent' }, + { + id: 'review', + include: 'parameterized', + depends_on: ['gather'], + with: { plan: '$gather.output' }, + }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const use = nodeById(workflows.get('parent')!, 'review__use'); + expect(use && 'prompt' in use ? use.prompt : '').toBe( + 'Parent: $gather.output; child: $review__gather.output' + ); + }); + + test('forwards an input through a nested include', () => { + const leaf = wf('leaf', [{ id: 'use', prompt: 'Leaf: $INPUTS.value' }]); + const middle = wf('middle', [ + { id: 'inner', include: 'leaf', with: { value: '$INPUTS.forwarded' } }, + ]); + const parent = wf('parent', [ + { id: 'plan', bash: 'echo plan' }, + { + id: 'outer', + include: 'middle', + depends_on: ['plan'], + with: { forwarded: '$plan.output' }, + }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(leaf, middle, parent)); + expect(errors).toHaveLength(0); + const use = nodeById(workflows.get('parent')!, 'outer__inner__use'); + expect(use && 'prompt' in use ? use.prompt : '').toBe('Leaf: $plan.output'); + }); + + test('substitutes in when expressions and inside fenced text', () => { + const block = wf('parameterized', [ + { + id: 'use', + prompt: '```\n$INPUTS.example $INPUTS.example\n``` empty=[$INPUTS.empty]', + when: "$INPUTS.condition == 'go'", + }, + ]); + const parent = wf('parent', [ + { id: 'gate', bash: 'echo go' }, + { + id: 'review', + include: 'parameterized', + depends_on: ['gate'], + with: { example: 'literal', empty: '', condition: '$gate.output' }, + }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const use = nodeById(workflows.get('parent')!, 'review__use'); + expect(use?.when).toBe("$gate.output == 'go'"); + expect(use && 'prompt' in use ? use.prompt : '').toBe('```\nliteral literal\n``` empty=[]'); + }); + + test('substitutes inputs across every other supported inline node surface', () => { + const block = wf('parameterized', [ + { id: 'shell', bash: 'echo $INPUTS.value' }, + { id: 'script', runtime: 'bun', script: 'console.log("$INPUTS.value")' }, + { + id: 'loop', + loop: { + prompt: 'Do $INPUTS.value', + until: 'DONE', + max_iterations: 1, + until_bash: 'test "$INPUTS.value" = done', + }, + }, + { id: 'approval', approval: { message: 'Approve $INPUTS.value?' } }, + { id: 'cancel', cancel: 'Stop: $INPUTS.value' }, + { + id: 'subrun', + workflow: 'child', + input: 'scope=$INPUTS.value', + fan_out: { items: '["$INPUTS.value"]' }, + }, + { + id: 'group', + loop_group: { + until: 'DONE', + max_iterations: 1, + until_bash: 'test "$INPUTS.value" = done', + nodes: [{ id: 'body', bash: 'echo $INPUTS.value' }], + }, + }, + ]); + const parent = wf('parent', [ + { id: 'review', include: 'parameterized', with: { value: 'done' } }, + ]); + + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); + expect(errors).toHaveLength(0); + const expanded = workflows.get('parent')!; + const loop = nodeById(expanded, 'review__loop'); + const approval = nodeById(expanded, 'review__approval'); + const group = nodeById(expanded, 'review__group'); + expect(nodeById(expanded, 'review__shell')).toMatchObject({ bash: 'echo done' }); + expect(nodeById(expanded, 'review__script')).toMatchObject({ script: 'console.log("done")' }); + expect(loop).toMatchObject({ + loop: { prompt: 'Do done', until_bash: 'test "done" = done' }, + }); + expect(approval).toMatchObject({ approval: { message: 'Approve done?' } }); + expect(nodeById(expanded, 'review__cancel')).toMatchObject({ cancel: 'Stop: done' }); + expect(nodeById(expanded, 'review__subrun')).toMatchObject({ + input: 'scope=done', + // fan_out.items is a live data-string surface that rewriteNodeOutputRefs already + // walks; the macro must walk it too or the literal reaches the executor, which + // JSON.parses it and spawns a child per unsubstituted placeholder. + fan_out: { items: '["done"]' }, + }); + expect(group).toMatchObject({ + loop_group: { + until_bash: 'test "done" = done', + nodes: [{ id: 'body', bash: 'echo done' }], + }, + }); + }); +}); + // --------------------------------------------------------------------------- // when-gate combination on entry nodes (include gate must not be discarded) // --------------------------------------------------------------------------- @@ -371,6 +692,21 @@ describe('expandWorkflowIncludes — command-file ref scan', () => { expect(err?.error).toContain("sibling node '$sib'"); }); + test('fails when a block command file references an include input', () => { + const [block, parent] = blockWithCommand(); + const commandContents = new Map([ + ['my-cmd', 'Review scope $INPUTS.scope.'], + ]); + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent), commandContents); + expect(workflows.has('parent')).toBe(false); + const message = errors.find(error => error.filename === 'parent')?.error; + expect(message).toContain("Node 'inc'"); + expect(message).toContain("command file 'my-cmd.md'"); + expect(message).toContain("included block 'cmdblk'"); + expect(message).toContain("parameter '$INPUTS.scope'"); + expect(message).toContain('inline the prompt'); + }); + test('passes when the command file has no cross-node reference', () => { const [block, parent] = blockWithCommand(); const commandContents = new Map([ @@ -382,15 +718,60 @@ describe('expandWorkflowIncludes — command-file ref scan', () => { expect(workflows.has('parent')).toBe(true); }); - test('does not fail expansion when the command file is unresolvable (null)', () => { + // A command body can never have inputs applied — it is read at execution time, after + // expansion. So `$INPUTS.` there is an unkeepable promise wherever it appears, + // and unlike the sibling-ref scan the fence has no bearing on it: the macro itself + // substitutes inside code spans, because `$INPUTS` has no documentation-only meaning. + test('fails when a command file references an include input inside a fenced block', () => { + const [block, parent] = blockWithCommand(); + const commandContents = new Map([ + ['my-cmd', 'Run this:\n\n```bash\necho "$INPUTS.scope"\n```\n'], + ]); + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent), commandContents); + expect(workflows.has('parent')).toBe(false); + expect(errors.find(error => error.filename === 'parent')?.error).toContain( + "parameter '$INPUTS.scope'" + ); + }); + + test('fails when a command file references an include input inside inline code', () => { + const [block, parent] = blockWithCommand(); + const commandContents = new Map([ + ['my-cmd', 'The scope is `$INPUTS.scope` — use it.'], + ]); + const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent), commandContents); + expect(workflows.has('parent')).toBe(false); + expect(errors.find(error => error.filename === 'parent')?.error).toContain( + "parameter '$INPUTS.scope'" + ); + }); + + // An unresolvable command file is an incomplete-information state, not an unsafe one. + // Failing it would drop workflows that never opted into this feature — including ones + // with no `with:` and no `$INPUTS` anywhere. + test('warns (not fails) when the command file cannot be resolved for scanning', () => { const [block, parent] = blockWithCommand(); const commandContents = new Map([['my-cmd', null]]); const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent), commandContents); - // Unresolvable → warn (asserted in loader.test.ts), never a hard error. expect(errors).toHaveLength(0); expect(workflows.has('parent')).toBe(true); }); + test('fails when an included loop command file references an include input', () => { + const block = wf('loopblk', [ + { id: 'repeat', loop: { command: 'loop-cmd', until: 'DONE', max_iterations: 1 } }, + ]); + const parent = wf('parent', [{ id: 'inc', include: 'loopblk', with: { scope: 'prod' } }]); + const { workflows, errors } = expandWorkflowIncludes( + mapOf(block, parent), + new Map([['loop-cmd', 'Review $INPUTS.scope.']]) + ); + expect(workflows.has('parent')).toBe(false); + expect(errors.find(error => error.filename === 'parent')?.error).toContain( + "command file 'loop-cmd.md'" + ); + }); + test('skips the scan entirely when no commandContents map is supplied', () => { const [block, parent] = blockWithCommand(); const { workflows, errors } = expandWorkflowIncludes(mapOf(block, parent)); diff --git a/packages/workflows/src/include-expander.ts b/packages/workflows/src/include-expander.ts index 639a30b339..ae3543d210 100644 --- a/packages/workflows/src/include-expander.ts +++ b/packages/workflows/src/include-expander.ts @@ -37,9 +37,11 @@ import { isBashNode, isScriptNode, isWorkflowNode, + INPUT_NAME_SOURCE, } from './schemas'; import { createLogger } from '@archon/paths'; import { validateDagStructure } from './loader'; +import { getFileBackedCommandName } from './command-file'; /** * Resolve the logger on every call rather than caching it at module scope. @@ -88,6 +90,14 @@ const OUTPUT_REF_PATTERN = /\$([a-zA-Z_][a-zA-Z0-9_-]*)\.output/g; */ const WHEN_REF_PATTERN = /\$([a-zA-Z_][a-zA-Z0-9_-]*)(?=\.[a-zA-Z_])/g; +/** + * Load-time include parameter references. Built from the same identifier source the + * `with:` key validator uses (INPUT_NAME_SOURCE in schemas/dag-node.ts) so a key that + * validates can never fail to match here — see that constant for why the drift between + * the two is silent in one direction. + */ +const INPUTS_REF = new RegExp(String.raw`\$INPUTS\.(${INPUT_NAME_SOURCE})`, 'g'); + /** Fenced (``` ```) and inline (` `` `) markdown code spans — documentation, not live refs. */ const CODE_SPAN_PATTERN = /```[\s\S]*?```|`[^`\n]*`/g; @@ -148,9 +158,15 @@ class IncludeExpansionError extends Error {} * workflow.input / workflow.fan_out.items) — canonical `.output` refs are LIVE (never * documentation) → rewritten verbatim. * - * KEEP IN SYNC (three ref-surface enumerations must agree): this rewrite, the loader's - * validateDagStructure scan, and the substituteNodeOutputRefs call sites in dag-executor.ts. - * Adding a substituted field to one means updating all three. + * KEEP IN SYNC (FOUR ref-surface enumerations must agree): this rewrite, applyInputsMacro + * below, the loader's validateDagStructure scan, and the substituteNodeOutputRefs call + * sites in dag-executor.ts. Adding a substituted field to one means updating all four. + * (The count read "three" while applyInputsMacro already existed and had already drifted — + * it was missing workflow.fan_out.items, which shipped literal `$INPUTS` text to the model.) + * + * applyInputsMacro is a SUPERSET of this function, not a mirror: it additionally walks the + * AI-turn surfaces below (systemPrompt / agents / approval.on_reject.prompt) that this + * rewrite skips. That asymmetry is deliberate — see the note on applyInputsMacro. */ function rewriteNodeOutputRefs(node: DagNode, rename: (id: string) => string): void { const code = (text: string): string => applyOutputRefRename(text, rename); @@ -189,6 +205,87 @@ function rewriteNodeOutputRefs(node: DagNode, rename: (id: string) => string): v } } +/** + * Apply an include node's input mapping to every inline text surface in the cloned node. + * Unlike output-ref rewriting, substitutions also apply inside Markdown code spans because + * `$INPUTS` has no documentation-only meaning. An inserted value may itself be a + * `$node.output` reference; it deliberately remains unresolved for the executor's existing + * runtime substitution pass. + * + * This walks a SUPERSET of rewriteNodeOutputRefs' field set, and the extra fields are the + * point. `$INPUTS` has no runtime resolution pass anywhere in the engine — load-time + * expansion is the ONLY path that resolves it. So the two functions have different + * fallbacks for a surface they skip: + * + * - a surface rewriteNodeOutputRefs misses is only a NAMESPACING miss; the executor's + * substituteNodeOutputRefs pass still resolves the ref at run time. + * - a surface this function misses is permanent. The literal `$INPUTS.` reaches + * the model as text, and because the field was never visited the name never reaches + * `missing` either — so a caller who forgot to supply it gets no load error. + * + * That is why systemPrompt / agents.*.prompt / agents.*.description / + * approval.on_reject.prompt are walked here despite being blind spots in the rewrite (a + * separate, lower-severity gap tracked on its own). Every model-facing string field must + * be walked here, whether or not the rewrite walks it. + */ +function applyInputsMacro(node: DagNode, args: Record, missing: Set): void { + const substitute = (text: string): string => + text.replace(INPUTS_REF, (match, name: string) => { + // `Object.hasOwn` rather than a plain `args[name]` lookup: a bare index read reaches + // Object.prototype, so an unsupplied `$INPUTS.toString` / `$INPUTS.constructor` + // would resolve to an inherited member and splice a native function body into the + // prompt instead of being reported as a missing input. Anything not supplied as an + // OWN key is missing, and missing always fails the load — never a silent passthrough. + const value = Object.hasOwn(args, name) ? args[name] : undefined; + if (value === undefined) { + missing.add(name); + return match; + } + return value; + }); + + if (node.when !== undefined) node.when = substitute(node.when); + + // Base AI-turn fields — valid on every AI node mode (command / prompt / loop_group), so + // they are walked outside the mode chain, like `when:`. Both go straight to the provider + // with no substitution of their own downstream. + if (node.systemPrompt !== undefined) node.systemPrompt = substitute(node.systemPrompt); + if (node.agents !== undefined) { + for (const agent of Object.values(node.agents)) { + agent.prompt = substitute(agent.prompt); + agent.description = substitute(agent.description); + } + } + + if (isLoopNode(node)) { + if (node.loop.prompt !== undefined) node.loop.prompt = substitute(node.loop.prompt); + if (node.loop.until_bash !== undefined) { + node.loop.until_bash = substitute(node.loop.until_bash); + } + } else if (isLoopGroupNode(node)) { + if (node.loop_group.until_bash !== undefined) { + node.loop_group.until_bash = substitute(node.loop_group.until_bash); + } + for (const body of node.loop_group.nodes) applyInputsMacro(body, args, missing); + } else if (isApprovalNode(node)) { + node.approval.message = substitute(node.approval.message); + if (node.approval.on_reject !== undefined) { + node.approval.on_reject.prompt = substitute(node.approval.on_reject.prompt); + } + } else if (isBashNode(node)) { + node.bash = substitute(node.bash); + } else if (isScriptNode(node)) { + node.script = substitute(node.script); + } else if (isWorkflowNode(node)) { + if (node.input !== undefined) node.input = substitute(node.input); + if (node.fan_out !== undefined) node.fan_out.items = substitute(node.fan_out.items); + } else if (isCancelNode(node)) { + node.cancel = substitute(node.cancel); + } else if ('prompt' in node && typeof node.prompt === 'string') { + node.prompt = substitute(node.prompt); + } +} + interface ExpandedInclude { /** The child's nodes, deep-cloned, id-namespaced, edges + refs rewired. */ namespaced: DagNode[]; @@ -213,13 +310,17 @@ function inlineInclude(includeNode: IncludeNode, childNodes: DagNode[]): Expande const sinkOriginalIds = childNodes.filter(n => !childDeps.has(n.id)).map(n => n.id); const parentDeps = includeNode.depends_on ?? []; + const missingInputs = new Set(); const namespaced = childNodes.map(cn => { const clone = structuredClone(cn); const wasEntry = (cn.depends_on ?? []).length === 0; - // Rewrite internal $id.output refs (child-top-level ids → namespaced) BEFORE renaming ids. + // Rewrite child-internal refs before inserting caller values. This ordering is + // load-bearing: a caller ref such as `$gather.output` must remain parent-scoped even + // when the included block also has a node named `gather`. rewriteNodeOutputRefs(clone, rename); + applyInputsMacro(clone, includeNode.with ?? {}, missingInputs); clone.id = prefix + cn.id; if (wasEntry) { @@ -257,6 +358,13 @@ function inlineInclude(includeNode: IncludeNode, childNodes: DagNode[]): Expande return clone; }); + if (missingInputs.size > 0) { + const names = [...missingInputs].sort().map(name => `$INPUTS.${name}`); + throw new IncludeExpansionError( + `Node '${includeNode.id}': included block '${includeNode.include}' references missing input${names.length === 1 ? '' : 's'} ${names.join(', ')}. Pass ${names.length === 1 ? 'it' : 'them'} through 'with:'.` + ); + } + return { namespaced, sinks: sinkOriginalIds.map(id => prefix + id), @@ -325,10 +433,21 @@ function warnDroppedWorkflowLevelFields(includeNode: IncludeNode, child: Workflo * A `command:` node's file content is read only at EXECUTION time, so the expander cannot * rewrite `$sibling.output` refs inside it the way it rewrites inline node text. If a * block's command file references a sibling node id that namespacing renames, the ref - * would silently substitute to '' at run time. Scan resolved command content (markdown - * fences stripped) for refs to any renamed id and FAIL the expansion on a hit; WARN when - * the file can't be resolved for scanning. Skipped entirely when no `commandContents` is - * supplied (e.g. unit tests that don't exercise command files). + * would silently substitute to '' at run time. This applies equally to a loop's deferred + * `loop.command` prompt. Scan resolved command content for refs to any renamed id, and for + * `$INPUTS.` parameters that can never be applied, and FAIL the expansion on a hit. + * + * 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 + * safety. That is why an UNRESOLVABLE file warns and continues instead of failing: it is + * an incomplete-information state, not an unsafe one, and the difference matters because + * failing it would drop workflows that never opted into inputs at all (no `with:`, no + * `$INPUTS` anywhere) — breaking the "undeclared includes keep working byte-for-byte" + * guarantee. Only a file we actually READ and found a problem in is a hard error. + * + * Skipped entirely when no `commandContents` is supplied (e.g. unit tests that don't + * exercise command files). */ function scanBlockCommandRefs( includeNode: IncludeNode, @@ -337,11 +456,12 @@ function scanBlockCommandRefs( ): void { const renamedIds = child.nodes.map(n => n.id); // every child top-level id gets a prefix for (const cn of child.nodes) { - if (!('command' in cn && typeof cn.command === 'string')) continue; - const content = commandContents.get(cn.command); + const commandName = getFileBackedCommandName(cn); + if (commandName === undefined) continue; + const content = commandContents.get(commandName); if (content === undefined || content === null) { getLog().warn( - { include: includeNode.id, target: child.name, command: cn.command, renamedIds }, + { include: includeNode.id, target: child.name, command: commandName, renamedIds }, 'include.command_file_unresolved_for_ref_scan' ); continue; @@ -352,10 +472,25 @@ function scanBlockCommandRefs( const refRe = new RegExp(`\\$${escapeRegExp(id)}(?=\\.[a-zA-Z_])`); if (refRe.test(stripped)) { throw new IncludeExpansionError( - `Node '${includeNode.id}': command file '${cn.command}.md' in included block '${child.name}' references sibling node '$${id}', which include namespacing renames to '${includeNode.id}__${id}'. Command-file contents are read at execution time and cannot be rewritten — inline the prompt, or restructure so the command has no cross-node reference.` + `Node '${includeNode.id}': command file '${commandName}.md' in included block '${child.name}' references sibling node '$${id}', which include namespacing renames to '${includeNode.id}__${id}'. Command-file contents are read at execution time and cannot be rewritten — inline the prompt, or restructure so the command has no cross-node reference.` ); } } + // Scanned against RAW content, not the fence-stripped copy the sibling scan uses. + // The sibling scan strips because a fenced `$other.output` can plausibly be an example + // the author wants rendered literally to the model. `$INPUTS` has no such reading — + // applyInputsMacro deliberately substitutes inside code spans, so a fenced + // `$INPUTS.` in an INLINE prompt is a live parameter. A command body can never + // have inputs applied at all, which makes writing one there an unkeepable promise + // wherever it appears. Stripping here would let exactly that promise through. + INPUTS_REF.lastIndex = 0; + const inputMatch = INPUTS_REF.exec(content); + INPUTS_REF.lastIndex = 0; + if (inputMatch?.[1] !== undefined) { + throw new IncludeExpansionError( + `Node '${includeNode.id}': command file '${commandName}.md' in included block '${child.name}' references parameter '$INPUTS.${inputMatch[1]}'. Command-file contents are read at execution time and cannot apply include inputs — inline the prompt instead.` + ); + } } } diff --git a/packages/workflows/src/loader.test.ts b/packages/workflows/src/loader.test.ts index c7ad5afefb..54fa4ad4f2 100644 --- a/packages/workflows/src/loader.test.ts +++ b/packages/workflows/src/loader.test.ts @@ -3932,6 +3932,48 @@ nodes: expect(err?.error).toContain("sibling node '$sib'"); }); + it('should fail expansion when a resolved block command file references an include input', async () => { + const workflowDir = join(testDir, '.archon', 'workflows'); + const commandsDir = join(testDir, '.archon', 'commands'); + await mkdir(workflowDir, { recursive: true }); + await mkdir(commandsDir, { recursive: true }); + + await writeFile(join(commandsDir, 'parameterized-runner.md'), 'Review $INPUTS.scope.'); + await writeFile( + join(workflowDir, 'parameterized-block.yaml'), + ` +name: parameterized-block +description: Block whose command references an include input +nodes: + - id: runner + command: parameterized-runner +` + ); + await writeFile( + join(workflowDir, 'parameterized-parent.yaml'), + ` +name: parameterized-parent +description: Includes the parameterized command block +nodes: + - id: review + include: parameterized-block + with: + scope: main +` + ); + + const result = await discoverWorkflows(testDir, { loadDefaults: false }); + expect(result.workflows.some(w => w.workflow.name === 'parameterized-parent')).toBe(false); + const message = result.errors.find( + error => error.filename === 'parameterized-parent.yaml' + )?.error; + expect(message).toContain("Node 'review'"); + expect(message).toContain("included block 'parameterized-block'"); + expect(message).toContain("command file 'parameterized-runner.md'"); + expect(message).toContain("parameter '$INPUTS.scope'"); + expect(message).toContain('inline the prompt'); + }); + it('should scan block command files in a configured custom command folder (config parity)', async () => { const workflowDir = join(testDir, '.archon', 'workflows'); const customCmds = join(testDir, 'my-cmds'); @@ -4006,7 +4048,9 @@ nodes: ); const result = await discoverWorkflows(testDir, { loadDefaults: false }); - // Unresolvable command → WARN, never a hard expansion error. + // Unresolvable command → WARN, never a hard expansion error. The scan is + // best-effort by construction; a file it cannot read is unverified, not unsafe, + // and dropping the workflow would break includes that never used this feature. const parentErrors = result.errors.filter(e => e.filename === 'ghost-parent.yaml'); expect(parentErrors).toHaveLength(0); expect(result.workflows.some(w => w.workflow.name === 'ghost-parent')).toBe(true); @@ -4015,6 +4059,45 @@ nodes: 'include.command_file_unresolved_for_ref_scan' ); }); + + it('should scan an included loop.command file for include inputs', async () => { + const workflowDir = join(testDir, '.archon', 'workflows'); + const commandDir = join(testDir, '.archon', 'commands'); + await mkdir(workflowDir, { recursive: true }); + await mkdir(commandDir, { recursive: true }); + await writeFile(join(commandDir, 'loop-review.md'), 'Review $INPUTS.scope.'); + await writeFile( + join(workflowDir, 'loop-block.yaml'), + ` +name: loop-block +description: Block with a deferred loop prompt +nodes: + - id: repeat + loop: + command: loop-review + until: DONE + max_iterations: 1 +` + ); + await writeFile( + join(workflowDir, 'loop-parent.yaml'), + ` +name: loop-parent +description: Includes the loop block +nodes: + - id: review + include: loop-block + with: + scope: production +` + ); + + const result = await discoverWorkflows(testDir, { loadDefaults: false }); + expect(result.workflows.some(w => w.workflow.name === 'loop-parent')).toBe(false); + expect(result.errors.find(error => error.filename === 'loop-parent.yaml')?.error).toContain( + "command file 'loop-review.md'" + ); + }); }); // ------------------------------------------------------------------------- diff --git a/packages/workflows/src/schemas.test.ts b/packages/workflows/src/schemas.test.ts index 84893e00d8..4570c8e57f 100644 --- a/packages/workflows/src/schemas.test.ts +++ b/packages/workflows/src/schemas.test.ts @@ -979,18 +979,45 @@ describe('dagNodeSchema — include', () => { expect(result.success).toBe(false); }); - test("include with 'with:' is rejected (not yet supported)", () => { + test("include accepts and retains a string-valued 'with:' mapping", () => { const result = dagNodeSchema.safeParse({ id: 'r', include: 'archon-review-block', - with: { pr: '$create.output' }, + with: { pr: '$create.output', base_branch: 'main', empty: '' }, }); + expect(result.success).toBe(true); + if (result.success) { + expect((result.data as IncludeNode).with).toEqual({ + pr: '$create.output', + base_branch: 'main', + empty: '', + }); + } + }); + + test.each([ + ['null', null], + ['an array', ['main']], + ['a non-string value', { branch: 42 }], + ['an invalid key', { 'bad.key': 'main' }], + ])("include rejects 'with:' when it is %s", (_description, withValue) => { + const result = dagNodeSchema.safeParse({ + id: 'r', + include: 'archon-review-block', + with: withValue, + }); + expect(result.success).toBe(false); + if (!result.success) { + expect(result.error.issues.some(issue => issue.path[0] === 'with')).toBe(true); + } + }); + + test("rejects the reserved node id 'INPUTS'", () => { + const result = dagNodeSchema.safeParse({ id: 'INPUTS', prompt: 'work' }); expect(result.success).toBe(false); if (!result.success) { - const withIssue = result.error.issues.find(i => i.message.includes('with:')); - expect(withIssue).toBeDefined(); - expect(withIssue?.message).toContain('not yet supported'); - expect(withIssue?.path).toEqual(['with']); + const idIssue = result.error.issues.find(issue => issue.path[0] === 'id'); + expect(idIssue?.message).toContain('$INPUTS.'); } }); diff --git a/packages/workflows/src/schemas/dag-node.ts b/packages/workflows/src/schemas/dag-node.ts index 2a88db09d8..ced1528851 100644 --- a/packages/workflows/src/schemas/dag-node.ts +++ b/packages/workflows/src/schemas/dag-node.ts @@ -446,17 +446,33 @@ export type CancelNode = z.infer & { script?: never; }; +/** + * Identifier grammar for an include input name. + * + * Shared deliberately with the `$INPUTS.` reference pattern in include-expander.ts, + * which builds its regex from this source. The two encode the identical concept and the + * drift between them is one-directional and silent: loosening this validator alone would + * let `with: {my.key: v}` pass while `$INPUTS.my.key` matches only `$INPUTS.my`, leaving + * `.key` as trailing literal text in the prompt. (The reverse drift fails loudly at load, + * because the matching `with:` key would be rejected here.) Sharing one source removes the + * dangerous direction. This is scoped to that pair only — the similar-looking node-id + * grammar elsewhere in the tree encodes a different concept and stays separate. + */ +export const INPUT_NAME_SOURCE = String.raw`[a-zA-Z_][a-zA-Z0-9_-]*`; +const INPUT_NAME_PATTERN = new RegExp(`^${INPUT_NAME_SOURCE}$`); + /** * Include node schema — a load-time directive that inlines another workflow's * nodes into this DAG at discovery time (see include-expander.ts). It carries no - * execution surface of its own: `include` is the target workflow name, and only - * the structural graph fields (id / depends_on / when / trigger_rule) are read by - * the expander. By the time a WorkflowDefinition reaches the executor, every - * include node has been replaced by its flattened, namespaced sub-DAG — the - * executor never sees one. + * execution surface of its own: `include` is the target workflow name, `with` is + * its load-time input mapping, and the structural graph fields (id / depends_on / + * when / trigger_rule) attach the expanded sub-DAG. By the time a + * WorkflowDefinition reaches the executor, every include node has been replaced + * by its flattened, namespaced sub-DAG — the executor never sees one. */ export const includeNodeSchema = dagNodeBaseSchema.extend({ include: z.string().min(1, "'include' must be a non-empty workflow name"), + with: z.record(z.string(), z.string()).optional(), }); /** DAG node that inlines another workflow's nodes at discovery time (load-time expansion) */ @@ -682,8 +698,15 @@ export const dagNodeFlatSchema = dagNodeBaseSchema.extend({ // over a data-driven item list. Only meaningful on a `workflow:` node (guarded in // superRefine). fan_out: fanOutConfigSchema.optional(), - // Reserved for Phase 1b input mapping. Present only so the superRefine below can - // fail fast when it appears on an include or workflow node ("not yet supported"). + // 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. with: z.unknown().optional(), // Script-only script: z.string().optional(), @@ -739,6 +762,13 @@ export const dagNodeSchema = dagNodeFlatSchema }); return z.NEVER; } + if (id === 'INPUTS') { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "node id 'INPUTS' is reserved for the $INPUTS. parameter surface", + path: ['id'], + }); + } const hasCommand = typeof data.command === 'string' && data.command.trim().length > 0; const hasPrompt = typeof data.prompt === 'string' && data.prompt.trim().length > 0; @@ -773,17 +803,42 @@ export const dagNodeSchema = dagNodeFlatSchema return z.NEVER; } - // 'with:' input mapping is deferred (Phase 1b for include; slice 2 for workflow) - // — reject it now with a clear message rather than silently dropping it - // (fail-fast). Only meaningful on include/workflow nodes; elsewhere 'with' is an - // unknown field and is stripped. + // `include.with` is a load-time, identifier-keyed string map. Keep the flat + // field raw so other node variants can still strip it contextually. if (hasInclude && data.with !== undefined) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: - "'with:' input mapping is not yet supported on include nodes (Phase 1). Remove it.", - path: ['with'], - }); + const prototype = + typeof data.with === 'object' && data.with !== null + ? Object.getPrototypeOf(data.with) + : undefined; + if ( + typeof data.with !== 'object' || + data.with === null || + Array.isArray(data.with) || + (prototype !== Object.prototype && prototype !== null) + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "'with' on include nodes must be an object mapping input names to strings", + path: ['with'], + }); + } else { + for (const [key, value] of Object.entries(data.with)) { + if (!INPUT_NAME_PATTERN.test(key)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `invalid include input name '${key}'; use letters, numbers, underscores, or hyphens and start with a letter or underscore`, + path: ['with'], + }); + } + if (typeof value !== 'string') { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `include input '${key}' must be a string`, + path: ['with'], + }); + } + } + } } if (hasWorkflow && data.with !== undefined) { ctx.addIssue({ @@ -1051,13 +1106,17 @@ export const dagNodeSchema = dagNodeFlatSchema return { ...base, ...shared, cancel: data.cancel.trim() } as CancelNode; } if (data.include !== undefined && data.include.trim().length > 0) { - // An include node is a load-time directive, not an executable node. It carries ONLY - // the structural graph fields (shared with `base` via `structuralBase`) plus the - // target name — the expander reads id / depends_on / when / trigger_rule to attach - // the sub-DAG (description just rides along). aiOnly / shared (retry) and the exec-only - // base fields (always_run / output_type / idle_timeout) are intentionally dropped; - // the loader warns about them via INCLUDE_NODE_IGNORED_FIELDS. - return { ...structuralBase, include: data.include.trim() } as IncludeNode; + // An include node is a load-time directive, not an executable node. It carries the + // structural graph fields, target name, and optional load-time input mapping. The + // expander reads those fields to attach and parameterize the sub-DAG (description just + // rides along). aiOnly / shared (retry) and the exec-only base fields (always_run / + // output_type / idle_timeout) are intentionally dropped; the loader warns about them + // via INCLUDE_NODE_IGNORED_FIELDS. + return { + ...structuralBase, + include: data.include.trim(), + ...(data.with !== undefined ? { with: data.with as Record } : {}), + } as IncludeNode; } if (data.workflow !== undefined && data.workflow.trim().length > 0) { // A workflow (sub-run) node makes no direct provider call, so it carries only @@ -1109,6 +1168,11 @@ export const dagNodeSchema = dagNodeFlatSchema // Type guards (preserved from original types.ts) // --------------------------------------------------------------------------- +/** Type guard: check if a DAG node is a command (named command file) node */ +export function isCommandNode(node: DagNode): node is CommandNode { + return 'command' in node && typeof node.command === 'string'; +} + /** Type guard: check if a DAG node is a bash (shell script) node */ export function isBashNode(node: DagNode): node is BashNode { return 'bash' in node && typeof node.bash === 'string'; diff --git a/packages/workflows/src/schemas/index.ts b/packages/workflows/src/schemas/index.ts index 4947067e8f..9efd4bbaf7 100644 --- a/packages/workflows/src/schemas/index.ts +++ b/packages/workflows/src/schemas/index.ts @@ -44,6 +44,8 @@ export { workflowNodeSchema, fanOutConfigSchema, dagNodeSchema, + INPUT_NAME_SOURCE, + isCommandNode, isBashNode, isLoopNode, isLoopGroupNode, diff --git a/packages/workflows/src/workflow-discovery.ts b/packages/workflows/src/workflow-discovery.ts index 4ba5450629..44f8d895a7 100644 --- a/packages/workflows/src/workflow-discovery.ts +++ b/packages/workflows/src/workflow-discovery.ts @@ -32,6 +32,7 @@ import { createLogger } from '@archon/paths'; import { isValidCommandName, MAX_DISCOVERY_DEPTH } from './command-validation'; import { parseWorkflow } from './loader'; import { expandWorkflowIncludes } from './include-expander'; +import { getFileBackedCommandName } from './command-file'; /** Lazy-initialized logger (deferred so test mocks can intercept createLogger) */ let cachedLog: ReturnType | undefined; @@ -269,10 +270,10 @@ async function resolveCommandContentForScan( } /** - * Pre-resolve command-file contents for every `command:` node that lives in a workflow - * reachable as an `include:` target (transitively). The include expander uses these to - * detect a block command file referencing a sibling id that namespacing renames. Touches - * disk only when includes exist; returns an empty map otherwise. + * Pre-resolve command-file contents for every file-backed command node (including + * `loop.command`) that lives in a workflow reachable as an `include:` target + * (transitively). The include expander uses these to validate deferred prompt bodies. + * Touches disk only when includes exist; returns an empty map otherwise. */ async function resolveIncludeBlockCommandContents( cwd: string | null, @@ -297,8 +298,9 @@ async function resolveIncludeBlockCommandContents( const workflow = byName.get(name); if (!workflow) continue; for (const node of workflow.nodes) { - if ('command' in node && typeof node.command === 'string' && !contents.has(node.command)) { - contents.set(node.command, await resolveCommandContentForScan(cwd, node.command, config)); + const commandName = getFileBackedCommandName(node); + if (commandName !== undefined && !contents.has(commandName)) { + contents.set(commandName, await resolveCommandContentForScan(cwd, commandName, config)); } } }