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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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 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