refactor: extract providers from @archon/core into @archon/providers#1137
Merged
refactor: extract providers from @archon/core into @archon/providers#1137
Conversation
Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior.
Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk.
Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughExtracts AI agent providers into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Core as Core / Orchestrator
participant Workflows as Workflows / DAG
participant ProvidersPkg as Providers (`@archon/providers`)
participant Provider as ProviderImpl (Claude/Codex)
participant SDK as ModelSDK
Client->>Core: trigger request / message
Core->>Workflows: resolve node & build base options
Workflows->>ProvidersPkg: deps.getAgentProvider(provider)
ProvidersPkg->>Provider: return provider instance
Core->>Provider: sendQuery(prompt, cwd, resumeId, SendQueryOptions{assistantConfig,nodeConfig})
Provider->>SDK: call provider SDK with mapped options
SDK-->>Provider: stream MessageChunk events
Provider-->>Core: yield MessageChunk generator
Core-->>Client: stream aggregated results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
PR Review SummaryReviewed by: 6 specialized agents (code-reviewer, docs-impact, type-design-analyzer, silent-failure-hunter, pr-test-analyzer, code-simplifier) Critical Issues (1 found)
Important Issues (6 found)
Suggestions (7 found)
Documentation Issues (9 stale references)
Strengths
Verdict: NEEDS FIXESMust address before merge:
Should address before merge: Recommended follow-ups: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
eslint.config.mjs (1)
23-23: Narrow this ignore pattern to helpers only.
'**/src/test/**'is broad and disables linting/type-aware checks for every file undersrc/test, not just fixtures/mocks. Prefer targeting specific helper paths (for example**/src/test/{fixtures,mocks}/**) so real test code stays covered.Based on learnings: Enforce strict TypeScript configuration with complete type annotations for all functions; no
anytypes without explicit justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.mjs` at line 23, The ignore pattern '**/src/test/**' in eslint.config.mjs is too broad—replace it with a narrower pattern that only skips helper files (e.g. '**/src/test/{fixtures,mocks}/**' or similar) so real test code remains linted; update the array entry that currently contains '**/src/test/**' to the new scoped pattern. Additionally enable strict TypeScript checks by ensuring your TypeScript/ESLint config enforces no implicit any and full type annotations (activate "strict": true in tsconfig.json and enable ESLint rules like "@typescript-eslint/no-explicit-any" and "@typescript-eslint/typedef" where applicable) so functions require complete type annotations.packages/providers/src/types.ts (1)
1-177: Complete the contract subpath withUnknownProviderError.The PR objective calls out
UnknownProviderErroras part of@archon/providers/types, but this file currently only exports the defaults, chunk types, andIAgentProvider. Without the shared error shape here, callers still need a runtime-package dependency or a duplicate type to distinguish bad provider IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/types.ts` around lines 1 - 177, Add a shared UnknownProviderError export to the providers contract so callers can type/identify missing provider errors without pulling runtime packages: define and export an interface or class named UnknownProviderError (e.g., export class UnknownProviderError extends Error { providerId?: string } or export interface UnknownProviderError { name: 'UnknownProviderError'; providerId?: string; message: string }) in the same file alongside MessageChunk and IAgentProvider, and ensure it is part of the public types surface so consumers can import UnknownProviderError from the contract subpath to detect bad provider IDs.packages/providers/tsconfig.json (1)
6-7: Exclude test support files from the package tsconfig.
"**/*.test.ts"missessrc/test/mocks/logger.ts, so that helper is now part of the main package compilation set. Excludingsrc/test/**keeps test-only mocks and theirpinotypes out of the runtime source surface.♻️ Proposed change
- "exclude": ["node_modules", "dist", "**/*.test.ts"] + "exclude": ["node_modules", "dist", "src/test/**/*", "**/*.test.ts"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/tsconfig.json` around lines 6 - 7, The package tsconfig's exclude array currently only excludes "**/*.test.ts" which still allows test helper files like src/test/mocks/logger.ts into compilation; update the tsconfig.json exclude array (the "exclude" property) to also exclude the test folder pattern (e.g., add "src/test/**") so all test-only mocks and types are omitted from the package build, leaving the existing "include" and other exclude entries intact.packages/core/src/services/title-generator.test.ts (1)
29-31: Stale type annotation —toolsis no longer used.The
optionstype annotation still includestools?: string[], but the implementation now passesnodeConfig: { allowed_tools: [] }instead. This doesn't break tests but creates misleading documentation.🧹 Suggested type update
const mockSendQuery = mock(async function* (): AsyncGenerator<MessageChunk> { yield { type: 'assistant', content: 'Summarize Project README' }; yield { type: 'result' }; }) as Mock< ( prompt: string, cwd: string, resumeSessionId?: string, - options?: { model?: string; tools?: string[] } + options?: { model?: string; nodeConfig?: { allowed_tools?: string[] } } ) => AsyncGenerator<MessageChunk> >;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/title-generator.test.ts` around lines 29 - 31, Update the stale type annotation on the function signature that returns AsyncGenerator<MessageChunk>: remove the obsolete "tools?: string[]" from the options type so it reads options?: { model?: string } (or equivalent) to match current implementation that uses nodeConfig: { allowed_tools: [] }; locate the options param in the test (the function returning AsyncGenerator<MessageChunk>) and adjust any related test/fixture types or comments to reflect the removal of tools.packages/workflows/src/dag-executor.test.ts (1)
96-125: Type the capability helpers against the provider contract.Give
mockClaudeCapabilitiesandmockCodexCapabilitiesan explicitProviderCapabilitiesreturn type so these mocks fail fast if the shared contract adds or renames a flag.As per coding guidelines "
**/*.{ts,tsx}: Use strict TypeScript configuration with complete type annotations for all functions; noanytypes without explicit justification`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 96 - 125, The helpers mockClaudeCapabilities and mockCodexCapabilities lack an explicit return type and should be annotated to match the shared ProviderCapabilities contract; update both function signatures to return ProviderCapabilities (e.g., const mockClaudeCapabilities = (): ProviderCapabilities => { ... } and likewise for mockCodexCapabilities) so TypeScript will enforce the capability flags align with the provider contract and surface errors if flags are added/renamed.
🤖 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/core/src/orchestrator/orchestrator-agent.test.ts`:
- Around line 96-101: The mock for getAgentProvider currently returns an empty
object from getCapabilities(), which breaks the ProviderCapabilities contract;
update the mock in the test so getCapabilities() returns a full, test-safe
ProviderCapabilities-shaped object (include required keys such as model,
supportsStreaming, maxTokens, inputFormats, outputFormats, and any other
contract fields) so downstream consumers like dag-executor see a realistic,
typed capabilities object; locate the mock definition for
getAgentProvider/getCapabilities in orchestrator-agent.test.ts and replace the
{} return with the complete capabilities stub.
In `@packages/core/src/workflows/store-adapter.test.ts`:
- Around line 47-49: The test is using mock.module('@archon/providers', ...)
with getAgentProvider returning an empty object which conflicts with other tests
that mock the same module with different shapes; update the mock to use a
consistent, shared shape across tests (e.g., provide a getAgentProvider
implementation that matches the full object shape used elsewhere including
sendQuery/getType/getCapabilities) or remove mock.module usage and import a
single shared test double for `@archon/providers`; specifically change the
mock.module call that defines getAgentProvider in store-adapter.test.ts so it
returns the same interface/implementation used by orchestrator-agent.test.ts (or
centralize the mock into a shared test helper) to eliminate order-dependent,
process-global conflicts.
In `@packages/providers/src/claude/config.ts`:
- Around line 21-27: The code currently filters raw.settingSources into valid
values and only assigns result.settingSources when valid.length > 0, which
silently broadens to Claude's default when the input was an explicit empty array
or contained typos; update the validation in claude/config.ts to detect invalid
inputs and either (a) preserve an explicit empty array by assigning
result.settingSources = [] when raw.settingSources is an array with no valid
entries, or (b) throw a clear validation error if raw.settingSources contains
unknown entries; locate the logic around raw.settingSources and
result.settingSources and implement one of these behaviors (preserve empty array
or throw) and include a brief error message mentioning invalid settingSources
values when throwing.
In `@packages/providers/src/claude/provider.ts`:
- Around line 395-418: The current check uses if (nodeConfig.skills) which is
truthy for an empty array and thus creates a synthetic 'Skill' agent even when
no skills exist; change the guard to check for a non-empty array (e.g.,
nodeConfig.skills && nodeConfig.skills.length > 0) before building agentId
'dag-node-skills', agentDef, assigning options.agents/options.agent, extending
options.allowedTools with 'Skill', and logging via getLog().info so the
synthetic agent/tool is only created when skills.length > 0.
In `@packages/providers/src/codex/provider.ts`:
- Around line 178-183: The code memoizes a single process-wide Codex instance so
passing assistantConfig.codexBinaryPath per request in sendQuery() only affects
the first request; update getCodex() to either stop using a single global
instance or to cache instances keyed by the binary path
(assistantConfig.codexBinaryPath) so each unique binary path returns its own
Codex process. Locate getCodex and change its memoization strategy to use the
codexBinaryPath as the cache key (or always spawn a new Codex when a different
path is provided), and ensure sendQuery()/provider.ts continues to pass
requestOptions?.assistantConfig.codexBinaryPath through to getCodex().
- Around line 476-479: The current getLog().warn call logs raw assistant output
via the outputPreview field (accumulatedText) which may contain PII; update the
log at the codex.structured_output_not_json site to remove outputPreview and
instead log only non-sensitive metadata (for example the output length via
accumulatedText.length and/or a deterministic non-reversible fingerprint like a
short hash), using the existing getLog().warn invocation and leaving the event
name 'codex.structured_output_not_json' unchanged.
- Around line 269-273: The loop that listens for streaming events in sendQuery
currently breaks silently when requestOptions?.abortSignal?.aborted is true;
instead throw a clear abort error so callers can't interpret a partial stream as
success. Update the for-await loop in sendQuery (the block iterating
result.events) to detect requestOptions?.abortSignal?.aborted and throw a
specific error (e.g., an AbortError or Error with "query aborted" message)
rather than calling getLog().info and break; ensure the thrown error is
documented in sendQuery's behavior so callers can handle cancellation
explicitly.
In `@packages/workflows/src/dag-executor.ts`:
- Around line 1401-1412: buildLoopNodeOptions currently only passes model, env,
and assistantConfig into loop nodes and executeLoopNode never receives
workflow-level options, so workflow-level fields (effort, thinking,
fallbackModel, betas, sandbox) are ignored for loop: nodes; update
buildLoopNodeOptions/executeLoopNode to either call/return
resolveNodeProviderAndModel(...) for the node (so loop nodes inherit the same
resolved provider/model/options) or explicitly include the workflow-level fields
(effort, thinking, fallbackModel, betas, sandbox) and the nodeConfig into the
returned SendQueryOptions so executeLoopNode receives the same
workflowLevelOptions used by prompt/command nodes (refer to functions
buildLoopNodeOptions, executeLoopNode, and resolveNodeProviderAndModel to locate
and apply the fix).
---
Nitpick comments:
In `@eslint.config.mjs`:
- Line 23: The ignore pattern '**/src/test/**' in eslint.config.mjs is too
broad—replace it with a narrower pattern that only skips helper files (e.g.
'**/src/test/{fixtures,mocks}/**' or similar) so real test code remains linted;
update the array entry that currently contains '**/src/test/**' to the new
scoped pattern. Additionally enable strict TypeScript checks by ensuring your
TypeScript/ESLint config enforces no implicit any and full type annotations
(activate "strict": true in tsconfig.json and enable ESLint rules like
"@typescript-eslint/no-explicit-any" and "@typescript-eslint/typedef" where
applicable) so functions require complete type annotations.
In `@packages/core/src/services/title-generator.test.ts`:
- Around line 29-31: Update the stale type annotation on the function signature
that returns AsyncGenerator<MessageChunk>: remove the obsolete "tools?:
string[]" from the options type so it reads options?: { model?: string } (or
equivalent) to match current implementation that uses nodeConfig: {
allowed_tools: [] }; locate the options param in the test (the function
returning AsyncGenerator<MessageChunk>) and adjust any related test/fixture
types or comments to reflect the removal of tools.
In `@packages/providers/src/types.ts`:
- Around line 1-177: Add a shared UnknownProviderError export to the providers
contract so callers can type/identify missing provider errors without pulling
runtime packages: define and export an interface or class named
UnknownProviderError (e.g., export class UnknownProviderError extends Error {
providerId?: string } or export interface UnknownProviderError { name:
'UnknownProviderError'; providerId?: string; message: string }) in the same file
alongside MessageChunk and IAgentProvider, and ensure it is part of the public
types surface so consumers can import UnknownProviderError from the contract
subpath to detect bad provider IDs.
In `@packages/providers/tsconfig.json`:
- Around line 6-7: The package tsconfig's exclude array currently only excludes
"**/*.test.ts" which still allows test helper files like
src/test/mocks/logger.ts into compilation; update the tsconfig.json exclude
array (the "exclude" property) to also exclude the test folder pattern (e.g.,
add "src/test/**") so all test-only mocks and types are omitted from the package
build, leaving the existing "include" and other exclude entries intact.
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 96-125: The helpers mockClaudeCapabilities and
mockCodexCapabilities lack an explicit return type and should be annotated to
match the shared ProviderCapabilities contract; update both function signatures
to return ProviderCapabilities (e.g., const mockClaudeCapabilities = ():
ProviderCapabilities => { ... } and likewise for mockCodexCapabilities) so
TypeScript will enforce the capability flags align with the provider contract
and surface errors if flags are added/renamed.
🪄 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: 4334841a-2d3b-4a41-aa30-ca676101d7f0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
CLAUDE.mdeslint.config.mjspackages/core/package.jsonpackages/core/src/config/config-types.tspackages/core/src/index.tspackages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/orchestrator/orchestrator-isolation.test.tspackages/core/src/orchestrator/orchestrator.test.tspackages/core/src/providers/factory.test.tspackages/core/src/providers/index.tspackages/core/src/services/title-generator.test.tspackages/core/src/services/title-generator.tspackages/core/src/types/index.tspackages/core/src/workflows/store-adapter.test.tspackages/core/src/workflows/store-adapter.tspackages/providers/package.jsonpackages/providers/src/claude/config.tspackages/providers/src/claude/index.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/binary-guard.test.tspackages/providers/src/codex/binary-resolver-dev.test.tspackages/providers/src/codex/binary-resolver.test.tspackages/providers/src/codex/binary-resolver.tspackages/providers/src/codex/config.tspackages/providers/src/codex/index.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/errors.tspackages/providers/src/factory.test.tspackages/providers/src/factory.tspackages/providers/src/index.tspackages/providers/src/test/mocks/logger.tspackages/providers/src/types.tspackages/providers/tsconfig.jsonpackages/server/package.jsonpackages/server/src/adapters/web.tspackages/workflows/package.jsonpackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/hooks.test.ts
💤 Files with no reviewable changes (4)
- packages/providers/src/codex/binary-resolver.ts
- packages/core/src/providers/index.ts
- packages/core/src/index.ts
- packages/core/src/providers/factory.test.ts
Comment on lines
+96
to
101
| mock.module('@archon/providers', () => ({ | ||
| getAgentProvider: mock(() => ({ | ||
| sendQuery: mock(async function* () {}), | ||
| getType: mock(() => 'claude'), | ||
| getCapabilities: mock(() => ({})), | ||
| })), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "orchestrator-agent.test.ts" -type f | head -5Repository: coleam00/Archon
Length of output: 114
🏁 Script executed:
cd /repo && wc -l packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 114
🏁 Script executed:
cd /repo && sed -n '85,115p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 114
🏁 Script executed:
cd /repo && find . -path "*/providers/types*" -name "*.ts" -type f | head -10Repository: coleam00/Archon
Length of output: 114
🏁 Script executed:
cd /repo && rg "ProviderCapabilities" --type ts -A 5 | head -80Repository: coleam00/Archon
Length of output: 114
🏁 Script executed:
git ls-files "packages/core/src/orchestrator/orchestrator-agent.test.ts"Repository: coleam00/Archon
Length of output: 116
🏁 Script executed:
wc -l packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 121
🏁 Script executed:
sed -n '85,115p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 955
🏁 Script executed:
find . -path "*/providers/*" -name "*.ts" | grep -i "type\|interface" | head -20Repository: coleam00/Archon
Length of output: 92
🏁 Script executed:
rg "ProviderCapabilities" --type ts -B 2 -A 10 | head -100Repository: coleam00/Archon
Length of output: 5924
🏁 Script executed:
grep -n "getCapabilities" packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 101
🏁 Script executed:
grep -n "getCapabilities" packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
cat -n packages/providers/src/types.ts | grep -A 15 "interface ProviderCapabilities"Repository: coleam00/Archon
Length of output: 503
🏁 Script executed:
sed -n '1,50p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 2006
🏁 Script executed:
grep -n "getCapabilities" packages/core/src/orchestrator/orchestrator-agent.ts -B 3 -A 5Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
rg "getCapabilities()" packages/core/src --type ts -B 2 -A 3 | head -80Repository: coleam00/Archon
Length of output: 537
🏁 Script executed:
cat -n packages/providers/src/codex/provider.ts | sed -n '50,90p'Repository: coleam00/Archon
Length of output: 1476
🏁 Script executed:
cat -n packages/providers/src/claude/provider.ts | grep -A 15 "getCapabilities"Repository: coleam00/Archon
Length of output: 536
🏁 Script executed:
rg "\.getCapabilities" packages/core/src/orchestrator --type ts -B 3 -A 3Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
grep -n "sendQuery\|getType\|getCapabilities" packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 158
🏁 Script executed:
sed -n '96,110p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 519
🏁 Script executed:
grep -A 20 "getAgentProvider: mock" packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 749
🏁 Script executed:
rg "capabilities" packages/core/src/orchestrator/orchestrator-agent.test.ts -iRepository: coleam00/Archon
Length of output: 97
🏁 Script executed:
rg "getCapabilities" packages/workflows/src/dag-executor.ts -B 3 -A 5Repository: coleam00/Archon
Length of output: 393
🏁 Script executed:
grep -n "getAgentProvider\|parseOrchestratorCommands\|filterToolIndicators" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -20Repository: coleam00/Archon
Length of output: 1544
🏁 Script executed:
sed -n '150,250p' packages/core/src/orchestrator/orchestrator-agent.test.ts | head -60Repository: coleam00/Archon
Length of output: 1740
🏁 Script executed:
rg "describe\|test\(" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -20Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
sed -n '194,350p' packages/core/src/orchestrator/orchestrator-agent.test.ts | grep -A 30 "describe('parseOrchestratorCommands'"Repository: coleam00/Archon
Length of output: 1426
🏁 Script executed:
grep -n "handleMessage" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -15Repository: coleam00/Archon
Length of output: 1187
🏁 Script executed:
sed -n '950,1050p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 3849
🏁 Script executed:
grep -A 50 "describe('handleMessage" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -70Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
cat -n packages/core/src/orchestrator/orchestrator-agent.ts | sed -n '1,100p'Repository: coleam00/Archon
Length of output: 4681
🏁 Script executed:
grep -n "getCapabilities\|\.getCapabilities" packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
sed -n '830,870p' packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 1553
🏁 Script executed:
grep -C 5 "getCapabilities" packages/workflows/src/dag-executor.ts | head -60Repository: coleam00/Archon
Length of output: 404
🏁 Script executed:
sed -n '820,900p' packages/core/src/orchestrator/orchestrator-agent.ts | grep -i "capability\|getCapabilities"Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
rg "executeWorkflow|createWorkflowDeps" packages/core/src/orchestrator/orchestrator-agent.ts -A 5 -B 2Repository: coleam00/Archon
Length of output: 1463
🏁 Script executed:
sed -n '1000,1200p' packages/core/src/orchestrator/orchestrator-agent.test.ts | grep -A 30 "describe('handleMessage"Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
rg "executeWorkflow\(" packages/core/src/orchestrator/orchestrator-agent.test.ts -A 3 -B 1Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
grep -n "aiClient.getCapabilities\|getAgentProvider" packages/workflows/src/dag-executor.ts -B 3 -A 10 | head -60Repository: coleam00/Archon
Length of output: 2060
🏁 Script executed:
sed -n '1000,1410p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 17354
🏁 Script executed:
grep -n "describe('handleMessage" packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
sed -n '820,950p' packages/core/src/orchestrator/orchestrator-agent.test.tsRepository: coleam00/Archon
Length of output: 5739
🏁 Script executed:
rg "getCapabilities" packages/core/src/orchestrator/orchestrator-agent.test.ts -C 3Repository: coleam00/Archon
Length of output: 223
🏁 Script executed:
grep -n "import.*ProviderCapabilities\|from.*providers" packages/core/src/orchestrator/orchestrator-agent.test.ts | head -15Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
sed -n '1,30p' packages/core/src/orchestrator/orchestrator-agent.test.ts | grep "import"Repository: coleam00/Archon
Length of output: 579
🏁 Script executed:
grep "getCapabilities" packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
rg "sendQuery\|getType\|getCapabilities" packages/core/src/orchestrator/orchestrator-agent.ts -nRepository: coleam00/Archon
Length of output: 41
🏁 Script executed:
sed -n '27,35p' packages/core/src/orchestrator/orchestrator-agent.tsRepository: coleam00/Archon
Length of output: 577
🏁 Script executed:
rg "from.*@archon/providers" packages/core/src --type ts | head -10Repository: coleam00/Archon
Length of output: 711
🏁 Script executed:
cat -n packages/providers/src/index.ts | grep "export.*ProviderCapabilities"Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
grep "ProviderCapabilities" packages/providers/src/index.tsRepository: coleam00/Archon
Length of output: 82
Return a full capabilities object from getCapabilities() (Line 100) to align with the ProviderCapabilities contract.
Using {} diverges from the provider interface contract and weakens test determinism. While this mock is not currently exercised by the orchestrator-agent tests, the provider's getCapabilities() is called downstream in dag-executor, and a complete typed mock improves test fidelity and prevents future regressions.
Suggested test-safe contract-aligned mock
+import type { ProviderCapabilities } from '@archon/providers/types';
+
+const mockCapabilities: ProviderCapabilities = {
+ sessionResume: false,
+ mcp: false,
+ hooks: false,
+ skills: false,
+ toolRestrictions: false,
+ structuredOutput: false,
+ envInjection: false,
+ costControl: false,
+ effortControl: false,
+ thinkingControl: false,
+ fallbackModel: false,
+ sandbox: false,
+};
+
mock.module('@archon/providers', () => ({
getAgentProvider: mock(() => ({
sendQuery: mock(async function* () {}),
getType: mock(() => 'claude'),
- getCapabilities: mock(() => ({})),
+ getCapabilities: mock(() => mockCapabilities),
})),
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mock.module('@archon/providers', () => ({ | |
| getAgentProvider: mock(() => ({ | |
| sendQuery: mock(async function* () {}), | |
| getType: mock(() => 'claude'), | |
| getCapabilities: mock(() => ({})), | |
| })), | |
| import type { ProviderCapabilities } from '@archon/providers/types'; | |
| const mockCapabilities: ProviderCapabilities = { | |
| sessionResume: false, | |
| mcp: false, | |
| hooks: false, | |
| skills: false, | |
| toolRestrictions: false, | |
| structuredOutput: false, | |
| envInjection: false, | |
| costControl: false, | |
| effortControl: false, | |
| thinkingControl: false, | |
| fallbackModel: false, | |
| sandbox: false, | |
| }; | |
| mock.module('@archon/providers', () => ({ | |
| getAgentProvider: mock(() => ({ | |
| sendQuery: mock(async function* () {}), | |
| getType: mock(() => 'claude'), | |
| getCapabilities: mock(() => mockCapabilities), | |
| })), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/orchestrator/orchestrator-agent.test.ts` around lines 96 -
101, The mock for getAgentProvider currently returns an empty object from
getCapabilities(), which breaks the ProviderCapabilities contract; update the
mock in the test so getCapabilities() returns a full, test-safe
ProviderCapabilities-shaped object (include required keys such as model,
supportsStreaming, maxTokens, inputFormats, outputFormats, and any other
contract fields) so downstream consumers like dag-executor see a realistic,
typed capabilities object; locate the mock definition for
getAgentProvider/getCapabilities in orchestrator-agent.test.ts and replace the
{} return with the complete capabilities stub.
Comment on lines
+47
to
49
| mock.module('@archon/providers', () => ({ | ||
| getAgentProvider: mock(() => ({})), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate all `@archon/providers` module mocks:"
rg -n --type ts "mock\.module\('@archon/providers'" -A8 -B2
echo
echo "Show getAgentProvider mock return shapes around those mocks:"
rg -n --type ts "getAgentProvider:\s*mock\(" -A6 -B2Repository: coleam00/Archon
Length of output: 9106
Consolidate @archon/providers mock implementations across test files.
Multiple test files mock @archon/providers with conflicting return shapes: store-adapter.test.ts returns {}, orchestrator-isolation.test.ts returns null, and orchestrator-agent.test.ts returns a full object with sendQuery, getType, and getCapabilities methods. With Bun's process-global and irreversible mock.module(), this pattern creates order-dependent test failures when tests run in the same process.
Per coding guidelines: "never mock.module() a module path that another test file also mocks with different implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/workflows/store-adapter.test.ts` around lines 47 - 49, The
test is using mock.module('@archon/providers', ...) with getAgentProvider
returning an empty object which conflicts with other tests that mock the same
module with different shapes; update the mock to use a consistent, shared shape
across tests (e.g., provide a getAgentProvider implementation that matches the
full object shape used elsewhere including sendQuery/getType/getCapabilities) or
remove mock.module usage and import a single shared test double for
`@archon/providers`; specifically change the mock.module call that defines
getAgentProvider in store-adapter.test.ts so it returns the same
interface/implementation used by orchestrator-agent.test.ts (or centralize the
mock into a shared test helper) to eliminate order-dependent, process-global
conflicts.
Comment on lines
+21
to
+27
| if (Array.isArray(raw.settingSources)) { | ||
| const valid = raw.settingSources.filter( | ||
| (s): s is 'project' | 'user' => s === 'project' || s === 'user' | ||
| ); | ||
| if (valid.length > 0) { | ||
| result.settingSources = valid; | ||
| } |
There was a problem hiding this comment.
Don't silently broaden settingSources on invalid input.
If this array contains a typo—or an explicit empty array—result.settingSources stays undefined, so downstream falls back to Claude's default ['project']. That re-enables project CLAUDE.md loading instead of honoring the user's restriction. Please validate and throw, or preserve the explicit empty array.
🛡️ Suggested fix
if (Array.isArray(raw.settingSources)) {
const valid = raw.settingSources.filter(
(s): s is 'project' | 'user' => s === 'project' || s === 'user'
);
- if (valid.length > 0) {
- result.settingSources = valid;
- }
+ if (valid.length !== raw.settingSources.length) {
+ throw new Error('claude.settingSources must contain only "project" or "user"');
+ }
+ result.settingSources = valid;
}As per coding guidelines, "**/*.ts: Fail fast with clear errors for unsupported or unsafe states — never silently swallow errors or silently broaden permissions; document fallback behavior with a comment when intentional and safe".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Array.isArray(raw.settingSources)) { | |
| const valid = raw.settingSources.filter( | |
| (s): s is 'project' | 'user' => s === 'project' || s === 'user' | |
| ); | |
| if (valid.length > 0) { | |
| result.settingSources = valid; | |
| } | |
| if (Array.isArray(raw.settingSources)) { | |
| const valid = raw.settingSources.filter( | |
| (s): s is 'project' | 'user' => s === 'project' || s === 'user' | |
| ); | |
| if (valid.length !== raw.settingSources.length) { | |
| throw new Error('claude.settingSources must contain only "project" or "user"'); | |
| } | |
| result.settingSources = valid; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/claude/config.ts` around lines 21 - 27, The code
currently filters raw.settingSources into valid values and only assigns
result.settingSources when valid.length > 0, which silently broadens to Claude's
default when the input was an explicit empty array or contained typos; update
the validation in claude/config.ts to detect invalid inputs and either (a)
preserve an explicit empty array by assigning result.settingSources = [] when
raw.settingSources is an array with no valid entries, or (b) throw a clear
validation error if raw.settingSources contains unknown entries; locate the
logic around raw.settingSources and result.settingSources and implement one of
these behaviors (preserve empty array or throw) and include a brief error
message mentioning invalid settingSources values when throwing.
Comment on lines
+395
to
+418
| if (nodeConfig.skills) { | ||
| const skills = nodeConfig.skills; | ||
| const agentId = 'dag-node-skills'; | ||
| const agentTools = options.tools ? [...(options.tools as string[]), 'Skill'] : ['Skill']; | ||
| const agentDef: { | ||
| description: string; | ||
| prompt: string; | ||
| skills: string[]; | ||
| tools: string[]; | ||
| model?: string; | ||
| } = { | ||
| description: 'DAG node with skills', | ||
| prompt: `You have preloaded skills: ${skills.join(', ')}. Use them when relevant.`, | ||
| skills, | ||
| tools: agentTools, | ||
| }; | ||
| if (options.model) agentDef.model = options.model; | ||
| options.agents = { [agentId]: agentDef }; | ||
| options.agent = agentId; | ||
| if (!options.allowedTools?.includes('Skill')) { | ||
| options.allowedTools = [...(options.allowedTools ?? []), 'Skill']; | ||
| } | ||
| getLog().info({ skills, agentId }, 'claude.skills_agent_created'); | ||
| } |
There was a problem hiding this comment.
Treat skills: [] as unset.
if (nodeConfig.skills) is true for an empty array, so this still creates the synthetic Skill agent/tool even when no skills were configured. The executor side already treats skills.length > 0 as the enabled case, so this should gate on nodeConfig.skills.length > 0 too.
💡 Minimal fix
- if (nodeConfig.skills) {
+ if (nodeConfig.skills && nodeConfig.skills.length > 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (nodeConfig.skills) { | |
| const skills = nodeConfig.skills; | |
| const agentId = 'dag-node-skills'; | |
| const agentTools = options.tools ? [...(options.tools as string[]), 'Skill'] : ['Skill']; | |
| const agentDef: { | |
| description: string; | |
| prompt: string; | |
| skills: string[]; | |
| tools: string[]; | |
| model?: string; | |
| } = { | |
| description: 'DAG node with skills', | |
| prompt: `You have preloaded skills: ${skills.join(', ')}. Use them when relevant.`, | |
| skills, | |
| tools: agentTools, | |
| }; | |
| if (options.model) agentDef.model = options.model; | |
| options.agents = { [agentId]: agentDef }; | |
| options.agent = agentId; | |
| if (!options.allowedTools?.includes('Skill')) { | |
| options.allowedTools = [...(options.allowedTools ?? []), 'Skill']; | |
| } | |
| getLog().info({ skills, agentId }, 'claude.skills_agent_created'); | |
| } | |
| if (nodeConfig.skills && nodeConfig.skills.length > 0) { | |
| const skills = nodeConfig.skills; | |
| const agentId = 'dag-node-skills'; | |
| const agentTools = options.tools ? [...(options.tools as string[]), 'Skill'] : ['Skill']; | |
| const agentDef: { | |
| description: string; | |
| prompt: string; | |
| skills: string[]; | |
| tools: string[]; | |
| model?: string; | |
| } = { | |
| description: 'DAG node with skills', | |
| prompt: `You have preloaded skills: ${skills.join(', ')}. Use them when relevant.`, | |
| skills, | |
| tools: agentTools, | |
| }; | |
| if (options.model) agentDef.model = options.model; | |
| options.agents = { [agentId]: agentDef }; | |
| options.agent = agentId; | |
| if (!options.allowedTools?.includes('Skill')) { | |
| options.allowedTools = [...(options.allowedTools ?? []), 'Skill']; | |
| } | |
| getLog().info({ skills, agentId }, 'claude.skills_agent_created'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/claude/provider.ts` around lines 395 - 418, The
current check uses if (nodeConfig.skills) which is truthy for an empty array and
thus creates a synthetic 'Skill' agent even when no skills exist; change the
guard to check for a non-empty array (e.g., nodeConfig.skills &&
nodeConfig.skills.length > 0) before building agentId 'dag-node-skills',
agentDef, assigning options.agents/options.agent, extending options.allowedTools
with 'Skill', and logging via getLog().info so the synthetic agent/tool is only
created when skills.length > 0.
Comment on lines
+178
to
+183
| const assistantConfig = requestOptions?.assistantConfig ?? {}; | ||
| const codexConfig = parseCodexConfig(assistantConfig); | ||
|
|
||
| // Initialize Codex SDK with binary path override (resolved from env/config/vendor). | ||
| // In dev mode, resolveCodexBinaryPath returns undefined and the SDK uses node_modules. | ||
| // In binary mode, it resolves from env/config/vendor or throws with install instructions. | ||
| const codex = await getCodex(mergedConfig?.assistants.codex.codexBinaryPath); | ||
| const threadOptions = buildThreadOptions(cwd, options); | ||
| // Initialize Codex SDK with binary path override | ||
| const codex = await getCodex(codexConfig.codexBinaryPath); | ||
| const threadOptions = buildThreadOptions(cwd, requestOptions?.model, assistantConfig); |
There was a problem hiding this comment.
assistantConfig.codexBinaryPath only works for the first request.
sendQuery() now passes a per-request binary override into getCodex(), but getCodex() still memoizes a single process-wide Codex instance. In a long-lived server, the first caller wins and later projects/workflows silently reuse the wrong Codex binary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/provider.ts` around lines 178 - 183, The code
memoizes a single process-wide Codex instance so passing
assistantConfig.codexBinaryPath per request in sendQuery() only affects the
first request; update getCodex() to either stop using a single global instance
or to cache instances keyed by the binary path (assistantConfig.codexBinaryPath)
so each unique binary path returns its own Codex process. Locate getCodex and
change its memoization strategy to use the codexBinaryPath as the cache key (or
always spawn a new Codex when a different path is provided), and ensure
sendQuery()/provider.ts continues to pass
requestOptions?.assistantConfig.codexBinaryPath through to getCodex().
Comment on lines
269
to
273
| for await (const event of result.events) { | ||
| // Check abort signal between events | ||
| if (options?.abortSignal?.aborted) { | ||
| if (requestOptions?.abortSignal?.aborted) { | ||
| getLog().info('query_aborted_between_events'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Aborting mid-stream must throw, not end cleanly.
This branch currently exits the event loop and then returns from sendQuery() without a result or an error. Callers see a normal EOF and can treat a partially streamed turn as successful instead of cancelled.
Suggested fix
- if (requestOptions?.abortSignal?.aborted) {
- getLog().info('query_aborted_between_events');
- break;
- }
+ if (requestOptions?.abortSignal?.aborted) {
+ getLog().info('query_aborted_between_events');
+ throw new Error('Query aborted');
+ }As per coding guidelines "**/*.ts: Fail fast with clear errors for unsupported or unsafe states — never silently swallow errors or silently broaden permissions; document fallback behavior with a comment when intentional and safe`."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for await (const event of result.events) { | |
| // Check abort signal between events | |
| if (options?.abortSignal?.aborted) { | |
| if (requestOptions?.abortSignal?.aborted) { | |
| getLog().info('query_aborted_between_events'); | |
| break; | |
| } | |
| for await (const event of result.events) { | |
| if (requestOptions?.abortSignal?.aborted) { | |
| getLog().info('query_aborted_between_events'); | |
| throw new Error('Query aborted'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/provider.ts` around lines 269 - 273, The loop
that listens for streaming events in sendQuery currently breaks silently when
requestOptions?.abortSignal?.aborted is true; instead throw a clear abort error
so callers can't interpret a partial stream as success. Update the for-await
loop in sendQuery (the block iterating result.events) to detect
requestOptions?.abortSignal?.aborted and throw a specific error (e.g., an
AbortError or Error with "query aborted" message) rather than calling
getLog().info and break; ensure the thrown error is documented in sendQuery's
behavior so callers can handle cancellation explicitly.
Comment on lines
+476
to
+479
| getLog().warn( | ||
| { outputPreview: accumulatedText.slice(0, 200) }, | ||
| 'codex.structured_output_not_json' | ||
| ); |
There was a problem hiding this comment.
Do not log assistant output previews here.
outputPreview captures raw model text, which can contain user content, secrets, or PII. This warning should log metadata only.
Suggested fix
- getLog().warn(
- { outputPreview: accumulatedText.slice(0, 200) },
- 'codex.structured_output_not_json'
- );
+ getLog().warn(
+ { outputLength: accumulatedText.length },
+ 'codex.structured_output_not_json'
+ );As per coding guidelines "**/*.ts: Use Pino logger factory from @archon/paths with event naming format {domain}.{action}_{state} (states: _started, _completed, _failed, _validated, _rejected). Never log API keys, tokens, user message content, or PII`."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getLog().warn( | |
| { outputPreview: accumulatedText.slice(0, 200) }, | |
| 'codex.structured_output_not_json' | |
| ); | |
| getLog().warn( | |
| { outputLength: accumulatedText.length }, | |
| 'codex.structured_output_not_json' | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/provider.ts` around lines 476 - 479, The current
getLog().warn call logs raw assistant output via the outputPreview field
(accumulatedText) which may contain PII; update the log at the
codex.structured_output_not_json site to remove outputPreview and instead log
only non-sensitive metadata (for example the output length via
accumulatedText.length and/or a deterministic non-reversible fingerprint like a
short hash), using the existing getLog().warn invocation and leaving the event
name 'codex.structured_output_not_json' unchanged.
Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers
The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings).
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/providers/src/codex/provider.ts (3)
473-481:⚠️ Potential issue | 🟠 MajorRemove
outputPreviewfrom this warning log.
accumulatedTextis raw model output. Logging even a 200-char preview can leak user content, secrets, or PII.Suggested fix
} catch { getLog().warn( - { outputPreview: accumulatedText.slice(0, 200) }, + { outputLength: accumulatedText.length }, 'codex.structured_output_not_json' );As per coding guidelines "
**/*.{ts,tsx}: Never log API keys or tokens - mask astoken.slice(0, 8) + '...'; never log user message content or PII`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/codex/provider.ts` around lines 473 - 481, The warning log in the JSON parse failure handling (inside the hasOutputFormat branch that uses accumulatedText and sets structuredOutput) currently includes a preview of accumulatedText which may leak user data; remove the outputPreview property from the getLog().warn call and replace it with a generic, non-sensitive message (e.g., 'codex.structured_output_not_json' with no preview or any masked content) so that accumulatedText is never logged; update the catch block around JSON.parse(accumulatedText) that calls getLog().warn to only emit non-sensitive context.
271-275:⚠️ Potential issue | 🟠 MajorAborting mid-stream should throw, not return cleanly.
This
breakfalls through to Line 500, so callers can observe a partial stream as a normal completion instead of an explicit cancellation.Suggested fix
for await (const event of result.events) { if (requestOptions?.abortSignal?.aborted) { getLog().info('query_aborted_between_events'); - break; + throw new Error('Query aborted'); }As per coding guidelines "
**/*.{ts,tsx}: Prefer throwing early with a clear error for unsupported or unsafe states - never silently swallow errors; never silently broaden permissions or capabilities`."Also applies to: 500-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/codex/provider.ts` around lines 271 - 275, The loop over result.events currently uses "break" when requestOptions?.abortSignal?.aborted is true, which silently completes the stream; instead throw an explicit abort error so callers observe cancellation. Replace the break in the for-await-of handling of result.events with throwing a clear AbortError (e.g., propagate requestOptions.abortSignal.reason if present or throw a new DOMException('Aborted', 'AbortError') / new Error('Query aborted')), keeping the existing getLog().info('query_aborted_between_events') call before throwing to preserve logging; ensure the thrown error propagates out of the async iterator so callers won't treat the stream as a normal completion.
183-185:⚠️ Potential issue | 🟠 MajorKey the Codex cache by
codexBinaryPath.
sendQuery()now passes a per-request binary override, butgetCodex()still memoizes a single process-wide instance. After the first request, later workflows silently reuse the wrong Codex binary.Fix sketch
- let codexInstance: Codex | null = null; - let codexInitPromise: Promise<Codex> | null = null; + const codexInstances = new Map<string, Codex>(); + const codexInitPromises = new Map<string, Promise<Codex>>(); async function getCodex(configCodexBinaryPath?: string): Promise<Codex> { - if (codexInstance) return codexInstance; + const resolvedPath = await resolveCodexBinaryPath(configCodexBinaryPath); + const cacheKey = resolvedPath ?? '__default__'; + + const existing = codexInstances.get(cacheKey); + if (existing) return existing; + + const pending = codexInitPromises.get(cacheKey); + if (pending) return pending; - if (!codexInitPromise) { - codexInitPromise = (async (): Promise<Codex> => { - const codexPathOverride = await resolveCodexBinaryPath(configCodexBinaryPath); - const instance = new Codex({ codexPathOverride }); - codexInstance = instance; + const init = (async (): Promise<Codex> => { + const instance = new Codex({ codexPathOverride: resolvedPath }); + codexInstances.set(cacheKey, instance); return instance; })().catch(err => { - codexInitPromise = null; + codexInitPromises.delete(cacheKey); throw err; }); - } - return codexInitPromise; + codexInitPromises.set(cacheKey, init); + return init; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/codex/provider.ts` around lines 183 - 185, The Codex instance is being memoized globally causing reuse of the wrong binary when requests pass a per-request codexBinaryPath; update the caching/memoization in getCodex so it keys instances by the provided codexBinaryPath (i.e., maintain a map from codexBinaryPath -> Codex instance) and ensure sendQuery / the call site still passes the per-request codexBinaryPath into getCodex; reference getCodex and sendQuery (and codexConfig.codexBinaryPath) to locate and change the memoization strategy so each unique binary path gets its own cached Codex process.
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.test.ts (1)
4582-4642: Add a loop-node assertion for the workflow-leveleffortfix.These cases only cover normal DAG nodes. The regression fixed in this PR was in
buildLoopNodeOptions, so the loop path is still unprotected here. Please add at least one loop-node case that proves workflow-leveleffortreachessendQuery()there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/dag-executor.test.ts` around lines 4582 - 4642, Add a test that exercises the loop-node path so the regression in buildLoopNodeOptions is covered: create a workflow with a loop node (e.g., node with type or properties that trigger buildLoopNodeOptions) and call executeDagWorkflow, then inspect mockSendQueryDag.mock.calls[0][3].nodeConfig.effort to assert it equals the workflow-level effort when the loop node has no per-node effort; also add a second loop-node test where the loop node includes an effort override and assert nodeConfig.effort matches that override. Use the existing test pattern (executeDagWorkflow, makeWorkflowRun, mockSendQueryDag) and reference buildLoopNodeOptions/sendQuery via the mockSendQueryDag call to locate the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/providers/src/codex/provider.ts`:
- Around line 473-481: The warning log in the JSON parse failure handling
(inside the hasOutputFormat branch that uses accumulatedText and sets
structuredOutput) currently includes a preview of accumulatedText which may leak
user data; remove the outputPreview property from the getLog().warn call and
replace it with a generic, non-sensitive message (e.g.,
'codex.structured_output_not_json' with no preview or any masked content) so
that accumulatedText is never logged; update the catch block around
JSON.parse(accumulatedText) that calls getLog().warn to only emit non-sensitive
context.
- Around line 271-275: The loop over result.events currently uses "break" when
requestOptions?.abortSignal?.aborted is true, which silently completes the
stream; instead throw an explicit abort error so callers observe cancellation.
Replace the break in the for-await-of handling of result.events with throwing a
clear AbortError (e.g., propagate requestOptions.abortSignal.reason if present
or throw a new DOMException('Aborted', 'AbortError') / new Error('Query
aborted')), keeping the existing getLog().info('query_aborted_between_events')
call before throwing to preserve logging; ensure the thrown error propagates out
of the async iterator so callers won't treat the stream as a normal completion.
- Around line 183-185: The Codex instance is being memoized globally causing
reuse of the wrong binary when requests pass a per-request codexBinaryPath;
update the caching/memoization in getCodex so it keys instances by the provided
codexBinaryPath (i.e., maintain a map from codexBinaryPath -> Codex instance)
and ensure sendQuery / the call site still passes the per-request
codexBinaryPath into getCodex; reference getCodex and sendQuery (and
codexConfig.codexBinaryPath) to locate and change the memoization strategy so
each unique binary path gets its own cached Codex process.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.test.ts`:
- Around line 4582-4642: Add a test that exercises the loop-node path so the
regression in buildLoopNodeOptions is covered: create a workflow with a loop
node (e.g., node with type or properties that trigger buildLoopNodeOptions) and
call executeDagWorkflow, then inspect
mockSendQueryDag.mock.calls[0][3].nodeConfig.effort to assert it equals the
workflow-level effort when the loop node has no per-node effort; also add a
second loop-node test where the loop node includes an effort override and assert
nodeConfig.effort matches that override. Use the existing test pattern
(executeDagWorkflow, makeWorkflowRun, mockSendQueryDag) and reference
buildLoopNodeOptions/sendQuery via the mockSendQueryDag call to locate the
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aef43b9b-92b6-4949-bbc8-af96bc06c4c1
📒 Files selected for processing (14)
.archon/workflows/e2e-claude-smoke.yaml.archon/workflows/e2e-codex-smoke.yamlCLAUDE.mdpackages/cli/package.jsonpackages/docs-web/src/content/docs/reference/architecture.mdpackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/index.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/deps.tspackages/workflows/src/hooks.test.ts
✅ Files skipped from review due to trivial changes (7)
- packages/cli/package.json
- packages/workflows/src/hooks.test.ts
- .archon/workflows/e2e-claude-smoke.yaml
- packages/docs-web/src/content/docs/reference/architecture.md
- .archon/workflows/e2e-codex-smoke.yaml
- CLAUDE.md
- packages/providers/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/providers/src/claude/provider.test.ts
- packages/providers/src/claude/provider.ts
- packages/workflows/src/dag-executor.ts
- Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/workflows/src/dag-executor.ts (1)
2349-2367: Consider reusingresolveNodeProviderAndModel()for loop nodes too.Loop dispatch still duplicates provider/model compatibility logic. Reusing the shared resolver would reduce drift risk and keep capability handling consistent across AI node types.
Also applies to: 2474-2485
🤖 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 2349 - 2367, The loop node code duplicates provider/model resolution and compatibility checks; replace the manual logic around loopProvider and loopModel and the subsequent isModelCompatible check by calling the shared resolver function resolveNodeProviderAndModel(node, workflowProvider, workflowModel, config) (or the existing signature used elsewhere) to obtain the resolved provider and model for loop nodes, then use its result for compatibility/capability decisions; update any variable names (loopProvider, loopModel) to use the returned values and remove the duplicated branches so loop nodes use the same resolution logic as other AI node types.
🤖 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 783-786: The warning currently logs raw provider system payload
via getLog().warn({ nodeId: node.id, systemContent: msg.content },
'dag.provider_warning_forwarded'); which may include user-derived text; change
this to avoid raw content by replacing msg.content with a normalized summary
(e.g., kind/type string) and a safe metric such as content length or a redacted
flag; update the getLog().warn call to include node.id and either { systemKind:
'<kind>', contentLength: msg.content?.length } or { systemRedacted: true,
contentLength: msg.content?.length } (use the same key names where systemContent
was used) so no user text is written to logs while preserving useful debugging
info.
---
Nitpick comments:
In `@packages/workflows/src/dag-executor.ts`:
- Around line 2349-2367: The loop node code duplicates provider/model resolution
and compatibility checks; replace the manual logic around loopProvider and
loopModel and the subsequent isModelCompatible check by calling the shared
resolver function resolveNodeProviderAndModel(node, workflowProvider,
workflowModel, config) (or the existing signature used elsewhere) to obtain the
resolved provider and model for loop nodes, then use its result for
compatibility/capability decisions; update any variable names (loopProvider,
loopModel) to use the returned values and remove the duplicated branches so loop
nodes use the same resolution logic as other AI node types.
🪄 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: 92a06e84-5e4e-4a9d-a8f3-6b77c70a9087
📒 Files selected for processing (1)
packages/workflows/src/dag-executor.ts
Comment on lines
783
to
786
| getLog().warn( | ||
| { nodeId: node.id, mcpStatus: msg.content }, | ||
| 'dag.mcp_server_connection_failed' | ||
| { nodeId: node.id, systemContent: msg.content }, | ||
| 'dag.provider_warning_forwarded' | ||
| ); |
There was a problem hiding this comment.
Avoid logging raw provider system payloads.
Line 784 logs msg.content verbatim. Provider system chunks can contain user-derived text; this increases sensitive-data retention risk in logs. Log a normalized warning kind (and maybe length) instead of full content.
Proposed fix
- getLog().warn(
- { nodeId: node.id, systemContent: msg.content },
- 'dag.provider_warning_forwarded'
- );
+ getLog().warn(
+ {
+ nodeId: node.id,
+ warningType: msg.content.startsWith('MCP server connection failed:')
+ ? 'mcp_connection_failed'
+ : 'provider_warning',
+ contentLength: msg.content.length,
+ },
+ 'dag.provider_warning_forwarded'
+ );As per coding guidelines, "Never log API keys, tokens ... user message content, or PII."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| getLog().warn( | |
| { nodeId: node.id, mcpStatus: msg.content }, | |
| 'dag.mcp_server_connection_failed' | |
| { nodeId: node.id, systemContent: msg.content }, | |
| 'dag.provider_warning_forwarded' | |
| ); | |
| getLog().warn( | |
| { | |
| nodeId: node.id, | |
| warningType: msg.content.startsWith('MCP server connection failed:') | |
| ? 'mcp_connection_failed' | |
| : 'provider_warning', | |
| contentLength: msg.content.length, | |
| }, | |
| 'dag.provider_warning_forwarded' | |
| ); |
🤖 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 783 - 786, The warning
currently logs raw provider system payload via getLog().warn({ nodeId: node.id,
systemContent: msg.content }, 'dag.provider_warning_forwarded'); which may
include user-derived text; change this to avoid raw content by replacing
msg.content with a normalized summary (e.g., kind/type string) and a safe metric
such as content length or a redacted flag; update the getLog().warn call to
include node.id and either { systemKind: '<kind>', contentLength:
msg.content?.length } or { systemRedacted: true, contentLength:
msg.content?.length } (use the same key names where systemContent was used) so
no user text is written to logs while preserving useful debugging info.
This was referenced Apr 13, 2026
12 tasks
This was referenced Apr 13, 2026
kagura-agent
pushed a commit
to kagura-agent/Archon
that referenced
this pull request
Apr 13, 2026
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
This was referenced Apr 14, 2026
tboome33
pushed a commit
to tboome33/Archon
that referenced
this pull request
Apr 14, 2026
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
This was referenced Apr 15, 2026
kagura-agent
pushed a commit
to kagura-agent/Archon
that referenced
this pull request
Apr 17, 2026
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
kagura-agent
pushed a commit
to kagura-agent/Archon
that referenced
this pull request
Apr 18, 2026
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
This was referenced Apr 19, 2026
This was referenced Apr 20, 2026
Closed
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…oleam00#1137) * refactor: extract providers from @archon/core into @archon/providers Move Claude and Codex provider implementations, factory, and SDK dependencies into a new @archon/providers package. This establishes a clean boundary: providers own SDK translation, core owns business logic. Key changes: - New @archon/providers package with zero-dep contract layer (types.ts) - @archon/workflows imports from @archon/providers/types — no mirror types - dag-executor delegates option building to providers via nodeConfig - IAgentProvider gains getCapabilities() for provider-agnostic warnings - @archon/core no longer depends on SDK packages directly - UnknownProviderError standardizes error shape across all surfaces Zero user-facing changes — same providers, same config, same behavior. * refactor: remove config type duplication and backward-compat re-exports Address review findings: - Move ClaudeProviderDefaults and CodexProviderDefaults to the @archon/providers/types contract layer as the single source of truth. @archon/core/config/config-types.ts now imports from there. - Remove provider re-exports from @archon/core (index.ts and types/). Consumers should import from @archon/providers directly. - Update @archon/server to depend on @archon/providers for MessageChunk. * refactor: move structured output validation into providers Each provider now normalizes its own structured output semantics: - Claude already yields structuredOutput from the SDK's native field - Codex now parses inline agent_message text as JSON when outputFormat is set, populating structuredOutput on the result chunk This eliminates the last provider === 'codex' branch from dag-executor, making it fully provider-agnostic. The dag-executor checks structuredOutput uniformly regardless of provider. Also removes the ClaudeCodexProviderDefaults deprecated alias — all consumers now use ClaudeProviderDefaults directly. * fix: address PR review — restore warnings, fix loop options, cleanup Critical fixes: - Restore MCP missing env vars user-facing warning (was silently dropped) - Restore Haiku + MCP tool search warning - Fix buildLoopNodeOptions to pass workflow-level nodeConfig (effort, thinking, betas, sandbox were silently lost for loop nodes) - Add TODO(coleam00#1135) comments documenting env-leak gate gap Cleanup: - Remove backward-compat type aliases from deps.ts (keep WorkflowTokenUsage) - Remove 26 unnecessary eslint-disable comments from test files - Trim internal helpers from providers barrel (withFirstMessageTimeout, getProcessUid, loadMcpConfig, buildSDKHooksFromYAML) - Add @archon/providers dep to CLI package.json - Fix 8 stale documentation paths pointing to deleted core/src/providers/ - Add E2E smoke test workflows for both Claude and Codex providers * fix: forward provider system warnings to users in dag-executor The dag-executor only forwarded system chunks starting with "MCP server connection failed:" — all other provider warnings (missing env vars, Haiku+MCP, structured output issues) were logged but never reached the user. Now forwards all system chunks starting with⚠️ (the prefix providers use for user-actionable warnings). * fix: add providers package to Dockerfile and fix CI module resolution - Add packages/providers/ to all three Dockerfile stages (deps, production package.json copy, production source copy) - Replace wildcard export map (./*) with explicit subpath entries to fix module resolution in CI (bun workspace linking) * chore: update bun.lock for providers package exports
This was referenced Apr 27, 2026
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
Extract all AI agent provider logic from
@archon/coreinto a new@archon/providerspackage. Providers own their SDK dependencies and translate raw config to SDK-specific options internally. The dag-executor is now fully provider-agnostic — zeroprovider === 'codex'orprovider === 'claude'branches.Zero user-facing changes — same providers, same config, same behavior.
Changes
@archon/providerspackage — ownsIAgentProviderinterface, Claude/Codex implementations, SDK deps, and a zero-dep contract layer (@archon/providers/types)@archon/coreno longer depends on SDK packages — imports from@archon/providersinstead; no provider re-exports (clean break)@archon/workflows/deps.ts— ~200 lines of mirror types replaced with direct imports from the contract layerdag-executor.ts— 9 provider-specific option-building branches replaced with capability warnings +nodeConfig/assistantConfigpass-throughstructured_output, Codex via inline JSON parsing)IAgentProvider.getCapabilities()— new method enabling provider-agnostic capability warningsUnknownProviderError— standardized error shape across all surfacesClaudeProviderDefaultsandCodexProviderDefaultscanonical definitions live in the contract layer;@archon/core/configimports from thereFiles Changed
44 files changed across 8 packages
File list
New package (
@archon/providers)packages/providers/— package.json, tsconfig, types, factory, errors, barrelpackages/providers/src/claude/— provider, config, testspackages/providers/src/codex/— provider, config, binary-resolver, testsUpdated packages
packages/core/— removedsrc/providers/, stripped SDK deps, updated consumerspackages/workflows/— deps.ts imports from contract layer, dag-executor refactoredpackages/server/— added@archon/providersdep forMessageChunkimportTesting
bun run validate)Not in scope
GET /api/providersendpoint (Phase 2)'claude' | 'codex'unions tostring(Phase 2)Summary by CodeRabbit
New Features
Bug Fixes
Documentation