feat(claude): support custom model registry#1587
feat(claude): support custom model registry#1587guanghuang wants to merge 1 commit intocoleam00:devfrom
Conversation
📝 WalkthroughWalkthroughAdds a Claude custom model registry (reads ~/.archon/claude-models.json), resolves provider-scoped aliases (e.g., ChangesCustom Claude Model Registry & Provider Integration
Sequence DiagramsequenceDiagram
participant User as Workflow/User
participant Provider as Claude Provider
participant Registry as ClaudeModelRegistry
participant Subprocess as Claude Code Subprocess
User->>Provider: sendQuery(model: "gateway/gpt", ...)
Provider->>Registry: resolve("gateway/gpt")
Registry->>Registry: read ~/.archon/claude-models.json and validate
Registry-->>Provider: ResolvedModel { resolvedId, providerName, matchedBy, env }
Provider->>Provider: merge registry env + request env
Provider->>Subprocess: spawn Claude subprocess with model=resolvedId and env
Subprocess-->>Provider: response
Provider-->>User: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/docs-web/src/content/docs/getting-started/ai-assistants.md (1)
132-170: ⚡ Quick winAdd a brief security note about secrets in
claude-models.json.The JSON example uses
YOUR_GATEWAY_API_KEYas a placeholder, but new users may paste real keys/tokens directly without realizing the file lives in~/.archon/in plaintext and survives across sessions. A one-line callout — e.g. "Stored in plaintext in your home directory; ensure file permissions restrict access (chmod 600) and never commit this file." — matches the PR objective ("secrets are stored only in the user-owned config file") and parallels the placeholder warning already added inconfiguration.mdLine 141.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md` around lines 132 - 170, Add a one-line security callout to the "Custom Claude-Compatible Providers" section warning that the example secrets in ~/.archon/claude-models.json are stored in plaintext and persist across sessions; instruct users to restrict file permissions (e.g., chmod 600) and never commit the file or keys to version control, and place this sentence near the JSON example or immediately before the paragraph describing apiKey/authToken/baseUrl so it's clearly associated with claude-models.json and the assistants.claude.model configuration.packages/providers/src/claude/provider.ts (2)
941-971: 💤 Low valueRegistry is re-read from disk on every
sendQuerycall.
new ClaudeModelRegistry()runsreadFileSyncagainst~/.archon/claude-models.jsonsynchronously on every invocation. In a workflow with many nodes (or under server load) this puts repeated synchronous disk I/O on the request path for a file that changes rarely. Consider lifting this to a process-level singleton (e.g. lazy-init module-scoped instance) or caching by file path with anfs.statSyncmtime check for hot reload.// model-registry.ts (sketch) let cached: ClaudeModelRegistry | undefined; export function getClaudeModelRegistry(): ClaudeModelRegistry { if (!cached) cached = new ClaudeModelRegistry(); return cached; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/provider.ts` around lines 941 - 971, The code instantiates a new ClaudeModelRegistry inside the request path (new ClaudeModelRegistry()) causing synchronous disk reads on every sendQuery call; fix it by introducing a module-scoped lazy singleton accessor (e.g., getClaudeModelRegistry()) that returns a cached ClaudeModelRegistry instance and only re-creates it when the backing file changes (use fs.statSync mtime check for hot-reload) and then replace occurrences of new ClaudeModelRegistry() in provider.ts with calls to getClaudeModelRegistry(); keep using registry.getError() and registry.resolve(rawModel) unchanged so logging and registryEnv behavior remain the same.
949-952: ⚡ Quick winSurface registry load errors to the caller, not just the log.
When
claude-models.jsonis malformed and the user'smodelwould have matched a registered alias, resolution silently falls through to passthrough and the Claude SDK reports a confusing "unknown model" error. The registry's parse error is only logged at warn level, so the actual root cause is buried. Consider yielding asystemchunk with the registry error when one exists (mirroring thenodeConfigWarningspattern at Lines 998–1000), so the user sees⚠️ Claude model registry failed to load: …before the SDK error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/provider.ts` around lines 949 - 952, The registry parse errors are only logged (registry.getError() -> getLog().warn) so callers don't see them; update the code path that handles registryError to also emit a system-facing chunk/message like the nodeConfigWarnings pattern (see nodeConfigWarnings handling around nodeConfigWarnings usage) — e.g., when registryError exists, in addition to getLog().warn({ error: registryError }, 'claude.model_registry_load_error'), create/yield a system chunk or push a system message containing "⚠️ Claude model registry failed to load: {registryError.message}" so the caller sees the registry parse error before any downstream "unknown model" SDK error.packages/providers/src/claude/model-registry.ts (1)
184-206: 💤 Low valueCross-provider id/name collisions resolve in
Object.entriesinsertion order — document or scope explicitly.Steps 2–4 iterate over all providers and return the first match. If two providers register the same model
id(orname), the winner is determined by JSON insertion order, which is stable per ECMAScript but easy to silently flip when users edit the file. Either (a) document this precedence in the JSDoc aboveresolve(), or (b) prefer provider-scoped lookups (already covered by step 1) and warn / return a deterministic error when an unscoped reference is ambiguous across providers. This isn't a bug today, but it's the kind of behavior that surprises users when they reorder a config file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/model-registry.ts` around lines 184 - 206, The resolve() logic currently returns the first match across Object.entries(this.providers) in steps 2–4 (case where code checks provider.models for id/name and calls buildResult), which makes cross-provider collisions depend on insertion order; update resolve() to detect ambiguous matches across providers for the same model id or name (collect all matching providers when searching in the loops), and if more than one provider matches return a deterministic error or warning (or require scoping) instead of returning the first winner; alternatively, add clear JSDoc above resolve() explaining insertion-order precedence if you prefer to keep current behavior.packages/docs-web/src/content/docs/reference/configuration.md (1)
99-141: 💤 Low valueConsider documenting
authTokenand credential precedence.The JSON example shows only
apiKey, but the field-mapping table at Lines 134–139 also listsauthToken(→ANTHROPIC_AUTH_TOKEN). New users will likely copy the example and miss that bearer-token gateways needauthTokeninstead. A one-line note (or a second example block mirroring the local provider inclaude-models.json.example) would close that gap. It would also help to clarify what happens when bothapiKeyandauthTokenare configured for the same provider, since the registry currently injects both env vars.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs-web/src/content/docs/reference/configuration.md` around lines 99 - 141, Add a one-line note after the claude-models.json example explaining that bearer-token gateways should use the authToken field (mapped to ANTHROPIC_AUTH_TOKEN) instead of apiKey, include a second small JSON snippet showing authToken used in place of apiKey (matching the existing example format), and clarify credential precedence by stating that the registry may inject both ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN but the provider should prefer authToken when both are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/claude/model-registry.ts`:
- Around line 118-122: The loop in model-registry.ts that validates providers
currently accepts empty strings for baseUrl and credentials; tighten the checks
in the provider validation (the for (const [name, provider] ...) block) so
provider.baseUrl must be a non-empty string (e.g., typeof provider.baseUrl ===
'string' && provider.baseUrl.trim() !== '') and require at least one non-empty
credential by checking that either provider.apiKey or provider.authToken is a
non-empty string (typeof ... === 'string' && ... .trim() !== ''); keep the
existing Array.isArray(provider.models) check and ensure failing these tighter
checks causes the entry to be skipped/produce a loadError so buildResult won't
receive empty baseUrl or credentials.
- Around line 223-227: The provider header handling in load() must validate
provider.headers entries and skip/reject ones containing control chars that will
break the "Name: Value" format: ensure header names do not contain ':' or '\n'
or '\r' and header values do not contain '\n' or '\r'; when such invalid entries
are found, do not include them in env.ANTHROPIC_CUSTOM_HEADERS and record a
descriptive loadError breadcrumb (e.g. referencing the provider id/name and
which header key was invalid). Update the logic around provider.headers ->
env.ANTHROPIC_CUSTOM_HEADERS in model-registry.ts (the load() path that maps
Object.entries(provider.headers)) to perform these checks, skip bad entries, and
set loadError accordingly.
---
Nitpick comments:
In `@packages/docs-web/src/content/docs/getting-started/ai-assistants.md`:
- Around line 132-170: Add a one-line security callout to the "Custom
Claude-Compatible Providers" section warning that the example secrets in
~/.archon/claude-models.json are stored in plaintext and persist across
sessions; instruct users to restrict file permissions (e.g., chmod 600) and
never commit the file or keys to version control, and place this sentence near
the JSON example or immediately before the paragraph describing
apiKey/authToken/baseUrl so it's clearly associated with claude-models.json and
the assistants.claude.model configuration.
In `@packages/docs-web/src/content/docs/reference/configuration.md`:
- Around line 99-141: Add a one-line note after the claude-models.json example
explaining that bearer-token gateways should use the authToken field (mapped to
ANTHROPIC_AUTH_TOKEN) instead of apiKey, include a second small JSON snippet
showing authToken used in place of apiKey (matching the existing example
format), and clarify credential precedence by stating that the registry may
inject both ANTHROPIC_API_KEY and ANTHROPIC_AUTH_TOKEN but the provider should
prefer authToken when both are present.
In `@packages/providers/src/claude/model-registry.ts`:
- Around line 184-206: The resolve() logic currently returns the first match
across Object.entries(this.providers) in steps 2–4 (case where code checks
provider.models for id/name and calls buildResult), which makes cross-provider
collisions depend on insertion order; update resolve() to detect ambiguous
matches across providers for the same model id or name (collect all matching
providers when searching in the loops), and if more than one provider matches
return a deterministic error or warning (or require scoping) instead of
returning the first winner; alternatively, add clear JSDoc above resolve()
explaining insertion-order precedence if you prefer to keep current behavior.
In `@packages/providers/src/claude/provider.ts`:
- Around line 941-971: The code instantiates a new ClaudeModelRegistry inside
the request path (new ClaudeModelRegistry()) causing synchronous disk reads on
every sendQuery call; fix it by introducing a module-scoped lazy singleton
accessor (e.g., getClaudeModelRegistry()) that returns a cached
ClaudeModelRegistry instance and only re-creates it when the backing file
changes (use fs.statSync mtime check for hot-reload) and then replace
occurrences of new ClaudeModelRegistry() in provider.ts with calls to
getClaudeModelRegistry(); keep using registry.getError() and
registry.resolve(rawModel) unchanged so logging and registryEnv behavior remain
the same.
- Around line 949-952: The registry parse errors are only logged
(registry.getError() -> getLog().warn) so callers don't see them; update the
code path that handles registryError to also emit a system-facing chunk/message
like the nodeConfigWarnings pattern (see nodeConfigWarnings handling around
nodeConfigWarnings usage) — e.g., when registryError exists, in addition to
getLog().warn({ error: registryError }, 'claude.model_registry_load_error'),
create/yield a system chunk or push a system message containing "⚠️ Claude model
registry failed to load: {registryError.message}" so the caller sees the
registry parse error before any downstream "unknown model" SDK error.
🪄 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: 90a9d59e-8550-4441-837f-7980e9f88c1a
📒 Files selected for processing (8)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/providers/package.jsonpackages/providers/src/claude/claude-models.json.examplepackages/providers/src/claude/model-registry.test.tspackages/providers/src/claude/model-registry.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.ts
917bd7d to
4e5b861
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/claude/model-registry.ts`:
- Around line 147-159: The provider validation currently only records messages
for some failures; update the validation in model-registry.ts so every skipped
provider shape appends a clear breadcrumb to validationMessages — specifically
add messages when "provider" is missing or not an object (use the existing
"name" to identify the entry) and when "provider.models" is not an array,
matching the style of the existing checks for baseUrl/apiKey/authToken; keep the
existing continue behavior but ensure each early-continue branch (the provider
object check and the Array.isArray(provider.models) check) pushes a descriptive
`"Provider \"${name}\" skipped: <reason>"` entry into validationMessages so all
ignored entries are diagnosable.
- Around line 131-136: The current validation allows providers to be null so
Object.entries(config.providers) can throw; tighten the guard around
parsed/(ClaudeModelsConfig).providers to require a non-null plain object and
reject arrays (e.g. check parsed && typeof parsed === 'object' && (parsed as
ClaudeModelsConfig).providers !== null && typeof (parsed as
ClaudeModelsConfig).providers === 'object' && !Array.isArray((parsed as
ClaudeModelsConfig).providers)); update the same check before any use of
Object.entries(config.providers) or in the load path that reads config.providers
to ensure you only iterate when providers is a real object (or use a small
isPlainObject utility and call that).
🪄 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: b0c2b3d7-9f53-44c3-bae3-98061e30f21c
📒 Files selected for processing (8)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/providers/package.jsonpackages/providers/src/claude/claude-models.json.examplepackages/providers/src/claude/model-registry.test.tspackages/providers/src/claude/model-registry.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.ts
✅ Files skipped from review due to trivial changes (4)
- packages/providers/src/claude/claude-models.json.example
- packages/providers/package.json
- packages/docs-web/src/content/docs/reference/configuration.md
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/providers/src/claude/provider.ts
- packages/providers/src/claude/provider.test.ts
| if (!provider || typeof provider !== 'object') continue; | ||
| if (!isNonEmptyString(provider.baseUrl)) { | ||
| validationMessages.push(`Provider "${name}" skipped: baseUrl must be a non-empty string`); | ||
| continue; | ||
| } | ||
| if (!isNonEmptyString(provider.apiKey) && !isNonEmptyString(provider.authToken)) { | ||
| validationMessages.push( | ||
| `Provider "${name}" skipped: apiKey or authToken must be a non-empty string` | ||
| ); | ||
| continue; | ||
| } | ||
| if (!Array.isArray(provider.models)) continue; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Record validation breadcrumbs for all skipped invalid providers.
Some invalid provider shapes are skipped silently (provider non-object, models non-array), while other invalid cases emit validationMessages. Make this consistent so users can diagnose why entries were ignored.
As per coding guidelines, “Prefer throwing early with a clear error for unsupported or unsafe states; never silently swallow errors or broaden permissions; document intentional fallback behavior with a comment”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/claude/model-registry.ts` around lines 147 - 159, The
provider validation currently only records messages for some failures; update
the validation in model-registry.ts so every skipped provider shape appends a
clear breadcrumb to validationMessages — specifically add messages when
"provider" is missing or not an object (use the existing "name" to identify the
entry) and when "provider.models" is not an array, matching the style of the
existing checks for baseUrl/apiKey/authToken; keep the existing continue
behavior but ensure each early-continue branch (the provider object check and
the Array.isArray(provider.models) check) pushes a descriptive `"Provider
\"${name}\" skipped: <reason>"` entry into validationMessages so all ignored
entries are diagnosable.
4e5b861 to
0af28fa
Compare
0af28fa to
05d0525
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/providers/src/claude/model-registry.ts`:
- Around line 131-133: ClaudeModelRegistry currently does synchronous file I/O
in its constructor via load(), causing readFileSync to run per-request; change
this to a cached module-level singleton factory (e.g., add getModelRegistry())
that reads the file once (or on file-change/TTL) and returns the same
ClaudeModelRegistry instance, update provider.ts to call getModelRegistry()
instead of new ClaudeModelRegistry(), and move the readFileSync logic out of the
constructor (keep load() for refreshes) so requests use the cached instance
rather than performing synchronous disk reads on every instantiation.
🪄 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: abb5c3c9-308b-4114-89a4-cad5ff696e02
📒 Files selected for processing (8)
packages/docs-web/src/content/docs/getting-started/ai-assistants.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/providers/package.jsonpackages/providers/src/claude/claude-models.json.examplepackages/providers/src/claude/model-registry.test.tspackages/providers/src/claude/model-registry.tspackages/providers/src/claude/provider.test.tspackages/providers/src/claude/provider.ts
✅ Files skipped from review due to trivial changes (3)
- packages/providers/package.json
- packages/providers/src/claude/claude-models.json.example
- packages/docs-web/src/content/docs/reference/configuration.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/docs-web/src/content/docs/getting-started/ai-assistants.md
- packages/providers/src/claude/provider.test.ts
- packages/providers/src/claude/provider.ts
| constructor() { | ||
| this.load(); | ||
| } |
There was a problem hiding this comment.
Synchronous file I/O on every request blocks the event loop.
ClaudeModelRegistry is instantiated per request in provider.ts (line 948), meaning readFileSync is called synchronously on every Claude inference request. This blocks the event loop for the duration of the disk read and adds latency to every request.
Consider a module-level singleton (or a cached instance with a short TTL/file-watch invalidation) so the file is read at most once (or on change):
🛠️ Suggested module-level singleton approach
+let _registrySingleton: ClaudeModelRegistry | undefined;
+
+export function getModelRegistry(): ClaudeModelRegistry {
+ if (!_registrySingleton) {
+ _registrySingleton = new ClaudeModelRegistry();
+ }
+ return _registrySingleton;
+}
export class ClaudeModelRegistry {Then in provider.ts, replace new ClaudeModelRegistry() with getModelRegistry().
Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/providers/src/claude/model-registry.ts` around lines 131 - 133,
ClaudeModelRegistry currently does synchronous file I/O in its constructor via
load(), causing readFileSync to run per-request; change this to a cached
module-level singleton factory (e.g., add getModelRegistry()) that reads the
file once (or on file-change/TTL) and returns the same ClaudeModelRegistry
instance, update provider.ts to call getModelRegistry() instead of new
ClaudeModelRegistry(), and move the readFileSync logic out of the constructor
(keep load() for refreshes) so requests use the cached instance rather than
performing synchronous disk reads on every instantiation.
Summary
~/.archon/claude-models.json, provider env injection, tests, docs, and a sanitized example file.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
provider.ts@anthropic-ai/claude-agent-sdkprovider.tsmodel-registry.tsmodel-registry.ts@archon/pathsgetArchonHome()to locate~/.archon/claude-models.json.model-registry.ts~/.archon/claude-models.jsonLabel Snapshot
risk: mediumsize: Mdocs|tests|configproviders:claudeChange Metadata
featuremultiLinked Issue
Validation Evidence (required)
Commands and result summary:
bun run --cwd packages/providers test bun run --cwd packages/providers type-check git diff --checkbun run validatewas not run because this change is isolated to provider/docs files and the package-level provider suite covers the touched runtime code.Security Impact (required)
NoNoYesYesIf any
Yes, describe risk and mitigation:The Claude provider can now read optional
~/.archon/claude-models.jsonand pass matched provider credentials to the Claude Code subprocess. The file is user-owned global config, built-in Claude models remain pass-through, example/docs use placeholders only, malformed configs degrade to pass-through with a warning, and request env remains the highest-precedence override.Compatibility / Migration
YesYesNoIf yes, exact upgrade steps:
No migration is required. Users who want custom Claude-compatible providers can create
~/.archon/claude-models.jsonusing the documented placeholder example and then set models such asgateway/gptin config or workflow nodes.Human Verification (required)
What was personally validated beyond CI:
provider/name, env injection for base URL/API key/custom headers, request env overriding registry env, pass-through for standard Claude models.Side Effects / Blast Radius (required)
claude.model_registry_load_error, unit tests for malformed config and pass-through behavior.Rollback Plan (required)
~/.archon/claude-models.jsonto disable custom alias resolution.Risks and Mitigations
provider/namelookup is supported and recommended.ANTHROPIC_CUSTOM_HEADERS, and tests cover header formatting.Summary by CodeRabbit
New Features
Documentation
Tests