refactor(workflows): trust the SDK for model validation#1463
Merged
Conversation
Drops cross-provider model inference and hard-coded model allow-lists. The string a workflow author writes in `model:` is forwarded to the SDK unchanged; the SDK and its API decide whether the model exists. Provider identity is the only thing Archon validates at load time — typos like `provider: claud` are caught early; everything else fails at runtime through the SDK's normal error path. Why this matters: a recent run on Sasha showed `provider: claude` + `model: opus[1m]` getting silently routed to Codex (because Codex's isModelCompatible was defined as the complement of Claude's, so anything not literally `sonnet|opus|haiku` matched). Codex then rejected the model as a `⚠️ ` system warning and the node "completed" in 2.1 seconds with empty output, after which the workflow opened a hallucinated PR. Three stacked bugs and two amplifiers; this commit removes all five. Changes: - Delete model-validation.ts entirely (inferProviderFromModel and isModelCompatible are gone). Drop the matching field from ProviderRegistration and from the claude/codex/pi entries. - Replace the resolver in executor.ts and dag-executor.ts (both the per-node and per-loop paths) with a flat `node.provider ?? workflow.provider ?? config.assistant`. Model never influences provider selection; load-time validation is just isRegisteredProvider on the resolved provider id. - Remove the dag-node Zod superRefine that recomputed model-compat — load-time provider validation moved to loader.ts. - Codex provider: stream loop now matches Claude's contract. error events that aren't followed by turn.completed yield `result.isError: true` (subtype `codex_stream_incomplete`) so the dag-executor's existing isError path catches them. turn.failed becomes `codex_turn_failed` with the same shape. Iterator close without a terminal event is itself a fail-stop. MCP-client errors remain filtered (Codex retries those internally). - dag-executor: AI nodes that exit the streaming loop with empty assistant text and no structured output now fail with `dag.node_empty_output` instead of completing silently — the Sasha bug's final amplifier. Bash/script/approval nodes are unaffected. Tests: model-validation.test.ts and isPiModelCompatible block deleted; codex provider tests rewritten to assert the new fail-stop contract; dag-executor empty-output test flipped to assert failure; new tests cover (a) loader rejecting unknown provider, (b) loader accepting any model string with a known provider, (c) executor passing provider+model through without re-routing, (d) executor throwing on unknown provider, (e) Codex synthesizing fail-stop on iterator close. Two cost-tracking tests adjusted to yield non-empty assistant text since their intent was cost accumulation, not empty-output handling. bun run validate: green (check:bundled, type-check, lint --max-warnings 0, format:check, all packages' test suites — 0 fail). End-to-end smoke (.archon/workflows/test-workflows/): - e2e-deterministic: PASS (engine healthy) - e2e-codex-smoke: PASS (Codex sendQuery + structured output work) - e2e-claude-smoke: FAIL with `error: unknown option '--no-env-file'` — this is a regression from the SDK 0.2.121 bump (#1460), not from this redesign. The Claude provider source is unchanged on this branch. To be fixed separately.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProvider/model compatibility checks were removed from registration and load-time inference. Provider resolution is explicit (node → workflow → config) and validated against the registry. Codex stream semantics changed to emit structured terminal result chunks on terminal/failed/incomplete conditions. Empty assistant output now fails AI nodes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Executor as Executor
participant Registry as Provider Registry
participant Provider as Codex Provider
participant Stream as Codex Stream
Executor->>Registry: resolve provider (node.provider ?? workflow.provider ?? config.assistant)
Registry-->>Executor: provider id or error (list of registered IDs)
Executor->>Provider: sendQuery(...)
Provider->>Stream: open async iterator
Stream-->>Provider: event: error (non-MCP)
alt followed by turn.completed
Provider-->>Executor: yield success result (log stream_error)
Provider-->>Stream: close
else followed by turn.failed
Provider-->>Executor: yield fail-stop result {isError:true, errorSubtype:codex_turn_failed, errors:[...]}
Provider-->>Stream: close
else stream closes without terminal event
Provider-->>Executor: yield fail-stop result {isError:true, errorSubtype:codex_stream_incomplete, errors:[...]}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
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/loader.ts (1)
280-294:⚠️ Potential issue | 🟠 MajorOnly the workflow-level
provideris validated here.Explicit node overrides still bypass load-time validation. A workflow with
nodes: [{ id: "n1", prompt: "...", provider: "typo" }]will parse successfully here and then die later indag-executor.ts, which leaves the new “reject unknown providers at load time” behavior half-implemented.Suggested fix
if (provider && !isRegisteredProvider(provider)) { return { workflow: null, error: { filename, error: `Unknown provider '${provider}'. Registered: ${getRegisteredProviders() .map(p => p.id) .join(', ')}`, errorType: 'validation_error', }, }; } + + for (const node of dagNodes) { + if (node.provider && !isRegisteredProvider(node.provider)) { + return { + workflow: null, + error: { + filename, + error: `Node '${node.id}': unknown provider '${node.provider}'. Registered: ${getRegisteredProviders() + .map(p => p.id) + .join(', ')}`, + errorType: 'validation_error', + }, + }; + } + }As per coding guidelines, "Provider is inherited from
.archon/config.yamlunless explicitly set; per-nodeproviderandmodeloverrides are supported".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/loader.ts` around lines 280 - 294, The loader currently only validates the workflow-level provider; update the load-time validation to also iterate over each node in the parsed nodes array (the nodes property on the workflow object) and, for any node that has a provider override, call isRegisteredProvider(node.provider) and reject with the same error shape used for workflow-level validation (include filename, node id, descriptive error string listing getRegisteredProviders().map(p => p.id).join(', '), and errorType 'validation_error') so unknown per-node providers fail at load time instead of later in dag-executor.ts.
🧹 Nitpick comments (1)
packages/workflows/src/executor.test.ts (1)
471-505: Assert the forwarded provider/model args, not just invocation count.Both tests still pass if
executeWorkflow()dropsmodelor rewrites the resolvedprovider, because they only check thatexecuteDagWorkflow()ran. Please assert the forwarded args from the mock call in each case.Suggested assertion shape
await executeWorkflow( deps, makePlatform(), 'conv-1', '/tmp', makeWorkflow({ model: 'sonnet' }), 'test message', 'db-conv-1' ); expect(mockExecuteDagWorkflow).toHaveBeenCalledTimes(1); + const defaultProviderCall = mockExecuteDagWorkflow.mock.calls[0]; + expect(defaultProviderCall?.[6]).toBe('claude'); + expect(defaultProviderCall?.[7]).toBe('sonnet');await executeWorkflow( deps, makePlatform(), 'conv-1', '/tmp', makeWorkflow({ provider: 'codex', model: 'sonnet' }), 'test message', 'db-conv-1' ); expect(mockExecuteDagWorkflow).toHaveBeenCalledTimes(1); + const explicitProviderCall = mockExecuteDagWorkflow.mock.calls[0]; + expect(explicitProviderCall?.[6]).toBe('codex'); + expect(explicitProviderCall?.[7]).toBe('sonnet');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/executor.test.ts` around lines 471 - 505, The tests currently only check mockExecuteDagWorkflow was called; change each test to also inspect the workflow object passed into mockExecuteDagWorkflow and assert its provider and model fields are what you expect: in the first test assert the resolved workflow argument has model === 'sonnet' and provider === the fallback assistant (e.g. 'claude'), and in the second test assert the resolved workflow argument has provider === 'codex' and model === 'sonnet'; locate the mock call by checking the arguments of mockExecuteDagWorkflow (the workflow object argument) from the first call and assert its provider/model properties accordingly.
🤖 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/dag-executor.ts`:
- Around line 2659-2679: The code currently returns a silent failed output when
a loop node has an unknown provider (logic around loopProvider and
isRegisteredProvider), which bypasses the common failure handling; instead
either call the shared resolveNodeProviderAndModel(node, workflowProvider,
workflowModel, config) helper used by other node types or throw an Error when
isRegisteredProvider(loopProvider) is false so the outer catch can emit
node_failed, log pre-execution/diagnostics, and surface a proper user-facing
failure; update the branch that computes loopProvider/loopModel in
dag-executor.ts to reuse resolveNodeProviderAndModel or to throw a descriptive
error referencing node.id and loopProvider rather than returning the ad-hoc
output object.
In `@packages/workflows/src/loader.ts`:
- Line 7: The import of isRegisteredProvider and getRegisteredProviders from the
full `@archon/providers` in loader.ts breaks the package boundary; remove those
runtime imports and stop doing provider registry validation inside parseWorkflow
directly. Instead either (A) add an optional validation callback parameter to
parseWorkflow(content, filename, validateProvider?) and invoke that callback for
provider checks (leaving parseWorkflow as pure YAML parsing when no callback is
provided), or (B) move the provider validation logic into a new exported
function (e.g., validateWorkflowProviders) that callers (server/index.ts,
cli.ts, core/config-loader.ts) call after they bootstrap the provider registry;
update references to isRegisteredProvider/getRegisteredProviders to be used only
by the new callback/function and import only types from
'@archon/providers/types' in loader.ts.
---
Outside diff comments:
In `@packages/workflows/src/loader.ts`:
- Around line 280-294: The loader currently only validates the workflow-level
provider; update the load-time validation to also iterate over each node in the
parsed nodes array (the nodes property on the workflow object) and, for any node
that has a provider override, call isRegisteredProvider(node.provider) and
reject with the same error shape used for workflow-level validation (include
filename, node id, descriptive error string listing
getRegisteredProviders().map(p => p.id).join(', '), and errorType
'validation_error') so unknown per-node providers fail at load time instead of
later in dag-executor.ts.
---
Nitpick comments:
In `@packages/workflows/src/executor.test.ts`:
- Around line 471-505: The tests currently only check mockExecuteDagWorkflow was
called; change each test to also inspect the workflow object passed into
mockExecuteDagWorkflow and assert its provider and model fields are what you
expect: in the first test assert the resolved workflow argument has model ===
'sonnet' and provider === the fallback assistant (e.g. 'claude'), and in the
second test assert the resolved workflow argument has provider === 'codex' and
model === 'sonnet'; locate the mock call by checking the arguments of
mockExecuteDagWorkflow (the workflow object argument) from the first call and
assert its provider/model properties accordingly.
🪄 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: 83377eab-0645-4819-855f-bd567fa0ba24
📒 Files selected for processing (19)
packages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/community/pi/index.tspackages/providers/src/community/pi/model-ref.test.tspackages/providers/src/community/pi/model-ref.tspackages/providers/src/community/pi/registration.tspackages/providers/src/registry.test.tspackages/providers/src/registry.tspackages/providers/src/types.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor-preamble.test.tspackages/workflows/src/executor.test.tspackages/workflows/src/executor.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/model-validation.test.tspackages/workflows/src/model-validation.tspackages/workflows/src/schemas/dag-node.ts
💤 Files with no reviewable changes (6)
- packages/providers/src/community/pi/registration.ts
- packages/workflows/src/model-validation.ts
- packages/workflows/src/model-validation.test.ts
- packages/providers/src/community/pi/model-ref.ts
- packages/providers/src/registry.test.ts
- packages/providers/src/types.ts
| import { isLoopNode, isApprovalNode, isCancelNode, isScriptNode } from './schemas'; | ||
| import { createLogger } from '@archon/paths'; | ||
| import { isModelCompatible } from './model-validation'; | ||
| import { isRegisteredProvider, getRegisteredProviders } from '@archon/providers'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Loader / discovery call sites ==\n'
rg -n --type=ts '\b(parseWorkflow|discoverWorkflowsWithConfig)\b'
printf '\n== Provider bootstrap call sites ==\n'
rg -n --type=ts '\b(registerBuiltinProviders|registerCommunityProviders)\s*\('
printf '\n== Runtime imports of `@archon/providers` from workflows package ==\n'
rg -n --type=ts "from '@archon/providers'" packages/workflowsRepository: coleam00/Archon
Length of output: 25799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the file size first
wc -l packages/workflows/src/loader.ts
# Read around line 7 (import section)
printf '\n=== Line 7 context (imports) ===\n'
sed -n '1,30p' packages/workflows/src/loader.ts
# Read lines 280-294 (provider validation in parseWorkflow)
printf '\n=== Lines 280-294 (workflow-level provider validation) ===\n'
sed -n '275,300p' packages/workflows/src/loader.ts
# Find where parseWorkflow is defined and what validation happens
printf '\n=== parseWorkflow function signature and validation ===\n'
sed -n '177,220p' packages/workflows/src/loader.tsRepository: coleam00/Archon
Length of output: 4089
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if isRegisteredProvider and getRegisteredProviders are in the types subpath
printf '\n=== Check `@archon/providers/types` exports ===\n'
fd -t f 'index.ts|types.ts' packages/providers/src/types
cat packages/providers/src/types/index.ts 2>/dev/null || echo "File not found"
# Find where node provider validation happens (if at all)
printf '\n=== Search for node-level provider validation ===\n'
rg -n 'node.*provider|provider.*node' packages/workflows/src/loader.ts -A 2 -B 2
# Check parseDagNode to see if it validates node-level provider
printf '\n=== parseDagNode implementation ===\n'
rg -n 'parseDagNode|dagNodeSchema.safeParse' packages/workflows/src/loader.ts -A 5 -B 2
# Look for where node providers are actually used/validated in executor
printf '\n=== Node provider handling in executor ===\n'
rg -n 'node.provider|nodeProvider' packages/workflows/src/executor.ts -A 3 -B 1 | head -50Repository: coleam00/Archon
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List the structure of the providers package
printf '\n=== Providers package structure ===\n'
fd -t f '.' packages/providers/src | head -30
# Check what's exported from providers main index
printf '\n=== Providers main index exports ===\n'
cat packages/providers/src/index.ts | head -50
# Find where isRegisteredProvider and getRegisteredProviders are defined
printf '\n=== Function definitions ===\n'
rg -n 'export.*function.*(isRegisteredProvider|getRegisteredProviders)' packages/providers
# Check if there's a types or contract subpath
printf '\n=== Check for types-only subpaths ===\n'
ls -la packages/providers/src/ 2>/dev/null || echo "No src directory"
# Look for node-level provider validation
printf '\n=== Node provider validation in loader ===\n'
rg -n 'parseDagNode\(\|nodeProvider' packages/workflows/src/loader.ts -B 2 -A 10 | head -60Repository: coleam00/Archon
Length of output: 4009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get more context on how nodes are parsed
printf '\n=== Full parseWorkflow flow around line 280-350 ===\n'
sed -n '270,350p' packages/workflows/src/loader.ts
# Find where parseDagNode is called and what it does
printf '\n=== parseDagNode and node parsing ===\n'
sed -n '300,400p' packages/workflows/src/loader.tsRepository: coleam00/Archon
Length of output: 8283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for where nodes are parsed and if provider is checked there
printf '\n=== Node provider field in DAG nodes ===\n'
sed -n '150,280p' packages/workflows/src/loader.ts | grep -A 10 -B 5 'node\|provider'
# Look for dagNodes parsing
printf '\n=== DAG nodes parsing section ===\n'
rg -n 'dagNodes|parseDagNode' packages/workflows/src/loader.ts -B 3 -A 10 | head -100
# Check the dag-node schema to see if provider is validated there
printf '\n=== dag-node schema provider field ===\n'
rg -n 'provider' packages/workflows/src/schemas/dag-node.ts -B 2 -A 2Repository: coleam00/Archon
Length of output: 8233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what's in types.ts to see if those functions could be there
printf '\n=== packages/providers/src/types.ts content ===\n'
head -100 packages/providers/src/types.ts
# Check how node-level provider is validated in the executor
printf '\n=== Node-level provider validation in dag-executor ===\n'
rg -n 'node.provider|nodeProvider.*validat' packages/workflows/src/dag-executor.ts -B 3 -A 5 | head -80
# Check if there's a way to inject provider validation
printf '\n=== Call sites of parseWorkflow to see if they could do validation ===\n'
rg -n 'parseWorkflow\(' packages/workflows/src/workflow-discovery.ts -B 2 -A 5Repository: coleam00/Archon
Length of output: 7672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact imports in loader.ts more carefully
printf '\n=== Exact imports in loader.ts (lines 1-20) ===\n'
sed -n '1,20p' packages/workflows/src/loader.ts
# Check if the functions are available from `@archon/providers/types`
printf '\n=== Check if isRegisteredProvider/getRegisteredProviders are exported from types ===\n'
rg -n 'export.*isRegisteredProvider|export.*getRegisteredProviders' packages/providers/src/types.ts
# Look at the learnings more carefully - check what the types subpath is supposed to contain
printf '\n=== Is there a `@archon/providers/types` entry point? ===\n'
find packages/providers -type f -name 'index.ts' -o -name 'package.json' | xargs grep -l 'types\|exports' 2>/dev/null | head -5
# Verify the comment about load-time validation being intentional
printf '\n=== Comment in dag-node.ts about provider validation timing ===\n'
sed -n '369,375p' packages/workflows/src/schemas/dag-node.tsRepository: coleam00/Archon
Length of output: 1083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the providers package.json for export configuration
printf '\n=== Providers package.json exports ===\n'
cat packages/providers/package.json | grep -A 20 '"exports"'
# Double-check what's in the types.ts file — is it truly contract-only?
printf '\n=== Full types.ts to verify no function exports ===\n'
wc -l packages/providers/src/types.ts
tail -100 packages/providers/src/types.ts
# Check if there's a separate types subpath index
printf '\n=== List all files in providers/src ===\n'
find packages/providers/src -maxdepth 1 -type f -name '*.ts' | sort
# Look at the comment in dag-node about timing
printf '\n=== dag-node.ts line 371 comment ===\n'
sed -n '369,380p' packages/workflows/src/schemas/dag-node.tsRepository: coleam00/Archon
Length of output: 5412
🏁 Script executed:
#!/bin/bash
set -euo pipefixeduo
# Check if parseWorkflow is called with any context that could pass validation
printf '\n=== parseWorkflow call sites and their context ===\n'
rg -n 'parseWorkflow\(' packages/workflows/src/workflow-discovery.ts -B 5 -A 2 | head -40
# Check if there's a pattern where validation could be injected
printf '\n=== Can parseWorkflow receive a validation callback? ===\n'
sed -n '177,185p' packages/workflows/src/loader.tsRepository: coleam00/Archon
Length of output: 116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if parseWorkflow is called with any context that could pass validation
printf '\n=== parseWorkflow call sites and their context ===\n'
rg -n 'parseWorkflow\(' packages/workflows/src/workflow-discovery.ts -B 5 -A 2 | head -40
# Check parseWorkflow signature
printf '\n=== parseWorkflow function signature ===\n'
sed -n '177,190p' packages/workflows/src/loader.tsRepository: coleam00/Archon
Length of output: 1290
Imports from @archon/providers violate the documented workflows package boundary.
Per learnings, archon/workflows should depend only on archon/git, archon/paths, archon/providers/types, hono/zod-openapi, and zod. Line 7 imports isRegisteredProvider and getRegisteredProviders from the full @archon/providers package instead of the contract-only types subpath. These functions live in registry.ts and cannot be moved to types.ts because they depend on mutable state.
Node-level provider validation is already present in the executor. The review comment claims it's missing, but dag-executor.ts (lines 350, 2662) validates node-level provider overrides using the same functions. The dag-node schema comment (line 371) documents that validation occurs at both levels: workflow-level during load (here) and node-level during execution (executor).
The architectural issue is the hard dependency, not runtime correctness. All entrypoints (server/index.ts, cli.ts, core/config-loader.ts) bootstrap the provider registry before calling parseWorkflow. However, this is a runtime contract, not an architectural guarantee. To preserve the boundary, consider:
- Extract provider validation to a separate step that callers invoke after bootstrapping, or
- Inject a validation callback into
parseWorkflow(content, filename, validateProvider?)so the function remains pure YAML parsing
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/loader.ts` at line 7, The import of
isRegisteredProvider and getRegisteredProviders from the full `@archon/providers`
in loader.ts breaks the package boundary; remove those runtime imports and stop
doing provider registry validation inside parseWorkflow directly. Instead either
(A) add an optional validation callback parameter to parseWorkflow(content,
filename, validateProvider?) and invoke that callback for provider checks
(leaving parseWorkflow as pure YAML parsing when no callback is provided), or
(B) move the provider validation logic into a new exported function (e.g.,
validateWorkflowProviders) that callers (server/index.ts, cli.ts,
core/config-loader.ts) call after they bootstrap the provider registry; update
references to isRegisteredProvider/getRegisteredProviders to be used only by the
new callback/function and import only types from '@archon/providers/types' in
loader.ts.
Collaborator
Author
PR Review Summary — multi-agent7 review agents ran against this PR (code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, code-simplifier). The core direction (delete model allow-lists, trust the SDK, fail-stop on empty output / stream-incomplete) is right and well-executed. The Codex stream loop is now airtight. Two regressions and a behavior-change-without-migration-note are the only real blockers. Critical Issues (2)
Important Issues (7)
Suggestions (8)
Strengths
VerdictNEEDS FIXES — two real regressions (C1, C2), one behavior change without migration note (I1), and 6 docs files that still describe deleted code (I7). Everything else is polish or test coverage. Recommended Actions (in order)
|
Critical:
- C1: empty-output guard now skips idle-timeout completions. The on-screen
message says "completed via idle timeout"; flipping that to a failure
contradicted the user-facing log. Added !nodeIdleTimedOut to the guard.
- C2: per-node provider identity is now validated at YAML load time.
Loader iterates dagNodes after parsing and rejects any unknown
provider id with "Node 'X': unknown provider 'Y'. Registered: ...".
The dag-executor's runtime check stays as defense-in-depth.
Important:
- I1: CHANGELOG entry under [Unreleased] > Changed describing the
resolver redesign + an explicit migration line for workflows that
relied on cross-provider model inference.
- I2: restored the dropped mockLogger.error('turn_failed') assertion in
the turn.failed-without-error-message test.
- I3: empty-output test now also asserts store.failWorkflowRun was
called, matching the parallel error_max_budget_usd test pattern.
- I4: new test that proves a node yielding zero assistant text but a
valid structuredOutput is treated as a successful completion (not
caught by the empty-output guard).
- I5: rewrote the post-loop comment in codex/provider.ts to be precise
about which dag-executor branch catches the synthesized result chunk
(the throwing msg.isError branch, distinct from the empty-output
guard's { state: 'failed' } return).
- I6: removed PR-era "redesign" / "Sasha workflow" references from
three test-file comments.
- I7: docs sweep for the deleted isModelCompatible field — six files
updated (CLAUDE.md, two docs guides, quick-reference, contributing
guide, architecture reference).
Polish:
- S3: dropped the dead sawTerminal flag in streamCodexEvents — both
terminal branches `return`, so reaching the post-loop block always
means no terminal fired. Pure simplification.
- S4: dropped parsePiModelRef and PiModelRef from community/pi/index.ts
exports. The parser is consumed only by Pi's provider.ts; making it
package-internal narrows the public surface.
- S6: new Codex test for the bare-stream-close case (zero events,
iterator just ends) — locks in the default fallback message used
when no captured non-MCP error is available.
- S7: new dag-executor test for per-node unknown-provider at runtime.
Bypasses the loader to exercise resolveNodeProviderAndModel's throw,
asserts the node_failed event carries the "unknown provider 'claud'"
detail (the workflow-level fail message is a generic summary).
bun run validate green across all 10 packages.
Collaborator
Author
|
Pushed Critical
Important
Polish addressed (4 of 8)
Polish deferred (4 of 8)
End-to-end smokes still pass on this branch via the linked |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/workflows/src/dag-executor.ts (1)
2663-2677:⚠️ Potential issue | 🟠 MajorThrow here instead of returning a fabricated failed output.
This branch still skips the shared pre-execution failure path at Lines 2870-2899, so an unknown loop provider won’t emit the usual
node_failedevent/message and only shows up later as a generic workflow failure. ReuseresolveNodeProviderAndModel()here, or throw and let the outer catch handle it.As per coding guidelines, "Prefer throwing early with clear errors for unsupported or unsafe states; never silently swallow errors or broaden permissions; document fallback behavior with a comment when intentional and safe".
🤖 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 2663 - 2677, The current branch fabricates a failed node output when encountering an unknown provider (using node.provider/loopProvider and isRegisteredProvider/getRegisteredProviders), which bypasses the shared pre-execution failure handling; change this to throw an Error instead or reuse resolveNodeProviderAndModel() so the outer try/catch emits the normal node_failed event. Specifically, replace the return block that constructs the failed output for unknown loopProvider with a thrown Error containing the node.id and loopProvider (and optionally available providers from getRegisteredProviders()), or call resolveNodeProviderAndModel(node, workflowProvider) here and let its thrown errors bubble up to the outer handler.packages/workflows/src/loader.ts (1)
7-7:⚠️ Potential issue | 🟠 MajorThis pulls
archon/workflowsacross the providers package boundary.
parseWorkflow()now needs the full@archon/providerspackage and a bootstrapped global registry just to validate provider ids. That reintroduces the SDK dependency chain into the workflows package and makes loader behavior depend on external mutable state. Please move provider validation behind an injected callback or a post-parse validation step soloader.tscan stay on the contract-only side.Based on learnings,
archon/workflowsdepends only onarchon/git,archon/paths,archon/providers/types,hono/zod-openapi, andzod; DB/AI/config injected viaWorkflowDeps.Also applies to: 280-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/loader.ts` at line 7, The loader currently imports isRegisteredProvider/getRegisteredProviders into parseWorkflow causing a hard dependency on `@archon/providers`; remove those imports from loader.ts and change parseWorkflow to no longer validate provider ids directly, instead accept an injected validation callback (e.g., validateProviderId) via WorkflowDeps or return the parsed workflow with provider ids left unvalidated and add a separate postParseValidateProviders(workflow, validateProviderId) utility; update call sites to pass a provider-validation function (or run post-parse validation) so archon/workflows remains contract-only and does not depend on the full providers package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md`:
- Around line 684-687: Change the bare fenced code block that contains the
example validation error "Unknown provider 'claud'. Registered: claude, codex,
pi" so the opening fence is labeled with the text language (i.e., replace ```
with ```text) to satisfy markdownlint MD040; locate the fenced block near the
Example validation error and update only the opening fence.
---
Duplicate comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2663-2677: The current branch fabricates a failed node output when
encountering an unknown provider (using node.provider/loopProvider and
isRegisteredProvider/getRegisteredProviders), which bypasses the shared
pre-execution failure handling; change this to throw an Error instead or reuse
resolveNodeProviderAndModel() so the outer try/catch emits the normal
node_failed event. Specifically, replace the return block that constructs the
failed output for unknown loopProvider with a thrown Error containing the
node.id and loopProvider (and optionally available providers from
getRegisteredProviders()), or call resolveNodeProviderAndModel(node,
workflowProvider) here and let its thrown errors bubble up to the outer handler.
In `@packages/workflows/src/loader.ts`:
- Line 7: The loader currently imports
isRegisteredProvider/getRegisteredProviders into parseWorkflow causing a hard
dependency on `@archon/providers`; remove those imports from loader.ts and change
parseWorkflow to no longer validate provider ids directly, instead accept an
injected validation callback (e.g., validateProviderId) via WorkflowDeps or
return the parsed workflow with provider ids left unvalidated and add a separate
postParseValidateProviders(workflow, validateProviderId) utility; update call
sites to pass a provider-validation function (or run post-parse validation) so
archon/workflows remains contract-only and does not depend on the full providers
package.
🪄 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: 197ce462-cdb1-4559-a62d-4ac6772833fc
📒 Files selected for processing (14)
CHANGELOG.mdCLAUDE.mdpackages/docs-web/src/content/docs/book/quick-reference.mdpackages/docs-web/src/content/docs/contributing/adding-a-community-provider.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/architecture.mdpackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/community/pi/index.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor.test.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.ts
💤 Files with no reviewable changes (2)
- packages/docs-web/src/content/docs/reference/architecture.md
- packages/providers/src/community/pi/index.ts
✅ Files skipped from review due to trivial changes (4)
- packages/docs-web/src/content/docs/book/quick-reference.md
- packages/docs-web/src/content/docs/contributing/adding-a-community-provider.md
- CLAUDE.md
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/workflows/src/executor.test.ts
- packages/providers/src/codex/provider.test.ts
Comment on lines
684
to
687
| Example validation error: | ||
| ``` | ||
| Model "sonnet" is not compatible with provider "codex" | ||
| Unknown provider 'claud'. Registered: claude, codex, pi | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
Line 685 opens a bare code fence, which triggers MD040 in markdownlint. Using text here keeps the docs clean and avoids validation noise.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 685-685: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs-web/src/content/docs/guides/authoring-workflows.md` around
lines 684 - 687, Change the bare fenced code block that contains the example
validation error "Unknown provider 'claud'. Registered: claude, codex, pi" so
the opening fence is labeled with the text language (i.e., replace ``` with
```text) to satisfy markdownlint MD040; locate the fenced block near the Example
validation error and update only the opening fence.
Two real issues from CodeRabbit's automated pass on db95e8a: 1. Empty-output fail-stop now applies to loop iterations too. The single-shot AI-node guard at executeNodeInternal only covered prompt/command nodes; executeLoopNode has its own streaming path, so a provider that closed cleanly with zero content could pause an interactive loop with a blank gate or burn the full max_iterations budget. Mirrors the contract of the single-shot guard: `fullOutput.trim() === '' && !iterationIdleTimedOut` fails the iteration with a `loop_iteration_failed` event carrying a clear error. Idle-timeout exits remain exempt for the same reason as single-shot nodes — the on-screen "completed via idle timeout" message would otherwise contradict the failure. 2. Unknown loop providers now throw instead of return-failed. The early-return path bypassed the layer dispatch's outer catch at line 2870, so loop nodes with an invalid per-node `provider:` field skipped the standard `node_failed` event, the user-facing message, and the pre-execution log entry. Throwing reuses the common failure path — same shape as resolveNodeProviderAndModel uses for non-loop nodes. Both align with CLAUDE.md's "fail fast, explicit errors, never silently swallow" principle. The third CodeRabbit finding (boundary violation for `@archon/providers` import in loader.ts) is consistent with existing precedent — `dag-executor.ts`, `executor.ts`, and `validator.ts` already import from the same path; the runtime contract (every entrypoint bootstraps the registry before parseWorkflow runs) is already enforced in tests and documented at `loader.test.ts:31`. bun run validate green across all 10 packages.
Collaborator
Author
|
Thanks @coderabbitai. Pushed Issue 1 (Major) — empty-output fail-stop on loop iterations ✅You're right: if (!iterationIdleTimedOut && fullOutput.trim() === '') {
// emit loop_iteration_failed event + return { state: 'failed' }
}Mirrors the single-shot contract: idle-timeout exits remain exempt (same C1 reasoning — the on-screen "completed via idle timeout" message would otherwise contradict the outcome). Uses Issue 2 (Major) — unknown loop providers should throw ✅Confirmed: the early-return at the dispatch site bypassed the outer catch at line 2870, so Issue 3 (Major) —
|
|
Tip For best results, initiate chat on the files or code changes.
Issue 1 (loop empty-output guard): The Issue 2 (unknown loop provider): Issue 3 (import boundary): Your evidence is persuasive. With ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
popemkt
added a commit
to popemkt/Archon
that referenced
this pull request
Apr 29, 2026
`refactor(workflows): trust the SDK for model validation` (bf1f471) removed `isModelCompatible` from `ProviderRegistration` entirely; Pi got the same treatment in its own registration. Mirror that for Copilot: drop the field from `registerProvider({...})`, the helper function and its export, and the registry-test block that exercised it. The `getProviderInfoList` projection assertion stays as a forward guard, matching the upstream Pi convention. Reported in PR coleam00#1351 review by @danielscholl. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This was referenced May 2, 2026
Closed
Merged
4 tasks
prospapledge88
added a commit
to prospapledge88/Archon
that referenced
this pull request
May 5, 2026
…ch 3 (#8) * feat(workflows): inline sub-agent definitions on DAG nodes (#1276) * feat(workflows): inline sub-agent definitions on DAG nodes Add `agents:` node field letting workflow YAML define Claude Agent SDK sub-agents inline, keyed by kebab-case ID. The main agent can spawn them via the Task tool — useful for map-reduce patterns where a cheap model briefs items and a stronger model reduces. Authors no longer need standalone `.claude/agents/*.md` files for workflow-scoped helpers; the definitions live with the workflow. Claude only. Codex and community providers without the capability emit a capability warning and ignore the field. Merges with the internal `dag-node-skills` wrapper when `skills:` is also set — user-defined agents win on ID collision. * fix(workflows): address PR #1276 review feedback Critical: - Re-export agentDefinitionSchema + AgentDefinition from schemas/index.ts (matches the "schemas/index.ts re-exports all" convention). Important: - Surface user-override of internal 'dag-node-skills' wrapper: warn-level provider log + platform message to the user when agents: redefines the reserved ID alongside skills:. User-wins behavior preserved (by design) but silent capability removal is now observable. - Add validator test coverage for the agents-capability warning (codex node with agents: → warning; claude node → no warning; no-agents field → no warning). - Strengthen NodeConfig.agents duplicate-type comment explaining the intentional circular-dep avoidance and pointing at the Zod schema as authoritative source. Actual extraction is follow-up work. Simplifications: - Drop redundant typeof check in validator (schema already enforces). - Drop unreachable Object.keys(...).length > 0 check in dag-executor. - Drop rot-prone "(out of v1 scope)" parenthetical. - Drop WHAT-only comment on AGENT_ID_REGEX. - Tighten AGENT_ID_REGEX to reject trailing/double hyphens (/^[a-z0-9]+(-[a-z0-9]+)*$/). Tests: - parseWorkflow strips agents on script: and loop: nodes (parallel to the existing bash: coverage). - provider emits warn log on dag-node-skills collision; no warn on non-colliding inline agents. Docs: - Renumber authoring-workflows Summary section (12b → 13; bump 13-19). - Add Pi capability-table row for inline agents (❌, Claude-only). - Add when-to-use guidance (agents: vs .claude/agents/*.md) in the new "Inline sub-agents" section. - Cross-link skills.md Related → inline-sub-agents. - CHANGELOG [Unreleased] Added entry for #1276. * fix(workflows): export ARTIFACTS_DIR, LOG_DIR, BASE_BRANCH to bash nodes (#1387) executeBashNode previously only merged explicit envVars on top of process.env. The three well-known workflow directories (artifactsDir, logDir, baseBranch) were passed as function parameters and used for compile-time substitution of $ARTIFACTS_DIR / $LOG_DIR / $BASE_BRANCH in the script body, but were never added to the subprocess environment. As a result, any script that relied on shell-runtime expansion — e.g. JSON_FILE="${ARTIFACTS_DIR}/foo.output.json" inside a heredoc, an inherited helper script, or a `bash -c` subshell — saw the variable unset and silently fell back to its default (typically an empty string or "."), writing artifacts to the workflow cwd instead of the nominal artifacts directory. Always build subprocessEnv from process.env plus the three well-known directories, then allow explicit envVars to override. Compile-time substitution behavior is unchanged; existing scripts that do not reference these variables are unaffected; user-supplied envVars still win on conflict. * fix(workflow): substitute $nodeId.output refs in approval messages (#1426) * fix(workflow): substitute \$nodeId.output refs in approval messages Approval node messages were emitted as raw strings, bypassing the substituteNodeOutputRefs() pass that prompt/bash/loop/cancel nodes all run. This made interactive workflows like atlas-onboard show literal "\$gather-context.output.repo_name" placeholders to humans at HITL gates, leaving them unable to know what they were approving. Fix: rendered the approval.message through substituteNodeOutputRefs once at the top of the standard approval gate path, then used the resolved string in all 4 emission sites (safeSendMessage, createWorkflowEvent, pauseWorkflowRun, event-emitter). Test: new dag-executor.test case wires a structured-output upstream node into an approval node and asserts pauseWorkflowRun receives the substituted message ("Repo: hcr-els | App: CCELS | Port: 3012") rather than the literal placeholders. Repro: any workflow with an approval node whose message references \$nodeId.output[.field]. Observed in the wild on atlas-onboard's confirm-context HITL gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(workflow): extend approval-substitution test to cover all 4 emission sites Per CodeRabbit review: the original test only verified pauseWorkflowRun received the substituted message, but the fix touches 4 emission sites. A future regression at safeSendMessage / createWorkflowEvent / event-emitter would silently leave the test passing while users still saw raw $node.output placeholders. Adds two additional assertions: - platform.sendMessage prompt contains substituted message + does NOT contain literal $gather-context.output placeholders - The persisted approval_requested workflow event's data.message is substituted Event-emitter assertion deferred (no existing pattern for spying on the global emitter in this test file). Two of three secondary surfaces covered closes the practical regression risk — both are user-visible (chat prompt + audit-log event); the emitter is internal only. Test count: 7 pass / 22 expect() (was 18). Full suite 193 pass / 353 expect() — no regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(workflows): add mutates_checkout to allow concurrent runs on live checkout (#1438) * feat(workflows): add mutates_checkout field to skip path-lock for concurrent runs Add `mutates_checkout: boolean` (optional, default true) to the workflow schema. When set to false, the executor skips the path-exclusive lock that serializes all runs on the same working path, allowing N concurrent runs on the same live checkout. The primary use case is `maintainer-review-pr`, which reads shared state but writes only to per-run artifact paths and GitHub PR comments — two parallel reviews of different PRs should not fail with "Workflow already active on this path". Changes: - `schemas/workflow.ts`: add optional `mutates_checkout` field - `loader.ts`: parse and propagate the field (warn-and-ignore on invalid values) - `executor.ts`: wrap path-lock guard in `if (workflow.mutates_checkout !== false)` - `executor.test.ts`: two new tests in the concurrent-run guard suite - `maintainer-review-pr.yaml`: opt in with `mutates_checkout: false` * test(workflows): add loader tests for mutates_checkout parsing - Add 5 tests covering false, true, omitted, and invalid (string "yes") values - Invalid non-boolean values are silently dropped with warn — now explicitly tested - Remove the // end mutates_checkout guard trailing comment (no precedent in file) - Clarify loader comment: "parse/warn pattern" not "warn-and-ignore pattern" to avoid implying the return style matches interactive * simplify: collapse nodeType/aiFields pair into single nonAiNode object in parseDagNode * feat(workflows): support explicit tags in workflow YAML (#1190) Add optional `tags: string[]` to `workflowBaseSchema`. Explicit values take precedence over keyword inference; `tags: []` suppresses inference end-to-end; omitting the field falls back to inference (backwards compatible). Non-array values warn-and-ignore matching the sibling `worktree`/`additionalDirectories` patterns. * feat(workflows): expose $LOOP_PREV_OUTPUT in loop node prompts (#1286) (#1367) * feat(workflows): expose $LOOP_PREV_OUTPUT in loop node prompts (#1286) Adds a new substitution variable that carries the previous loop iteration's cleaned output into the next iteration's prompt. Empty on iteration 1; the prior iteration's output (after stripCompletionTags) on iteration 2+. Why: fresh_context: true loops have no way to reference what the previous pass produced or why it failed without dragging the full session forward. $LOOP_PREV_OUTPUT closes that gap with zero session-cost — same trust boundary as $nodeId.output, no new external surface. Changes: - packages/workflows/src/executor-shared.ts: substituteWorkflowVariables accepts a 10th positional loopPrevOutput arg and substitutes $LOOP_PREV_OUTPUT (defaults to ''). - packages/workflows/src/dag-executor.ts: executeLoopNode passes lastIterationOutput on iteration 2+ (and explicit '' on iteration 1 / the first iteration of an interactive resume, since lastIterationOutput is a per-call variable that does not survive resume metadata). - Unit tests: 3 new cases in executor-shared.test.ts. - Integration tests: 2 new cases in dag-executor.test.ts verifying the prompt sent to the AI on iter 1 vs iter 2, and that the value reflects cleaned output (no <promise> tags). - Docs: variables.md, loop-nodes.md (new "Retry-on-failure" pattern), CLAUDE.md variable reference. Backward compatibility: prompts that don't reference $LOOP_PREV_OUTPUT are unaffected. All 843 workflow tests + type-check + lint + format:check + bun run validate pass locally. * docs: address coderabbit review on variables/loop-nodes - variables.md: include $LOOP_PREV_OUTPUT in substitution-order list and availability table to match the new variable row at line 30 - loop-nodes.md: document the interactive-resume exception where the first iteration after an approval-gate resume still receives an empty $LOOP_PREV_OUTPUT regardless of iteration number (per dag-executor.ts L1781-1783 where i === startIteration always clears prev output) * docs(changelog): add Unreleased entry for $LOOP_PREV_OUTPUT (#1367 review) * test(loop): add resume-from-approval integration test for $LOOP_PREV_OUTPUT (#1367 review) Per maintainer-review-pr suggestion (Wirasm): two-call integration test covering the resume-from-approval scenario. - Call 1: fresh interactive loop pauses at the gate after iteration 1 and asserts $LOOP_PREV_OUTPUT substitutes to empty on iter 1 (no prior output) plus the gate pause is recorded. - Call 2: resumed run with metadata.approval populated. The first resumed iteration must substitute $LOOP_PREV_OUTPUT to '', NOT to the paused run's iter-1 output (which lived in a different process and is not persisted). $LOOP_USER_INPUT still flows through as normal. Locks the documented invariant at dag-executor.ts:1769-1772. --------- Co-authored-by: voidborne-d <DottyEstradalco@allergist.com> * refactor(workflows): trust the SDK for model validation (#1463) * refactor(workflows): trust the SDK for model validation Drops cross-provider model inference and hard-coded model allow-lists. The string a workflow author writes in `model:` is forwarded to the SDK unchanged; the SDK and its API decide whether the model exists. Provider identity is the only thing Archon validates at load time — typos like `provider: claud` are caught early; everything else fails at runtime through the SDK's normal error path. Why this matters: a recent run on Sasha showed `provider: claude` + `model: opus[1m]` getting silently routed to Codex (because Codex's isModelCompatible was defined as the complement of Claude's, so anything not literally `sonnet|opus|haiku` matched). Codex then rejected the model as a `⚠️` system warning and the node "completed" in 2.1 seconds with empty output, after which the workflow opened a hallucinated PR. Three stacked bugs and two amplifiers; this commit removes all five. Changes: - Delete model-validation.ts entirely (inferProviderFromModel and isModelCompatible are gone). Drop the matching field from ProviderRegistration and from the claude/codex/pi entries. - Replace the resolver in executor.ts and dag-executor.ts (both the per-node and per-loop paths) with a flat `node.provider ?? workflow.provider ?? config.assistant`. Model never influences provider selection; load-time validation is just isRegisteredProvider on the resolved provider id. - Remove the dag-node Zod superRefine that recomputed model-compat — load-time provider validation moved to loader.ts. - Codex provider: stream loop now matches Claude's contract. error events that aren't followed by turn.completed yield `result.isError: true` (subtype `codex_stream_incomplete`) so the dag-executor's existing isError path catches them. turn.failed becomes `codex_turn_failed` with the same shape. Iterator close without a terminal event is itself a fail-stop. MCP-client errors remain filtered (Codex retries those internally). - dag-executor: AI nodes that exit the streaming loop with empty assistant text and no structured output now fail with `dag.node_empty_output` instead of completing silently — the Sasha bug's final amplifier. Bash/script/approval nodes are unaffected. Tests: model-validation.test.ts and isPiModelCompatible block deleted; codex provider tests rewritten to assert the new fail-stop contract; dag-executor empty-output test flipped to assert failure; new tests cover (a) loader rejecting unknown provider, (b) loader accepting any model string with a known provider, (c) executor passing provider+model through without re-routing, (d) executor throwing on unknown provider, (e) Codex synthesizing fail-stop on iterator close. Two cost-tracking tests adjusted to yield non-empty assistant text since their intent was cost accumulation, not empty-output handling. bun run validate: green (check:bundled, type-check, lint --max-warnings 0, format:check, all packages' test suites — 0 fail). End-to-end smoke (.archon/workflows/test-workflows/): - e2e-deterministic: PASS (engine healthy) - e2e-codex-smoke: PASS (Codex sendQuery + structured output work) - e2e-claude-smoke: FAIL with `error: unknown option '--no-env-file'` — this is a regression from the SDK 0.2.121 bump (#1460), not from this redesign. The Claude provider source is unchanged on this branch. To be fixed separately. * fix(workflows): address review on #1463 Critical: - C1: empty-output guard now skips idle-timeout completions. The on-screen message says "completed via idle timeout"; flipping that to a failure contradicted the user-facing log. Added !nodeIdleTimedOut to the guard. - C2: per-node provider identity is now validated at YAML load time. Loader iterates dagNodes after parsing and rejects any unknown provider id with "Node 'X': unknown provider 'Y'. Registered: ...". The dag-executor's runtime check stays as defense-in-depth. Important: - I1: CHANGELOG entry under [Unreleased] > Changed describing the resolver redesign + an explicit migration line for workflows that relied on cross-provider model inference. - I2: restored the dropped mockLogger.error('turn_failed') assertion in the turn.failed-without-error-message test. - I3: empty-output test now also asserts store.failWorkflowRun was called, matching the parallel error_max_budget_usd test pattern. - I4: new test that proves a node yielding zero assistant text but a valid structuredOutput is treated as a successful completion (not caught by the empty-output guard). - I5: rewrote the post-loop comment in codex/provider.ts to be precise about which dag-executor branch catches the synthesized result chunk (the throwing msg.isError branch, distinct from the empty-output guard's { state: 'failed' } return). - I6: removed PR-era "redesign" / "Sasha workflow" references from three test-file comments. - I7: docs sweep for the deleted isModelCompatible field — six files updated (CLAUDE.md, two docs guides, quick-reference, contributing guide, architecture reference). Polish: - S3: dropped the dead sawTerminal flag in streamCodexEvents — both terminal branches `return`, so reaching the post-loop block always means no terminal fired. Pure simplification. - S4: dropped parsePiModelRef and PiModelRef from community/pi/index.ts exports. The parser is consumed only by Pi's provider.ts; making it package-internal narrows the public surface. - S6: new Codex test for the bare-stream-close case (zero events, iterator just ends) — locks in the default fallback message used when no captured non-MCP error is available. - S7: new dag-executor test for per-node unknown-provider at runtime. Bypasses the loader to exercise resolveNodeProviderAndModel's throw, asserts the node_failed event carries the "unknown provider 'claud'" detail (the workflow-level fail message is a generic summary). bun run validate green across all 10 packages. * fix(workflows): address CodeRabbit review on #1463 Two real issues from CodeRabbit's automated pass on db95e8a6: 1. Empty-output fail-stop now applies to loop iterations too. The single-shot AI-node guard at executeNodeInternal only covered prompt/command nodes; executeLoopNode has its own streaming path, so a provider that closed cleanly with zero content could pause an interactive loop with a blank gate or burn the full max_iterations budget. Mirrors the contract of the single-shot guard: `fullOutput.trim() === '' && !iterationIdleTimedOut` fails the iteration with a `loop_iteration_failed` event carrying a clear error. Idle-timeout exits remain exempt for the same reason as single-shot nodes — the on-screen "completed via idle timeout" message would otherwise contradict the failure. 2. Unknown loop providers now throw instead of return-failed. The early-return path bypassed the layer dispatch's outer catch at line 2870, so loop nodes with an invalid per-node `provider:` field skipped the standard `node_failed` event, the user-facing message, and the pre-execution log entry. Throwing reuses the common failure path — same shape as resolveNodeProviderAndModel uses for non-loop nodes. Both align with CLAUDE.md's "fail fast, explicit errors, never silently swallow" principle. The third CodeRabbit finding (boundary violation for `@archon/providers` import in loader.ts) is consistent with existing precedent — `dag-executor.ts`, `executor.ts`, and `validator.ts` already import from the same path; the runtime contract (every entrypoint bootstraps the registry before parseWorkflow runs) is already enforced in tests and documented at `loader.test.ts:31`. bun run validate green across all 10 packages. * fix: ensure all PR-creating workflows target $BASE_BRANCH (#1479) - Add --base $BASE_BRANCH to gh pr create in archon-architect, archon-refactor-safely, and archon-implement-issue - Add verify-pr-base bash node to all 9 PR-creating workflows that auto-corrects via gh pr edit if the AI mis-targets - Rewire downstream depends_on edges through verify-pr-base - Regenerate bundled-defaults.generated.ts * fix(workflows): skip markdown code blocks in $nodeId.output validation (#1478) The DAG-structure validator scans `node.when`, `node.prompt`, and `loop.prompt` strings for `$nodeId.output` references. Prompt bodies in builder-style workflows embed fenced and inline code as documentation for the LLM (e.g. `archon-workflow-builder` shows how to author a script node), and those literal `$<other-node>.output` mentions were being treated as real cross-node references. Result: `archon-workflow-builder` (a bundled default) failed to load, and `bun run cli workflow run archon-workflow-builder ...` reported "references unknown node '$other-node.output'". Strip triple-backtick fenced blocks and single-backtick inline code from prompt and loop.prompt before scanning. `when:` clauses are JS-like expressions and never carry markdown code, so they pass through unchanged. Real cross-node refs in prose continue to validate. Also wraps one bare `$nodeId.output` mention in `archon-workflow-builder.yaml` Rules section in inline backticks so it reads as documentation alongside the surrounding `$nodeId.output` mentions that already use this style. Closes #1413 * fix(workflows): stop sweeping scratch artifacts from every git add -A site (#1506) * fix(simplify): stage only edited files, forbid scratch artifacts The simplify command used `git add -A`, which sweeps untracked review/ report files (e.g. `review/scope.md` left by upstream review nodes) into the simplification commit. Replace it with explicit per-file staging using the list of paths edited in Phase 2, plus a forbidden-paths list so review artifacts, PR-body scratch files, and anything under `$ARTIFACTS_DIR` cannot leak into the commit. * fix(fix-github-issue): forbid scratch artifacts in create-pr step The inline create-pr prompt told the agent to "stage and commit" any uncommitted changes, which lets transient artifacts from upstream nodes (`.pr-body.md`, `review/scope.md`, scratch reports) land in the implementation commit and PR diff. Replace the loose instruction with explicit per-file staging, a forbidden-paths list, and a rule that any PR body file written for `--body-file` must live at `$ARTIFACTS_DIR/pr-body.md` or `/tmp/` — never inside the worktree. Applied to both the default and experimental variants. * fix(workflows): purge remaining git add -A in worktree-context steps Same class of bug as the simplify and create-pr fixes: every worktree-facing default that used `git add -A` could sweep transient review/scratch artifacts (`.pr-body.md`, `review/scope.md`, `*-report.md`, anything left under `$ARTIFACTS_DIR`) into the commit. Replace with explicit per-file staging plus a forbidden-paths list and a `git status --porcelain` verification step. Touched: - commands: archon-create-pr, archon-finalize-pr, archon-fix-issue, archon-implement-issue, archon-implement-review-fixes - workflows: archon-piv-loop (3 sites), archon-ralph-dag, archon-refactor-safely Intentionally left as `git add -A`: - archon-release.yaml: working tree validated clean before this step; comment already explains why. - archon-adversarial-dev.yaml: operates inside `$ARTIFACTS_DIR/app/`, a dedicated scratch repo, not the user's worktree. * fix(workflows): substitute array/object node output fields as JSON (#1482) * chore: update Homebrew formula for v0.3.9 * chore(release-skill): use --help (not version) for Step 1.5 smoke probe (#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. * chore(test-release-skill): preserve archon-stable across test cycles 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. * fix(providers/pi): install PI_PACKAGE_DIR shim so Pi workflows run in a compiled binary (#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. * feat(providers): autodetect canonical binary install paths for Claude and Codex (#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. * fix(providers/test): use os.homedir() instead of $HOME in claude binary 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> * fix(server): contain Discord login failure so it doesn't kill the server (#1365) Reported in #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> * docs(script-nodes): dedicated guide + teach the archon skill (#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 * docs/skill: general hardening — fix inaccuracies, fill workflow/CLI/env gaps, add good-practices + troubleshooting (#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 #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 #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 #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 #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 #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 (#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 (f10b989e for C1, 66d2b86e 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(#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(#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. * chore(workflows): switch default Opus pin to opus[1m] alias (#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). * fix(workflow): migrate piv-loop plan handoff to $ARTIFACTS_DIR (#1398) * fix(workflow): migrate piv-loop plan handoff to $ARTIFACTS_DIR (#1380) The create-plan node used a relative path (.claude/archon/plans/{slug}.plan.md) that the AI agent would sometimes write to a different location, breaking all downstream nodes that glob for the plan file. Migrated all plan/progress file references to $ARTIFACTS_DIR/plan.md and $ARTIFACTS_DIR/progress.txt, matching the pattern used by archon-fix-github-issue and other workflows. Changes: - Replace slug-based plan path with $ARTIFACTS_DIR/plan.md in create-plan node - Replace ls -t glob discovery with direct $ARTIFACTS_DIR/plan.md reads in refine-plan, code-review, and fix-feedback nodes - Replace empty-string guard with file-existence check in implement-setup bash - Migrate progress.txt references in implement loop to $ARTIFACTS_DIR/ - Add explicit plan/progress paths in finalize node - Regenerated bundled-defaults.generated.ts Fixes #1380 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(workflow): address review findings in archon-piv-loop - Rename 'Step 2: Write the Plan' to 'Step 2: Plan File Location' to eliminate the duplicate heading that collided with Step 3's identical title in the create-plan node - Guard implement-setup against a 0-task plan file: exit 1 with a clear error when no '### Task N:' sections are found, preventing a silent no-op implement loop - Remove 2>/dev/null from code-review commit so pre-commit hook failures and other stderr are visible to the agent instead of silently swallowed - Replace '|| true' on git push in finalize with an explicit WARNING echo so push failures (auth, upstream conflict, no remote) surface to the agent rather than being silently ignored - Regenerate bundled-defaults.generated.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(workflows): regenerate bundled defaults to match opus[1m] alias The bundle was stale relative to the YAML sources after #1395 merged — check:bundled was failing CI. Regenerated; no YAML edits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(workflows): add anyFailed status derivation coverage for DAG executor (#1403) PIV Task 1: Adds three new tests in a dedicated describe block 'executeDagWorkflow -- final status derivation' covering the anyFailed branch (dag-executor.ts ~line 2956) that previously had no direct test: - one success + one independent failure calls failWorkflowRun (not completeWorkflowRun) - multiple successes + one failure calls failWorkflowRun (not completeWorkflowRun) - trigger_rule: none_failed skips dependent node but anyFailed still marks run failed Fixes #1381. * docs/skill: add parameter-matrix.md quick-lookup reference New reference for the archon skill: a single-glance lookup of which parameter works on which node type, an intent-based "how do I..." table, a consolidated silent-failure catalog, and an inline agents: section (previously only referenced via archon.diy). Purpose is complementary, not duplicative: - workflow-dag.md remains the authoring guide - dag-advanced.md remains the hooks/MCP/skills/retry deep-dive - good-practices.md remains the patterns and anti-patterns - parameter-matrix.md is the grep-this-first lookup when you know the outcome you want but not which field gets you there Also registers the new reference in SKILL.md routing table. * docs: point contributors at PR template and Closes #N convention Add explicit references to .github/PULL_REQUEST_TEMPLATE.md in both CONTRIBUTING.md and CLAUDE.md, plus a reminder to link issues with Closes/Fixes/Resolves so they auto-close on merge. Repo-triage runs were flagging dozens of partially-filled or unlinked PRs each cycle. * feat(workflows): add maintainer-standup workflow for daily PR/issue triage (#1428) * feat(workflows): add maintainer-standup workflow for daily PR/issue triage Daily morning briefing that pulls origin/dev, triages all open PRs and assigned issues against direction.md, and surfaces progress vs. the previous run. Designed for live-checkout use (worktree.enabled: false) so it can read its own state. Layout under .archon/maintainer-standup/: - direction.md (committed) — project north-star: what Archon IS / IS NOT. Drives PR P4 polite-decline classification with cited clauses. - README.md / profile.md.example — setup docs and template for new maintainers. - profile.md, state.json, briefs/YYYY-MM-DD.md — gitignored, pe…
5 tasks
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
1 participant
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
provider: claude+model: opus[1m]get silently routed to Codex on a recent run (Sasha workflow run6b4636965c6639b9fba66e52cbd88fb2), which Codex rejected as a⚠️system warning. The implement node "completed" in 2.1 s with empty output and the workflow happily opened a hallucinated PR. Root cause: hard-coded model allow-lists in the provider registry + cross-provider model inference + Codex'serrorevent being treated as a recoverable warning + empty assistant output passing as success.model-validation.tsentirely. Removed theisModelCompatiblefield fromProviderRegistrationand from the claude/codex/pi entries.executor.tsanddag-executor.ts(per-node + per-loop paths) with a flatnode.provider ?? workflow.provider ?? config.assistant. Model never influences provider selection. Load-time validation is justisRegisteredProvideron the resolved provider id.resultchunk.errorevents captured (non-MCP) but not yielded as⚠️; iterator close withoutturn.completed/turn.failedsynthesizes a fail-stopresult.isError: true(subtypecodex_stream_incomplete).turn.failedbecomescodex_turn_failed.command:/prompt:). Bash/script/approval nodes are unaffected — they have their own dispatch.IAgentProvider.sendQuerycontract. TheMessageChunkdiscriminated union. The MCP-failure filtering in the dag-executor. The capability-warnings block. Pi'sparsePiModelRefparser (still needed to split<backend>/<model>refs). The web UI /GET /api/providersroute (already excluded the deleted field).UX Journey
Before
```
workflow YAML:
provider: claude
model: opus[1m] # ← invalid Claude alias
nodes:
- id: implement
command: archon-fix-issue
runtime:
inferProviderFromModel('opus[1m]', 'claude')
→ Claude.isModelCompatible('opus[1m]') = false (literal alias check, no [1m] suffix)
→ Codex.isModelCompatible('opus[1m]') = true (defined as Claude-complement)
→ returns 'codex' ← silent re-route
Codex SDK rejects 'opus[1m]':⚠️ ...' }
event.type === 'error' { message: "...not supported..." }
→ yield { type: 'system', content: '
→ continue
→ SDK closes without 'turn.completed' or 'turn.failed'
→ no result.isError chunk ever emitted
→ dag-executor's loop exits cleanly
→ node 'implement' completed in 2.148s with empty output ✓ (silently)
validate, create-pr proceed against an unmodified worktree
PR opened describing changes that don't exist.
```
After
```
workflow YAML:
provider: claude
model: opus[1m] # passed through to SDK as-is
runtime:
resolveProvider:
node.provider (undefined) ?? workflow.provider ('claude') ?? config.assistant
→ 'claude' ← workflow root wins
isRegisteredProvider('claude') = true
Claude SDK receives model='opus[1m]'
→ SDK rejects with isError result OR
→ invalid model error surfaces up the chain ← fail-stop, loud
workflow halts at implement node.
validate/create-pr never run on stale state.
```
(The fix for the literal
opus[1m]typo is the user writingclaude-opus-4-7[1m]instead — the full Claude SDK model ID with the 1M-context suffix. Archon doesn't second-guess the string.)Architecture Diagram
Before
```
DagNode.model ─────┐
▼
┌─────────────────────┐ ┌──────────────────────┐
│ inferProviderFrom- │──────▶│ Claude.isModel- │
│ Model(model, def) │ │ Compatible(model) │ → false
│ │ └──────────────────────┘
│ │ ┌──────────────────────┐
│ │──────▶│ Codex.isModel- │
│ │ │ Compatible (= !Cl.) │ → true ← misroute
│ │ └──────────────────────┘
│ returns provider id │
└─────────────────────┘
│
▼
┌────────────────────┐
│ resolveNodeProvider│
│ AndModel │ workflow.provider IGNORED
└────────────────────┘ when node.model is set
```
After
```
DagNode.provider ────┐
WorkflowDef.provider ┼──▶ resolveProvider() ──▶ isRegisteredProvider() ──▶ throw if unknown
config.assistant ────┘ (fail at load)
```
Connection inventory:
dag-executormodel-validation.tsexecutormodel-validation.tsloadermodel-validation.ts@archon/providers#isRegisteredProviderschemas/dag-node.tsmodel-validation.tsProviderRegistrationtypeisModelCompatiblefielddag-executor@archon/providers#isRegisteredProvider/getRegisteredProvidersexecutorloadererroreventresult.isErrorchunksystem: '⚠️ ...'warningturn.failedresult.isErrorchunksystem: '❌ ...'warningresult.isErrorchunk if no terminalLabel Snapshot
risk: mediumsize: Mworkflows,providersworkflows:executor,workflows:dag-executor,workflows:loader,providers:codex,providers:registryChange Metadata
refactormulti(workflows + providers)Linked Issue
Validation Evidence (required)
bun run validate # EXIT=0All five gates pass on the rebased branch —
check:bundled,type-check(10 packages),lint --max-warnings 0,format:check,test(every package, every file0 fail).End-to-end smoke (
.archon/workflows/test-workflows/, run via the linkedarchonbinary against the redesign branch with a sentinelARCHON_REDESIGN_SENTINEL_$$=...in Archon's.env):e2e-deterministicstripped 23 keys from .../Archon (.env, .env.local)PASS: all deterministic nodes produced outpute2e-codex-smokestripped 23 keysPASS: simple='4' structured='{category:math}'e2e-claude-smokestripped 23 keysPASS: simple='4'No
node_empty_output(would fire if any AI node yielded zero output), nocodex_stream_incomplete(would fire if Codex closed without a terminal event), nocodex_turn_failed. All three workflows complete cleanly.Security Impact (required)
Compatibility / Migration
provider:andmodel:keep working unchanged. Workflows that relied on cross-provider model inference (e.g. set onlymodel: gpt-5.2with noprovider:, expecting Archon to pickcodex) will now fall back toconfig.assistant— they need either an explicitprovider:or a config default. This is the misbehavior the redesign fixes: the previous inference made model strings authoritative over the workflow's stated provider.Human Verification (required)
archonCLI on the rebased branch..envproof thatstripCwdEnv()still fires per run (stripped 23 keysin stderr) — env isolation is unaffected by this redesign.resulton iterator close.parsePiModelRefis preserved (Pi still needs to split<backend>/<model>refs internally); the empty-output check intentionally only applies to AI nodes (bash/script/approval go through their own dispatch and are not on this path).Side Effects / Blast Radius (required)
command:/prompt:/loop:) — that is, every non-trivial workflow. Bash, script, approval, cancel nodes are unaffected.dag.node_empty_output. Such workflows should add an explicit completion message to the prompt or use a non-AI node type.provider:(see Compatibility section).dag.node_empty_output,codex_stream_incomplete, and the existingdag.node_sdk_error_resultcover the three failure modes uniformly.Rollback Plan (required)
git revert <merge-sha>. No data, config, or schema changes — pure code refactor.errorevents reappearing as⚠️system chunks in chat.Risks and Mitigations
model: gpt-5.2with noprovider:to route to codex even whendefaultAssistant: claude).provider: codexto the workflow root. The error message at load time names the misconfigured provider id and lists registered ones; the fix is one YAML line.Reply with "done.") or use a non-AI node type.Summary by CodeRabbit
Bug Fixes
Changes
Documentation