refactor: extract provider metadata seam for Phase 2 registry readiness#1185
refactor: extract provider metadata seam for Phase 2 registry readiness#1185
Conversation
- Add static capability constants (capabilities.ts) for Claude and Codex - Export getProviderCapabilities() from @archon/providers for capability queries without provider instantiation - Add inferProviderFromModel() to model-validation.ts, replacing three copy-pasted inline inference blocks in executor.ts and dag-executor.ts - Replace throwaway provider instantiation in dag-executor with static capability lookup (getProviderCapabilities) - Add orchestrator warning when env vars are configured but provider doesn't support envInjection
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds static provider capability constants and a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator
participant ProviderFactory
participant Provider
participant Logger
Client->>Orchestrator: send request (with project env)
Orchestrator->>ProviderFactory: getProviderCapabilities(providerKey)
ProviderFactory-->>Orchestrator: ProviderCapabilities (envInjection: true/false)
alt envInjection supported
Orchestrator->>Provider: forward request (env included)
else envInjection unsupported
Orchestrator->>Logger: emit warning (unsupported_env_injection, providerKey, varCount)
Orchestrator->>Provider: forward request (env omitted)
end
Provider-->>Orchestrator: response
Orchestrator-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
🧹 Nitpick comments (1)
packages/providers/src/index.ts (1)
16-20: Keep the metadata seam out of the main barrel.Re-exporting
getProviderCapabilitiesand the static capability objects from the same barrel that also re-exportsClaudeProvider/CodexProvidermeans consumers likepackages/workflows/src/dag-executor.tsstill pull in provider implementation modules just to read metadata. A dedicated metadata subpath would preserve the zero-SDK boundary this refactor is trying to create.Based on learnings, "Place contract layer types (
IAgentProvider,SendQueryOptions,MessageChunk) in a dedicated contract subpath (zero SDK dependencies). Keep SDK-specific implementations separate. Avoid SDK dependency leakage to other packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/index.ts` around lines 16 - 20, Move the metadata exports out of the main provider barrel: stop re-exporting getProviderCapabilities, CLAUDE_CAPABILITIES and CODEX_CAPABILITIES alongside implementation exports like ClaudeProvider/CodexProvider; instead create a dedicated metadata barrel (e.g. a metadata subpath) that exports getProviderCapabilities and the static capability objects and update consumers to import metadata from that subpath; ensure contract-layer types (IAgentProvider, SendQueryOptions, MessageChunk) also live in the dedicated contract/metadata subpath so SDK-specific implementations remain isolated and the main provider barrel only exports implementation symbols such as getAgentProvider/ClaudeProvider/CodexProvider.
🤖 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/providers/src/factory.ts`:
- Around line 49-57: getProviderCapabilities currently returns shared capability
singletons (CLAUDE_CAPABILITIES, CODEX_CAPABILITIES) by reference; produce and
return an immutable snapshot instead to prevent consumer mutation from affecting
global state. Modify getProviderCapabilities to create a defensive copy of the
selected capability object (e.g., shallow/deep clone as appropriate for nested
properties) and freeze it (or deep-freeze) before returning; preserve the
UnknownProviderError(...) usage for the default case and continue to reference
REGISTERED_PROVIDERS when constructing the error. Ensure the cloning/freezing
logic is applied to both CLAUDE_CAPABILITIES and CODEX_CAPABILITIES returns so
callers receive an immutable snapshot.
In `@packages/workflows/src/model-validation.test.ts`:
- Around line 76-82: The test and helper treat the model name "inherit" as a
Claude alias; change inferProviderFromModel to treat "inherit" as the sentinel
meaning "use existing provider" (i.e., do not map it to 'claude' — return
undefined or null), and update the test in model-validation.test.ts so
expect(inferProviderFromModel('inherit','codex')) reflects that sentinel
behavior instead of 'claude'; also ensure the call sites that use node.provider
?? inferProviderFromModel(node.model, workflowProvider) (refer to
dag-executor.ts) will preserve the workflowProvider when inferProviderFromModel
returns the sentinel/undefined.
In `@packages/workflows/src/model-validation.ts`:
- Around line 24-26: inferProviderFromModel currently classifies the string
'inherit' as Claude because isClaudeModel('inherit') returns true; add an
explicit guard in inferProviderFromModel to return defaultProvider when model
=== 'inherit' (i.e., check model === 'inherit' before calling isClaudeModel) so
the configured defaultProvider isn't overridden, or alternatively update
isClaudeModel to exclude the literal 'inherit' from Claude detection; reference
the inferProviderFromModel, isClaudeModel and defaultProvider symbols when
making the change.
---
Nitpick comments:
In `@packages/providers/src/index.ts`:
- Around line 16-20: Move the metadata exports out of the main provider barrel:
stop re-exporting getProviderCapabilities, CLAUDE_CAPABILITIES and
CODEX_CAPABILITIES alongside implementation exports like
ClaudeProvider/CodexProvider; instead create a dedicated metadata barrel (e.g. a
metadata subpath) that exports getProviderCapabilities and the static capability
objects and update consumers to import metadata from that subpath; ensure
contract-layer types (IAgentProvider, SendQueryOptions, MessageChunk) also live
in the dedicated contract/metadata subpath so SDK-specific implementations
remain isolated and the main provider barrel only exports implementation symbols
such as getAgentProvider/ClaudeProvider/CodexProvider.
🪄 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: a2a309bf-dccd-4c8a-a753-6f790ea12603
📒 Files selected for processing (13)
packages/core/src/orchestrator/orchestrator-agent.test.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/providers/src/claude/capabilities.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/capabilities.tspackages/providers/src/codex/provider.tspackages/providers/src/factory.test.tspackages/providers/src/factory.tspackages/providers/src/index.tspackages/workflows/src/dag-executor.tspackages/workflows/src/executor.tspackages/workflows/src/model-validation.test.tspackages/workflows/src/model-validation.ts
| export function getProviderCapabilities(type: string): ProviderCapabilities { | ||
| switch (type) { | ||
| case 'claude': | ||
| return CLAUDE_CAPABILITIES; | ||
| case 'codex': | ||
| return CODEX_CAPABILITIES; | ||
| default: | ||
| throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]); | ||
| } |
There was a problem hiding this comment.
Return an immutable snapshot here.
Line 52 and Line 54 hand out the shared capability singletons by reference. Because those same objects are now part of the public surface, one accidental mutation by a consumer can silently change later capability checks for every caller in-process.
Possible fix
export function getProviderCapabilities(type: string): ProviderCapabilities {
switch (type) {
case 'claude':
- return CLAUDE_CAPABILITIES;
+ return { ...CLAUDE_CAPABILITIES };
case 'codex':
- return CODEX_CAPABILITIES;
+ return { ...CODEX_CAPABILITIES };
default:
throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]);
}
}📝 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.
| export function getProviderCapabilities(type: string): ProviderCapabilities { | |
| switch (type) { | |
| case 'claude': | |
| return CLAUDE_CAPABILITIES; | |
| case 'codex': | |
| return CODEX_CAPABILITIES; | |
| default: | |
| throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]); | |
| } | |
| export function getProviderCapabilities(type: string): ProviderCapabilities { | |
| switch (type) { | |
| case 'claude': | |
| return { ...CLAUDE_CAPABILITIES }; | |
| case 'codex': | |
| return { ...CODEX_CAPABILITIES }; | |
| default: | |
| throw new UnknownProviderError(type, [...REGISTERED_PROVIDERS]); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/factory.ts` around lines 49 - 57,
getProviderCapabilities currently returns shared capability singletons
(CLAUDE_CAPABILITIES, CODEX_CAPABILITIES) by reference; produce and return an
immutable snapshot instead to prevent consumer mutation from affecting global
state. Modify getProviderCapabilities to create a defensive copy of the selected
capability object (e.g., shallow/deep clone as appropriate for nested
properties) and freeze it (or deep-freeze) before returning; preserve the
UnknownProviderError(...) usage for the default case and continue to reference
REGISTERED_PROVIDERS when constructing the error. Ensure the cloning/freezing
logic is applied to both CLAUDE_CAPABILITIES and CODEX_CAPABILITIES returns so
callers receive an immutable snapshot.
| it('should infer claude from Claude model names', () => { | ||
| expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude'); | ||
| expect(inferProviderFromModel('opus', 'codex')).toBe('claude'); | ||
| expect(inferProviderFromModel('haiku', 'codex')).toBe('claude'); | ||
| expect(inferProviderFromModel('inherit', 'codex')).toBe('claude'); | ||
| expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude'); | ||
| }); |
There was a problem hiding this comment.
Don't classify inherit as Claude.
inherit is a sentinel for "keep the default provider", not a Claude model alias. If this expectation drives the helper, the new node.provider ?? inferProviderFromModel(node.model, workflowProvider) call sites in packages/workflows/src/dag-executor.ts will reroute model: inherit nodes from Codex workflows to Claude.
Suggested test update
it('should infer claude from Claude model names', () => {
expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude');
expect(inferProviderFromModel('opus', 'codex')).toBe('claude');
expect(inferProviderFromModel('haiku', 'codex')).toBe('claude');
- expect(inferProviderFromModel('inherit', 'codex')).toBe('claude');
expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude');
});
+
+ it('should keep the default provider for inherit', () => {
+ expect(inferProviderFromModel('inherit', 'claude')).toBe('claude');
+ expect(inferProviderFromModel('inherit', 'codex')).toBe('codex');
+ });📝 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.
| it('should infer claude from Claude model names', () => { | |
| expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('opus', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('haiku', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('inherit', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude'); | |
| }); | |
| it('should infer claude from Claude model names', () => { | |
| expect(inferProviderFromModel('sonnet', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('opus', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('haiku', 'codex')).toBe('claude'); | |
| expect(inferProviderFromModel('claude-opus-4-6', 'codex')).toBe('claude'); | |
| }); | |
| it('should keep the default provider for inherit', () => { | |
| expect(inferProviderFromModel('inherit', 'claude')).toBe('claude'); | |
| expect(inferProviderFromModel('inherit', 'codex')).toBe('codex'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/model-validation.test.ts` around lines 76 - 82, The
test and helper treat the model name "inherit" as a Claude alias; change
inferProviderFromModel to treat "inherit" as the sentinel meaning "use existing
provider" (i.e., do not map it to 'claude' — return undefined or null), and
update the test in model-validation.test.ts so
expect(inferProviderFromModel('inherit','codex')) reflects that sentinel
behavior instead of 'claude'; also ensure the call sites that use node.provider
?? inferProviderFromModel(node.model, workflowProvider) (refer to
dag-executor.ts) will preserve the workflowProvider when inferProviderFromModel
returns the sentinel/undefined.
| if (!model) return defaultProvider; | ||
| if (isClaudeModel(model)) return 'claude'; | ||
| return 'codex'; |
There was a problem hiding this comment.
'inherit' is incorrectly inferred as Claude.
inferProviderFromModel() currently treats 'inherit' as Claude via isClaudeModel(), which can incorrectly override the configured default provider and route execution to the wrong provider.
Proposed fix
export function inferProviderFromModel(
model: string | undefined,
defaultProvider: 'claude' | 'codex'
): 'claude' | 'codex' {
if (!model) return defaultProvider;
+ if (model === 'inherit') return defaultProvider;
if (isClaudeModel(model)) return 'claude';
return 'codex';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/model-validation.ts` around lines 24 - 26,
inferProviderFromModel currently classifies the string 'inherit' as Claude
because isClaudeModel('inherit') returns true; add an explicit guard in
inferProviderFromModel to return defaultProvider when model === 'inherit' (i.e.,
check model === 'inherit' before calling isClaudeModel) so the configured
defaultProvider isn't overridden, or alternatively update isClaudeModel to
exclude the literal 'inherit' from Claude detection; reference the
inferProviderFromModel, isClaudeModel and defaultProvider symbols when making
the change.
🔍 Comprehensive PR ReviewPR: #1185 — refactor: extract provider metadata seam for Phase 2 registry readiness SummaryClean, well-scoped structural refactor. Extracts provider capability data into static constants, adds Verdict: ✅
All five agents independently recommend APPROVE. No blocking issues. ✅ What's Good
🟢 Low Issues (All Optional / Defer-friendly)View 12 low-priority observationsL1: YAGNI — capability constants exported from public index with no external consumers📍
Options: Remove exports (strict YAGNI) | Keep with comment | Leave as-is L2: Dead
|
| Title | Priority |
|---|---|
Add test for orchestrator.unsupported_env_injection warning branch |
P3 |
Fix stale IAgentProvider example in architecture.md |
P3 |
Remove dead _deps param from resolveNodeProviderAndModel |
P3 |
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: .archon/artifacts/runs/02e22ef7e616f7a5c0dc9435519cbba7/review/
- Remove CLAUDE_CAPABILITIES/CODEX_CAPABILITIES from public index (YAGNI — callers should use getProviderCapabilities(), not raw constants) - Remove dead _deps parameter from resolveNodeProviderAndModel and its two call-sites (no longer needed after static capability lookup refactor) - Update factory.ts module JSDoc to mention both exported functions - Add edge-case tests for getProviderCapabilities: empty string and case-sensitive throws (parity with existing getAgentProvider tests) - Add test for inferProviderFromModel with empty string (returns default, documenting the falsy-string shortcut)
Review fixes applied (commit 42bd5f0)The review returned APPROVE with 0 CRITICAL/HIGH findings — all 12 were LOW. Addressed the actionable ones: L1 — YAGNI: Removed L2 — Dead parameter: Removed L5/L6 — Test edge cases: Added L8 — Comment: Updated Findings not fixed: L3/L12 (ordering dependency — safe today, flag for Phase 2), L4/L7 (test gaps for code paths with no current production trigger), L9–L11 (docs — out of scope per review agent recommendations). |
🎯 Workflow SummaryPlan: Implementation vs Plan
📋 Deviations from Plan (1)
Review Summary
All 5 review agents independently recommend APPROVE. The 7 remaining LOW findings are either pre-existing gaps or intentional deferrals to Phase 2. ✅ Quick Wins Applied (Pre-merge)
📋 Suggested Follow-Up Issues
Reply with:
|
…ss (coleam00#1185) * refactor: extract provider metadata seam for Phase 2 registry readiness - Add static capability constants (capabilities.ts) for Claude and Codex - Export getProviderCapabilities() from @archon/providers for capability queries without provider instantiation - Add inferProviderFromModel() to model-validation.ts, replacing three copy-pasted inline inference blocks in executor.ts and dag-executor.ts - Replace throwaway provider instantiation in dag-executor with static capability lookup (getProviderCapabilities) - Add orchestrator warning when env vars are configured but provider doesn't support envInjection * refactor: address LOW findings from code review - Remove CLAUDE_CAPABILITIES/CODEX_CAPABILITIES from public index (YAGNI — callers should use getProviderCapabilities(), not raw constants) - Remove dead _deps parameter from resolveNodeProviderAndModel and its two call-sites (no longer needed after static capability lookup refactor) - Update factory.ts module JSDoc to mention both exported functions - Add edge-case tests for getProviderCapabilities: empty string and case-sensitive throws (parity with existing getAgentProvider tests) - Add test for inferProviderFromModel with empty string (returns default, documenting the falsy-string shortcut)
…ss (coleam00#1185) * refactor: extract provider metadata seam for Phase 2 registry readiness - Add static capability constants (capabilities.ts) for Claude and Codex - Export getProviderCapabilities() from @archon/providers for capability queries without provider instantiation - Add inferProviderFromModel() to model-validation.ts, replacing three copy-pasted inline inference blocks in executor.ts and dag-executor.ts - Replace throwaway provider instantiation in dag-executor with static capability lookup (getProviderCapabilities) - Add orchestrator warning when env vars are configured but provider doesn't support envInjection * refactor: address LOW findings from code review - Remove CLAUDE_CAPABILITIES/CODEX_CAPABILITIES from public index (YAGNI — callers should use getProviderCapabilities(), not raw constants) - Remove dead _deps parameter from resolveNodeProviderAndModel and its two call-sites (no longer needed after static capability lookup refactor) - Update factory.ts module JSDoc to mention both exported functions - Add edge-case tests for getProviderCapabilities: empty string and case-sensitive throws (parity with existing getAgentProvider tests) - Add test for inferProviderFromModel with empty string (returns default, documenting the falsy-string shortcut)
…ss (coleam00#1185) * refactor: extract provider metadata seam for Phase 2 registry readiness - Add static capability constants (capabilities.ts) for Claude and Codex - Export getProviderCapabilities() from @archon/providers for capability queries without provider instantiation - Add inferProviderFromModel() to model-validation.ts, replacing three copy-pasted inline inference blocks in executor.ts and dag-executor.ts - Replace throwaway provider instantiation in dag-executor with static capability lookup (getProviderCapabilities) - Add orchestrator warning when env vars are configured but provider doesn't support envInjection * refactor: address LOW findings from code review - Remove CLAUDE_CAPABILITIES/CODEX_CAPABILITIES from public index (YAGNI — callers should use getProviderCapabilities(), not raw constants) - Remove dead _deps parameter from resolveNodeProviderAndModel and its two call-sites (no longer needed after static capability lookup refactor) - Update factory.ts module JSDoc to mention both exported functions - Add edge-case tests for getProviderCapabilities: empty string and case-sensitive throws (parity with existing getAgentProvider tests) - Add test for inferProviderFromModel with empty string (returns default, documenting the falsy-string shortcut)
1 participant
Summary
@archon/providersseam where capabilities and model inference are queryable without instantiation. This cleanup stabilises that boundary before any registry work begins.inferProviderFromModel()(model-validation.ts) andgetProviderCapabilities()(factory.ts + index.ts), backed by static capability constants incapabilities.tsper provider. Replaced all three inference sites and the one throwaway-instantiation site. Added orchestrator warning for env vars when provider doesn't supportenvInjection.'claude' | 'codex'union widened tostring. No registry introduced. No API endpoint changes. No executor/orchestrator decomposition. See plan for full exclusion list.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
executor.tsmodel-validation.tsinferProviderFromModel()dag-executor.tsmodel-validation.tsdag-executor.tsfactory.tsgetProviderCapabilities()factory.tsclaude/capabilities.tsfactory.tscodex/capabilities.tsClaudeProviderclaude/capabilities.tsgetCapabilities()returns shared constantCodexProvidercodex/capabilities.tsgetCapabilities()returns shared constantorchestrator-agent.ts@archon/providersgetProviderCapabilitiesfor env warningLabel Snapshot
risk: lowsize: Sproviders,workflows,coreproviders:factory,workflows:executor,core:orchestratorChange Metadata
refactormultiLinked Issue
Validation Evidence (required)
Security Impact (required)
Compatibility / Migration
getProviderCapabilities()is additive; no existing exports removedHuman Verification (required)
inferProviderFromModel()correctly handlesundefined, Claude aliases (sonnet,opus,haiku,claude-*), and non-Claude model namesgetProviderCapabilities()returns correct static values without provider instantiationenvInjectionmodel: 'inherit'(treated as non-Claude → codex), unknown provider type →UnknownProviderErrorSide Effects / Blast Radius (required)
@archon/providers,@archon/workflows(executor + dag-executor),@archon/core(orchestrator)getCapabilities()returned at runtimeinferProviderFromModel()andgetProviderCapabilities()are both tested explicitlyRollback Plan
git revert HEAD— no DB changes, no config changesRisks and Mitigations
factory.test.tsassertsgetProviderCapabilities(type)matchesprovider.getCapabilities()at runtime for both Claude and Codex — any drift will fail testsSummary by CodeRabbit
New Features
Refactor
Tests