feat: runtime workflow inputs (--set KEY=VALUE / inputs API field)#1338
Open
gemmawood wants to merge 19 commits intocoleam00:devfrom
Open
feat: runtime workflow inputs (--set KEY=VALUE / inputs API field)#1338gemmawood wants to merge 19 commits intocoleam00:devfrom
gemmawood wants to merge 19 commits intocoleam00:devfrom
Conversation
Allows callers to pass key=value pairs at workflow fire time that are
substituted into prompts, model, and provider fields — eliminating the
need for duplicate YAML files per model variant.
Schema:
- Add `inputs:` block to WorkflowDefinition (name → {description,
default, required})
- Add `inputs: z.record(z.string()).optional()` to RunWorkflowBody
Execution:
- `resolveInputs()` merges declared defaults with runtime values and
validates required inputs
- `applyInputsToString()` substitutes $KEY in model/provider before
provider inference runs
- `substituteWorkflowVariables()` and `buildPromptWithContext()` accept
an optional `inputs` map applied before built-in variables
- All DAG node executor functions thread `inputs` through to substitution
Threading:
- HandleMessageContext and WorkflowRoutingContext gain `inputs?`
- orchestrator-agent passes inputs through dispatchOrchestratorWorkflow
and handleWorkflowRunCommand (AI-driven invocation path excluded)
- Background dispatch in orchestrator passes inputs to executeWorkflow
CLI:
- `archon workflow run <name> --set KEY=VALUE` (repeatable flag)
parsed via Node.js parseArgs with `multiple: true`
API:
- POST /api/workflows/:name/run accepts `{ inputs: Record<string,string> }`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds runtime workflow inputs: CLI Changes
Sequence DiagramsequenceDiagram
participant User as User / API Client
participant CLI as CLI Parser
participant Server as Server (API)
participant Orch as Orchestrator
participant Exec as Executor
participant Subst as Substitution Engine
User->>CLI: provide --set KEY=VALUE
User->>Server: POST /api/workflows/:name/run with inputs
CLI->>Orch: dispatch workflow (extraContext.inputs)
Server->>Orch: dispatch workflow (extraContext.inputs)
Orch->>Exec: executeWorkflow(runtimeInputs)
Exec->>Exec: resolveInputs() (merge defaults, validate required)
Exec->>Subst: substituteWorkflowVariables(template, inputs)
Subst-->>Exec: substituted prompts/commands/model/provider
Exec-->>Orch: workflow execution result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/commands/workflow.ts (1)
655-669:⚠️ Potential issue | 🟠 MajorStore runtime inputs in workflow run metadata and restore them on resume/approval/rejection.
The initial
workflowRunCommandpassesoptions.inputstoexecuteWorkflow(line 668), which resolves them once for model/provider substitution. However,resumeWorkflowCommand,workflowApproveCommand, andworkflowRejectCommandcallworkflowRunCommandwithout inputs (lines 899, 961, 1021), and theWorkflowRunschema stores only genericmetadata—there is noinputsfield. WhenexecuteWorkflowis re-invoked on resume, it receivesundefinedforruntimeInputs, causing validation to fail if the workflow has required inputs that lack defaults.Persist the resolved inputs in run metadata during initial execution and restore them when resuming:
// In executor.ts, after resolveInputs: const resolvedInputs = resolveInputs(workflow.inputs, runtimeInputs); // Store in preCreatedRun metadata for later restoration if (!preCreatedRun?.metadata) preCreatedRun.metadata = {}; preCreatedRun.metadata.resolvedInputs = resolvedInputs; // On resume, retrieve and pass back: const storedInputs = resumableRun?.metadata?.resolvedInputs as Record<string, string> | undefined; await executeWorkflow(..., storedInputs);packages/workflows/src/dag-executor.ts (1)
1873-1889:⚠️ Potential issue | 🟡 MinorMissing
inputsinuntil_bashsubstitution.Every other
substituteWorkflowVariablescall in this file now threadsinputs(bash, script, loop prompt, approval on_reject). Theloop.until_bashexpression is the one exception and will still receive the raw$KEYliteral if a user references a runtime input in their bash termination condition — quietly producing the wrong exit-code evaluation and either an infinite loop or premature completion.🐛 Proposed fix
const { prompt: bashPrompt } = substituteWorkflowVariables( loop.until_bash, workflowRun.id, workflowRun.user_message, artifactsDir, baseBranch, docsDir, - issueContext + issueContext, + undefined, + undefined, + inputs );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.ts` around lines 1873 - 1889, The call to substituteWorkflowVariables for loop.until_bash omits the inputs parameter, so runtime inputs referenced in loop.until_bash (loop.until_bash) are not substituted and can break loop evaluation; update the substituteWorkflowVariables invocation used for loop.until_bash to pass the current inputs object (same way other calls do), ensuring you call substituteWorkflowVariables(loop.until_bash, workflowRun.id, workflowRun.user_message, artifactsDir, baseBranch, docsDir, issueContext, inputs) before feeding the result into substituteNodeOutputRefs and execFileAsync.packages/core/src/orchestrator/orchestrator-agent.ts (1)
671-680:⚠️ Potential issue | 🟠 MajorNL approval resume drops runtime inputs — required-input workflows will fail on approval.
Lines 672–680 call
dispatchOrchestratorWorkflowwithout passinginputs, defaulting the parameter toundefined. When the executor callsresolveInputson resume, it throws"Required workflow input \"X\" was not provided"for any required input without a default — even though the workflow was originally invoked with that input via API or--set.The
WorkflowRunrecord stores only approval context in metadata (e.g.,approval,loop_user_input); no field exists to persist the original inputs for rehydration on resume. Compare to the web foreground resume path (line 302), which correctly passesinputstoexecuteWorkflow.Store original resolved inputs in
metadataduring workflow creation and restore them in the NL approval resume,/workflow approve, and/workflow resumepaths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 671 - 680, The resume path calls dispatchOrchestratorWorkflow without the original resolved inputs causing resolveInputs to fail; update the workflow creation flow to persist the resolved inputs into the WorkflowRun.metadata (e.g., metadata.resolved_inputs) when the run is first created, ensure they are serialized safely, and update the NL approval resume code (the block that references pausedRun and calls dispatchOrchestratorWorkflow), the `/workflow approve` and `/workflow resume` handlers to read pausedRun.metadata.resolved_inputs and pass that object as the inputs argument into dispatchOrchestratorWorkflow (matching how executeWorkflow is called on the web foreground path) so resolveInputs can rehydrate required values; also add null-safe checks and any necessary type conversions when reading/writing metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/cli.ts`:
- Line 207: The CLI flag --set is defined (set: { type: 'string', multiple: true
}) but not documented in the help text; update the function printUsage() to
include a concise entry for --set (showing it accepts multiple key=value pairs
or file references), add a short description of its purpose (runtime input
override), and include an example invocation demonstrating multiple uses (e.g.
--set key1=val1 --set key2=val2). Ensure the help output formatting matches
other flags in printUsage() so --set appears in the options list and examples.
In `@packages/workflows/src/executor-shared.ts`:
- Around line 306-313: The loop that substitutes runtime inputs (the inputs
object iterating in executor-shared.ts) builds a RegExp from key without
escaping and calls String.prototype.replace with a string replacement, which can
throw for keys containing regex metacharacters and misinterpret replacement
tokens like $& or $$. Fix by escaping the input key before constructing the
RegExp (so names like "FOO(" don't break) and call replace with a function
callback that returns the raw value (instead of a replacement string) to
preserve literal characters; update the code that references result, inputs and
the new RegExp construction accordingly.
In `@packages/workflows/src/executor.ts`:
- Around line 335-355: Node-level provider/model overrides are not being
input-substituted like workflow-level ones; applyInputsToString is used for
workflow.model/provider but resolveNodeProviderAndModel currently reads
node.model/node.provider verbatim causing literals like "$MY_MODEL" to be sent
to isModelCompatible and the SDK. Fix by threading resolvedInputs into
resolveNodeProviderAndModel and calling applyInputsToString on node.model and
node.provider before the existing logic (use the same inference flow as
executor.ts: applyInputsToString -> inferProviderFromModel -> fall back to
config.assistant), updating all references to
nodeResolvedModel/nodeResolvedProvider inside resolveNodeProviderAndModel so
compatibility checks (isModelCompatible) and returned values use the substituted
strings.
- Around line 212-222: The regex built from input keys in applyInputsToString
(and likewise in substituteWorkflowVariables) interpolates raw keys into new
RegExp which breaks or mis-matches when keys contain regex metacharacters; fix
by escaping keys before embedding them in the RegExp or, preferably, reject
invalid keys early: add key validation to workflowBaseSchema (enforce
/^[A-Za-z_][A-Za-z0-9_]*$/) and also validate/normalize CLI/API inputs at
ingestion, and update applyInputsToString and substituteWorkflowVariables to use
an escapedKey helper (or the validated key) when constructing the RegExp so new
RegExp(...) never receives unescaped metacharacters.
- Around line 180-206: reject runtime or declared input keys that collide with
engine-provided workflow variables by adding validation at the start of
resolveInputs: define a reserved set {ARGUMENTS, WORKFLOW_ID, ARTIFACTS_DIR,
BASE_BRANCH, DOCS_DIR, LOOP_USER_INPUT, REJECTION_REASON, USER_MESSAGE} and also
disallow node-output style keys containing a dot (e.g. any key with '.' or
matching '*.output'); if any key in inputsDef or runtimeInputs matches a
reserved name or contains a dot, throw a clear Error referencing resolveInputs
and listing the offending keys so callers get immediate feedback (alternatively
change the thrown Error to a warning log if you prefer non-fatal behavior, but
implement the check in resolveInputs before merging and before the required
check).
In `@packages/workflows/src/schemas/workflow.ts`:
- Around line 90-95: The inputs record currently allows arbitrary keys and can
shadow built-in variables; update the schema for inputs (the symbol inputs that
uses workflowInputSchema) to restrict keys to the uppercase identifier pattern
/^[A-Z][A-Z0-9_]*$/ and validate keys are not one of the reserved names
(WORKFLOW_ID, USER_MESSAGE, ARGUMENTS, ARTIFACTS_DIR, BASE_BRANCH, DOCS_DIR,
LOOP_USER_INPUT, REJECTION_REASON, CONTEXT, EXTERNAL_CONTEXT, ISSUE_CONTEXT);
implement this by replacing the unconstrained z.record(workflowInputSchema) with
a keyed-record validation that enforces the regex on each key and rejects any
key in the reserved set (using a refinement/check on record keys) so inputs
cannot shadow built-in variables used by
substituteWorkflowVariables/applyInputsToString.
---
Outside diff comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 671-680: The resume path calls dispatchOrchestratorWorkflow
without the original resolved inputs causing resolveInputs to fail; update the
workflow creation flow to persist the resolved inputs into the
WorkflowRun.metadata (e.g., metadata.resolved_inputs) when the run is first
created, ensure they are serialized safely, and update the NL approval resume
code (the block that references pausedRun and calls
dispatchOrchestratorWorkflow), the `/workflow approve` and `/workflow resume`
handlers to read pausedRun.metadata.resolved_inputs and pass that object as the
inputs argument into dispatchOrchestratorWorkflow (matching how executeWorkflow
is called on the web foreground path) so resolveInputs can rehydrate required
values; also add null-safe checks and any necessary type conversions when
reading/writing metadata.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1873-1889: The call to substituteWorkflowVariables for
loop.until_bash omits the inputs parameter, so runtime inputs referenced in
loop.until_bash (loop.until_bash) are not substituted and can break loop
evaluation; update the substituteWorkflowVariables invocation used for
loop.until_bash to pass the current inputs object (same way other calls do),
ensuring you call substituteWorkflowVariables(loop.until_bash, workflowRun.id,
workflowRun.user_message, artifactsDir, baseBranch, docsDir, issueContext,
inputs) before feeding the result into substituteNodeOutputRefs and
execFileAsync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e4ff86e-698a-4b20-97b5-0866fcd16672
📒 Files selected for processing (11)
packages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator.tspackages/core/src/types/index.tspackages/server/src/routes/api.tspackages/server/src/routes/schemas/workflow.schemas.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-shared.tspackages/workflows/src/executor.tspackages/workflows/src/schemas/workflow.ts
| 'download-only': { type: 'boolean' }, | ||
| scope: { type: 'string' }, | ||
| force: { type: 'boolean' }, | ||
| set: { type: 'string', multiple: true }, |
There was a problem hiding this comment.
Document --set in CLI usage.
The flag is parsed, but printUsage() does not list it or show an example, so the new runtime-input feature is hard to discover from archon help.
📝 Proposed help text update
--json Output machine-readable JSON (for workflow list)
+ --set KEY=VALUE Set a workflow runtime input; repeatable for multiple inputs
--workflow <name> Workflow to run for 'continue' (default: archon-assist) archon workflow run plan --cwd /path/to/repo "Add dark mode"
+ archon workflow run plan --set TICKET=123 --set PRIORITY=high "Add dark mode"
archon workflow run implement --branch feature-auth "Implement auth"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/cli.ts` at line 207, The CLI flag --set is defined (set: {
type: 'string', multiple: true }) but not documented in the help text; update
the function printUsage() to include a concise entry for --set (showing it
accepts multiple key=value pairs or file references), add a short description of
its purpose (runtime input override), and include an example invocation
demonstrating multiple uses (e.g. --set key1=val1 --set key2=val2). Ensure the
help output formatting matches other flags in printUsage() so --set appears in
the options list and examples.
Comment on lines
+306
to
+313
| // Apply runtime inputs first — substitute $KEY patterns before built-in variables. | ||
| // This allows input values to contain built-in variable references if needed. | ||
| let result = prompt; | ||
| if (inputs) { | ||
| for (const [key, value] of Object.entries(inputs)) { | ||
| result = result.replace(new RegExp(`\\$${key}(?![A-Za-z0-9_])`, 'g'), value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check its size
wc -l packages/workflows/src/executor-shared.tsRepository: coleam00/Archon
Length of output: 104
🏁 Script executed:
# Read the section around lines 306-313 with context
sed -n '290,320p' packages/workflows/src/executor-shared.ts | cat -nRepository: coleam00/Archon
Length of output: 1567
🏁 Script executed:
# Search for escapeRegExp function in the file
grep -n "escapeRegExp" packages/workflows/src/executor-shared.tsRepository: coleam00/Archon
Length of output: 367
🏁 Script executed:
# Get full function context for substituteWorkflowVariables
ast-grep --pattern $'function substituteWorkflowVariables($$$) {
$$$
}'Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Read the escapeRegExp function definition
sed -n '399,410p' packages/workflows/src/executor-shared.ts | cat -nRepository: coleam00/Archon
Length of output: 577
🏁 Script executed:
# Check the full context and understand the implementation better
sed -n '390,440p' packages/workflows/src/executor-shared.ts | cat -nRepository: coleam00/Archon
Length of output: 2506
🏁 Script executed:
# Verify the regex escaping concern with a test
python3 << 'EOF'
import re
# Test 1: Unescaped metacharacters in regex - should throw
try:
pattern = re.compile(f"\\${'FOO('}(?![A-Za-z0-9_])", re.MULTILINE)
print("Test 1 PASSED: No error with unescaped FOO(")
except Exception as e:
print(f"Test 1 FAILED: {type(e).__name__}: {e}")
# Test 2: Replacement token behavior - does it interpret $& and $$?
test_string = "The value is $FOO"
value = "TEST_$&_VALUE" # Contains replacement token
try:
result = re.sub(r"\$FOO(?![A-Za-z0-9_])", value, test_string)
print(f"Test 2: Raw string result: {result}")
if "$&" in result:
print("Test 2: Replacement tokens NOT interpreted (Python re.sub behaves like callback)")
except Exception as e:
print(f"Test 2 ERROR: {e}")
# Test 3: JavaScript-like behavior - in Python str.replace doesn't support special tokens like $&
# But let's check what JavaScript does with raw replacement strings
print("\nNote: JavaScript String.prototype.replace DOES interpret special tokens:")
print(" - $& = matched substring")
print(" - $$ = literal $")
print(" - $` = substring before match")
print(" - $' = substring after match")
print(" - $n = nth capture group")
EOFRepository: coleam00/Archon
Length of output: 474
Escape input keys and use a replacement callback.
key is interpolated into a regex without escaping, so an input name like FOO( will throw ("unterminated subpattern"). Also pass value via a callback so replacement tokens like $& (matched substring) and $$ (literal $) remain literal user input instead of being reinterpreted by JavaScript's String.prototype.replace.
🛡️ Proposed fix
if (inputs) {
for (const [key, value] of Object.entries(inputs)) {
- result = result.replace(new RegExp(`\\$${key}(?![A-Za-z0-9_])`, 'g'), value);
+ const escapedKey = escapeRegExp(key);
+ result = result.replace(new RegExp(`\\$${escapedKey}(?![A-Za-z0-9_])`, 'g'), () => value);
}
}🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 310-310: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\$${key}(?![A-Za-z0-9_]), 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/executor-shared.ts` around lines 306 - 313, The loop
that substitutes runtime inputs (the inputs object iterating in
executor-shared.ts) builds a RegExp from key without escaping and calls
String.prototype.replace with a string replacement, which can throw for keys
containing regex metacharacters and misinterpret replacement tokens like $& or
$$. Fix by escaping the input key before constructing the RegExp (so names like
"FOO(" don't break) and call replace with a function callback that returns the
raw value (instead of a replacement string) to preserve literal characters;
update the code that references result, inputs and the new RegExp construction
accordingly.
Comment on lines
+90
to
+95
| /** | ||
| * Named input parameters for this workflow. Values are substituted at run time | ||
| * via `$INPUT_NAME` syntax in prompts and in top-level `model`/`provider` fields. | ||
| * Callers supply values via `--set KEY=VALUE` (CLI) or `inputs` (API/SDK). | ||
| */ | ||
| inputs: z.record(workflowInputSchema).optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the workflow.ts schema file
cat -n packages/workflows/src/schemas/workflow.ts | head -100Repository: coleam00/Archon
Length of output: 5029
🏁 Script executed:
# Search for substitution logic and built-in variable handling
rg -A 10 "substitut|WORKFLOW_ID|ARGUMENTS|ARTIFACTS_DIR|BASE_BRANCH" packages/workflows/src --type ts | head -150Repository: coleam00/Archon
Length of output: 12378
🏁 Script executed:
# Search for substituteWorkflowVariables to understand the full substitution order
rg -B 5 -A 30 "substituteWorkflowVariables" packages/workflows/src/executor.ts | head -100Repository: coleam00/Archon
Length of output: 3190
🏁 Script executed:
# Search for the actual substituteWorkflowVariables function definition
rg -B 5 -A 50 "function substituteWorkflowVariables" packages/workflows/src/executor.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Search for substituteWorkflowVariables more broadly
rg "substituteWorkflowVariables" packages/workflows/src --type ts -B 3 -A 15Repository: coleam00/Archon
Length of output: 33732
🏁 Script executed:
# Get the full implementation of substituteWorkflowVariables
sed -n '1,200p' packages/workflows/src/executor-shared.ts | head -150Repository: coleam00/Archon
Length of output: 4729
🏁 Script executed:
# Continue reading executor-shared.ts to find substituteWorkflowVariables implementation
sed -n '200,400p' packages/workflows/src/executor-shared.tsRepository: coleam00/Archon
Length of output: 7847
🏁 Script executed:
# Check Zod z.record() API usage - verify the two-argument form
rg "z\.record\(" packages/workflows/src --type ts -B 2 -A 2Repository: coleam00/Archon
Length of output: 2143
🏁 Script executed:
# Verify Zod z.record() API - check Zod documentation or types
npm list zod 2>/dev/null | head -5Repository: coleam00/Archon
Length of output: 103
🏁 Script executed:
# Check package.json for zod version
cat packages/workflows/package.json | grep -A 5 -B 5 "zod"Repository: coleam00/Archon
Length of output: 313
🏁 Script executed:
# Check how z.record() is used in tests or other parts of the codebase
rg "z\.record\(" packages/workflows --type ts -B 1 -A 3 | head -60Repository: coleam00/Archon
Length of output: 2139
🏁 Script executed:
# Search for workflow input tests or examples
rg "inputs:" packages/workflows --type yaml -B 2 -A 5 | head -100Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
# Search for workflow input tests in TypeScript
rg "inputs.*:" packages/workflows/src --type ts -B 2 -A 5 | grep -A 5 "inputs" | head -80Repository: coleam00/Archon
Length of output: 6011
Constrain workflow input names to prevent shadowing built-in variables.
The inputs schema accepts any YAML key, allowing input names to collide with built-in workflow variables. Because runtime inputs are substituted before built-in variables (in applyInputsToString → substituteWorkflowVariables), an input named WORKFLOW_ID or BASE_BRANCH will shadow the documented variable and break execution semantics.
Enforce uppercase identifier syntax ([A-Z][A-Z0-9_]*) and reject reserved names: WORKFLOW_ID, USER_MESSAGE, ARGUMENTS, ARTIFACTS_DIR, BASE_BRANCH, DOCS_DIR, LOOP_USER_INPUT, REJECTION_REASON, CONTEXT, EXTERNAL_CONTEXT, ISSUE_CONTEXT.
🛡️ Proposed schema constraint
+const reservedWorkflowInputNames = new Set([
+ 'WORKFLOW_ID',
+ 'USER_MESSAGE',
+ 'ARGUMENTS',
+ 'ARTIFACTS_DIR',
+ 'BASE_BRANCH',
+ 'CONTEXT',
+ 'EXTERNAL_CONTEXT',
+ 'ISSUE_CONTEXT',
+ 'DOCS_DIR',
+ 'LOOP_USER_INPUT',
+ 'REJECTION_REASON',
+]);
+
+const workflowInputNameSchema = z
+ .string()
+ .regex(/^[A-Z][A-Z0-9_]*$/, 'Input names must use uppercase identifier syntax')
+ .refine(name => !reservedWorkflowInputNames.has(name), {
+ message: 'Input name is reserved for a built-in workflow variable',
+ });
+
/**
* Named input parameters for this workflow. Values are substituted at run time
* via `$INPUT_NAME` syntax in prompts and in top-level `model`/`provider` fields.
* Callers supply values via `--set KEY=VALUE` (CLI) or `inputs` (API/SDK).
*/
- inputs: z.record(workflowInputSchema).optional(),
+ inputs: z.record(workflowInputNameSchema, workflowInputSchema).optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Named input parameters for this workflow. Values are substituted at run time | |
| * via `$INPUT_NAME` syntax in prompts and in top-level `model`/`provider` fields. | |
| * Callers supply values via `--set KEY=VALUE` (CLI) or `inputs` (API/SDK). | |
| */ | |
| inputs: z.record(workflowInputSchema).optional(), | |
| const reservedWorkflowInputNames = new Set([ | |
| 'WORKFLOW_ID', | |
| 'USER_MESSAGE', | |
| 'ARGUMENTS', | |
| 'ARTIFACTS_DIR', | |
| 'BASE_BRANCH', | |
| 'CONTEXT', | |
| 'EXTERNAL_CONTEXT', | |
| 'ISSUE_CONTEXT', | |
| 'DOCS_DIR', | |
| 'LOOP_USER_INPUT', | |
| 'REJECTION_REASON', | |
| ]); | |
| const workflowInputNameSchema = z | |
| .string() | |
| .regex(/^[A-Z][A-Z0-9_]*$/, 'Input names must use uppercase identifier syntax') | |
| .refine(name => !reservedWorkflowInputNames.has(name), { | |
| message: 'Input name is reserved for a built-in workflow variable', | |
| }); | |
| /** | |
| * Named input parameters for this workflow. Values are substituted at run time | |
| * via `$INPUT_NAME` syntax in prompts and in top-level `model`/`provider` fields. | |
| * Callers supply values via `--set KEY=VALUE` (CLI) or `inputs` (API/SDK). | |
| */ | |
| inputs: z.record(workflowInputNameSchema, workflowInputSchema).optional(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/schemas/workflow.ts` around lines 90 - 95, The inputs
record currently allows arbitrary keys and can shadow built-in variables; update
the schema for inputs (the symbol inputs that uses workflowInputSchema) to
restrict keys to the uppercase identifier pattern /^[A-Z][A-Z0-9_]*$/ and
validate keys are not one of the reserved names (WORKFLOW_ID, USER_MESSAGE,
ARGUMENTS, ARTIFACTS_DIR, BASE_BRANCH, DOCS_DIR, LOOP_USER_INPUT,
REJECTION_REASON, CONTEXT, EXTERNAL_CONTEXT, ISSUE_CONTEXT); implement this by
replacing the unconstrained z.record(workflowInputSchema) with a keyed-record
validation that enforces the regex on each key and rejects any key in the
reserved set (using a refinement/check on record keys) so inputs cannot shadow
built-in variables used by substituteWorkflowVariables/applyInputsToString.
Three issues flagged in PR review:
1. loop.until_bash substituteWorkflowVariables omitted the inputs
parameter, so $INPUT_NAME references in until_bash conditions were
not substituted. Pass inputs as the last argument.
2. Resolved inputs were not persisted when a workflow run was first
created, meaning they were lost on any resume path. Store them in
WorkflowRun.metadata.resolved_inputs at createWorkflowRun time.
3. Resume paths did not restore the original runtime inputs:
- Natural-language approval: read pausedRun.metadata.resolved_inputs
and pass to dispatchOrchestratorWorkflow so resolveInputs can
rehydrate them on the resumed execution.
- findResumableRun path: after activating the resume, fall back to
resumableRun.metadata.resolved_inputs when no runtimeInputs were
supplied by the caller.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workflows/src/executor.ts (1)
524-554:⚠️ Potential issue | 🟠 Major
resolved_inputsnot persisted on the orchestrator (preCreatedRun) path — breaks resume for the main dispatch path.The metadata block at lines 527-531 only runs inside
if (!workflowRun). When the orchestrator dispatches viapreCreatedRun, this branch is skipped entirely, soresolved_inputsis never written toWorkflowRun.metadata. The resume-restore logic at lines 448-457 then has nothing to read back, and$INPUT_NAMEsubstitutions are lost across pause/resume for the exact path the PR's resume fix is meant to cover (orchestrator-created runs are the common case).Either update the pre-created run's metadata here (merge
resolved_inputsinto existing metadata viaupdateWorkflowRun), or ensure the orchestrator persists it at creation time. Also consider strippingresolved_inputsbefore overwritingmetadatain any future write to avoid losing it.🔧 Sketch of one approach
if (!workflowRun) { // Create workflow run record try { const runMetadata: Record<string, unknown> = {}; if (issueContext) runMetadata.github_context = issueContext; if (resolvedInputs && Object.keys(resolvedInputs).length > 0) { runMetadata.resolved_inputs = resolvedInputs; } workflowRun = await deps.store.createWorkflowRun({ ... metadata: runMetadata, ... }); } catch (error) { ... } + } else if (resolvedInputs && Object.keys(resolvedInputs).length > 0 + && !(workflowRun.metadata as Record<string, unknown> | undefined)?.resolved_inputs) { + // Backfill resolved_inputs onto the orchestrator-provided run so resume works. + try { + await deps.store.updateWorkflowRun(workflowRun.id, { + metadata: { ...(workflowRun.metadata ?? {}), resolved_inputs: resolvedInputs }, + }); + } catch (err) { + getLog().warn({ err, workflowRunId: workflowRun.id }, 'workflow.persist_resolved_inputs_failed'); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.ts` around lines 524 - 554, The metadata assignment for resolved_inputs only runs when workflowRun is newly created, so when a preCreatedRun path supplies an existing workflowRun you must merge resolved_inputs into that run's metadata instead of skipping it; locate the block around workflowRun and deps.store.createWorkflowRun and, if workflowRun exists and resolvedInputs is non-empty, call deps.store.updateWorkflowRun (or equivalent update method) to merge/append resolved_inputs into workflowRun.metadata (don't overwrite other metadata keys), ensuring resolved_inputs is persisted for resume/restore logic that reads WorkflowRun.metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/executor.ts`:
- Around line 448-457: The current check uses if (!runtimeInputs) which misses
an empty object ({}), so replace it with an explicit check like if
(runtimeInputs == null || Object.keys(runtimeInputs).length === 0) to ensure
resume restoration runs when the caller supplied an empty inputs map; inside
that block validate resumableRun.metadata?.resolved_inputs (currently cast as
Record<string,string>) by iterating its entries and building a filtered
Record<string,string> that includes only string values (or coerce/skip
non-strings) before passing to resolveInputs(workflow.inputs, storedInputs) and
assigning to resolvedInputs.
---
Outside diff comments:
In `@packages/workflows/src/executor.ts`:
- Around line 524-554: The metadata assignment for resolved_inputs only runs
when workflowRun is newly created, so when a preCreatedRun path supplies an
existing workflowRun you must merge resolved_inputs into that run's metadata
instead of skipping it; locate the block around workflowRun and
deps.store.createWorkflowRun and, if workflowRun exists and resolvedInputs is
non-empty, call deps.store.updateWorkflowRun (or equivalent update method) to
merge/append resolved_inputs into workflowRun.metadata (don't overwrite other
metadata keys), ensuring resolved_inputs is persisted for resume/restore logic
that reads WorkflowRun.metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01e569d2-755a-477a-a753-77766cc2ce64
📒 Files selected for processing (3)
packages/core/src/orchestrator/orchestrator-agent.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workflows/src/dag-executor.ts
- packages/core/src/orchestrator/orchestrator-agent.ts
- Validate stored inputs on resume by filtering to string-only values, guarding against non-string or malformed metadata entries - Persist resolved_inputs into metadata when workflowRun already exists via preCreatedRun, so resume-path restores work correctly if interrupted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/workflows/src/executor.ts (1)
212-220:⚠️ Potential issue | 🟠 MajorEscape or validate input keys before building the RegExp.
Line 219 still interpolates runtime input keys directly into
new RegExp(...); keys likeFOO.BARmatch unintended tokens, and malformed keys likeBAD[can throw during workflow execution. This was already flagged in the previous review, but remains present here.🛡️ Proposed fix
+const REGEX_META = /[.*+?^${}()|[\]\\]/g; +function escapeForRegex(value: string): string { + return value.replace(REGEX_META, '\\$&'); +} + function applyInputsToString( value: string | undefined, inputs: Record<string, string> ): string | undefined { if (!value || Object.keys(inputs).length === 0) return value; let result = value; for (const [key, val] of Object.entries(inputs)) { - result = result.replace(new RegExp(`\\$${key}(?![A-Za-z0-9_])`, 'g'), val); + result = result.replace(new RegExp(`\\$${escapeForRegex(key)}(?![A-Za-z0-9_])`, 'g'), val); } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.ts` around lines 212 - 220, The applyInputsToString function builds a RegExp from runtime keys without escaping, which can throw or match incorrectly; add a small helper (e.g., escapeRegExp) and use it when constructing the regex for each key so metacharacters in key (like '.' or '[') are escaped before calling new RegExp, i.e., replace new RegExp(`\\$${key}(?![A-Za-z0-9_])`, 'g') with a regex that uses escapeRegExp(key); optionally validate keys against an allowed charset before escaping if you want stricter input rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/executor.ts`:
- Around line 331-357: The code currently calls resolveInputs(...) to produce
resolvedInputs and then immediately computes effectiveModel/effectiveProvider
via applyInputsToString(workflow.model, resolvedInputs) and
applyInputsToString(workflow.provider, resolvedInputs) before the resume block
that may replace resolvedInputs from metadata.resolved_inputs; move the resume
restoration of metadata.resolved_inputs to before resolving inputs into
provider/model (i.e., restore/merge metadata.resolved_inputs into resolvedInputs
prior to calling applyInputsToString) or, if you prefer a smaller change,
recompute effectiveModel/effectiveProvider and resolvedProvider/resolvedModel
immediately after restoring metadata.resolved_inputs so that applyInputsToString
uses the resumed values for workflow.model/workflow.provider.
---
Duplicate comments:
In `@packages/workflows/src/executor.ts`:
- Around line 212-220: The applyInputsToString function builds a RegExp from
runtime keys without escaping, which can throw or match incorrectly; add a small
helper (e.g., escapeRegExp) and use it when constructing the regex for each key
so metacharacters in key (like '.' or '[') are escaped before calling new
RegExp, i.e., replace new RegExp(`\\$${key}(?![A-Za-z0-9_])`, 'g') with a regex
that uses escapeRegExp(key); optionally validate keys against an allowed charset
before escaping if you want stricter input rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e3d08f6-70b4-4661-a174-88a08ab41e37
📒 Files selected for processing (1)
packages/workflows/src/executor.ts
…r resume - Escape regex metacharacters in applyInputsToString so keys like FOO.BAR or BAD[ don't cause unexpected matches or throw at runtime - Move effectiveModel/effectiveProvider computation to after the resume block so model/provider always reflects the final resolvedInputs (resume may restore different inputs from stored metadata) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/workflows/src/executor.ts (2)
336-340:⚠️ Potential issue | 🟠 MajorRestore stored inputs before required validation.
Line 340 can throw for a required input before the resume block can load
metadata.resolved_inputs. Also, Line 417 treats{}as caller-supplied inputs and skips restoration, losing stored overrides on resume. DeferresolveInputs(...)until after resume input selection, and treat empty runtime inputs as absent.🐛 Suggested restructuring
- let resolvedInputs = resolveInputs(workflow.inputs, runtimeInputs); + let inputsForResolution = runtimeInputs; + let resolvedInputs: Record<string, string>; ... - if (!runtimeInputs) { + const hasRuntimeInputs = runtimeInputs && Object.keys(runtimeInputs).length > 0; + if (!hasRuntimeInputs) { const rawStoredInputs = resumableRun.metadata?.resolved_inputs; ... if (storedInputs && Object.keys(storedInputs).length > 0) { - resolvedInputs = resolveInputs(workflow.inputs, storedInputs); + inputsForResolution = storedInputs; } } ... + resolvedInputs = resolveInputs(workflow.inputs, inputsForResolution); + // Resolve provider and model now that resolvedInputs is final (resume may have updated it).Also applies to: 415-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.ts` around lines 336 - 340, The code calls resolveInputs(workflow.inputs, runtimeInputs) too early, which can validate required inputs before stored metadata.resolved_inputs is restored and also treats an empty {} runtimeInputs as an explicit caller value; to fix, move the call to resolveInputs until after the resume/metadata-loading block (use metadata.resolved_inputs when present to restore prior overrides before validation), treat an empty object runtimeInputs as "absent" (i.e., only merge/override when runtimeInputs has keys), and ensure the final resolvedInputs value comes from resolveInputs(workflow.inputs, mergedRuntimeInputs) after restoration so required-input validation uses the restored inputs; changes affect any code referencing resolvedInputs, resolveInputs, runtimeInputs, and metadata.resolved_inputs.
180-206:⚠️ Potential issue | 🟡 MinorReject input keys that collide with engine variables.
resolveInputsstill accepts keys likeARGUMENTS,WORKFLOW_ID, orfoo.output, which can silently shadow built-in variables/node outputs during substitution. Add a fast validation step for declared and runtime keys before merging.🛡️ Suggested guard
+const RESERVED_WORKFLOW_INPUT_KEYS = new Set([ + 'ARGUMENTS', + 'WORKFLOW_ID', + 'ARTIFACTS_DIR', + 'BASE_BRANCH', + 'DOCS_DIR', + 'LOOP_USER_INPUT', + 'REJECTION_REASON', +]); + +function assertSafeInputKeys(...sources: Array<Record<string, unknown> | undefined>): void { + const invalid = sources + .flatMap(source => (source ? Object.keys(source) : [])) + .filter( + key => + key.length === 0 || + RESERVED_WORKFLOW_INPUT_KEYS.has(key) || + /^[A-Za-z0-9_-]+\.output$/.test(key) + ); + + if (invalid.length > 0) { + throw new Error(`Invalid workflow input key(s): ${invalid.join(', ')}`); + } +} + function resolveInputs( inputsDef: Record<string, WorkflowInput> | undefined, runtimeInputs: Record<string, string> | undefined ): Record<string, string> { + assertSafeInputKeys(inputsDef, runtimeInputs); const resolved: Record<string, string> = {};As per coding guidelines, Workflow variable substitution supports:
$1,$2,$3,$ARGUMENTS,$ARTIFACTS_DIR,$WORKFLOW_ID,$BASE_BRANCH,$DOCS_DIR,$LOOP_USER_INPUT,$REJECTION_REASON.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.ts` around lines 180 - 206, resolveInputs currently allows input names that collide with engine variables or node outputs; before merging inputsDef and runtimeInputs, add a fast validation that scans both declared keys (inputsDef) and runtime keys (runtimeInputs) and throws an Error if any key matches a reserved engine name (ARGUMENTS, ARTIFACTS_DIR, WORKFLOW_ID, BASE_BRANCH, DOCS_DIR, LOOP_USER_INPUT, REJECTION_REASON) or if a key would shadow node outputs (e.g., endsWith('.output') or contains '.output'); implement this check at the top of resolveInputs (using inputsDef and runtimeInputs symbols) so the invalid key is rejected early with a clear error message instead of silently shadowing built-ins.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/workflows/src/executor.ts`:
- Around line 527-535: The log event name 'workflow_provider_resolved' used in
the getLog().info call should follow the repository's structured naming
convention; change the event string to 'workflow.provider_resolved' where
getLog().info is invoked (the call referencing workflow.name, resolvedProvider,
providerSource, resolvedModel) so the domain separator is included and the event
matches {domain}.{action}_{state} formatting.
- Around line 223-224: The replacement currently uses result.replace(regex, val)
which treats $ sequences in val specially; change the replacement to use a
callback so the literal input is inserted unchanged: in the loop that iterates
over Object.entries(inputs) (the variables result, inputs and escapeForRegex),
replace the second argument with a function that returns the input value (e.g.,
() => val) instead of the raw string so $-sequences are not interpreted by
String.prototype.replace.
---
Duplicate comments:
In `@packages/workflows/src/executor.ts`:
- Around line 336-340: The code calls resolveInputs(workflow.inputs,
runtimeInputs) too early, which can validate required inputs before stored
metadata.resolved_inputs is restored and also treats an empty {} runtimeInputs
as an explicit caller value; to fix, move the call to resolveInputs until after
the resume/metadata-loading block (use metadata.resolved_inputs when present to
restore prior overrides before validation), treat an empty object runtimeInputs
as "absent" (i.e., only merge/override when runtimeInputs has keys), and ensure
the final resolvedInputs value comes from resolveInputs(workflow.inputs,
mergedRuntimeInputs) after restoration so required-input validation uses the
restored inputs; changes affect any code referencing resolvedInputs,
resolveInputs, runtimeInputs, and metadata.resolved_inputs.
- Around line 180-206: resolveInputs currently allows input names that collide
with engine variables or node outputs; before merging inputsDef and
runtimeInputs, add a fast validation that scans both declared keys (inputsDef)
and runtime keys (runtimeInputs) and throws an Error if any key matches a
reserved engine name (ARGUMENTS, ARTIFACTS_DIR, WORKFLOW_ID, BASE_BRANCH,
DOCS_DIR, LOOP_USER_INPUT, REJECTION_REASON) or if a key would shadow node
outputs (e.g., endsWith('.output') or contains '.output'); implement this check
at the top of resolveInputs (using inputsDef and runtimeInputs symbols) so the
invalid key is rejected early with a clear error message instead of silently
shadowing built-ins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81791c38-0978-436a-af77-e25269dc11e3
📒 Files selected for processing (1)
packages/workflows/src/executor.ts
…putsToString
- Use () => val callback in applyInputsToString so $-sequences in input
values (e.g. $1) are not interpreted as backreferences by String.replace
- Add assertSafeInputKeys guard in resolveInputs to reject reserved engine
variable names (ARGUMENTS, WORKFLOW_ID, etc.) and node output shadowing
- Move resolveInputs call to after the resume block; treat empty {} runtimeInputs
as absent; merge storedInputsFromResume + runtimeInputs once so required-input
validation runs with the full restored input set
- Change log event 'workflow_provider_resolved' → 'workflow.provider_resolved'
to match the repository's domain.action_state naming convention
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/workflows/src/executor.ts (1)
175-193:⚠️ Potential issue | 🟠 MajorTighten input-key validation to match the substitution grammar.
assertSafeInputKeysstill allows unsafe shapes likeFOO-BAR,A[,USER_MESSAGE, and positional names.FOO-BARcan be mis-substituted because the boundary is(?![A-Za-z0-9_]), so$FOOmatches the prefix of$FOO-BAR; metacharacter keys also remain risky for downstream prompt substitution.Prefer one explicit key grammar plus a complete reserved set:
🛡️ Proposed validation tightening
const RESERVED_WORKFLOW_INPUT_KEYS = new Set([ + '1', + '2', + '3', 'ARGUMENTS', + 'USER_MESSAGE', 'WORKFLOW_ID', 'ARTIFACTS_DIR', 'BASE_BRANCH', 'DOCS_DIR', 'LOOP_USER_INPUT', 'REJECTION_REASON', + 'CONTEXT', + 'EXTERNAL_CONTEXT', + 'ISSUE_CONTEXT', ]); +const WORKFLOW_INPUT_KEY_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/; + function assertSafeInputKeys(...sources: (Record<string, unknown> | undefined)[]): void { const invalid = sources .flatMap(source => (source ? Object.keys(source) : [])) .filter( key => - key.length === 0 || + !WORKFLOW_INPUT_KEY_PATTERN.test(key) || RESERVED_WORKFLOW_INPUT_KEYS.has(key) || - /^[A-Za-z0-9_-]+\.output$/.test(key) + key.includes('.') ); if (invalid.length > 0) { - throw new Error(`Invalid workflow input key(s): ${invalid.join(', ')}`); + throw new Error( + `Invalid workflow input key(s): ${invalid.join(', ')}. ` + + 'Use /^[A-Za-z_][A-Za-z0-9_]*$/ and avoid reserved workflow variables.' + ); } }#!/bin/bash # Description: Demonstrate why the current key grammar is unsafe and inspect downstream input substitution. # Expected: # - The hyphenated-key demo prints "x-BAR", showing prefix substitution. # - Any raw key interpolation in executor-shared.ts should be fixed or made unreachable by stricter key validation. node <<'NODE' const REGEX_META = /[.*+?^${}()|[\]\\]/g; const escapeForRegex = s => s.replace(REGEX_META, '\\$&'); function applyInputsToString(value, inputs) { let result = value; for (const [key, val] of Object.entries(inputs)) { result = result.replace( new RegExp(`\\$${escapeForRegex(key)}(?![A-Za-z0-9_])`, 'g'), () => val ); } return result; } console.log(applyInputsToString('$FOO-BAR', { FOO: 'x', 'FOO-BAR': 'y' })); NODE rg -n -C3 'Object\.entries\(inputs\)|new RegExp\(`\\\\\$\$\{key\}|replace\(.*value\)' packages/workflows/src/executor-shared.ts packages/workflows/src/executor.tsAs per coding guidelines, workflow variable substitution supports
$1,$2,$3,$ARGUMENTS,$ARTIFACTS_DIR,$WORKFLOW_ID,$BASE_BRANCH,$DOCS_DIR,$LOOP_USER_INPUT, and$REJECTION_REASON.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.ts` around lines 175 - 193, The input-key validator in assertSafeInputKeys is too permissive; tighten it to only allow either positional numeric names (/^\d+$/) or identifier-style names (/^[A-Za-z_][A-Za-z0-9_]*$/) and keep rejecting reserved names (RESERVED_WORKFLOW_INPUT_KEYS) and keys matching /\.output$/; update the invalid calculation inside assertSafeInputKeys (referencing the function assertSafeInputKeys, the constant RESERVED_WORKFLOW_INPUT_KEYS, and the existing /\.output$/ check) so any key that doesn't match the numeric or identifier grammar is treated as invalid and causes the same error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/workflows/src/executor.ts`:
- Around line 175-193: The input-key validator in assertSafeInputKeys is too
permissive; tighten it to only allow either positional numeric names (/^\d+$/)
or identifier-style names (/^[A-Za-z_][A-Za-z0-9_]*$/) and keep rejecting
reserved names (RESERVED_WORKFLOW_INPUT_KEYS) and keys matching /\.output$/;
update the invalid calculation inside assertSafeInputKeys (referencing the
function assertSafeInputKeys, the constant RESERVED_WORKFLOW_INPUT_KEYS, and the
existing /\.output$/ check) so any key that doesn't match the numeric or
identifier grammar is treated as invalid and causes the same error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 032ac1c0-e5f5-49e2-8981-3e61df1d5cad
📒 Files selected for processing (1)
packages/workflows/src/executor.ts
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
Replace the ad-hoc empty/dot-output checks with a strict /^[A-Za-z_][A-Za-z0-9_]*$/ pattern so hyphenated keys (FOO-BAR) and keys with dots are rejected before they can cause prefix-match ambiguity or node-output shadowing during substitution. Expand RESERVED_WORKFLOW_INPUT_KEYS with positional names (1, 2, 3), USER_MESSAGE, CONTEXT, EXTERNAL_CONTEXT, and ISSUE_CONTEXT to cover the full set of built-in workflow variables. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…be (coleam00#1359) The pre-flight binary smoke does a bare `bun build --compile` — it deliberately skips `scripts/build-binaries.sh` to stay fast. That means packages/paths/src/bundled-build.ts retains its dev defaults, including BUNDLED_IS_BINARY = false. version.ts branches on BUNDLED_IS_BINARY: when true it returns the embedded string; when false it calls getDevVersion(), which reads package.json at `SCRIPT_DIR/../../../../package.json`. Inside a compiled binary SCRIPT_DIR resolves under `$bunfs/root/`, the walk produces a CWD- relative path that doesn't exist, and the smoke aborts with "Failed to read version: package.json not found" — a false positive. Hit during the 0.3.8 release attempt: the real Pi lazy-load fix was working end-to-end; the smoke test was the only thing failing. Use --help instead. It exercises the same module-init graph (so it still catches the real failure modes the skill lists — Pi package.json init crash, Bun --bytecode bugs, CJS wrapper issues, circular imports under minify) but has no dev/binary branch, so no false positive. Also add a longer comment block explaining why --help is preferred, so this doesn't get "normalized" back to `version` by a future drive-by.
The brew path of /test-release runs `brew uninstall` in Phase 5 to leave the system in its pre-test state. For operators using the dual-homebrew pattern (renamed brew binary at `/opt/homebrew/bin/archon-stable` so it coexists with a `bun link` dev `archon`), that uninstall wipes the Cellar dir the `archon-stable` symlink points into → `archon-stable` becomes dangling → `brew cleanup` sweeps it away on the next brew op. Next time the operator wants stable, they have to manually re-run `brew-upgrade-archon`. Fix: make the skill aware of `archon-stable` and restore it transparently. - Phase 2 item 4: detect the `archon-stable` symlink before any brew op; export `ARCHON_STABLE_WAS_INSTALLED=yes` so Phase 5 knows to restore it. Only triggers for the brew path (curl-mac/curl-vps don't touch brew so they leave `archon-stable` alone). - Phase 5 brew path: after `brew uninstall + untap`, if the flag was set, re-tap + re-install + rename. Verifies the restored `archon-stable` reports a version and warns (non-fatal) if the rename target is missing. Documents the tradeoff: the restored version is "whatever the tap ships today", not necessarily the pre-test version — usually that's what the operator wants (the release they just tested becomes stable) but the back-version-QA case requires a manual `brew-upgrade-archon` after. - Phase 1 confirmation banner now mentions that `archon-stable` will be preserved so the operator isn't surprised by the reinstall during Phase 5. No changes to curl-mac/curl-vps paths. No changes to Phase 4 test suite.
… a compiled binary (coleam00#1360) v0.3.9 made Pi boot-safe: lazy-loading its imports meant `archon version` no longer crashed on `@mariozechner/pi-coding-agent/dist/config.js`'s module-init `readFileSync(getPackageJsonPath())`. That's what the `provider-lazy-load.test.ts` regression test guards. The fix was only half the problem though. When a Pi workflow actually runs, sendQuery() triggers the dynamic import — and Pi's config.js module-init fires then, hitting the exact same ENOENT on `dirname(process.execPath)/package.json`. Discovered by running `archon workflow run test-pi` against a locally-compiled 0.3.9 binary: [main] Failed: ENOENT: no such file or directory, open '/private/tmp/package.json' at readFileSync (unknown) at <anonymous> (/$bunfs/root/archon-providertest:184:7889) at init_config Boot-safe ≠ runtime-safe. The `/test-release` run for 0.3.9 passed because it only exercised `archon-assist` (Claude); Pi was never actually invoked on the released binary. Fix: before the dynamic `import('@mariozechner/pi-coding-agent')` in sendQuery, install a PI_PACKAGE_DIR shim. Pi's config.js checks `process.env.PI_PACKAGE_DIR` first in its `getPackageDir()` and short-circuits the `dirname(process.execPath)` walk. We write a minimal `{name, version, piConfig:{}}` stub to `tmpdir()/archon-pi-shim/package.json` (idempotent — existsSync check) and set the env var. Pi only reads `piConfig.name`, `piConfig.configDir`, and `version` from that file, all optional, so the stub surface is genuinely minimal. Localized to PiProvider: no global state, no mutation of any shared config, no upstream fork. Claude and Codex providers are unaffected (their SDKs don't have this class of module-init side effect). Verified end-to-end: built a compiled archon binary with this patch, ran `archon workflow run test-pi --no-worktree` (Pi workflow with model `anthropic/claude-haiku-4-5`), got a clean response. Before the patch, same binary crashed at `dag_node_started` with the ENOENT above. Regression test added: asserts `PI_PACKAGE_DIR` is set after sendQuery hits even its fast-fail "no model" path. Together with the existing `provider-lazy-load.test.ts` (boot-safe) this covers both halves.
… and Codex (coleam00#1361) Both binary resolvers previously stopped at env-var + explicit config and threw a "not found" error when neither was set. Users who followed the upstream-recommended install flow (Anthropic's `curl install.sh` for Claude, `npm install -g @openai/codex`) still had to manually set either `CLAUDE_BIN_PATH` / `CODEX_BIN_PATH` or the corresponding config field before any workflow could run. Add a tier-N autodetect step between the explicit config tier and the install-instructions throw. Purely additive: env and config still win when set (precedence covered by new tests). On autodetect miss, the same install-instructions error fires as before. Claude probe list (verified against docs.claude.com "Uninstall Claude Code → Native installation" section): - $HOME/.local/bin/claude (mac/linux native installer) - $USERPROFILE\.local\bin\claude.exe (Windows native installer) Codex probe list (verified against openai/codex README; npm global- install puts the binary at `{npm_prefix}/bin/<name>` on POSIX, `{npm_prefix}\<name>.cmd` on Windows): - $HOME/.npm-global/bin/codex (user-set `npm config set prefix`) - /opt/homebrew/bin/codex (mac arm64 with homebrew-node) - /usr/local/bin/codex (mac intel / linux system node) - %APPDATA%\npm\codex.cmd (Windows npm global default) - $HOME\.npm-global\codex.cmd (Windows user-set prefix) Not probed (explicit override still required): - Custom npm prefixes — `npm root -g` would need a subprocess per resolve, too much surface for a probe helper - `brew install --cask codex` — cask layout isn't a PATH binary - Manual GitHub Releases extracts — placement is user-determined - `~/.bun/bin/codex` — not documented in openai/codex README Pi provider intentionally has no equivalent change: the Pi SDK is bundled into the archon binary (no subprocess), so there's no "binary" to resolve. Pi auth lives at `~/.pi/agent/auth.json` which the SDK already finds by default, and the PR A shim (`PI_PACKAGE_DIR`) handles the package-dir case via Pi's own documented escape hatch. E2E verified: removed both config entries from ~/.archon/config.yaml, rebuilt compiled binary, ran `archon workflow run archon-assist` and a Codex workflow. Logs showed `source: 'autodetect'` for both, responses returned cleanly.
…ry autodetect test The native-installer autodetect test computed its expected path from process.env.HOME, but the implementation uses node:os homedir(). On Windows, HOME is typically unset (Windows uses USERPROFILE), so the test fell back to '/Users/test' while the resolver returned the real home dir — making the spy's path-equality check fail and breaking CI on windows-latest. Mirror the implementation by importing homedir() from node:os and joining with node:path so the expected path matches the actual platform-resolved home and separator. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ver (coleam00#1365) Reported in coleam00#1365: a user running `archon serve` with DISCORD_BOT_TOKEN set but the "Message Content Intent" toggle disabled in the Discord Developer Portal saw the entire server crash with `Used disallowed intents`. Discord rejects the gateway connection (close code 4014) when a privileged intent is requested without being enabled, and the unguarded `await discord.start()` propagated the error all the way up, taking the web UI down with it. Wrap discord.start() in try/catch — log the failure with an actionable hint (special-cased for the disallowed-intent error) and continue running. Other adapters and the web UI come up regardless. The shutdown handler already uses optional chaining (`discord?.stop()`) so nulling discord after a failed start is safe. Other adapters (Telegram, Slack, GitHub, Gitea, GitLab) have the same unguarded-start pattern but are out of scope for this fix — addressing them is tracked separately. Also expanded the Discord setup docs with a caution callout that names the exact error string and the new log event so users can grep for both. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…0#1362) * docs(script-nodes): add dedicated guide and teach the archon skill how to write them Script nodes (script:) have been a first-class DAG node type since v0.3.3 but were documented only as one-liners in CLAUDE.md and a CI smoke test. Claude Code reading the archon skill would see "Four Node Types: command, prompt, bash, loop" and reach for bash+node/python one-liners instead of a proper script node — losing bun's --no-env-file isolation, uv's --with dependency pins, and the .archon/scripts/ reuse story. - New packages/docs-web/src/content/docs/guides/script-nodes.md mirroring the structure of loop-nodes.md / approval-nodes.md: schema, inline vs named dispatch, runtime/deps semantics, scripts directory precedence (repo > home), extension-runtime mapping, env isolation, stdout/stderr contract, patterns, and the explicit list of ignored AI fields. - guides/authoring-workflows.md and guides/index.md updated so the new guide is discoverable from both the node-types table and the guides landing page. - reference/variables.md calls out the no-shell-quote difference between bash: and script: substitution — a subtle correctness trap when adapting a bash pattern into a script node. - Sidebar order bumped +1 on hooks/mcp-servers/skills/global-workflows/ remotion-workflow to slot script-nodes at order 5 next to the other node-type guides. - .claude/skills/archon/SKILL.md: replaces stale "Four Node Types" (which also silently omitted approval and cancel) with the accurate seven, with a script-node code block showing both inline and named patterns. - references/workflow-dag.md: full Script Node section covering dispatch, resolution, deps, stdout contract, and the list of AI-only fields that are ignored; validation-rules list updated. - references/dag-advanced.md and references/variables.md: retry-support line corrected; no-shell-quote note added. - examples/dag-workflow.yaml: added an extract-labels TypeScript script node and updated the header comment. * fix(docs): review follow-ups for script-node guide - skills example: extract-labels was reading process.env.ISSUE_JSON which is never set; use String.raw`$fetch-issue.output` so the upstream bash node's JSON is actually consumed - guides/script-nodes.md + skills/workflow-dag.md: idle_timeout is accepted but ignored on script (and bash) nodes — executeScriptNode only reads node.timeout. Clarify that script/bash use `timeout`, not idle_timeout - archon-workflow-builder.yaml: prompt enumerated only bash/prompt/command/loop, so the AI builder could never propose script or approval nodes. Add both (plus examples + rule about script output not being shell-quoted) and regenerate bundled defaults - book/dag-workflows.md + book/quick-reference.md + adapters/web.md: fill in the node-type references that were missing script, approval, and cancel. adapters/web.md also overclaimed "loop" in the palette — NodePalette.tsx only drags command/prompt/bash, so note that the other kinds are YAML-only
…nv gaps, add good-practices + troubleshooting (coleam00#1363) * fix(skill/when): document the full `when:` operator set and compound expressions The skill reference previously stated "operators: ==, != only" which is materially wrong — the condition evaluator supports ==, !=, <, >, <=, >= plus && / || compound expressions with && binding tighter than ||, plus dot-notation JSON field access. An agent authoring a workflow from the skill would think half the operators don't exist. Replaces the single-sentence section with a structured reference covering: - All six comparison operators (string and numeric modes) - Compound expressions with precedence rules and short-circuit eval - JSON dot notation semantics and failure modes - The fail-closed rules in full (invalid expression, non-numeric side, missing field, skipped upstream) Grounded in packages/workflows/src/condition-evaluator.ts. * feat(skill): document Approval and Cancel node types Approval and cancel nodes are first-class DAG node types (approval since the workflow lifecycle work in coleam00#871, cancel as a guarded-exit primitive) but the skill never described either one. An agent reading the skill and asked to "add a review gate before implementation" or "stop the workflow if the input is unsafe" would fall back to bash + exit 1, losing the proper semantics (cancelled vs. failed, on_reject AI rework, web UI auto-resume). Approval node coverage (references/workflow-dag.md, SKILL.md): - Full configuration block with message, capture_response, on_reject - The interactive: true workflow-level requirement for web UI delivery - Approve/reject commands across all platforms (CLI, slash, natural language) and the capture_response → $node-id.output flow - Ignored-fields list + the on_reject.prompt AI sub-node exception Cancel node coverage (references/workflow-dag.md, SKILL.md): - Single-field schema (cancel: "<reason>") - Lifecycle: cancelled (not failed); in-flight parallel nodes stopped; no DAG auto-resume path - The "cancel: vs bash-exit-1" decision rule (expected precondition miss vs. check itself failing) - Two canonical patterns — upstream-classification gate, pre-expensive-step gate Validation-rules list updated to enumerate approval/cancel constraints (message non-empty, on_reject.max_attempts range 1-10, cancel reason non-empty), plus a forward note that script: joins the mutually-exclusive set once PR coleam00#1362 lands. Placement in both files is after the Loop section and before the validation section, so this commit stays additive with respect to PR coleam00#1362's Script node insertion between Bash and Loop — rebase is clean. * feat(skill): document workflow-level fields beyond name/provider/model The skill's Schema section previously showed only name, description, provider, and model at the workflow level — which is most of a stub. Agents asked to "use the 1M-context Claude beta" or "run this under a network sandbox" or "add a fallback model in case Opus rate-limits" had no way to discover that any of these fields existed at the workflow level. Adds a comprehensive Workflow-Level Fields section covering: - Core: name, description, provider, model, interactive (with explicit callout that interactive: true is REQUIRED for approval/loop gates on web UI — a common footgun) - Isolation: worktree.enabled for pin-on/pin-off (the only worktree field at workflow level; baseBranch/copyFiles/path/initSubmodules are config.yaml only, so a cross-reference points there) - Claude SDK advanced: effort, thinking, fallbackModel, betas, sandbox, with explicit per-node-only exceptions (maxBudgetUsd, systemPrompt) - Codex-specific: modelReasoningEffort (with note that it's NOT the same as Claude's effort — this has confused users), webSearchMode, additionalDirectories - A complete worked example combining sandbox + approval + interactive All fields cross-referenced against packages/workflows/src/schemas/workflow.ts and packages/workflows/src/schemas/dag-node.ts. * feat(skill/loop): document interactive loops and gate_message Interactive loop nodes pause between iterations for human feedback via /workflow approve — used by archon-piv-loop and archon-interactive-prd. The skill's Loop Nodes section previously omitted both interactive: true and gate_message entirely, so an agent writing a guided-refinement workflow wouldn't know the feature exists or that gate_message is required at parse time. Adds: - interactive and gate_message rows to the config table (marking gate_message as required when interactive: true — enforced by the loader's superRefine) - A dedicated "Interactive Loops" subsection explaining the 6-step iterate-pause-approve-resume flow - Explicit call-out that $LOOP_USER_INPUT populates ONLY on the first iteration of a resumed session — easy to miss and a common surprise - Workflow-level interactive: true requirement for web UI delivery (loader warning otherwise) so the full-flow example is complete - Note that until_bash substitution DOES shell-quote $nodeId.output (unlike script bodies) — called out since the audit surfaced this inconsistency * fix(skill/cli): complete the CLI command reference with missing lifecycle commands The CLI reference previously documented only list, run, cleanup, validate, complete, version, setup, and chat — missing nearly every workflow lifecycle command an agent needs to operate a paused, failed, or stuck run. The interactive-workflows reference assumed these commands existed without actually documenting them. Adds full documentation for: - archon workflow status — show running workflow(s) - archon workflow approve <run-id> [comment] — resume approval gate (also populates $LOOP_USER_INPUT on interactive loops and the gate node's output when capture_response: true) - archon workflow reject <run-id> [reason] — reject gate; cancels or triggers on_reject rework depending on node config - archon workflow cancel <run-id> — terminate running/paused with in-flight subprocess kill - archon workflow abandon <run-id> — mark stuck row cancelled without subprocess kill (for orphan-cleanup after server crashes — matches the coleam00#1216 precedent) - archon workflow resume <run-id> [message] — force-resume specific run (auto-resume is default; this is for explicit override) - archon workflow cleanup [days] — disk hygiene for old terminal runs (with explicit callout that it does NOT transition 'running' rows, a common confusion) - archon workflow event emit — used inside loop prompts for state signalling; documented so agents don't invent their own mechanism - archon continue <branch> [flags] [msg] — iterative-session entry point with --workflow and --no-context flags Also: - Adds --allow-env-keys flag to the `workflow run` flag table with audit-log context and the env-leak-gate remediation use case - Adds an "Auto-resume without --resume" note disambiguating when --resume is needed vs. when auto-resume handles it - Adds --include-closed flag to `isolation cleanup`, which was previously missing; converts the flag list to a structured table - Explains the cancel/abandon distinction (live subprocess vs. orphan) All grounded in packages/cli/src/commands/workflow.ts, continue.ts, and isolation.ts. * feat(skill/repo-init): add scripts/ and state/, three-path env model, per-project env injection The repo-init reference was missing two first-class .archon/ directories (scripts/ since v0.3.3, state/ since the workflow-state feature) and had nothing to say about env — the coleam00#1 thing a user hits on first-run when their repo has a .env file with API keys. Directory tree updates: - Adds .archon/scripts/ with the extension->runtime rule (.ts/.js -> bun, .py -> uv) so agents know where to put named scripts referenced by script: nodes. - Adds .archon/state/ with explicit "always gitignore" callout — these are runtime artifacts, not source. Previously undocumented in the skill. - Adds .archon/.env (repo-scoped Archon env) and distinguishes it from the target repo's top-level .env. - Adds a "What each directory is for" list so the structure isn't just a tree with no narrative. .gitignore guidance: - state/ and .env added as must-gitignore (state/ matches CLAUDE.md and reference/archon-directories.md — skill was lagging). - mcp/ demoted to conditional — gitignore only if you hardcode secrets. New "Three-Path Env Model" section: - ~/.archon/.env (trusted, user), <cwd>/.archon/.env (trusted, repo), <cwd>/.env (UNTRUSTED, target project — stripped from subprocess env). - Precedence (override: true across archon-owned paths) and the observable [archon] loaded N keys / stripped K keys log lines so operators can verify what actually happened. - Decision tree for where to put API keys vs. target-project env vs. things Archon shouldn't touch. - Links to archon setup --scope home|project with --force for writing to the right file with timestamped backups. New "Per-Project Env Injection" section: - Documents both managed surfaces: .archon/config.yaml env: block (git-committed, $REF expansion) and Web UI Settings → Projects → Env Vars (DB-stored, never returned over API). - Names every execution surface that receives the injected vars: Claude/Codex/Pi subprocess, bash: nodes, script: nodes, and direct codebase-scoped chat. - Documents the env-leak gate with all 5 remediation paths so an agent hitting "Cannot register: env has sensitive keys" knows the options. Grounded in CHANGELOG v0.3.7 (three-path env + setup flags), v0.3.0 (env-leak gate), and reference/security.md on the docs site. * fix(skill/authoring-commands): correct override paths and add home-scoped commands The file-location and discovery sections described an override layout that does not match the actual resolver. It showed: .archon/commands/defaults/archon-assist.md # Overrides the bundled and claimed `.archon/commands/defaults/` was where repo-level overrides lived. In fact the resolver (executor-shared.ts:152-200 + command- validation.ts) walks `.archon/commands/` 1 level deep and uses basename matching — putting `archon-assist.md` at the top of `.archon/commands/` is the canonical way to override the bundled version. The `defaults/` subfolder is a Archon-internal convention for shipping bundled defaults, not a user-facing override pattern. Also, home-scoped commands (`~/.archon/commands/`, shipped in v0.3.7) were completely absent — agents authoring personal helpers wouldn't know they could live at the user level and be shared across every repo. Changes: - File Location section now shows all three discovery scopes (repo, home, bundled) with precedence ordering and 1-level subfolder rules - Duplicate-basename rule documented as a user error surface - Discovery and Priority section rewritten with accurate 3-step lookup order — no more references to the nonexistent defaults/ override path - Adds the Web UI "Global (~/.archon/commands/)" palette label note so users authoring helpers for the builder know what to expect No code changes — this is a pure fix of stale/incorrect skill reference material. * feat(skill): add workflow good-practices and troubleshooting reference pages Closes two gaps from the audit. The skill previously had zero guidance on designing multi-node workflows (what to avoid, what to reach for first, how to structure artifact chains) and zero guidance on where to look when things go wrong (log paths, env-leak gate remediations, orphan-row cleanup, resume semantics). New references/good-practices.md (9 Good Practices + 7 Anti-Patterns): - Use deterministic nodes (bash:/script:) for deterministic work, AI for reasoning — the single biggest quality lever - output_format required whenever downstream when: reads a field — the most common source of "workflow silently routes wrong" - trigger_rule: none_failed_min_one_success after conditional branches — the classic bug where all_success fails because a skipped when:-gated branch doesn't count as a success - context: fresh requires artifacts for state passing — commands must explicitly "read $ARTIFACTS_DIR/..." when downstream of fresh - Cheap models (haiku) for glue, strong for substance - Workflow descriptions as routing affordances - Validate (archon validate workflows) + smoke-run before shipping - Artifact-chain-first design - worktree.enabled: true for code-changing workflows (reversibility) - Anti-patterns with before/after YAML examples for each (AI-for-tests, free-form when: matching, context: fresh without artifacts, long flat AI-node layers, secrets in YAML, retry on loop nodes, tiny max_iterations, missing workflow-level interactive:, tool-restricted MCP nodes) New references/troubleshooting.md: - Log location (~/.archon/workspaces/<owner>/<repo>/logs/<run-id>.jsonl) with jq recipes for common queries (last assistant message, failed events, full stream) - Artifact location for cross-node handoff debugging - 9 Common Failure Modes, each with root cause + concrete fix: - $BASE_BRANCH unresolvable - Env-leak gate (5 remediations) - Claude/Codex binary not found (compiled-binary-only) - "running" forever (AI working / orphan / idle_timeout) - Mid-workflow failure and auto-resume semantics - Approval gate missing on web UI (workflow-level interactive:) - MCP plugin connection noise (filtered by design) - Empty $nodeId.output / field access (4 causes) - Diagnostic command cheat sheet (list, status, isolation list, validate, tail-log, --verbose, LOG_LEVEL=debug) - Escalation protocol (version + validate + log tail + CHANGELOG + issue) SKILL.md routing table now dispatches "Workflow good practices / anti-patterns" and "Troubleshoot a failing / stuck workflow" to the new references so an agent can find them without having to know they exist. * docs(book): update node-types coverage from four to all seven The book is the curated first-contact reading path (landing page → "Get Started" → /book/). Both dag-workflows.md and quick-reference.md were stuck on "four node types" — missing script, approval, and cancel. A user reading the book as their first introduction would form an incomplete mental model, then find three more node types in the reference section later with no explanation of when they arrived. book/dag-workflows.md: - "four node types" → "seven node types. Exactly one mode field is required per node" - Table now lists Command, Prompt, Bash, Script, Loop, Approval, Cancel with one-line "when to use" for each, and cross-links to the dedicated guide pages for Script / Loop / Approval - New sections below the table for Script (inline + named examples with runtime and deps), Approval (with the interactive: true workflow-level note that's easy to miss), and Cancel (guarded-exit pattern) — keeping the existing narrative shape for Bash and Loop book/quick-reference.md: - Node Options table now includes script, approval, cancel rows - agents row added (inline sub-agents, Claude-only) - New "Script-specific fields" and "Approval-specific fields" subsections so the cheat-sheet is actually complete rather than pointing users elsewhere for the required constraints - Retry row callout that loop nodes hard-error on retry — previously omitted - bash timeout note widened to cover script timeout (same semantics) Both files are docs-web content; the CI build on the docs-script-nodes PR (coleam00#1362) previously validated the Starlight build path with a similar table addition, so this should render clean. * fix(skill/cli): remove nonexistent \`archon workflow cancel\`, fix workflow status jq recipe Two accuracy issues from the PR code-reviewer (comment 4311243858). C1: \`archon workflow cancel <run-id>\` does NOT exist as a CLI subcommand. The switch at packages/cli/src/cli.ts:318-485 dispatches on list / run / status / resume / abandon / approve / reject / cleanup / event — running \`archon workflow cancel\` hits the default case and exits with "Unknown workflow subcommand: cancel" (cli.ts:478-484). Active cancellation is only available via: - /workflow cancel <run-id> chat slash command (all platforms) - Cancel button on the Web UI dashboard - POST /api/workflows/runs/{runId}/cancel REST endpoint cli-commands.md: removed the \`### archon workflow cancel <run-id>\` subsection; kept the \`abandon\` subsection but made it explicit that abandon does NOT kill a subprocess. Added a call-out box at the bottom of the abandon section explaining where to go for actual cancellation. troubleshooting.md "running forever" section: split the original cancel-vs-abandon advice into three bullets — Web UI / CLI abandon (for orphans, no subprocess kill) / chat \`/workflow cancel\` (for live runs that need interruption). Added an explicit "there is no archon workflow cancel CLI subcommand" parenthetical since the wrong command was being suggested in flow. I1: the \`archon workflow list --json\` diagnostic used an incorrect jq filter. workflow list's --json output (workflow.ts:185-219) has shape { workflows: [{ name, description, provider?, model?, ... }], errors: [...] } with no \`runs\` field — \`jq '.workflows[] | select(.runs)'\` returns empty unconditionally. Replaced with \`archon workflow status --json | jq '.runs[]'\`, which matches the actual shape of workflowStatusCommand at workflow.ts:852+ ({ runs: WorkflowRun[] }). Also tightened the narration to distinguish JSON from human-readable status output. No change to the commit history in this PR — these are follow-up fixes to claims I introduced in earlier commits of this branch (f10b989 for C1, 66d2b86 for I1). * fix(skill): remove env-leak gate references (feature was removed in provider extraction) C2 from the PR code-reviewer (comment 4311243858). The pre-spawn env-leak gate was removed from the codebase during the provider-extraction refactor — see TODO(coleam00#1135) at packages/providers/src/claude/provider.ts:908. Zero hits for --allow-env-keys / allowEnvKeys / allow_env_keys / allow_target_repo_keys across packages/. The CLI's parseArgs (cli.ts:182-208) has no --allow-env-keys option, and because parseArgs uses strict: false, an unknown --allow-env-keys would be silently ignored rather than error. What remains accurate and is NOT touched: - Three-Path Env Model section (user/repo archon-owned envs are loaded; target repo <cwd>/.env keys are stripped from process.env at boot) still correctly describes current behavior, grounded in packages/paths/src/strip-cwd-env.ts + env-integration.test.ts - Per-Project Env Injection section (Option 1: .archon/config.yaml env: block; Option 2: Web UI Settings → Projects → Env Vars) is unchanged — both remain the sanctioned way to get env vars into subprocesses Removed claims (all three files): - cli-commands.md: --allow-env-keys flag row in the workflow run flags table - repo-init.md: the "Env-leak gate" subsection at the end of Per-Project Env Injection listing 5 remediations (all of which reference UI/CLI/ config surfaces that don't exist). Replaced with a succinct callout that explains the actual current behavior — target repo .env keys are stripped, workflows that need those values should use managed injection — so the reader still gets the "where to put my env vars" answer - troubleshooting.md: the "Cannot register: codebase has sensitive env keys" section (error message that can no longer be emitted) If the env-leak gate is ever resurrected per TODO(coleam00#1135), the docs can be re-added then. The CHANGELOG v0.3.0 entry describing the gate is a historical record of past behavior and does not need to be rewritten. * fix(skill/troubleshooting): correct JSONL event type names and field name C3 from the PR code-reviewer (comment 4311243858). The troubleshooting reference's event-types table used _started / _completed / _failed suffixes, but packages/workflows/src/logger.ts:19-30 shows the actual WorkflowEvent.type enum is: workflow_start | workflow_complete | workflow_error | assistant | tool | validation | node_start | node_complete | node_skipped | node_error The second jq recipe also queried `.event` but the discriminator is `.type`. Fixes: - Event table: renamed columns (_started → _start, _completed → _complete, _failed → _error). Explicitly called out the field name as `type` so the reader knows what jq selector to use - Replaced the "tool_use / tool_result" row with a single `tool` row and listed its actual payload fields (tool_name, tool_input, duration_ms, tokens) — tool_use/tool_result are SDK message kinds that appear within the AI stream, not top-level log event types - Added a `validation` row (was missing; it's emitted by workflow-level validation calls with `check` and `result` fields) - Removed `retry_attempt` row — this event type is not emitted to the JSONL file. Retry bookkeeping goes through pino logs, not the workflow log file - Added an explicit callout that loop_iteration_started / loop_iteration_completed (and other emitter-only events) go through the workflow event emitter + DB workflow_events table, NOT the JSONL file. Pointed readers to the DB or Web UI for loop-level detail. This distinguishes the two parallel event systems — easy to conflate (store.ts:11-17 uses _started/_completed/_failed for the DB side, logger.ts uses _start/_complete/_error for JSONL) - Fixed the "all failed events" jq recipe: .event → .type and _failed → _error - Minor cleanup: the inline "tool_use events" mention in the "running forever" section said the wrong event name — updated to "tool or assistant events in the tail" Grounded in packages/workflows/src/logger.ts (canonical JSONL event shape) and packages/workflows/src/store.ts (the parallel DB event naming, which the reviewer correctly flagged as different and worth keeping distinct). * fix(skill): two stragglers from the code-reviewer audit Cleanup of two references that slipped through the earlier C1 and C3 fixes: - references/troubleshooting.md:126: \`node_failed\` → \`node_error\` (the "Node output is empty" diagnostics section references the JSONL log, which uses the logger.ts enum — not the DB workflow_events table which does use \`node_failed\`). The C3 fix corrected the event table and one jq recipe but missed this inline mention. - references/interactive-workflows.md:106: removed \`archon workflow cancel <run-id>\` (nonexistent CLI subcommand) from the troubleshooting bullet. This was pre-existing before the hardening PR but fell within the C1 remediation scope. Replaced with the correct triage: reject (approval gate only) vs abandon (orphan cleanup, no subprocess kill) vs chat /workflow cancel (actual subprocess termination). Grounded in the same sources as the earlier C1/C3 commits: packages/cli/src/cli.ts:318-485 (no cancel case) and packages/workflows/src/logger.ts:19-30 (JSONL type enum). * feat(skill): point to archon.diy as the canonical docs source The skill had no reference to archon.diy (the live docs site built from packages/docs-web/). Several reference files said "see the docs site" without naming the URL, leaving the agent to guess or grep the repo for the hostname. An agent with the skill loaded should know that when the distilled reference pages don't cover a case, the full canonical docs are one WebFetch away. SKILL.md: new "Richer Context: archon.diy" section between Routing and Running Workflows. Covers: - When to reach for the live docs (longer examples, tutorial framing, features the skill only mentions in passing, "where's that documented?" user questions) - URL map — 13 starting points covering getting-started, book (tutorial series), guides/ (authoring + per-node-type + per-node-feature), reference/ (variables, CLI, security, architecture, configuration, troubleshooting), adapters/, deployment/ - Precedence: skill refs first (context-cheap, tuned for agents), docs site as escalation. Prevents agents defaulting to WebFetch when a local skill ref already covers the answer Also upgrades the 5 existing generic "docs site" mentions across reference files to concrete archon.diy URLs with anchor fragments where helpful: - good-practices.md: Inline sub-agents pattern → archon.diy/guides/ authoring-workflows/#inline-sub-agents - troubleshooting.md: "Install page on the docs site" → archon.diy/ getting-started/installation/ - workflow-dag.md: "Workflow Description Best Practices" → anchor link; sandbox schema reference → archon.diy/guides/authoring-workflows/ #claude-sdk-advanced-options - repo-init.md: Security Model reference → archon.diy/reference/ security/#target-repo-env-isolation (deep-link into the section that covers the <cwd>/.env strip behavior) URL source of truth: astro.config.mjs:5 (site: 'https://archon.diy'). URL structure mirrors packages/docs-web/src/content/docs/<section>/ <page>.md — verified by the 62 pages the docs build produces.
…#1395) Anthropic's Opus 4.7 landed 2026-04-16; on the Anthropic API, opus / opus[1m] now resolve to 4.7 with a 1M context window at standard pricing. Using the alias instead of the hard-pinned claude-opus-4-6[1m] lets bundled default workflows auto-track the recommended Opus version. No explicit effort is set, so nodes inherit the per-model default (xhigh on 4.7, high on 4.6).
Author
|
Rebased/merged latest dev and addressed the review feedback. This should be ready for maintainer review when convenient. |
Collaborator
|
@gemmawood this PR appears to fully address #1127. Consider adding |
Collaborator
|
Hi @gemmawood — thanks for opening this PR. This repository uses a PR template at
Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly. If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank. |
Author
|
Hi @Wirasm — thanks for the review. I've filled out all the template sections in the PR description (UX Journey, Architecture Diagram, Label Snapshot, Change Metadata, Linked Issue with |
Collaborator
Review SummaryVerdict: minor-fixes-needed Your PR adds a well-structured runtime workflow inputs feature ( Blocking issues
Suggested fixes
Minor / nice-to-have
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: comment-quality, test-coverage, docs-impact. |
…g, assertSafeInputKeys; document --set and inputs: block - Export three private helpers so they can be tested directly - Add 28 unit tests covering defaults, runtime overrides, required validation, invalid/reserved key rejection, word-boundary matching, and multi-key substitution - Add substituteWorkflowVariables tests for runtime input substitution - Document --set flag in cli.md reference table - Document inputs request body field in api.md with example - Add Runtime Inputs section to authoring-workflows.md with full worked example - Add $INPUT_NAME to variables.md reference table and substitution order Addresses reviewer feedback requesting test coverage for the new input-resolution functions and documentation for the --set CLI flag and inputs API field. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
Hi @Wirasm — addressed all the blocking and suggested items: Blocking:
Suggested:
Minor:
|
Collaborator
|
Thanks for the thorough work here, @gemmawood — the Before I review the diff in detail, could you tell me a bit about the concrete use case driving this? Specifically:
I ask because there's a related, narrower primitive in flight at #1433 — If your pain is specifically "I want to run this workflow on a different model for one invocation", #1433 covers that with much smaller surface area (no new YAML schema, no A secondary concern with the current design: Read #1433 and let me know which use case matches yours — happy to merge whichever solves your real problem, or both if they genuinely cover different angles. |
…uts field Trims multi-line block comments to no comments — the field names and types are self-documenting; the full explanation now lives in authoring-workflows.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
Concrete use case: Yes, the duplication is real and present right now. I maintain three kanban-develop variants ( Re #1433: If
If you'd rather land #1433 first and have this PR build on top of it (removing Re namespace collision: Fair concern. The current design leans on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kanban-develop.yaml,kanban-develop-gpt-4o.yaml, etc., identical except for a single field.inputs:declaration in workflow YAML;--set KEY=VALUECLI flag;inputsfield in POST/api/workflows/:name/run; runtime values substituted into prompts, model, and provider before execution.inputs:are unaffected. AI-driven workflow invocation does not accept caller inputs (the AI decides what to run).How it works
Declare inputs in your workflow YAML:
```yaml
name: my-workflow
description: Example workflow
model: $MODEL
provider: $PROVIDER
inputs:
MODEL:
description: The model to use
default: claude-sonnet-4-6
required: false
PROVIDER:
description: AI provider
default: anthropic
nodes:
type: task
prompt: Run a task using $MODEL on $PROVIDER...
```
Override at runtime:
```bash
CLI
archon workflow run my-workflow --set MODEL=gpt-4o --set PROVIDER=openai
API
POST /api/workflows/my-workflow/run
{ "conversationId": "...", "message": "...", "inputs": { "MODEL": "gpt-4o", "PROVIDER": "openai" } }
```
Resolution order: runtime values > YAML defaults; missing required inputs fail fast with a clear error.
Changes
Schema (
packages/workflows/src/schemas/workflow.ts)workflowInputSchema:{ description?, default?, required? }inputs: z.record(workflowInputSchema).optional()added toworkflowBaseSchemaExecution (
packages/workflows/src/executor.ts)assertSafeInputKeys(): validates keys against/^[A-Za-z_][A-Za-z0-9_]*$/and a reserved-name blocklistresolveInputs(): merges declared defaults with runtime values, validates required inputsescapeForRegex()+applyInputsToString(): safe$KEYsubstitution inmodel/providerbefore provider inferenceexecuteWorkflow()gains optionalruntimeInputsparameterresolved_inputspersisted toWorkflowRun.metadataon both freshly-created and orchestrator-pre-created run paths; restored on resume, approval, and rejectionPrompt substitution (
packages/workflows/src/executor-shared.ts)substituteWorkflowVariables()andbuildPromptWithContext()accept optionalinputsmapDAG executor (
packages/workflows/src/dag-executor.ts)inputsthreaded throughexecuteDagWorkflow→executeNodeInternal→ all node type executors, includinguntil_bashOrchestrator threading (
packages/core/)HandleMessageContext.inputs?andWorkflowRoutingContext.inputs?orchestrator-agent.ts: inputs flow throughhandleWorkflowRunCommandanddispatchOrchestratorWorkflow; resume/approve/reject restore stored inputs from metadataorchestrator.ts: background dispatch passes inputs toexecuteWorkflowAPI (
packages/server/src/routes/)RunWorkflowBodygainsinputs: z.record(z.string()).optional()api.tsextracts and forwards inputs asextraContext.inputsCLI (
packages/cli/src/)parseArgsoptionset: { type: 'string', multiple: true }(repeatable)--set KEY=VALUEparsed and forwarded toexecuteWorkflowasinputsUX Journey
Before
```
User Archon Filesystem
──── ────── ──────────
wants to run with ← no parameterization
different model maintain 3 YAML variants ────▶ kanban.yaml
kanban-gpt4o.yaml
kanban-o3.yaml
archon workflow run kanban-gpt4o (must pick the right file)
```
After
```
User Archon Filesystem
──── ────── ──────────
archon workflow run resolves inputs ─────────────▶ kanban.yaml (one file)
kanban merges defaults + --set
--set MODEL=gpt-4o substitutes $MODEL in
prompt, model:, provider:
validates required inputs
executes with effective model
```
Architecture Diagram
Before
```
CLI ──────────────────▶ Server API ──▶ Orchestrator ──▶ executeWorkflow(workflow, opts)
│
└──▶ DAG executor (no inputs)
```
After
```
CLI [
] ──────────────▶ Server API [] ──▶ Orchestrator [] ──▶ executeWorkflow(workflow, opts, inputs) []--set KEY=VALUE inputs field inputs context │
├──▶ resolveInputs() [+]
├──▶ assertSafeInputKeys() [+]
├──▶ applyInputsToString() [+]
├──▶ metadata.resolved_inputs [+]
└──▶ DAG executor [
]]└── all node executors [
```
Connection inventory:
--setWorkflowRunOptions.inputscli.tsWorkflowRunOptions.inputsexecuteWorkflowRunWorkflowBody.inputsextraContext.inputsexecuteWorkflowresolveInputsexecuteWorkflowWorkflowRun.metadata.resolved_inputsexecuteWorkflowDAG executor inputsmetadata.resolved_inputsLabel Snapshot
risk: lowsize: Lworkflows,server,cli,coreworkflows:executor,workflows:dag-executor,cli:workflow,server:api,core:orchestratorChange Metadata
featureworkflowsLinked Issue
Validation Evidence (required)
```bash
bun test packages/workflows/src/executor.test.ts # 28/28 pass
bun test packages/workflows/src/dag-executor.test.ts # 166/166 pass
bun --filter @archon/workflows type-check # clean
bun --filter @archon/cli type-check # clean
bun --filter @archon/server type-check # clean
bun --filter @archon/core type-check # clean
```
bun testinvocation produces 77 failures due to pre-existing global state interference — confirmed againstmainwith the same command. Not caused by these changes.Security Impact (required)
Input key validation (
assertSafeInputKeys) blocks reserved workflow variable names and enforces/^[A-Za-z_][A-Za-z0-9_]*$/before any substitution occurs. Regex construction usesescapeForRegexto prevent metacharacter injection.Compatibility / Migration
inputs:field is optional; workflows without it are unaffectedresolved_inputsstored in the existingmetadataJSON columnHuman Verification (required)
inputs:run identically to before--set MODEL=gpt-4ooverrides$MODELin both the prompt body and themodel:top-level fieldresolved_inputsis restored correctly on workflow resume (interrupting a run mid-execution and resuming continues with the same inputs)What was not verified:
resolved_inputsin metadata)Side Effects / Blast Radius (required)
assertSafeInputKeysand the reserved-key blocklistRollback Plan (required)
git revertthis PR; the feature is additive with no schema migrationsinputs:or a caller passes--set/inputsInvalid workflow input key(s)error at run start; literal$KEYin prompts if a key is undeclaredRisks and Mitigations
FOO.BAR,BAD[)assertSafeInputKeysenforces/^[A-Za-z_][A-Za-z0-9_]*$/; keys with dots, hyphens, or regex metacharacters are rejected before any substitution.escapeForRegexalso used defensively inapplyInputsToString.resolved_inputspersisted toWorkflowRun.metadataon initial execution (both fresh-create and preCreatedRun paths); restored from metadata before re-runningresolveInputson resume, approve, and reject paths.🤖 Generated with Claude Code