feat(kha-267): finalize multi-provider tabbed UI and state refactor - #40
Merged
Conversation
…nthropic defaults On proxy stop, remove all ThroneKeeper env vars from .claude/settings.json and let Claude Code use its built-in defaults. This eliminates the fetchAnthropicDefaults() API call during revert, which failed for OAuth (Pro/Max) users and returned outdated 4.5-era model names. Also updates hardcoded fallbacks to current 4.6 -latest aliases. Closes: KHA-272 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- New provider-router.js: ProviderContext (per-tier state) + ProviderRouter (model→tier→provider mapping) classes with smart key validation - key-resolver.js: add resolveApiKeyForProvider() and buildUpstreamHeadersForContext() (additive exports, no breaking changes) - index.js: integrate router from MIXED_PROVIDERS_CONFIG env var; route each request to correct upstream provider; use effectiveProvider/BaseUrl/Key instead of globals inside /v1/messages handler - lib/config.js: add mixedProviders default (null = single-provider mode) - 29 unit tests covering ProviderContext, ProviderRouter, createRouterFromEnv, smart key validation, and mixed endpoint kinds (Anthropic + OpenAI in same session) 100% backward compatible: when MIXED_PROVIDERS_CONFIG is absent, all behavior is identical to single-provider mode.
…xed providers Phase 2: Extension & Config Schema - config.ts (v1.1.0): add TierProviderBindingSchema, MixedProviderConfigSchema, enableMixedProviders feature flag, mixed-provider invariant checks - messages.ts (v1.1.0): add SaveMixedProvidersMessage type, mixedProviders in ConfigLoaded payload, expandenableMixedProviders in feature flag enum - ProxyManager.ts: serialize MIXED_PROVIDERS_CONFIG env var with smart key resolution (resolves keys from SecretStorage per unique provider) - extension.ts: pass mixedProviders config when starting proxy - package.json: add claudeThrone.mixedProviders VS Code configuration property TypeScript compiles clean (zero errors). All 29 router tests pass.
…andler Phase 3: Webview UI - PanelViewProvider.ts: add saveMixedProviders message handler, handleSaveMixedProviders method, mixedProviders in config payload, enableMixedProviders in KNOWN_FLAGS, HTML toggle UI - main.js: add mixedProviders state, enableMixedProviders feature flag state, checkbox event listener, config hydration, visibility tied to 3-model mode Toggle appears as sub-option under '3 Different Models', hidden when disabled. Status indicator shows when mixed mode is active.
…tion invariant Phase 4: Presets, CLI Stubs & Documentation - docs/mixed-providers.md: Document mixed-provider architecture, schema, security - CONSTITUTION.md: Add Invariant 6 (Strict Isolation for Mixed Providers) - cli/commands/mixed.js: Create Phase 4 stub for CLI functionality - mixed-presets.json: Create Phase 4 stub for CLI presets
|
🤖 Hi @KHAEntertainment, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Mixed Provider Routing: per-request model→tier resolution via ProviderContext/ProviderRouter, transformer pipeline, env/secret serialization for MIXED_PROVIDERS_CONFIG, extension/webview schema/UI and proxy integration, tests, docs, and an invariant enforcing per-tier secret validation and request isolation. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,255,0.5)
actor Webview as VS Code Webview
participant Extension as Extension Host
participant SecretStorage as VS Code SecretStorage
participant Proxy as Local Proxy
participant Router as ProviderRouter
participant Upstream as Upstream Provider
end
Webview->>Extension: saveMixedProviders({enabled, reasoning, completion, value})
Extension->>SecretStorage: resolve keys for unique providers
Extension->>Proxy: startProxy(with MIXED_PROVIDERS_CONFIG env)
Webview->>Proxy: POST /v1/messages { model: "some-model", ... }
Proxy->>Router: resolve("some-model")
Router-->>Proxy: ProviderContext(providerId, baseUrl, key, endpointKind)
Proxy->>Proxy: build upstream URL & headers from ProviderContext
Proxy->>Upstream: forward transformed request (provider-specific headers)
Upstream-->>Proxy: stream/return response
Proxy-->>Webview: relay transformed response/events
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
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 |
|
🤖 I'm sorry @KHAEntertainment, but I was unable to process your request. Please see the logs for more details. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c33b1b147e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3517
to
+3520
| let endpointKind = 'openai'; | ||
| if (providers[pid]) { | ||
| if (pid === 'anthropic' || pid === 'bedrock') endpointKind = 'anthropic'; | ||
| } else if (state.customProviders) { |
There was a problem hiding this comment.
buildTierPayload defaults every built-in provider tier to endpointKind: 'openai' and only special-cases anthropic/bedrock, so tiers using deepseek, glm, minimax, or kimi are serialized as OpenAI-compatible. In mixed mode this makes the proxy send /v1/chat/completions + Bearer auth instead of Anthropic-native /v1/messages + x-api-key, which breaks routed requests for those providers.
Useful? React with 👍 / 👎.
Comment on lines
+3525
to
+3528
| let baseUrl = ''; | ||
| if (state.customProviders) { | ||
| const custom = state.customProviders.find(p => p.id === pid); | ||
| if (custom?.baseUrl) baseUrl = custom.baseUrl; |
There was a problem hiding this comment.
For non-custom providers, the mixed-tier payload leaves baseUrl empty, so MIXED_PROVIDERS_CONFIG lacks per-tier upstream URLs. The runtime then falls back to the global base URL (routedContext?.baseUrl || normalizedBaseUrl), causing cross-provider mixes to be routed to the wrong backend host instead of each tier’s selected provider.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
index.js (1)
954-960:⚠️ Potential issue | 🟠 MajorGuard uses global
endpointKindinstead ofeffectiveEndpointKind— potential routing bug.This guard at line 956 checks the global
endpointKind, but after the mixed-provider changes,effectiveEndpointKindmay differ from the global value.Scenario: If the global provider is Anthropic-native (e.g.,
glm) but a request routes to an OpenAI-compatible provider (e.g.,openrouter),isAnthropicNativeis correctlyfalse(fromeffectiveEndpointKind), so control reaches the OpenAI path. However, the guard still sees the globalendpointKindasANTHROPIC_NATIVEand throws an incorrect "endpoint kind mismatch" error.🐛 Proposed fix
// Comment 2: Guard - never run OpenAI↔Anthropic mapping when endpoint-kind is Anthropic // At this point, isAnthropicNative is false, so we proceed with OpenAI-compatible flow - if (endpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE) { + if (effectiveEndpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE) { console.error('[Guard Violation] Anthropic-native endpoint reached OpenAI conversion path - this should not happen') reply.code(500) return { error: { message: 'Internal error: endpoint kind mismatch', type: 'internal_error' } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 954 - 960, The guard is checking the global endpointKind constant instead of the per-request effectiveEndpointKind, causing false positives; update the conditional in the guard (the block that currently tests "if (endpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE) { ... }") to use effectiveEndpointKind (or the same computed variable used to set isAnthropicNative) so the check reads against effectiveEndpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE and returns the error only when the effective (per-request) endpoint kind actually indicates Anthropic-native.extensions/thronekeeper/out/extension.js (1)
196-223:⚠️ Potential issue | 🟠 MajorRemove
-latestsuffix from hardcoded Claude fallback model IDs.The fallback aliases use
claude-opus-4-6-latest,claude-sonnet-4-6-latest, andclaude-haiku-4-5-latest, but Anthropic's current API aliases omit the-latestsuffix:claude-opus-4-6,claude-sonnet-4-6, andclaude-haiku-4-5. When the/v1/modelsendpoint is unavailable, these invalid IDs become the cached defaults and will fail when Claude Code tries to use them downstream. (docs.anthropic.com)Fix
- const opus = selectBestModel(opusModels, 'claude-opus-4-6-latest'); + const opus = selectBestModel(opusModels, 'claude-opus-4-6'); - const sonnet = selectBestModel(sonnetModels, 'claude-sonnet-4-6-latest'); + const sonnet = selectBestModel(sonnetModels, 'claude-sonnet-4-6'); - const haiku = selectBestModel(haikuModels, 'claude-haiku-4-5-latest'); + const haiku = selectBestModel(haikuModels, 'claude-haiku-4-5'); - return { opus: 'claude-opus-4-6-latest', sonnet: 'claude-sonnet-4-6-latest', haiku: 'claude-haiku-4-5-latest' }; + return { opus: 'claude-opus-4-6', sonnet: 'claude-sonnet-4-6', haiku: 'claude-haiku-4-5' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/out/extension.js` around lines 196 - 223, The hardcoded Claude fallback IDs in the catch block of fetchAnthropicDefaults are using the deprecated "-latest" suffix and must be changed to the API aliases without it; update the return in the catch of fetchAnthropicDefaults to return opus: 'claude-opus-4-6', sonnet: 'claude-sonnet-4-6', haiku: 'claude-haiku-4-5' (replace the current 'claude-opus-4-6-latest', 'claude-sonnet-4-6-latest', 'claude-haiku-4-5-latest') so cached defaults are valid for downstream Claude usage.
🟡 Minor comments (4)
docs/mixed-providers.md-61-73 (1)
61-73:⚠️ Potential issue | 🟡 MinorThis section no longer matches the implementation.
The code now resolves mixed-provider keys during proxy startup and already renders tabbed provider controls in the webview. Describing SecretStorage lookup as “per request” and the UI as “single toggle only” gives readers the wrong mental model right where they’ll copy their setup from.
As per coding guidelines, "Update documentation in
docs/andREADME.mdfor user-facing changes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/mixed-providers.md` around lines 61 - 73, Update the docs to match current implementation: change the description of key resolution to state that mixed-provider keys are resolved at proxy startup (not per-request) and mention where startup-resolved keys come from (MIXED_PROVIDERS_CONFIG / mixed-presets.json / SecretStorage integration during startup), update the Webview UI section to reflect that tabbed provider controls are rendered (not a single mix-provider toggle), and remove or reword any references to per-request SecretStorage lookups and Phase 3b UI TODOs; reference ProviderRouter, effectiveProvider, MIXED_PROVIDERS_CONFIG, saveMixedProviders, mixed-presets.json and the Webview/tabbed provider controls while keeping the security/isolation guarantees intact.extensions/thronekeeper/src/views/PanelViewProvider.ts-2003-2046 (1)
2003-2046:⚠️ Potential issue | 🟡 MinorAdd input validation before accessing nested properties in
handleSaveMixedProviders.The method accesses nested properties like
msg.reasoning.providerId,msg.completion.baseUrl, etc. without first validating that these objects exist. If the webview sends a malformed message (or if there's a state synchronization issue), this could throw an uncaught exception.Think of it like unpacking a shipping box — you want to check the box actually contains inner boxes before reaching into them.
🛡️ Proposed fix
private async handleSaveMixedProviders(msg: any) { + // Validate required tier objects exist + if (!msg.reasoning || !msg.completion || !msg.value) { + this.log.appendLine(`[handleSaveMixedProviders] ERROR: Missing tier data in message`) + return + } + const cfg = vscode.workspace.getConfiguration('claudeThrone') const applyScope = cfg.get<string>('applyScope', 'workspace') const target = this.getConfigurationTarget(applyScope)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 2003 - 2046, handleSaveMixedProviders currently assumes msg and its nested objects (msg.reasoning, msg.completion, msg.value) exist and will throw if malformed; add input validation at the start of handleSaveMixedProviders to guard against missing or non-object msg fields (e.g., check msg && typeof msg === 'object' and each of msg.reasoning, msg.completion, msg.value are objects), log a clear error and return early if validation fails, and when building mixedConfig use safe access (optional chaining or defaults) to populate providerId/baseUrl/model/displayModel/endpointKind so cfg.update('mixedProviders', mixedConfig, target) and postConfig() are only called with a well-formed config.provider-router.js-281-288 (1)
281-288:⚠️ Potential issue | 🟡 MinorUse the injected env for debug gating.
Line 281 reads
process.env.DEBUGeven though this factory accepts anenvobject. That makescreateRouterFromEnv(fakeEnv)awkward to test and harder to reuse outside the main process, because the caller's DEBUG flag is ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider-router.js` around lines 281 - 288, The debug gating currently reads process.env.DEBUG inside the createRouterFromEnv factory; change that to use the injected env parameter (e.g. env.DEBUG or env.debug) so callers of createRouterFromEnv(fakeEnv) control debug output. Update the conditional that prints router.toDebugObject() to check the env argument (and ensure createRouterFromEnv accepts/threads the env object through to this scope), leaving the existing log messages (router.toDebugObject(), validation.uniqueProviders, router.tierMap) unchanged.extensions/thronekeeper/out/extension.js-341-355 (1)
341-355:⚠️ Potential issue | 🟡 MinorUpdate the revert copy everywhere this command is surfaced.
This command now deliberately hands control back to Claude Code's built-in defaults, but
extensions/thronekeeper/src/views/PanelViewProvider.ts:1522-1530still logs and shows "Anthropic defaults". That makes the same success path read like two different products depending on where the user clicked.Also applies to: 439-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/out/extension.js` around lines 341 - 355, The UI/logging still says "Anthropic defaults" even though updateClaudeSettings(settingsDir, ...) now restores Claude Code's built-in defaults; update the success message(s) in PanelViewProvider (the UI code that logs/shows the result around the success path) to say "Claude Code defaults" or "restored to Claude Code defaults" instead of "Anthropic defaults", and likewise find and replace the other occurrence referenced (the second block around 439-445) so all user-visible messages and logs match the new behavior of the updateClaudeSettings call.
🧹 Nitpick comments (3)
extensions/thronekeeper/src/services/Models.ts (1)
259-265: Use structured error metadata for manual-entry routing.Current string-only errors work, but they’re brittle for callers (like matching by prose). A stable error code makes handling deterministic.
♻️ Suggested hardening
- if (provider === 'minimax') { - throw new Error('Minimax models must be entered manually.') - } - if (provider === 'kimi') { - throw new Error('Kimi models must be entered manually.') - } + if (provider === 'minimax' || provider === 'kimi') { + const err = new Error(`${provider} models must be entered manually.`) + ;(err as any).code = 'MANUAL_MODEL_ENTRY_REQUIRED' + ;(err as any).provider = provider + throw err + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/services/Models.ts` around lines 259 - 265, Replace brittle string-only throws in Models.ts with a structured error type: create and export a ManualEntryError class (e.g., class ManualEntryError extends Error) that includes a stable error code like code = 'MANUAL_ENTRY_REQUIRED' and a provider property, then change the two branches that currently do throw new Error('Minimax models must be entered manually.') / throw new Error('Kimi models must be entered manually.') to throw new ManualEntryError('Manual entry required for provider', { provider: 'minimax' }) / throw new ManualEntryError(..., { provider: 'kimi' }); ensure callers can use instanceof ManualEntryError or check error.code/provider to route to the Manual Entry UI.tests/provider-router.test.js (1)
269-273: Consider adding tests for partially malformed tier configs.The
ProviderRouterconstructor validates that all three tiers must be present, but per Context snippet 1 (lines 134-147), the constructor specifically checksconfig.reasoning,config.completion, andconfig.value. You've tested{},null, and single-tier configs — it would be valuable to add tests for configs where a tier exists but has malformed/empty nested data (e.g.,{ reasoning: {}, completion: {...}, value: {...} }).This helps verify the router gracefully handles edge cases from potentially corrupted
MIXED_PROVIDERS_CONFIGserialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/provider-router.test.js` around lines 269 - 273, Add tests that exercise partially malformed tier configs for the ProviderRouter constructor: create cases where config has all three top-level keys but one tier is an empty object or missing required nested fields (e.g., reasoning: {}, completion: {…}, value: {…}) and assert that new ProviderRouter(...) throws. Target the ProviderRouter constructor and add assertions similar to the existing tests to cover combinations like { reasoning: {}, completion: valid, value: valid }, { reasoning: valid, completion: {}, value: valid } and { reasoning: valid, completion: valid, value: {} } to ensure malformed nested tier data is rejected.extensions/thronekeeper/out/extension.js (1)
1034-1034: Don't push the same disposables twice.These command disposables were already added to
context.subscriptionsat Line 507. Adding them again here means shutdown walks the same handles twice; probably harmless today, but it's an easy cleanup footgun.As per coding guidelines "Prevent duplicate event listeners; check cleanup in Start/Stop hydration sequence."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/out/extension.js` at line 1034, The listed disposables (openPanel, storeOpenRouterKey, storeOpenAIKey, storeTogetherKey, storeDeepseekKey, storeGlmKey, storeKimiKey, storeMinimaxKey, storeCustomKey, storeAnyKey, storeAnthropicKey, refreshAnthropicDefaults, startProxy, stopProxy, status, applyToClaudeCode, revertApply, log, checkConfigHealthCommand) are being pushed into context.subscriptions a second time; remove this duplicate push and ensure these disposables are only added once (reuse the initial push that occurs earlier around the first registration instead of re-pushing them here), or centralize the array of disposables into a single variable and push that variable only once during activation to prevent duplicate disposal on shutdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 34022-34043: The constructor that builds tierMap currently
overwrites duplicate model IDs; update the population logic (the for loop that
iterates Object.entries(this.contexts) and calls this.tierMap.set) to detect
duplicates: if ctx.model is already present in this.tierMap (use
this.tierMap.has(ctx.model)) then reject/raise an error or log and abort
initialization (or otherwise surface a config validation error) citing both tier
names and the conflicting model ID; alternatively, if duplicates are
intentionally allowed, change the lookup strategy used by resolve(modelName) to
include the tier signal (e.g., use a composite key or maintain a map of
modelName -> array of {tier,context}) and update resolve() to disambiguate based
on payload-provided tier. Ensure references to tierMap, resolve(), and the
constructor/context population loop are updated consistently.
- Around line 34710-34743: When a router exists and payload.model is provided
but router.resolve(payload.model) returns falsy, fail closed instead of silently
falling back: return a 400 (or appropriate) error and stop processing rather
than using defaults. Ensure the code path uses routedContext as the single
source of truth when it exists (set routedContext from router.resolve and derive
effectiveProvider/effectiveBaseUrl/effectiveKey/effectiveEndpointKind and
headers exclusively from routedContext.getHeaders()), and only use
normalizedBaseUrl/provider/key/buildUpstreamHeaders when there is no router or
no payload.model; update the logic around router.resolve, routedContext,
effective* variables, headers, and the missing-key check to be deterministic and
avoid mixed fallback behavior (functions/vars: router.resolve, routedContext,
effectiveProvider/effectiveBaseUrl/effectiveKey/effectiveEndpointKind, headers,
ensureEndpointKindReady, buildUpstreamHeaders).
In `@extensions/thronekeeper/out/services/ProxyManager.js`:
- Around line 480-488: When mixed mode is enabled in ProxyManager, the startup
must fail fast if any provider key is missing: in the loop that iterates
uniqueProviders (the block using this.secrets.getProviderKey and providerKeys),
if a key lookup returns falsy for any pid, throw a descriptive Error (e.g.
`throw new Error('[ProxyManager] Mixed mode enabled but missing provider key
for: ' + pid)`) instead of only logging a warning; ensure the thrown error
bubbles out of the initialization/startup routine so startup is blocked. Apply
the same change to the other similar block (around the 490-503 area) that checks
provider keys so both code paths enforce the fail-fast behavior.
In `@extensions/thronekeeper/out/views/PanelViewProvider.js`:
- Around line 563-565: The current implementation of handleListModels uses a
single this.currentSequenceToken for all providers, causing cross-provider
race-condition drops; change to track tokens and cached models per provider/tab:
introduce a map like this.sequenceTokens (keyed by requestedProvider or
this.runtimeProvider fallback) and a models cache keyed by provider, increment
the provider-specific token when issuing a request in handleListModels, store
the issued token in a local variable, and before discarding a late response
compare the response token against this.sequenceTokens[provider]; update cache
this.modelsByProvider[provider] on successful responses. Use the symbols
handleListModels, requestedProvider, this.runtimeProvider,
this.currentSequenceToken (replace), and create this.sequenceTokens and
this.modelsByProvider to locate and modify the code.
- Around line 1814-1849: handleStartProxy currently ignores the saved
mixedProviders; update handleStartProxy to read the saved mixedProviders from
the vscode configuration
(vscode.workspace.getConfiguration('claudeThrone').get('mixedProviders')) or use
this.getConfigurationTarget like handleSaveMixedProviders, and pass that config
into this.proxy.start(...) (or into whatever startOptions object the proxy
expects) so the proxy is started in mixed-provider mode when the panel's
Save/Start flow is used; keep references to handleSaveMixedProviders,
mixedProviders, and this.proxy.start when making the change and add a log line
indicating the mixed config being passed.
In `@extensions/thronekeeper/src/extension.ts`:
- Around line 597-611: Validate mixedProviders before calling proxy.start:
import and use MixedProviderConfigSchema.safeParse (from
extensions/claude-throne/src/schemas) to validate the cfg.get result stored in
mixedProviders; if safeParse returns success, pass the parsed value to
proxy.start, otherwise log the validation errors via log.appendLine and throw or
abort startup so the service fails fast instead of silently falling back to
single-provider mode; update the code around the mixedProviders check and the
await proxy!.start(...) call to use the validated value.
In `@extensions/thronekeeper/src/services/ProxyManager.ts`:
- Around line 494-503: The loop in ProxyManager that builds providerKeys uses
this.secrets.getProviderKey(pid) which misses the special Anthropic secret path
used elsewhere, so mixed tiers end up with null keys and only a warning is
logged; update the loop in ProxyManager to resolve Anthropic with the dedicated
secret method used by the codebase (e.g., call the same Anthropic retrieval like
this.secrets.getAnthropicKey(pid) or equivalent when pid === 'anthropic') and,
for any provider where no key is returned, fail fast by throwing an error (or
return a rejected promise) instead of just logging a warning so the proxy does
not start with missing keys.
In `@extensions/thronekeeper/webview/main.js`:
- Around line 3345-3372: The add button click handler in initMixedTabListeners
currently flips threeModelToggle.checked and sets state.twoModelMode directly
without invoking onTwoModelToggle() or persisting the change, so the UI change
isn't saved; update the handler to (1) set threeModelToggle.checked = true only
if needed and then call onTwoModelToggle() to run the normal toggle logic
(instead of directly setting state.twoModelMode and calling updateTwoModelUI()),
(2) after incrementing mixedTabState.tabCount and switching the tab
(switchToTab(tabId)), call saveState() and saveMixedState() so the new
multi-tab/three-model state is persisted. Ensure you still update the
addBtn/tabBtn display logic and only call these functions when the
checkbox/threeToggle transition actually occurs.
- Around line 3551-3552: The mixed-tab event bindings are being initialized at
module load which can run before the DOM exists; move the
initMixedTabListeners() invocation into the DOM-ready initialization path
(inside the existing init() function or the DOMContentLoaded handler that init()
uses) so it runs after markup is available; locate the initMixedTabListeners()
call and remove the top-level module invocation, then call
initMixedTabListeners() from within init() (or the same DOMContentLoaded
callback) to ensure add/close/tab buttons are bound reliably.
- Around line 3391-3432: switchToTab currently changes state.provider and model
selections directly but doesn't run the same sync/update steps as normal
provider changes; call the same routines after you set state.provider and models
so UI and engine stay in sync: invoke updateProviderUI(),
updateSelectedModelsDisplay(), persist by calling saveState(), and send the same
"updateProvider" message (or call the existing updateProvider handler) so the
extension host receives the change; ensure these calls occur right after you set
state.provider and update
state.reasoningModel/state.codingModel/state.valueModel (and after models are
loaded if loadModels is asynchronous).
- Around line 564-580: The mixedProvidersCheckbox change currently never
persists a disabled state because saveMixedState() only emits an enabled config;
update saveMixedState() so it always writes the current enabled value (including
enabled: false) into the outgoing config payload (e.g., emit
config.mixedProviders = { enabled: state.featureFlags.enableMixedProviders }
even when false), and ensure handleConfigLoaded() reads
config.mixedProviders.enabled and applies it to
state.featureFlags.enableMixedProviders and the UI
(mixedProvidersCheckbox.checked and updateMixedProvidersUI()) so unchecking
truly persists and is restored on hydrate/reload; reference the
mixedProvidersCheckbox change handler, saveMixedState(), and
handleConfigLoaded() when making these edits.
- Around line 3515-3538: buildTierPayload currently determines endpointKind from
providers or custom.provider.endpointKind but ignores state.endpointOverrides
and thus mislabels anthopic-compatible providers like "deepseek" and "glm";
update buildTierPayload to first check state.endpointOverrides for an override
entry for the pid and use that endpointKind if present, then fall back to: if
providers[pid] exists use the provider-based logic (treat "anthropic",
"bedrock", and declared anthopic-compatible ids like "deepseek" and "glm" as
'anthropic'), else look at state.customProviders for baseUrl only (do not use
custom.endpointKind) and finally default to 'openai'; ensure the function
returns endpointKind set from endpointOverrides when available and only uses
automatic detection otherwise.
In `@key-resolver.js`:
- Around line 397-405: The branch that sets headers based on ctx.endpointKind
only checks ENDPOINT_KIND.ANTHROPIC_NATIVE so contexts with canonical
'anthropic' get the wrong Authorization header; update the logic in the
headers-setting block (where ctx.endpointKind is inspected and headers are
assigned: headers['x-api-key'], headers['anthropic-version'],
headers['anthropic-beta'], else Authorization) to normalize or accept canonical
values — e.g., compare ctx.endpointKind after lowercasing to both
ENDPOINT_KIND.ANTHROPIC_NATIVE and the canonical string 'anthropic' (or map
canonical names to constants) so requests for Anthropic use x-api-key and
related headers instead of Bearer.
In `@mixed-presets.json`:
- Around line 6-20: The preset entries under "config" (the "reasoning",
"completion", and "value" tier blocks) are missing the required mixed-provider
field `baseUrl`, so they fail mixed-provider schema validation; update each tier
object (reasoning, completion, value) to include the provider's `baseUrl`
alongside `providerId` and `model` (and ensure the same fix is applied to the
other similar preset block noted in the comment), using the actual endpoint for
each provider so the mixedProviders loader passes validation.
In `@provider-router.js`:
- Around line 263-269: Parse MIXED_PROVIDERS_CONFIG as before but validate the
resulting object against the shared mixed-provider schema in
extensions/claude-throne/src/schemas/ before constructing ProviderRouter: run
the parsed config through the schema validator (use the existing
schema/validator helper), convert any deprecated "coding" key to "completion"
(or merge its contents into "completion"), and ensure required fields like
provider.baseUrl and provider.model are present and strings; if validation
fails, log the validation errors and return null instead of calling new
ProviderRouter(config, endpointOverrides), only constructing ProviderRouter
after the schema validator succeeds.
- Around line 167-173: When building the model → { tier, context } lookup (the
this.tierMap population that iterates this.contexts and calls Map.set with
ctx.model), detect if a model key already exists and the existing entry points
at a different upstream context; in that case reject the configuration (throw or
log and exit) instead of overwriting so ambiguous model collisions are not
allowed; implement this check using this.tierMap.has(model) and compare the
existing entry’s context/tier to ctx before deciding to set, and apply the same
guard to the other identical population block around the 190-194 region.
---
Outside diff comments:
In `@extensions/thronekeeper/out/extension.js`:
- Around line 196-223: The hardcoded Claude fallback IDs in the catch block of
fetchAnthropicDefaults are using the deprecated "-latest" suffix and must be
changed to the API aliases without it; update the return in the catch of
fetchAnthropicDefaults to return opus: 'claude-opus-4-6', sonnet:
'claude-sonnet-4-6', haiku: 'claude-haiku-4-5' (replace the current
'claude-opus-4-6-latest', 'claude-sonnet-4-6-latest', 'claude-haiku-4-5-latest')
so cached defaults are valid for downstream Claude usage.
In `@index.js`:
- Around line 954-960: The guard is checking the global endpointKind constant
instead of the per-request effectiveEndpointKind, causing false positives;
update the conditional in the guard (the block that currently tests "if
(endpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE) { ... }") to use
effectiveEndpointKind (or the same computed variable used to set
isAnthropicNative) so the check reads against effectiveEndpointKind ===
ENDPOINT_KIND.ANTHROPIC_NATIVE and returns the error only when the effective
(per-request) endpoint kind actually indicates Anthropic-native.
---
Minor comments:
In `@docs/mixed-providers.md`:
- Around line 61-73: Update the docs to match current implementation: change the
description of key resolution to state that mixed-provider keys are resolved at
proxy startup (not per-request) and mention where startup-resolved keys come
from (MIXED_PROVIDERS_CONFIG / mixed-presets.json / SecretStorage integration
during startup), update the Webview UI section to reflect that tabbed provider
controls are rendered (not a single mix-provider toggle), and remove or reword
any references to per-request SecretStorage lookups and Phase 3b UI TODOs;
reference ProviderRouter, effectiveProvider, MIXED_PROVIDERS_CONFIG,
saveMixedProviders, mixed-presets.json and the Webview/tabbed provider controls
while keeping the security/isolation guarantees intact.
In `@extensions/thronekeeper/out/extension.js`:
- Around line 341-355: The UI/logging still says "Anthropic defaults" even
though updateClaudeSettings(settingsDir, ...) now restores Claude Code's
built-in defaults; update the success message(s) in PanelViewProvider (the UI
code that logs/shows the result around the success path) to say "Claude Code
defaults" or "restored to Claude Code defaults" instead of "Anthropic defaults",
and likewise find and replace the other occurrence referenced (the second block
around 439-445) so all user-visible messages and logs match the new behavior of
the updateClaudeSettings call.
In `@extensions/thronekeeper/src/views/PanelViewProvider.ts`:
- Around line 2003-2046: handleSaveMixedProviders currently assumes msg and its
nested objects (msg.reasoning, msg.completion, msg.value) exist and will throw
if malformed; add input validation at the start of handleSaveMixedProviders to
guard against missing or non-object msg fields (e.g., check msg && typeof msg
=== 'object' and each of msg.reasoning, msg.completion, msg.value are objects),
log a clear error and return early if validation fails, and when building
mixedConfig use safe access (optional chaining or defaults) to populate
providerId/baseUrl/model/displayModel/endpointKind so
cfg.update('mixedProviders', mixedConfig, target) and postConfig() are only
called with a well-formed config.
In `@provider-router.js`:
- Around line 281-288: The debug gating currently reads process.env.DEBUG inside
the createRouterFromEnv factory; change that to use the injected env parameter
(e.g. env.DEBUG or env.debug) so callers of createRouterFromEnv(fakeEnv) control
debug output. Update the conditional that prints router.toDebugObject() to check
the env argument (and ensure createRouterFromEnv accepts/threads the env object
through to this scope), leaving the existing log messages
(router.toDebugObject(), validation.uniqueProviders, router.tierMap) unchanged.
---
Nitpick comments:
In `@extensions/thronekeeper/out/extension.js`:
- Line 1034: The listed disposables (openPanel, storeOpenRouterKey,
storeOpenAIKey, storeTogetherKey, storeDeepseekKey, storeGlmKey, storeKimiKey,
storeMinimaxKey, storeCustomKey, storeAnyKey, storeAnthropicKey,
refreshAnthropicDefaults, startProxy, stopProxy, status, applyToClaudeCode,
revertApply, log, checkConfigHealthCommand) are being pushed into
context.subscriptions a second time; remove this duplicate push and ensure these
disposables are only added once (reuse the initial push that occurs earlier
around the first registration instead of re-pushing them here), or centralize
the array of disposables into a single variable and push that variable only once
during activation to prevent duplicate disposal on shutdown.
In `@extensions/thronekeeper/src/services/Models.ts`:
- Around line 259-265: Replace brittle string-only throws in Models.ts with a
structured error type: create and export a ManualEntryError class (e.g., class
ManualEntryError extends Error) that includes a stable error code like code =
'MANUAL_ENTRY_REQUIRED' and a provider property, then change the two branches
that currently do throw new Error('Minimax models must be entered manually.') /
throw new Error('Kimi models must be entered manually.') to throw new
ManualEntryError('Manual entry required for provider', { provider: 'minimax' })
/ throw new ManualEntryError(..., { provider: 'kimi' }); ensure callers can use
instanceof ManualEntryError or check error.code/provider to route to the Manual
Entry UI.
In `@tests/provider-router.test.js`:
- Around line 269-273: Add tests that exercise partially malformed tier configs
for the ProviderRouter constructor: create cases where config has all three
top-level keys but one tier is an empty object or missing required nested fields
(e.g., reasoning: {}, completion: {…}, value: {…}) and assert that new
ProviderRouter(...) throws. Target the ProviderRouter constructor and add
assertions similar to the existing tests to cover combinations like { reasoning:
{}, completion: valid, value: valid }, { reasoning: valid, completion: {},
value: valid } and { reasoning: valid, completion: valid, value: {} } to ensure
malformed nested tier data is rejected.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 573648f7-4bc4-43ad-9018-7b4ac03b940e
⛔ Files ignored due to path filters (8)
extensions/thronekeeper/out/extension.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/schemas/config.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/schemas/messages.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/Models.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/ProxyManager.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/views/PanelViewProvider.js.mapis excluded by!**/*.mapextensions/thronekeeper/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
CONSTITUTION.mdcli/commands/mixed.jsdocs/mixed-providers.mdextensions/thronekeeper/bundled/proxy/index.cjsextensions/thronekeeper/out/extension.jsextensions/thronekeeper/out/schemas/config.jsextensions/thronekeeper/out/schemas/messages.jsextensions/thronekeeper/out/services/Models.jsextensions/thronekeeper/out/services/ProxyManager.jsextensions/thronekeeper/out/views/PanelViewProvider.jsextensions/thronekeeper/package.jsonextensions/thronekeeper/src/extension.tsextensions/thronekeeper/src/schemas/config.tsextensions/thronekeeper/src/schemas/messages.tsextensions/thronekeeper/src/services/Models.tsextensions/thronekeeper/src/services/ProxyManager.tsextensions/thronekeeper/src/views/PanelViewProvider.tsextensions/thronekeeper/webview/main.jsindex.jskey-resolver.jslib/config.jsmixed-presets.jsonprovider-router.jstests/provider-router.test.js
Comment on lines
+263
to
+269
| export function createRouterFromEnv(env = process.env, endpointOverrides = {}) { | ||
| const raw = env.MIXED_PROVIDERS_CONFIG | ||
| if (!raw) return null | ||
|
|
||
| try { | ||
| const config = JSON.parse(raw) | ||
| const router = new ProviderRouter(config, endpointOverrides) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validate MIXED_PROVIDERS_CONFIG with the shared schema before constructing the router.
JSON.parse() plus the tier-presence check is still too thin here. A stale coding key, missing baseUrl, or non-string model will fail late and just fall back to single-provider mode. Please run this boundary through the existing mixed-provider schema so the env contract stays in lockstep with the rest of the feature.
As per coding guidelines "Validate message/config contracts against schemas in extensions/claude-throne/src/schemas/." and "Use 'completion' key for storage operations (deprecated: 'coding' was deprecated in favor of 'completion')."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@provider-router.js` around lines 263 - 269, Parse MIXED_PROVIDERS_CONFIG as
before but validate the resulting object against the shared mixed-provider
schema in extensions/claude-throne/src/schemas/ before constructing
ProviderRouter: run the parsed config through the schema validator (use the
existing schema/validator helper), convert any deprecated "coding" key to
"completion" (or merge its contents into "completion"), and ensure required
fields like provider.baseUrl and provider.model are present and strings; if
validation fails, log the validation errors and return null instead of calling
new ProviderRouter(config, endpointOverrides), only constructing ProviderRouter
after the schema validator succeeds.
5 tasks
KHAEntertainment
added
area:proxy
Owner
Author
|
Addressed the remaining mixed-provider review feedback in Summary:
Local verification:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/phase4-hydration.test.js (1)
380-410: 🛠️ Refactor suggestion | 🟠 MajorUse the canonical
completionterminology in test state, notcodingModel.The test fixtures here (and again in the "Comment 4" block around lines 557/567/576) refer to
state.codingModeland fan it out intocompletionon save. That's a subtle footgun: in the rest of the codebasecompletionis the canonical slot, andcodingwas explicitly retired. Keeping the old label alive in tests makes it easy to accidentally reintroduce it elsewhere via copy-paste.♻️ Suggested rename for clarity
const state = { provider: 'openrouter', reasoningModel: 'claude-3.5-sonnet', - codingModel: 'claude-3.5-haiku', + completionModel: 'claude-3.5-haiku', valueModel: 'claude-3-opus', ... } ... - state.modelsByProvider[previousProvider].completion = state.codingModel + state.modelsByProvider[previousProvider].completion = state.completionModel state.modelsByProvider[previousProvider].value = state.valueModel ... global.vscode.postMessage({ type: 'saveModels', providerId: previousProvider, reasoning: state.reasoningModel, - completion: state.codingModel, + completion: state.completionModel, value: state.valueModel })Same swap applies in the "Comment 4" describe block (
state.codingModel→state.completionModel).As per coding guidelines: "Storage operations use 'completion' key (never 'coding')." and "Provider map uses canonical keys:
{ reasoning, completion, value }."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phase4-hydration.test.js` around lines 380 - 410, Replace the retired "coding" key in the test fixtures with the canonical "completion" name: rename any use of state.codingModel to state.completionModel and update places that assign it into modelsByProvider (e.g., state.modelsByProvider[previousProvider].completion = state.completionModel) and any local variables in the same test block (including the other "Comment 4" describe block). Keep the postMessage payload using the completion property (providerId, reasoning, completion, value) unchanged but source it from state.completionModel so the test uses the canonical completion slot everywhere.extensions/thronekeeper/src/views/PanelViewProvider.ts (2)
1351-1597:⚠️ Potential issue | 🟠 MajorUse mixed-provider tier models before single-provider startup guards.
mixedProvidersis validated and passed toproxy.start, but startup still requiresreasoningModel/completionModel/valueModelfromthis.runtimeProvider. A valid mixed config can be blocked just because the active tab/provider does not have all legacy globals saved.🛠️ Suggested direction
if (mixedProvidersRaw?.enabled) { const parsed = MixedProviderConfigSchema.safeParse(mixedProvidersRaw) @@ mixedProviders = parsed.data this.log.appendLine(`[handleStartProxy] Mixed provider config enabled and validated`) } @@ if (modelSelectionsByProvider[this.runtimeProvider]) { @@ } else { this.log.appendLine(`[handleStartProxy] No models found for provider ${this.runtimeProvider} in modelSelectionsByProvider`) } + + if (mixedProviders) { + reasoningModel = mixedProviders.reasoning.displayModel || mixedProviders.reasoning.model + completionModel = mixedProviders.completion.displayModel || mixedProviders.completion.model + valueModel = mixedProviders.value.displayModel || mixedProviders.value.model + this.log.appendLine(`[handleStartProxy] Using models from mixed-provider tier config`) + } @@ - if (!reasoningModel || reasoningModel.trim() === '') { + if (!reasoningModel || reasoningModel.trim() === '') {As per coding guidelines, When implementing Start/Stop proxy operations, ensure atomic updates: save both legacy globals (reasoningModel, completionModel, valueModel) and provider-specific selections together to prevent partial state corruption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 1351 - 1597, The startup currently blocks if provider-specific legacy globals (reasoningModel/completionModel/valueModel) are missing even when a validated mixedProviders config exists; update handleStartProxy to treat mixedProviders as a valid source for models by (1) extracting/assigning reasoningModel, completionModel, and valueModel from mixedProviders when mixedProviders is non-null before performing the missing-model guards and stale-model checks, (2) using those mixed-derived models when calling hydrateGlobalKeysFromProvider and proxy.start (proxy.start already receives mixedProviders), and (3) ensuring any config save path atomically writes both global keys and modelSelectionsByProvider together so a validated mixedProviders config never gets blocked by legacy missing keys (referencing mixedProviders, reasoningModel, completionModel, valueModel, hydrateGlobalKeysFromProvider, and proxy.start).
331-332:⚠️ Potential issue | 🟠 MajorPass the full
setModelFromListmessage so tab provider context is preserved.Line 332 still calls the old positional form, so any
msg.providerfrom a secondary tab is dropped and the new payload handling at Lines 1869-1874 never runs. This can save a tab’s model under whichever provider is currently global.🛠️ Proposed fix
case 'setModelFromList': - await this.handleSetModelFromList(msg.modelId, msg.modelType) + await this.handleSetModelFromList(msg) breakAs per coding guidelines, Maintain provider model selections with atomic structure: { reasoning, completion, value } per provider, never storing individual model keys without full context.
Also applies to: 1865-1874
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 331 - 332, The call to handleSetModelFromList is passing only positional args and dropping msg.provider; change the invocation to pass the full message object (the entire msg) so handleSetModelFromList can read msg.provider and the atomic model structure { reasoning, completion, value } and trigger the payload handling in handleSetModelFromList (the code at lines ~1869-1874). Update any other similar calls (see the other invocation region around lines ~1865-1874) to pass the full msg instead of individual fields to ensure per-provider selections are preserved.
♻️ Duplicate comments (2)
extensions/thronekeeper/src/views/PanelViewProvider.ts (1)
581-698:⚠️ Potential issue | 🟠 MajorScope model-load freshness by provider/tab instead of one global token.
requestedProviderlets multiple tabs load in parallel, but Line 693 still compares against a singlecurrentSequenceToken. A later Provider 2 request can make a slower Provider 1 response look stale, like one traffic light controlling every lane.🛠️ Suggested direction
private modelsCache: Map<string, { models: any[], timestamp: number }> = new Map() // Comment 4: Sequence token tracking for race protection private sequenceTokenCounter: number = 0 private currentSequenceToken: number | null = null + private currentSequenceTokenByProvider: Map<string, number> = new Map() @@ this.currentSequenceToken = requestSequenceNum !== null ? requestSequenceNum : this.sequenceTokenCounter + if (requestSequenceNum !== null) { + this.currentSequenceTokenByProvider.set(provider, requestSequenceNum) + } @@ sequenceToken = `seq-${this.sequenceTokenCounter}` this.currentSequenceToken = this.sequenceTokenCounter + this.currentSequenceTokenByProvider.set(provider, this.sequenceTokenCounter) @@ - if (responseSequenceNum !== null && this.currentSequenceToken !== null && responseSequenceNum < this.currentSequenceToken) { - this.log.appendLine(`[handleListModels] DISCARDING late response - sequence token ${sequenceToken} is older than current ${this.currentSequenceToken}`) + const latestForProvider = this.currentSequenceTokenByProvider.get(provider) + if (responseSequenceNum !== null && latestForProvider !== undefined && responseSequenceNum < latestForProvider) { + this.log.appendLine(`[handleListModels] DISCARDING late response - sequence token ${sequenceToken} is older than current ${latestForProvider} for provider ${provider}`) return // Discard late response }Based on learnings, In provider-aware model loading logic, validate request/response token matching and cache models by provider to prevent race conditions from concurrent provider switches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 581 - 698, handleListModels currently uses a single global currentSequenceToken which causes cross-provider races when multiple tabs request models; change to a per-provider sequence token map (e.g., this.sequenceTokenByProvider) and a per-provider counter (e.g., this.sequenceCounterByProvider) so tokens are generated, stored and compared per provider; update the token generation/assignment logic in handleListModels (where sequenceToken, requestToken, sequenceTokenCounter and this.currentSequenceToken are handled) to read/write the per-provider entries keyed by provider, and change the late-response check to compare the parsed responseSequenceNum against the provider-specific current token rather than this.currentSequenceToken; keep modelsCache keyed by provider as used and ensure you use sequenceToken (not requestToken) in post payloads.extensions/thronekeeper/bundled/proxy/index.cjs (1)
35078-35090:⚠️ Potential issue | 🟡 MinorMixed-mode endpoint negotiation is skipped, but the fall-through fallback still sneaks in.
With
routedContextset,ensureEndpointKindReady()is (correctly) skipped — the router already knows the kind. ButeffectiveEndpointKind = routedContext?.endpointKind || endpointKindstill falls back to the module-levelendpointKindif the routed context ever has an empty/undefined kind. SinceProviderContextonly assignsthis.endpointKindinside the!== "auto"branch (lines 33965–33970) and otherwise callsinferEndpointKindSync, an "auto" value that inference can't resolve leavesthis.endpointKindundefined and the handler silently uses the default provider's kind. Worth asserting thatProviderContextalways produces a concrete kind (e.g., default the else-branch assignment, or throw) so mixed-mode stays on its own rails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 35078 - 35090, When routedContext is present the handler still falls back to the module-level endpointKind via effectiveEndpointKind = routedContext?.endpointKind || endpointKind, which allows an unresolved "auto" ProviderContext to leak its undefined kind; fix this by making ProviderContext always produce a concrete kind: in ProviderContext (the constructor/initialization that currently branches on !== "auto" and calls inferEndpointKindSync), assign this.endpointKind to the result of inferEndpointKindSync() and if that returns falsy throw a clear error (or set a safe explicit default) so inferEndpointKindSync cannot leave this.endpointKind undefined and the handler won’t silently use the module-level endpointKind.
🧹 Nitpick comments (7)
tests/phase5-ui-optimization.test.js (1)
210-240: Guard against fake-timer leakage if an assertion fails mid-test.Right now
vi.useRealTimers()on line 239 only runs if everything before it passes. If an expectation throws (or the debounced path regresses and produces more than one call), fake timers stay armed and the next test in the file silently inherits them — which tends to produce spooky, unrelated-looking failures later on. It's the testing equivalent of leaving the oven on when you leave the house.🛡️ Proposed fix using try/finally (or equivalently, a `beforeEach`/`afterEach` pair)
it('debouncing reduces render calls during rapid typing', async () => { vi.useFakeTimers(); - const renderCalls = []; - ... - await vi.advanceTimersByTimeAsync(200); - expect(renderCalls.length).toBe(1); - expect(renderCalls[0]).toBe('test'); - vi.useRealTimers(); + try { + const renderCalls = []; + // ...existing body... + await vi.advanceTimersByTimeAsync(200); + expect(renderCalls.length).toBe(1); + expect(renderCalls[0]).toBe('test'); + } finally { + vi.useRealTimers(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phase5-ui-optimization.test.js` around lines 210 - 240, The test currently calls vi.useFakeTimers() but only restores with vi.useRealTimers() at the end, which leaks fake timers if an assertion throws; wrap the timer-handling in a try/finally so vi.useRealTimers() always runs (i.e., call vi.useFakeTimers() before the try, run the test body including debouncedRender calls and await vi.advanceTimersByTimeAsync(200) inside the try, and call vi.useRealTimers() in the finally block). Reference the existing uses of vi.useFakeTimers, vi.useRealTimers, debounce, debouncedRender and renderCalls when making the change.extensions/thronekeeper/tests/vscode-shim.ts (1)
71-71:TreeItemshould at least accept the constructor args it's called with.VS Code's real
TreeItemis typically instantiated asnew TreeItem(label, collapsibleState), and user subclasses often chain throughsuper(label, state). An empty class silently drops those arguments, which is fine until a test actually touchesthis.labelorthis.collapsibleStateand getsundefined. Cheap to future-proof:🛡️ Suggested fix
-export class TreeItem {} +export class TreeItem { + label?: string | { label: string } + collapsibleState?: number + constructor(label?: string | { label: string }, collapsibleState?: number) { + this.label = label + this.collapsibleState = collapsibleState + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/tests/vscode-shim.ts` at line 71, The test shim for class TreeItem currently drops constructor args; update the TreeItem class to define a constructor like constructor(label?, collapsibleState?) and assign the values to instance properties (this.label and this.collapsibleState) so tests and subclasses that call super(label, state) receive the same fields as real VS Code TreeItem; keep parameter types permissive (optional) to match usage in tests and preserve backwards compatibility.tests/models.list.test.ts (1)
116-136: Skipped timeout test — worth tracking as a follow-up.This
it.skipwith a TODO note is a known gap in coverage for the AbortController-driven timeout path. Think of it like leaving a "beware of dog" sign but no dog — the behavior is still a real concern, it's just not actively guarded by a test right now. Given this PR touches provider/model-loading pathways, losing the timeout assertion is a minor risk for regressions arounderrorType: 'timeout'classification.A couple of options to consider when you come back to this:
- Run the
AbortControllerwiring under real timers but with a much smaller timeout injected via dependency (e.g., a per-testtimeoutMs), so fake timers don't have to interact with the mocked promise.- Or, drive the abort directly by invoking the listener that
Modelsattaches toopts.signal, bypassing timers entirely.Happy to open a tracking issue and/or sketch a real-timer variant of this test if you'd like.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/models.list.test.ts` around lines 116 - 136, The skipped test 'classifies timeout when request exceeds per-request timeout' leaves the AbortController timeout path untested for listModels; fix by either (A) switching this test to real timers (remove vi.useFakeTimers()) and inject a tiny per-request timeout argument into listModels so the request naturally aborts quickly, or (B) keep fake timers but avoid relying on timer-driven aborts by directly invoking the abort listener on the mocked request signal (in mockRequest's implementation access opts.signal and call opts.signal.dispatchEvent(new Event('abort')) or call the attached listener) to simulate AbortError; update the mockRequest implementation used in this test to reject with an AbortError shape (name='AbortError', code='UND_ERR_ABORTED') and then re-enable the it (remove .skip).tests/phase4-hydration.test.js (1)
370-378:global.vscodeassignment leaks between tests.Setting
global.vscodeinside the test without restoring it in anafterEachmeans any subsequent test that inspectsglobalThis.vscode(or thevscodemodule mock) inherits this stub. It's a small thing, but with Vitest's single-worker policy (per the repo guideline), state bleed like this is the usual suspect when a downstream test mysteriously starts failing.🧹 Suggested cleanup
describe('Comment 1: Provider Change - Save Before Switch', () => { + let prevGlobalVscode + beforeEach(() => { prevGlobalVscode = global.vscode }) + afterEach(() => { global.vscode = prevGlobalVscode }) it('saves old provider models before provider update', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phase4-hydration.test.js` around lines 370 - 378, The test assigns a stub to global.vscode (and uses postMessageCalls) without restoring it, which leaks state across tests; wrap the assignment in a setup/teardown pair by saving the original global.vscode before replacing it and restoring (or deleting) it in an afterEach/teardown hook so other tests don't inherit the stub — locate the global.vscode assignment and postMessageCalls usage in the test and add an afterEach that restores originalGlobalVscode (or deletes global.vscode if undefined originally) and clears postMessageCalls.tests/utils.js (1)
89-97: SSE error path sends a JSON body undertext/event-streamcontent-type.When
statusCodeis non-2xx you're sendingContent-Type: text/event-streamheaders and then a JSON payload. It works for the current assertions, but it's a bit like pouring orange juice into a wine glass and labeling it Chardonnay — anything actually parsing the response as SSE will try to read it as event frames and may throw before the test gets to inspect the error. For error responses, the more consistent choice is to switch the content-type toapplication/json.♻️ Suggested fix
} else if (mode === 'sse') { - res.writeHead(statusCode, { - 'content-type': 'text/event-stream', - 'cache-control': 'no-cache', - connection: 'keep-alive', - }) if (statusCode < 200 || statusCode >= 300) { + res.writeHead(statusCode, { 'content-type': 'application/json' }) res.end(JSON.stringify({ error: { message: `status ${statusCode}` } })) return } + res.writeHead(statusCode, { + 'content-type': 'text/event-stream', + 'cache-control': 'no-cache', + connection: 'keep-alive', + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils.js` around lines 89 - 97, The SSE test helper writes headers with res.writeHead and always sets 'content-type' to 'text/event-stream' even when statusCode is non-2xx; change the error path so when statusCode < 200 || statusCode >= 300 you set the response Content-Type to 'application/json' before calling res.end(JSON.stringify(...)) so error consumers parse JSON correctly (adjust the headers passed to res.writeHead or call res.setHeader on the error branch using the same res object).tests/provider-router.test.js (1)
1-6: Add a companion test for tabbed model-loading token isolation.This suite covers routing well, but the PR also changes provider-aware model loading. Please add a focused test that simulates two providers/tabs resolving out of order and asserts the stale response/error for one provider does not clobber the other.
As per coding guidelines, When testing provider-aware model loading, verify that stale responses with mismatched tokens or providers are ignored and not rendered to the UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/provider-router.test.js` around lines 1 - 6, Add a new unit test in tests/provider-router.test.js that exercises provider-aware model-loading token isolation: use ProviderContext and ProviderRouter to simulate two distinct providers/tabs initiating model loads with different tokens, resolve the second provider's async load first and then resolve the first (stale) one out-of-order, and assert that the stale response/error with a mismatched token/provider is ignored (does not overwrite the second provider's state or render). Ensure the test creates controllable async promises for model resolution, wires them through the same loading route used by createRouterFromEnv/ProviderRouter, and verifies final UI/state only reflects the most recent matching token/provider.extensions/thronekeeper/bundled/proxy/index.cjs (1)
34604-34622:getModelTransformerscustom-provider fallback is a little over-eager.When
providerId === "custom", the code flattens every other provider's config and merges those entries in alongside*and the custom-specific config. That means a custom endpoint that happens to expose a model name matching, say, a GLM or OpenRouter pattern will inherit those providers' transformers. This might be intentional ("best effort match for unknown upstreams"), but it's also the kind of thing that quietly turns onreasoningorenhancetoolfor a user's private deployment and makes debugging confusing. A short comment explaining the intent — or restricting fallback to a curated subset — would help future-you.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 34604 - 34622, getModelTransformers currently flattens every non-"custom" provider config into fallbackProviderConfigs which can erroneously apply other providers' transformers to unknown custom endpoints; change the fallback behavior so that when providerId === "custom" you only merge entries from a curated whitelist of trusted providers (e.g., an array like curatedProviders = ["openai","anthropic","openrouter","glm"] used to filter Object.entries(config)) instead of all providers, and add a short comment above getModelTransformers explaining why custom fallbacks are restricted to that whitelist (reference symbols: getModelTransformers, providerId, config, fallbackProviderConfigs, matchesPattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 33898-34452: The bundled index.cjs (contains ProviderRouter,
ProviderContext, TransformerRegistry, transform/ reverseTransform functions,
etc.) is committed and will always trip the >1MB CI size guard; stop committing
generated artifacts and fix the source so the bundle is produced at build time:
remove index.cjs from git (or move it out), add it to .gitignore, and ensure
bundle-proxy.mjs/esbuild regenerates it during vscode:prepublish or install;
additionally update CI (check-binaries) either to whitelist this file if you
must keep it or, preferably, remove the file from the repo and rely on the build
step so changes to source files like ProviderRouter,
normalizeMixedProviderConfig, TransformerRegistry and transformer modules
produce an updated bundle automatically.
- Around line 35540-35549: Streaming SSE path currently forwards chunks without
applying reverse transformers; ensure streaming clients get the same transformed
events as buffered responses by invoking the reverse-transform pipeline
per-event. Inside the SSE/streaming loop (the code that iterates and writes SSE
chunks), call applyReverseTransformers with the same transformerConfigs and
transformerRegistry (passing the per-event payload as { content: [...] } or
equivalent) and replace the raw event content with transformedResponse.content
before emitting; also ensure the enhancetool/reasoning handlers are reachable
via transformerConfigs so their logic (reasoning -> thinking_delta/thinking)
runs for each streamed chunk. Validate Array.isArray handling like the buffered
path (use transformedResponse.content when array) and fall back to the original
chunk only if no transformers apply.
In `@extensions/thronekeeper/src/views/PanelViewProvider.ts`:
- Around line 2141-2148: The close controls (elements with class "tab-close"
inside the provider tab buttons like elements with ids providerTab-1 and
providerTab-2) are currently spans and not keyboard-focusable/activatable;
replace each <span class="tab-close" data-close="N"> with a proper <button>
element (or an element with role="button" and tabindex="0") that includes an
accessible label (aria-label="Remove provider N" or similar) and preserves the
existing data-close attribute and styles, and ensure any click handler currently
bound to ".tab-close" continues to work with the new button (update event
listeners if they target spans specifically).
- Around line 2057-2094: The enabled mixed-provider path allows empty strings
because buildTier() defaults baseUrl/model/displayModel to '' so
MixedProviderConfigSchema can't reject them; update validation by (A) changing
buildTier to avoid defaulting required strings to empty (only default providerId
from fallbackTier) so missing fields are undefined, and (B) update the
MixedProviderConfigSchema in extensions/claude-throne/src/schemas to use
z.string().min(1) (or equivalent non-empty string validators) for baseUrl,
model, and displayModel, then run
MixedProviderConfigSchema.safeParse(mixedConfig) as before in
handleSaveMixedProviders to reject empty values.
In `@extensions/thronekeeper/tests/vscode-shim.ts`:
- Around line 73-84: The EventEmitter shim's event currently returns undefined
and should be implemented as an Event<T> that accepts a listener (and optional
thisArg or options) and returns a Disposable so callers can push the result into
disposables; update EventEmitter.event to register the listener in an internal
listeners array, return an object with dispose() that removes that listener, and
ensure fire(value?: T) iterates the listeners and invokes them; keep
EventEmitter.dispose to clear listeners. Reference EventEmitter.event,
EventEmitter.fire and EventEmitter.dispose when making the changes.
In `@extensions/thronekeeper/webview/main.js`:
- Around line 394-400: The OpenAI provider metadata in the openai object sets
baseUrl to "https://api.openai.com/v1", which causes
ProviderContext.getUpstreamUrl() to produce duplicate "/v1/v1/..." when it
appends "/v1/chat/completions"; update the openai.baseUrl value in
extensions/thronekeeper/webview/main.js (the openai object) to the root API host
"https://api.openai.com" (i.e., drop the trailing "/v1") so
ProviderContext.getUpstreamUrl() constructs correct upstream URLs.
- Around line 3502-3531: The removeTab function hides a provider tab but doesn’t
clear any entries in mixedTierAssignments that reference the removed tab ID, so
update removeTab (the function that manipulates mixedTabState and calls
saveMixedState) to iterate mixedTierAssignments and remove or reset any
assignment values equal to the removed tabId (and similarly clear any related
mixedValueAssignments if present), then call saveMixedState; ensure you
reference mixedTabState.providerIds and the removed tabId when
filtering/resetting those assignment entries so no stale assignments are
serialized after a tab is removed.
In `@provider-router.js`:
- Around line 225-247: The current check throws whenever a model already exists
on a different tier, even if it points to the exact same upstream context;
change the guard so we only throw when the existing entry points to a different
upstream context AND the tier differs. Concretely, update the condition that
uses existing.context, providerId, baseUrl, endpointKind, model and
existing.tier to: throw only if (!sameContext && existing.tier !== tier). Allow
the code to continue (and set this.tierMap for the new tier) when sameContext is
true across multiple tiers, and avoid overwriting this.normalizedTierMap in that
case so the original entry remains authoritative.
In `@tests/phase4-hydration.test.js`:
- Around line 55-56: Remove the explicit ".ts" extension from the dynamic import
(import '../extensions/thronekeeper/src/views/PanelViewProvider' and then assign
PanelViewProvider from the module) and rename every test state property and
variable named codingModel to completionModel (all places where state objects or
mocks currently use codingModel should become completionModel) so storage ops
use the canonical "completion" key; also add an afterEach hook that deletes
global.vscode (e.g., afterEach(() => { delete global.vscode })) to prevent mock
leakage between tests.
In `@vitest.config.js`:
- Line 7: Replace the hardcoded fileURLToPath(new
URL('./node_modules/undici/index.js', import.meta.url)) alias for 'undici' with
Node's resolution via require.resolve to honor undici's package exports (use
require.resolve('undici') inside the vitest config), and if the original reason
was selecting the Node build variant, add a resolver option like
resolve.conditions: ['node'] in the Vitest/Rollup/Vite resolve config; update
the alias key 'undici' accordingly and remove the brittle fileURLToPath
construction.
---
Outside diff comments:
In `@extensions/thronekeeper/src/views/PanelViewProvider.ts`:
- Around line 1351-1597: The startup currently blocks if provider-specific
legacy globals (reasoningModel/completionModel/valueModel) are missing even when
a validated mixedProviders config exists; update handleStartProxy to treat
mixedProviders as a valid source for models by (1) extracting/assigning
reasoningModel, completionModel, and valueModel from mixedProviders when
mixedProviders is non-null before performing the missing-model guards and
stale-model checks, (2) using those mixed-derived models when calling
hydrateGlobalKeysFromProvider and proxy.start (proxy.start already receives
mixedProviders), and (3) ensuring any config save path atomically writes both
global keys and modelSelectionsByProvider together so a validated mixedProviders
config never gets blocked by legacy missing keys (referencing mixedProviders,
reasoningModel, completionModel, valueModel, hydrateGlobalKeysFromProvider, and
proxy.start).
- Around line 331-332: The call to handleSetModelFromList is passing only
positional args and dropping msg.provider; change the invocation to pass the
full message object (the entire msg) so handleSetModelFromList can read
msg.provider and the atomic model structure { reasoning, completion, value } and
trigger the payload handling in handleSetModelFromList (the code at lines
~1869-1874). Update any other similar calls (see the other invocation region
around lines ~1865-1874) to pass the full msg instead of individual fields to
ensure per-provider selections are preserved.
In `@tests/phase4-hydration.test.js`:
- Around line 380-410: Replace the retired "coding" key in the test fixtures
with the canonical "completion" name: rename any use of state.codingModel to
state.completionModel and update places that assign it into modelsByProvider
(e.g., state.modelsByProvider[previousProvider].completion =
state.completionModel) and any local variables in the same test block (including
the other "Comment 4" describe block). Keep the postMessage payload using the
completion property (providerId, reasoning, completion, value) unchanged but
source it from state.completionModel so the test uses the canonical completion
slot everywhere.
---
Duplicate comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 35078-35090: When routedContext is present the handler still falls
back to the module-level endpointKind via effectiveEndpointKind =
routedContext?.endpointKind || endpointKind, which allows an unresolved "auto"
ProviderContext to leak its undefined kind; fix this by making ProviderContext
always produce a concrete kind: in ProviderContext (the
constructor/initialization that currently branches on !== "auto" and calls
inferEndpointKindSync), assign this.endpointKind to the result of
inferEndpointKindSync() and if that returns falsy throw a clear error (or set a
safe explicit default) so inferEndpointKindSync cannot leave this.endpointKind
undefined and the handler won’t silently use the module-level endpointKind.
In `@extensions/thronekeeper/src/views/PanelViewProvider.ts`:
- Around line 581-698: handleListModels currently uses a single global
currentSequenceToken which causes cross-provider races when multiple tabs
request models; change to a per-provider sequence token map (e.g.,
this.sequenceTokenByProvider) and a per-provider counter (e.g.,
this.sequenceCounterByProvider) so tokens are generated, stored and compared per
provider; update the token generation/assignment logic in handleListModels
(where sequenceToken, requestToken, sequenceTokenCounter and
this.currentSequenceToken are handled) to read/write the per-provider entries
keyed by provider, and change the late-response check to compare the parsed
responseSequenceNum against the provider-specific current token rather than
this.currentSequenceToken; keep modelsCache keyed by provider as used and ensure
you use sequenceToken (not requestToken) in post payloads.
---
Nitpick comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 34604-34622: getModelTransformers currently flattens every
non-"custom" provider config into fallbackProviderConfigs which can erroneously
apply other providers' transformers to unknown custom endpoints; change the
fallback behavior so that when providerId === "custom" you only merge entries
from a curated whitelist of trusted providers (e.g., an array like
curatedProviders = ["openai","anthropic","openrouter","glm"] used to filter
Object.entries(config)) instead of all providers, and add a short comment above
getModelTransformers explaining why custom fallbacks are restricted to that
whitelist (reference symbols: getModelTransformers, providerId, config,
fallbackProviderConfigs, matchesPattern).
In `@extensions/thronekeeper/tests/vscode-shim.ts`:
- Line 71: The test shim for class TreeItem currently drops constructor args;
update the TreeItem class to define a constructor like constructor(label?,
collapsibleState?) and assign the values to instance properties (this.label and
this.collapsibleState) so tests and subclasses that call super(label, state)
receive the same fields as real VS Code TreeItem; keep parameter types
permissive (optional) to match usage in tests and preserve backwards
compatibility.
In `@tests/models.list.test.ts`:
- Around line 116-136: The skipped test 'classifies timeout when request exceeds
per-request timeout' leaves the AbortController timeout path untested for
listModels; fix by either (A) switching this test to real timers (remove
vi.useFakeTimers()) and inject a tiny per-request timeout argument into
listModels so the request naturally aborts quickly, or (B) keep fake timers but
avoid relying on timer-driven aborts by directly invoking the abort listener on
the mocked request signal (in mockRequest's implementation access opts.signal
and call opts.signal.dispatchEvent(new Event('abort')) or call the attached
listener) to simulate AbortError; update the mockRequest implementation used in
this test to reject with an AbortError shape (name='AbortError',
code='UND_ERR_ABORTED') and then re-enable the it (remove .skip).
In `@tests/phase4-hydration.test.js`:
- Around line 370-378: The test assigns a stub to global.vscode (and uses
postMessageCalls) without restoring it, which leaks state across tests; wrap the
assignment in a setup/teardown pair by saving the original global.vscode before
replacing it and restoring (or deleting) it in an afterEach/teardown hook so
other tests don't inherit the stub — locate the global.vscode assignment and
postMessageCalls usage in the test and add an afterEach that restores
originalGlobalVscode (or deletes global.vscode if undefined originally) and
clears postMessageCalls.
In `@tests/phase5-ui-optimization.test.js`:
- Around line 210-240: The test currently calls vi.useFakeTimers() but only
restores with vi.useRealTimers() at the end, which leaks fake timers if an
assertion throws; wrap the timer-handling in a try/finally so vi.useRealTimers()
always runs (i.e., call vi.useFakeTimers() before the try, run the test body
including debouncedRender calls and await vi.advanceTimersByTimeAsync(200)
inside the try, and call vi.useRealTimers() in the finally block). Reference the
existing uses of vi.useFakeTimers, vi.useRealTimers, debounce, debouncedRender
and renderCalls when making the change.
In `@tests/provider-router.test.js`:
- Around line 1-6: Add a new unit test in tests/provider-router.test.js that
exercises provider-aware model-loading token isolation: use ProviderContext and
ProviderRouter to simulate two distinct providers/tabs initiating model loads
with different tokens, resolve the second provider's async load first and then
resolve the first (stale) one out-of-order, and assert that the stale
response/error with a mismatched token/provider is ignored (does not overwrite
the second provider's state or render). Ensure the test creates controllable
async promises for model resolution, wires them through the same loading route
used by createRouterFromEnv/ProviderRouter, and verifies final UI/state only
reflects the most recent matching token/provider.
In `@tests/utils.js`:
- Around line 89-97: The SSE test helper writes headers with res.writeHead and
always sets 'content-type' to 'text/event-stream' even when statusCode is
non-2xx; change the error path so when statusCode < 200 || statusCode >= 300 you
set the response Content-Type to 'application/json' before calling
res.end(JSON.stringify(...)) so error consumers parse JSON correctly (adjust the
headers passed to res.writeHead or call res.setHeader on the error branch using
the same res object).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dd4e054-ee92-4a83-852e-c96bfbcf735f
⛔ Files ignored due to path filters (5)
extensions/thronekeeper/out/extension.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/Models.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/ProxyManager.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/views/PanelViewProvider.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
.gitignoredocs/mixed-providers.mdextensions/thronekeeper/.vscodeignoreextensions/thronekeeper/bundled/proxy/index.cjsextensions/thronekeeper/out/extension.jsextensions/thronekeeper/out/services/Models.jsextensions/thronekeeper/out/services/ProxyManager.jsextensions/thronekeeper/out/views/PanelViewProvider.jsextensions/thronekeeper/package.jsonextensions/thronekeeper/src/extension.tsextensions/thronekeeper/src/services/Models.tsextensions/thronekeeper/src/services/ProxyManager.tsextensions/thronekeeper/src/views/PanelViewProvider.tsextensions/thronekeeper/tests/integration.test.tsextensions/thronekeeper/tests/vscode-shim.tsextensions/thronekeeper/tsconfig.jsonextensions/thronekeeper/vitest.config.tsextensions/thronekeeper/vitest.integration.config.tsextensions/thronekeeper/webview/main.jsindex.jskey-resolver.jsmixed-presets.jsonpackage.jsonprovider-router.jstests/contracts.messages.schema.test.jstests/endpoints.models.test.tstests/messages.contract.test.jstests/messages.error.test.jstests/messages.transformers.integration.test.jstests/model-selection.test.jstests/models.list.test.tstests/phase4-hydration.test.jstests/phase5-ui-optimization.test.jstests/provider-router.test.jstests/utils.jstsconfig.jsonvitest.config.js
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- extensions/thronekeeper/tsconfig.json
- extensions/thronekeeper/vitest.config.ts
- extensions/thronekeeper/tests/integration.test.ts
- package.json
- extensions/thronekeeper/vitest.integration.config.ts
- extensions/thronekeeper/.vscodeignore
- tsconfig.json
- mixed-presets.json
- docs/mixed-providers.md
🚧 Files skipped from review as they are similar to previous changes (7)
- extensions/thronekeeper/package.json
- key-resolver.js
- extensions/thronekeeper/src/services/Models.ts
- extensions/thronekeeper/src/extension.ts
- extensions/thronekeeper/src/services/ProxyManager.ts
- extensions/thronekeeper/out/extension.js
- index.js
Comment on lines
+33898
to
+34452
| // ../../provider-router.js | ||
| var MIXED_TIERS = ["reasoning", "completion", "value"]; | ||
| function normalizeEndpointKind(endpointKind2) { | ||
| const kind = String(endpointKind2 || "auto").toLowerCase(); | ||
| if (kind === "anthropic" || kind === "anthropic-native") return "anthropic"; | ||
| if (kind === "openai" || kind === "openai-compatible") return "openai"; | ||
| return "auto"; | ||
| } | ||
| function normalizeMixedProviderConfig(input) { | ||
| const source = { ...input }; | ||
| if (!source.completion && source.coding) { | ||
| source.completion = source.coding; | ||
| } | ||
| delete source.coding; | ||
| if (source.enabled === false) { | ||
| return { enabled: false }; | ||
| } | ||
| const errors = []; | ||
| const normalized = { enabled: source.enabled !== false }; | ||
| for (const tier of MIXED_TIERS) { | ||
| const binding = source[tier]; | ||
| if (!binding || typeof binding !== "object" || Array.isArray(binding)) { | ||
| errors.push(`${tier} must be an object`); | ||
| continue; | ||
| } | ||
| const providerId = typeof binding.providerId === "string" ? binding.providerId.trim() : ""; | ||
| const baseUrl2 = typeof binding.baseUrl === "string" ? binding.baseUrl.trim() : ""; | ||
| const model2 = typeof binding.model === "string" ? binding.model.trim() : ""; | ||
| if (!providerId) errors.push(`${tier}.providerId must be a non-empty string`); | ||
| if (!baseUrl2) errors.push(`${tier}.baseUrl must be a non-empty string`); | ||
| if (!model2) errors.push(`${tier}.model must be a non-empty string`); | ||
| normalized[tier] = { | ||
| ...binding, | ||
| providerId, | ||
| baseUrl: baseUrl2, | ||
| model: model2, | ||
| endpointKind: normalizeEndpointKind(binding.endpointKind) | ||
| }; | ||
| } | ||
| return errors.length > 0 ? { errors } : normalized; | ||
| } | ||
| var ProviderContext = class { | ||
| /** | ||
| * @param {object} config | ||
| * @param {string} config.providerId - Provider identifier (e.g., 'glm', 'minimax', 'kimi', 'custom') | ||
| * @param {string} config.baseUrl - Upstream base URL (e.g., 'https://api.z.ai/api/anthropic') | ||
| * @param {string} config.key - API key for this provider | ||
| * @param {string} config.model - Model name at the upstream provider (without namespace prefix) | ||
| * @param {string} config.tier - Which tier this context serves: 'reasoning', 'completion', or 'value' | ||
| * @param {string} [config.endpointKind] - Override endpoint kind; auto-detected if omitted | ||
| * @param {Object<string,string>} [config.endpointOverrides] - Optional endpoint kind overrides map | ||
| */ | ||
| constructor({ | ||
| providerId, | ||
| baseUrl: baseUrl2, | ||
| key: key2, | ||
| model: model2, | ||
| tier, | ||
| endpointKind: endpointKind2, | ||
| endpointOverrides = {} | ||
| }) { | ||
| this.providerId = providerId; | ||
| this.baseUrl = (baseUrl2 || "").replace(/\/+$/, ""); | ||
| this.key = key2; | ||
| this.model = model2; | ||
| this.tier = tier; | ||
| const normalizedEndpointKind = normalizeEndpointKind(endpointKind2); | ||
| if (normalizedEndpointKind !== "auto") { | ||
| if (normalizedEndpointKind === "anthropic") { | ||
| this.endpointKind = ENDPOINT_KIND.ANTHROPIC_NATIVE; | ||
| } else if (normalizedEndpointKind === "openai") { | ||
| this.endpointKind = ENDPOINT_KIND.OPENAI_COMPATIBLE; | ||
| } | ||
| } else { | ||
| this.endpointKind = inferEndpointKindSync(providerId, baseUrl2, endpointOverrides); | ||
| } | ||
| } | ||
| /** | ||
| * Whether this provider uses Anthropic-native protocol. | ||
| * @returns {boolean} | ||
| */ | ||
| isAnthropicNative() { | ||
| return this.endpointKind === ENDPOINT_KIND.ANTHROPIC_NATIVE; | ||
| } | ||
| /** | ||
| * Build the correct upstream request URL for this provider. | ||
| * @returns {string} Full URL for the API endpoint | ||
| */ | ||
| getUpstreamUrl() { | ||
| if (this.isAnthropicNative()) { | ||
| return `${this.baseUrl}/v1/messages`; | ||
| } | ||
| return `${this.baseUrl}/v1/chat/completions`; | ||
| } | ||
| /** | ||
| * Build the correct authentication and content-type headers for this provider. | ||
| * @returns {Object<string,string>} Headers object ready for fetch() | ||
| */ | ||
| getHeaders() { | ||
| const headers = { | ||
| "Content-Type": "application/json", | ||
| ...providerSpecificHeaders(this.providerId) | ||
| }; | ||
| if (!this.key) { | ||
| return headers; | ||
| } | ||
| if (this.isAnthropicNative()) { | ||
| headers["x-api-key"] = this.key; | ||
| headers["anthropic-version"] = process.env.ANTHROPIC_VERSION || "2023-06-01"; | ||
| if (process.env.ANTHROPIC_BETA) { | ||
| headers["anthropic-beta"] = process.env.ANTHROPIC_BETA; | ||
| } | ||
| } else { | ||
| headers["Authorization"] = `Bearer ${this.key}`; | ||
| } | ||
| return headers; | ||
| } | ||
| /** | ||
| * Returns a safe (redacted) representation for logging. | ||
| * @returns {object} | ||
| */ | ||
| toDebugObject() { | ||
| return { | ||
| providerId: this.providerId, | ||
| baseUrl: this.baseUrl, | ||
| model: this.model, | ||
| tier: this.tier, | ||
| endpointKind: this.endpointKind, | ||
| hasKey: !!this.key | ||
| }; | ||
| } | ||
| }; | ||
| var ProviderRouter = class { | ||
| /** | ||
| * @param {object} config - Parsed MIXED_PROVIDERS_CONFIG | ||
| * @param {object} config.reasoning - { providerId, baseUrl, key, model, endpointKind? } | ||
| * @param {object} config.completion - { providerId, baseUrl, key, model, endpointKind? } | ||
| * @param {object} config.value - { providerId, baseUrl, key, model, endpointKind? } | ||
| * @param {Object<string,string>} [endpointOverrides={}] - Optional endpoint kind overrides | ||
| */ | ||
| constructor(config, endpointOverrides = {}) { | ||
| if (!config || !config.reasoning || !config.completion || !config.value) { | ||
| throw new Error( | ||
| "[ProviderRouter] Invalid config: must include reasoning, completion, and value tiers" | ||
| ); | ||
| } | ||
| const validation = normalizeMixedProviderConfig(config); | ||
| if (validation.errors) { | ||
| throw new Error(`[ProviderRouter] Invalid config: ${validation.errors.join("; ")}`); | ||
| } | ||
| this.contexts = { | ||
| reasoning: new ProviderContext({ | ||
| ...validation.reasoning, | ||
| tier: "reasoning", | ||
| endpointOverrides | ||
| }), | ||
| completion: new ProviderContext({ | ||
| ...validation.completion, | ||
| tier: "completion", | ||
| endpointOverrides | ||
| }), | ||
| value: new ProviderContext({ | ||
| ...validation.value, | ||
| tier: "value", | ||
| endpointOverrides | ||
| }) | ||
| }; | ||
| this.tierMap = /* @__PURE__ */ new Map(); | ||
| this.normalizedTierMap = /* @__PURE__ */ new Map(); | ||
| for (const [tier, ctx] of Object.entries(this.contexts)) { | ||
| if (ctx.model) { | ||
| const normalizedModel = ctx.model.toLowerCase(); | ||
| const existing = this.normalizedTierMap.get(normalizedModel); | ||
| if (existing) { | ||
| const existingContext = existing.context; | ||
| const sameContext = existingContext.providerId === ctx.providerId && existingContext.baseUrl === ctx.baseUrl && existingContext.endpointKind === ctx.endpointKind && existingContext.model === ctx.model; | ||
| if (!sameContext || existing.tier !== tier) { | ||
| throw new Error( | ||
| `[ProviderRouter] Model "${ctx.model}" is assigned to multiple mixed-provider tiers (${existing.tier}, ${tier})` | ||
| ); | ||
| } | ||
| } | ||
| const entry = { tier, context: ctx }; | ||
| this.tierMap.set(ctx.model, entry); | ||
| this.normalizedTierMap.set(normalizedModel, entry); | ||
| } | ||
| } | ||
| } | ||
| /** | ||
| * Given a model name from an incoming request, resolve which ProviderContext to use. | ||
| * | ||
| * @param {string} modelName - The model name from payload.model | ||
| * @returns {{ tier: string, context: ProviderContext } | null} Resolved context, or null if not found | ||
| */ | ||
| resolve(modelName) { | ||
| if (!modelName) return null; | ||
| const exact = this.tierMap.get(modelName); | ||
| if (exact) return exact; | ||
| return this.normalizedTierMap.get(modelName.toLowerCase()) || null; | ||
| } | ||
| /** | ||
| * Get the ProviderContext for a specific tier directly. | ||
| * | ||
| * @param {'reasoning' | 'completion' | 'value'} tier | ||
| * @returns {ProviderContext} | ||
| */ | ||
| getContextForTier(tier) { | ||
| return this.contexts[tier] || null; | ||
| } | ||
| /** | ||
| * Smart key validation: check that every unique provider ID has a key stored. | ||
| * If two tiers share the same provider, only one key is needed. | ||
| * | ||
| * @returns {{ valid: boolean, missing: string[], uniqueProviders: string[] }} | ||
| */ | ||
| validate() { | ||
| const uniqueProviders = /* @__PURE__ */ new Set(); | ||
| const providerToKey = /* @__PURE__ */ new Map(); | ||
| for (const ctx of Object.values(this.contexts)) { | ||
| uniqueProviders.add(ctx.providerId); | ||
| if (ctx.key) { | ||
| providerToKey.set(ctx.providerId, true); | ||
| } | ||
| } | ||
| const missing = []; | ||
| for (const pid of uniqueProviders) { | ||
| if (!providerToKey.has(pid)) { | ||
| missing.push(pid); | ||
| } | ||
| } | ||
| return { | ||
| valid: missing.length === 0, | ||
| missing, | ||
| uniqueProviders: [...uniqueProviders] | ||
| }; | ||
| } | ||
| /** | ||
| * Returns a safe (redacted) representation of the full router state for logging. | ||
| * @returns {object} | ||
| */ | ||
| toDebugObject() { | ||
| return { | ||
| reasoning: this.contexts.reasoning.toDebugObject(), | ||
| completion: this.contexts.completion.toDebugObject(), | ||
| value: this.contexts.value.toDebugObject(), | ||
| tierMapSize: this.tierMap.size, | ||
| models: [...this.tierMap.keys()] | ||
| }; | ||
| } | ||
| }; | ||
| function createRouterFromEnv(env = process.env, endpointOverrides = {}) { | ||
| const raw = env.MIXED_PROVIDERS_CONFIG; | ||
| if (!raw) return null; | ||
| try { | ||
| const parsed = JSON.parse(raw); | ||
| const config = normalizeMixedProviderConfig(parsed); | ||
| if (config.enabled === false) { | ||
| console.log("[ProviderRouter] Mixed provider config disabled; using single-provider mode"); | ||
| return null; | ||
| } | ||
| if (config.errors) { | ||
| console.error( | ||
| `[ProviderRouter] Invalid MIXED_PROVIDERS_CONFIG: ${config.errors.join("; ")}` | ||
| ); | ||
| return null; | ||
| } | ||
| const router2 = new ProviderRouter(config, endpointOverrides); | ||
| const validation = router2.validate(); | ||
| if (!validation.valid) { | ||
| console.error( | ||
| `[ProviderRouter] Missing API keys for providers: ${validation.missing.join(", ")}` | ||
| ); | ||
| console.error( | ||
| "[ProviderRouter] Mixed provider mode requires stored keys for all unique providers." | ||
| ); | ||
| } | ||
| if (env.DEBUG || env.debug) { | ||
| console.log("[ProviderRouter] Initialized:", JSON.stringify(router2.toDebugObject(), null, 2)); | ||
| } else { | ||
| console.log( | ||
| `[ProviderRouter] Mixed mode active: ${validation.uniqueProviders.length} unique providers, models: [${[...router2.tierMap.keys()].join(", ")}]` | ||
| ); | ||
| } | ||
| return router2; | ||
| } catch (err) { | ||
| console.warn( | ||
| "[ProviderRouter] Failed to parse MIXED_PROVIDERS_CONFIG, falling back to single-provider mode:", | ||
| err.message | ||
| ); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| // ../../transformers.js | ||
| var TransformerRegistry = class { | ||
| constructor() { | ||
| this.transformers = /* @__PURE__ */ new Map(); | ||
| } | ||
| /** | ||
| * Register a transformer by name | ||
| * @param {string} name - Transformer name | ||
| * @param {object} transformer - Object with transform/reverseTransform functions | ||
| */ | ||
| register(name, transformer) { | ||
| if (!transformer.transform || typeof transformer.transform !== "function") { | ||
| throw new Error(`Transformer ${name} must export a transform function`); | ||
| } | ||
| if (!transformer.reverseTransform || typeof transformer.reverseTransform !== "function") { | ||
| throw new Error(`Transformer ${name} must export a reverseTransform function`); | ||
| } | ||
| this.transformers.set(name, transformer); | ||
| } | ||
| /** | ||
| * Get a transformer by name | ||
| * @param {string} name - Transformer name | ||
| * @returns {object|null} Transformer object or null if not found | ||
| */ | ||
| get(name) { | ||
| return this.transformers.get(name) || null; | ||
| } | ||
| /** | ||
| * Check if a transformer is registered | ||
| * @param {string} name - Transformer name | ||
| * @returns {boolean} | ||
| */ | ||
| has(name) { | ||
| return this.transformers.has(name); | ||
| } | ||
| }; | ||
| async function applyTransformers(transformerConfigs, data, registry) { | ||
| let result = data; | ||
| for (const config of transformerConfigs) { | ||
| const [name, options] = Array.isArray(config) ? config : [config, {}]; | ||
| const transformer = registry.get(name); | ||
| if (!transformer) { | ||
| console.error(`[Transformer] Transformer '${name}' not found in registry, skipping`); | ||
| continue; | ||
| } | ||
| try { | ||
| result = await transformer.transform(result, options); | ||
| } catch (error) { | ||
| console.error(`[Transformer] Error in ${name}.transform():`, error); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
| async function applyReverseTransformers(transformerConfigs, data, registry) { | ||
| let result = data; | ||
| for (let i = transformerConfigs.length - 1; i >= 0; i--) { | ||
| const config = transformerConfigs[i]; | ||
| const [name, options] = Array.isArray(config) ? config : [config, {}]; | ||
| const transformer = registry.get(name); | ||
| if (!transformer) { | ||
| console.error(`[Transformer] Transformer '${name}' not found in registry, skipping`); | ||
| continue; | ||
| } | ||
| try { | ||
| result = await transformer.reverseTransform(result, options); | ||
| } catch (error) { | ||
| console.error(`[Transformer] Error in ${name}.reverseTransform():`, error); | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| // ../../transformers/tooluse.js | ||
| async function transform(request, options = {}) { | ||
| if (request.tools && Array.isArray(request.tools) && request.tools.length > 0) { | ||
| if (!request.tool_choice) { | ||
| request.tool_choice = { type: "auto" }; | ||
| } | ||
| } | ||
| return request; | ||
| } | ||
| async function reverseTransform(response, options = {}) { | ||
| return response; | ||
| } | ||
| var tooluse_default = { | ||
| transform, | ||
| reverseTransform | ||
| }; | ||
|
|
||
| // ../../transformers/reasoning.js | ||
| async function transform2(request, options = {}) { | ||
| return request; | ||
| } | ||
| async function reverseTransform2(response, options = {}) { | ||
| if (response.type === "content_block_delta") { | ||
| if (response.delta?.reasoning) { | ||
| const { reasoning, ...restDelta } = response.delta; | ||
| return { | ||
| type: "content_block_delta", | ||
| index: response.index, | ||
| delta: { | ||
| ...restDelta, | ||
| type: "thinking_delta", | ||
| thinking: reasoning | ||
| } | ||
| }; | ||
| } | ||
| if (response.delta?.reasoning_content) { | ||
| const { reasoning_content, ...restDelta } = response.delta; | ||
| return { | ||
| type: "content_block_delta", | ||
| index: response.index, | ||
| delta: { | ||
| ...restDelta, | ||
| type: "thinking_delta", | ||
| thinking: reasoning_content | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| if (response.type === "content_block_start") { | ||
| if (response.content_block?.reasoning) { | ||
| const { reasoning, ...restBlock } = response.content_block; | ||
| return { | ||
| type: "content_block_start", | ||
| index: response.index, | ||
| content_block: { | ||
| ...restBlock, | ||
| type: "thinking", | ||
| thinking: reasoning | ||
| } | ||
| }; | ||
| } | ||
| if (response.content_block?.reasoning_content) { | ||
| const { reasoning_content, ...restBlock } = response.content_block; | ||
| return { | ||
| type: "content_block_start", | ||
| index: response.index, | ||
| content_block: { | ||
| ...restBlock, | ||
| type: "thinking", | ||
| thinking: reasoning_content | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| if (response.content && Array.isArray(response.content)) { | ||
| response.content = response.content.map((block) => { | ||
| if (block.reasoning) { | ||
| const { reasoning, ...rest } = block; | ||
| return { | ||
| ...rest, | ||
| type: "thinking", | ||
| thinking: reasoning | ||
| }; | ||
| } | ||
| if (block.reasoning_content) { | ||
| const { reasoning_content, ...rest } = block; | ||
| return { | ||
| ...rest, | ||
| type: "thinking", | ||
| thinking: reasoning_content | ||
| }; | ||
| } | ||
| return block; | ||
| }); | ||
| } | ||
| return response; | ||
| } | ||
| var reasoning_default = { | ||
| transform: transform2, | ||
| reverseTransform: reverseTransform2 | ||
| }; | ||
|
|
||
| // ../../transformers/maxtoken.js | ||
| async function transform3(request, options = {}) { | ||
| const configuredMaxTokens = options.max_tokens; | ||
| if (configuredMaxTokens) { | ||
| if (!request.max_tokens) { | ||
| request.max_tokens = configuredMaxTokens; | ||
| } else { | ||
| if (request.max_tokens > configuredMaxTokens) { | ||
| request.max_tokens = configuredMaxTokens; | ||
| } | ||
| } | ||
| } | ||
| return request; | ||
| } | ||
| async function reverseTransform3(response, options = {}) { | ||
| return response; | ||
| } | ||
| var maxtoken_default = { | ||
| transform: transform3, | ||
| reverseTransform: reverseTransform3 | ||
| }; | ||
|
|
||
| // ../../transformers/enhancetool.js | ||
| async function transform4(request, options = {}) { | ||
| if (request.tools && Array.isArray(request.tools)) { | ||
| request.tools = request.tools.map((tool) => { | ||
| if (!tool.input_schema) { | ||
| tool.input_schema = { | ||
| type: "object", | ||
| properties: {}, | ||
| required: [] | ||
| }; | ||
| } | ||
| if (!tool.input_schema.properties) { | ||
| tool.input_schema.properties = {}; | ||
| } | ||
| if (!Array.isArray(tool.input_schema.required)) { | ||
| tool.input_schema.required = []; | ||
| } | ||
| return tool; | ||
| }); | ||
| } | ||
| return request; | ||
| } | ||
| async function reverseTransform4(response, options = {}) { | ||
| if (response.type === "content_block_delta" && response.delta?.type === "input_json_delta") { | ||
| return response; | ||
| } | ||
| if (response.type === "tool_use" && response.input) { | ||
| try { | ||
| if (typeof response.input === "string") { | ||
| response.input = JSON.parse(response.input); | ||
| } | ||
| if (typeof response.input !== "object" || response.input === null) { | ||
| console.error("[Transformer:enhancetool] Invalid tool input, using empty object:", response.input); | ||
| response.input = {}; | ||
| } | ||
| } catch (error) { | ||
| console.error("[Transformer:enhancetool] Failed to parse tool input:", error); | ||
| response.input = {}; | ||
| } | ||
| return response; | ||
| } | ||
| if (response.content && Array.isArray(response.content)) { | ||
| response.content = response.content.map((block) => { | ||
| if (block.type === "tool_use" && block.input) { | ||
| try { | ||
| if (typeof block.input === "string") { | ||
| block.input = JSON.parse(block.input); | ||
| } | ||
| if (typeof block.input !== "object" || block.input === null) { | ||
| console.error("[Transformer:enhancetool] Invalid tool input, using empty object:", block.input); | ||
| block.input = {}; | ||
| } | ||
| } catch (error) { | ||
| console.error("[Transformer:enhancetool] Failed to parse tool input:", error); | ||
| block.input = {}; | ||
| } | ||
| } | ||
| return block; | ||
| }); | ||
| } | ||
| return response; | ||
| } | ||
| var enhancetool_default = { | ||
| transform: transform4, | ||
| reverseTransform: reverseTransform4 | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm this is the file tripping the size guard and check repo conventions
fd -t f -S +500k
rg -nP 'bundled/proxy/index\.cjs' -g '!bundled/**'Repository: KHAEntertainment/thronekeeper
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Check for CI/GitHub Actions workflows and size checks
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "size\|lfs\|large" 2>/dev/null | head -10Repository: KHAEntertainment/thronekeeper
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check .gitignore for bundled file patterns
cat .gitignore 2>/dev/null | grep -E "bundled|\.cjs|node_modules" || echo "No .gitignore or no matching patterns"Repository: KHAEntertainment/thronekeeper
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Look for recent commits mentioning the bundled file or size issues
git log --oneline --grep="bundled\|index\.cjs\|large\|size" 2>/dev/null | head -20 || echo "Git log not available"Repository: KHAEntertainment/thronekeeper
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check if there are build/package.json scripts that should generate this
fd -t f "package\.json" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -A10 "scripts" {} | grep -E "build|bundle|bundle-cjs"' 2>/dev/nullRepository: KHAEntertainment/thronekeeper
Length of output: 337
🏁 Script executed:
#!/bin/bash
# Check what the regression workflow actually tests for
cat .github/workflows/regression.ymlRepository: KHAEntertainment/thronekeeper
Length of output: 4215
🏁 Script executed:
#!/bin/bash
# Check the bundle script to understand build process
cat extensions/thronekeeper/scripts/bundle-proxy.mjs 2>/dev/null | head -50Repository: KHAEntertainment/thronekeeper
Length of output: 995
🏁 Script executed:
#!/bin/bash
# Check git status - is the bundled file tracked?
git ls-files | grep "bundled/proxy/index.cjs"Repository: KHAEntertainment/thronekeeper
Length of output: 120
Bundled artifact is tracked in git and will fail the size guard — fix upstream and consider build-time generation.
This 1.3MB index.cjs is generated by bundle-proxy.mjs (via esbuild) during vscode:prepublish, but it's tracked in git. The check-binaries CI job already flags files >1MB in diffs, so this will fail on every PR touching the source. Fix the underlying issue in the source files (index.js, provider-router.js, transformers.js, etc.) since the bundle regenerates automatically. Then choose one approach: (a) remove the bundled file from git and generate it at build/install time, (b) add it to .gitignore and handle distribution separately, or (c) whitelist it in the CI check if it must stay committed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 33898 - 34452,
The bundled index.cjs (contains ProviderRouter, ProviderContext,
TransformerRegistry, transform/ reverseTransform functions, etc.) is committed
and will always trip the >1MB CI size guard; stop committing generated artifacts
and fix the source so the bundle is produced at build time: remove index.cjs
from git (or move it out), add it to .gitignore, and ensure
bundle-proxy.mjs/esbuild regenerates it during vscode:prepublish or install;
additionally update CI (check-binaries) either to whitelist this file if you
must keep it or, preferably, remove the file from the repo and rely on the build
step so changes to source files like ProviderRouter,
normalizeMixedProviderConfig, TransformerRegistry and transformer modules
produce an updated bundle automatically.
Comment on lines
+3502
to
+3531
| function removeTab(tabId) { | ||
| const tabBtn = document.getElementById(`providerTab-${tabId}`); | ||
| if (tabBtn) tabBtn.style.display = 'none'; | ||
|
|
||
| mixedTabState.providerIds[tabId] = ''; | ||
|
|
||
| if (mixedTabState.activeTab === tabId) { | ||
| switchToTab('primary'); | ||
| } | ||
|
|
||
| if (tabId === '1' && mixedTabState.tabCount === 2) { | ||
| mixedTabState.providerIds['1'] = mixedTabState.providerIds['2']; | ||
| mixedTabState.providerIds['2'] = ''; | ||
|
|
||
| const tab2Btn = document.getElementById('providerTab-2'); | ||
| if (tab2Btn) tab2Btn.style.display = 'none'; | ||
|
|
||
| const tab1Btn = document.getElementById('providerTab-1'); | ||
| if (tab1Btn) { | ||
| tab1Btn.style.display = 'inline-block'; | ||
| updateTabLabel('1'); | ||
| } | ||
| } | ||
|
|
||
| mixedTabState.tabCount = Math.max(0, mixedTabState.tabCount - 1); | ||
|
|
||
| const addBtn = document.getElementById('addProviderTabBtn'); | ||
| if (addBtn) addBtn.style.display = 'inline-block'; | ||
|
|
||
| saveMixedState(); |
Contributor
There was a problem hiding this comment.
Clear tier assignments that point to a removed tab.
removeTab() hides the tab but leaves state.mixedTierAssignments untouched. If completion/value was assigned to that provider, saveMixedState() will keep serializing the removed provider — the UI removes the car from the train, but the route map still sends cargo to it.
🐛 Proposed fix
function removeTab(tabId) {
+ const removedProviderId = mixedTabState.providerIds[tabId];
+ const primaryProviderId = mixedTabState.providerIds.primary || state.provider;
const tabBtn = document.getElementById(`providerTab-${tabId}`);
if (tabBtn) tabBtn.style.display = 'none';
mixedTabState.providerIds[tabId] = '';
+ if (removedProviderId) {
+ ['reasoning', 'completion', 'value'].forEach(tier => {
+ if (state.mixedTierAssignments?.[tier] === removedProviderId) {
+ state.mixedTierAssignments[tier] = primaryProviderId;
+ }
+ });
+ }
if (mixedTabState.activeTab === tabId) {
switchToTab('primary');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@extensions/thronekeeper/webview/main.js` around lines 3502 - 3531, The
removeTab function hides a provider tab but doesn’t clear any entries in
mixedTierAssignments that reference the removed tab ID, so update removeTab (the
function that manipulates mixedTabState and calls saveMixedState) to iterate
mixedTierAssignments and remove or reset any assignment values equal to the
removed tabId (and similarly clear any related mixedValueAssignments if
present), then call saveMixedState; ensure you reference
mixedTabState.providerIds and the removed tabId when filtering/resetting those
assignment entries so no stale assignments are serialized after a tab is
removed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/regression.yml (1)
118-123:⚠️ Potential issue | 🟡 MinorThe
--diff-filter=Acheck only catches newly added files, not modified ones that balloon in size.Unlike the VSIX check above it, which catches all file changes, the large file detector here uses
--diff-filter=A, which explicitly filters to Added files only. That means if someone modifies an existing tracked file and grows it from 500KB to 5MB—say, by regenerating a bundled proxy or committing output from a build—it will sail through unchallenged, since--diff-filter=AexcludesM(Modified) and only matchesA(Added).Given the learning that large binaries >1MB should not be committed, consider changing
--diff-filter=Ato--diff-filter=AMto catch both newly added and modified files:Proposed fix: Check both added and modified files
- large_files=$(git diff --name-only --diff-filter=A "origin/${{ github.base_ref }}"...HEAD | xargs -I {} find {} -type f -size +1M 2>/dev/null || true) + large_files=$(git diff --name-only --diff-filter=AM "origin/${{ github.base_ref }}"...HEAD | xargs -I {} find {} -type f -size +1M 2>/dev/null || true)If the narrowing to Added-only was intentional (to tolerate legitimate growth of existing files), a comment in the workflow explaining that rationale would help future maintainers understand the trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/regression.yml around lines 118 - 123, The large file detector currently uses git diff with --diff-filter=A which only catches Added files; update the command that sets large_files (the git diff ... --diff-filter=A ... | xargs ... find ...) to use --diff-filter=AM so it also catches Modified files that grew >1MB, and if the original Added-only behavior was intentional instead add an inline comment above the large_files assignment explaining why Modified files are excluded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/smoke.sh`:
- Around line 16-18: Update the health check and final request curl calls so
HTTP error status codes cause the smoke test to fail: in wait_for_server(),
replace the plain curl that only checks connection with a curl that uses -w
'%{http_code}' (to capture the response code) and verify the code equals "200"
(failing otherwise); similarly add -f to the final curl that posts to
"${BASE_URL}/v1/messages" so it exits non‑zero on HTTP 4xx/5xx. Locate and
update the curl invocations referenced by wait_for_server and the final request
near BASE_URL to use these flags and explicit HTTP-code checks.
---
Outside diff comments:
In @.github/workflows/regression.yml:
- Around line 118-123: The large file detector currently uses git diff with
--diff-filter=A which only catches Added files; update the command that sets
large_files (the git diff ... --diff-filter=A ... | xargs ... find ...) to use
--diff-filter=AM so it also catches Modified files that grew >1MB, and if the
original Added-only behavior was intentional instead add an inline comment above
the large_files assignment explaining why Modified files are excluded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 710a0e42-9290-4a9d-8db9-a2aba542412f
⛔ Files ignored due to path filters (1)
extensions/thronekeeper/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/regression.ymlextensions/thronekeeper/package.jsonscripts/smoke.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- extensions/thronekeeper/package.json
Owner
Author
|
Follow-up review fixes pushed in 80cec66. Addressed the remaining CodeRabbit threads around per-provider request tokens, mixed-provider config validation, concrete endpoint-kind enforcement, streaming transformer reversal, tab accessibility/removal cleanup, OpenAI base URL normalization, VS Code shim behavior, duplicate model ID handling, and Vitest/undici setup. Local verification passed:
GitHub checks are green on 80cec66, CodeRabbit completed successfully, and there are currently 0 unresolved review threads. |
Owner
Author
|
Publisher follow-up pushed in 43c988f. Note for this PR and future VSIX builds: the |
Owner
Author
|
Follow-up fix for the Kimi/MiniMax presets and mixed-provider tab activation is pushed in Implemented:
Verification:
GitHub Actions are green on the new commit; CodeRabbit is still pending review at the time of this comment. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
index.js (3)
861-867:⚠️ Potential issue | 🟠 MajorForward the routed provider-native model, not the request/display model.
Once a request resolves to
routedContext, the upstream payload should useroutedContext.model. Otherwise a namespaced/display model used for routing can be sent to the provider as-is, like forwarding the shipping label instead of the actual SKU.Possible fix
if (isAnthropicNative) { - const anthropicPayload = buildAnthropicPayload(payload) + const anthropicPayload = buildAnthropicPayload({ + ...payload, + model: routedContext?.model || payload.model, + })const responseWarnings = [] - const selectedModel = payload.model + const selectedModel = routedContext?.model || payload.model || (payload.thinking ? models.reasoning : models.completion)Also applies to: 1086-1090
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 861 - 867, The Anthropic upstream payload is built with the original request model instead of the routed provider-native model; when isAnthropicNative is true, ensure buildAnthropicPayload (and the analogous provider payload builders around the other noted block) receive routedContext.model (or swap payload.model to routedContext.model) so the forwarded request uses the provider-native model identifier rather than the display/routing model.
533-571:⚠️ Potential issue | 🟠 MajorBase readiness on the mixed-provider router when it is active.
isReady = !!keystill requires the single-provider global key. In mixed-provider mode, all usable keys can live insideMIXED_PROVIDERS_CONFIG, so/healthreports503 unhealthyeven though routed/v1/messagesrequests can succeed.Possible fix
async function healthCheckHandler(request, reply) { - const isReady = !!key // Proxy is ready when API key is available + const routerValidation = router?.validate() + const isReady = router + ? routerValidation.valid + : !!key // Single-provider mode is ready when the global API key is available const status = isReady ? 'ok' : 'unhealthy'if (!isReady) { healthResponse.missingKey = true - healthResponse.keySourcesTried = KEY_ENV_HINT + healthResponse.keySourcesTried = router + ? `Missing mixed-provider keys: ${routerValidation.missing.join(', ')}` + : KEY_ENV_HINT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 533 - 571, Change readiness logic in healthCheckHandler so it considers mixed-provider routing: replace the simple isReady = !!key with a check that returns true if a global key exists OR the mixed-provider router is active and reports available/healthy providers (e.g., router && router.hasHealthyProviders() or router.isReady()). Use that combined boolean for status and the 503 reply decision. Also, when key is missing include missing-key diagnostics in healthResponse (keep detectionSource, endpointKind, lastProbedAt, baseUrl fields) so downstream consumers can tell whether readiness came from the single-provider key or the router.
1749-1768:⚠️ Potential issue | 🟠 MajorEmit a valid Anthropic SSE delta for reasoning content.
This starts a
textcontent block, then sendsdelta: { reasoning: ... }without atype. Anthropic clients expect typed deltas, so reasoning streams may be ignored or fail parsing. Either convert it to a text delta or start a proper thinking block.Safer text-delta fallback
} else if (delta && (delta.reasoning || delta.reasoning_content)) { if (!textBlockStarted) { textBlockStarted = true await sendContentSSE('content_block_start', { type: 'content_block_start', index: 0, content_block: { type: 'text', text: '' } }) } const reasoningText = delta.reasoning || delta.reasoning_content accumulatedReasoning += reasoningText await sendContentSSE('content_block_delta', { type: 'content_block_delta', index: 0, delta: { - reasoning: reasoningText + type: 'text_delta', + text: reasoningText } }) }As per coding guidelines, streaming uses SSE and should verify event order:
message_start,content_block_*,message_stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.js` around lines 1749 - 1768, The code is sending an untyped reasoning delta (delta.reasoning) which Anthropic SSE clients may ignore; update the sendContentSSE call in the branch that handles delta.reasoning/delta.reasoning_content (referencing sendContentSSE, textBlockStarted, accumulatedReasoning, content_block_start, content_block_delta) to emit a typed delta: either convert the reasoning into a text delta (delta: { type: 'text', text: reasoningText }) so it fits the opened text content_block, or if you intend a thinking block, start a content_block_start with type: 'thinking' and then emit content_block_delta with delta: { type: 'thinking', reasoning: reasoningText }; also ensure events maintain correct streaming order (message_start → content_block_start → content_block_delta → message_stop).extensions/thronekeeper/bundled/proxy/index.cjs (1)
35806-35825:⚠️ Potential issue | 🟠 MajorStart reasoning streams as thinking blocks, not text blocks.
The reverse transformer turns this delta into
thinking_delta, but the block was just opened as{ type: "text" }. Clients then see a text block receiving thinking deltas—right event order, wrong rail gauge.🧭 Keep the block type aligned
if (!textBlockStarted) { textBlockStarted = true; await sendContentSSE("content_block_start", { type: "content_block_start", index: 0, content_block: { - type: "text", - text: "" + type: "thinking", + thinking: "" } }); } const reasoningText = delta.reasoning || delta.reasoning_content; accumulatedReasoning += reasoningText; await sendContentSSE("content_block_delta", { type: "content_block_delta", index: 0, delta: { - reasoning: reasoningText + type: "thinking_delta", + thinking: reasoningText } });Based on learnings: Streaming uses SSE; verify event order:
message_start,content_block_*,message_stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 35806 - 35825, The code opens a text block when receiving reasoning deltas, causing thinking deltas to be applied to a text block; change the branch that handles delta.reasoning or delta.reasoning_content to start a content block with type "thinking" (use the same sendContentSSE("content_block_start", { type: "content_block_start", content_block: { type: "thinking", text: "" } })) instead of "text", and ensure the same start/delta/stop SSE order is preserved; update the boolean used to guard block start (e.g., textBlockStarted) or add a separate thinkingBlockStarted flag so accumulatedReasoning, the use of sendContentSSE for "content_block_delta" and the sequence message_start → content_block_start → content_block_delta → message_stop remain correct.
♻️ Duplicate comments (1)
extensions/thronekeeper/bundled/proxy/index.cjs (1)
34073-34105:⚠️ Potential issue | 🟠 MajorReject same-model duplicates too, or resolve by tier explicitly.
The
sameContextexception still leavespayload.modelas a single mailbox for multiple tiers. Worse,sameContextignoreskey, so two tiers with the same provider/base/model but different tokens silently route through the first context.🛡️ Fail-fast option
const normalizedModel = ctx.model.toLowerCase(); const existing = this.normalizedTierMap.get(normalizedModel); if (existing) { - const existingContext = existing.context; - const sameContext = existingContext.providerId === ctx.providerId && existingContext.baseUrl === ctx.baseUrl && existingContext.endpointKind === ctx.endpointKind && existingContext.model === ctx.model; - if (!sameContext) { - throw new Error( - `[ProviderRouter] Model "${ctx.model}" is assigned to multiple mixed-provider tiers (${existing.tier}, ${tier})` - ); - } - continue; + throw new Error( + `[ProviderRouter] Model "${ctx.model}" is assigned to multiple mixed-provider tiers (${existing.tier}, ${tier})` + ); }If duplicates are intentional, include an explicit tier signal in the request/lookup key instead of resolving by bare model name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 34073 - 34105, The constructor currently allows multiple tiers to register the same model if their providerId/baseUrl/endpointKind/model match (sameContext), which ignores differing auth keys and still leaves payload.model ambiguous; modify the registration and resolution logic in the ProviderRouter: when iterating this.contexts (use symbols: contexts, normalizedTierMap, tierMap, sameContext, entry) treat any duplicate model across different tiers as an error unless the incoming lookup explicitly includes the tier; either (A) reject duplicates unconditionally by throwing if normalizedModel or ctx.model already exists with a different tier, or (B) support tier-qualified lookups by storing entries keyed by `${tier}:${model}` and update resolve(modelName) to parse an optional `tier:` prefix to pick the exact tier entry (fall back to normalized maps only when no tier specified and only one tier exists). Ensure tokens/keys are considered by preventing silent routing when providerId/baseUrl/model match but auth differs.
🧹 Nitpick comments (2)
extensions/thronekeeper/out/views/PanelViewProvider.js (1)
1863-1892:fallbackTieris a half-built life raft.
fallbackTieronly carriesproviderIdandendpointKind, so when a tier is missing on a disabled save,buildTierproduces{ providerId: 'openrouter', baseUrl: undefined, model: undefined, endpointKind: 'auto' }. Schema validation is skipped for disabled configs, so this undefined-ridden object cheerfully lands in VS Code settings. The moment the user flipsenabled=truelater without touching all three tiers again,handleStartProxy's schema check will reject it — not because of the current save, but because of the ghost of a prior partial save.Two reasonable options:
- Only merge provided tiers on disabled saves, leaving the rest untouched (read existing
mixedProvidersand patch in place), or- Require complete tier data even when
enabled=falseand reject partial payloads the same way as enabled ones.The current "accept everything but store half" middle ground is the worst of both. Calling it out as a refactor rather than a bug since the enabled path still validates before use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/out/views/PanelViewProvider.js` around lines 1863 - 1892, The current save path constructs a partial fallbackTier (providerId/endpointKind) so buildTier produces incomplete tier objects that get persisted when enabled is false; later enabling causes schema failures in handleStartProxy. Fix by either (A) when enabled === false, read the existing mixedProviders config and only patch in tiers that were explicitly provided (update buildTier to return undefined when no binding exists and merge with existing settings before writing), or (B) always validate mixedConfig with MixedProviderConfigSchema.safeParse and reject the save when any tier fields are missing (i.e., move the safeParse check out of the enabled branch). Update references to fallbackTier, buildTier and mixedConfig and the save/merge logic in handleSaveMixedProviders accordingly so disabled saves never persist partially populated tier objects.tests/provider-router.test.js (1)
295-303: Strengthen the regex pattern for duplicate-model error assertion.The error message in
provider-router.js:249contains "is assigned to multiple mixed-provider tiers", so the current regex works. However, this pattern is fragile—if the error message is reworded (e.g., "is mapped to multiple tiers" or "spans multiple tiers"), the test will fail with an unhelpful regex-mismatch error instead of clearly signaling a logic failure. Use a broader pattern like/duplicate|multiple/ior assert on a structured error property (code,name) instead for clarity and resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/provider-router.test.js` around lines 295 - 303, The test's regex for the duplicate-model error is too brittle; update the assertion in the 'rejects duplicate model IDs across tiers' test so it matches a more resilient pattern or structured error: replace the current toThrow(/assigned to multiple/) check with a case-insensitive broader regex such as /duplicate|multiple/ or change to assert on an error property thrown by new ProviderRouter (e.g., error.name or error.code) to detect the duplicate-model failure; locate the test block using the `ProviderRouter` constructor in this test and make the assertion change there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 34167-34197: The current initialization swallows invalid
MIXED_PROVIDERS_CONFIG by returning null (same as "disabled"), so change the
logic in the ProviderRouter initialization block: when config.errors exists or
router2.validate() reports invalid (validation.valid is false), do not return
null—throw a descriptive Error (or return a distinct failure object) so callers
can detect a bad mixed-provider config; only return null when the env flag is
absent or enabled: false. Update the try/catch around parsing of
MIXED_PROVIDERS_CONFIG (the block that constructs new ProviderRouter, calls
validate(), and currently logs errors) to rethrow or propagate the error instead
of falling back to single-provider mode.
- Around line 34122-34140: The current validate() only ensures at least one
ProviderContext per providerId has a key, which can miss cases where
routedContext.key (used at runtime) is absent on the actual ProviderContext;
update validate() to either (A) require every ProviderContext in this.contexts
with the same providerId to have a non-empty ctx.key (replace the
providerToKey/uniqueProviders logic with a per-ctx check) or (B) normalize keys
before validation by building a canonical key for each providerId and copying it
into every matching ProviderContext (so each ProviderContext.key is populated) —
refer to validate(), this.contexts, ctx.providerId, ctx.key, routedContext.key
and the ProviderContext shape when implementing the fix.
- Line 33353: The PROVIDERS.kimi mapping currently reuses the Anthropic env
slot, causing Kimi to read credentials from "ANTHROPIC_API_KEY"; update the
provider-to-key mapping for PROVIDERS.kimi to use its dedicated key name
"KIMI_API_KEY" instead of "ANTHROPIC_API_KEY" where the mapping is defined (look
for the array value assigned to [PROVIDERS.kimi] in the proxy index.cjs), and
also update any docs/UI prompts that reference Anthropic for Kimi to instruct
users to store a Kimi-specific key "KIMI_API_KEY".
In `@extensions/thronekeeper/out/schemas/config.js`:
- Around line 64-69: MixedProviderConfigSchema currently requires reasoning,
completion, and value even when enabled is false; update the schema
(MixedProviderConfigSchema) so those TierProviderBindingSchema fields are
optional unless enabled is true—e.g. make reasoning, completion, and value
optional (or use .partial()) and add a refinement or transform that enforces
their presence/shape only when enabled === true; update any duplicate schema
instances noted around the other lines (same pattern at the other occurrence) to
match this conditional validation.
In `@extensions/thronekeeper/out/views/PanelViewProvider.js`:
- Around line 1695-1705: The code mutates this.currentProvider directly in
handleSetModelFromList which can leave modelsCache, sequenceTokenCounter and
provider token state stale; change handleSetModelFromList to either call
this.handleUpdateProvider(payload.provider) (so handleUpdateProvider will clear
modelsCache, reset sequenceTokenCounter, wipe tokens and emit an empty models
render) before any persistence/postConfig/model-list reads, or remove the silent
assignment and require the webview to send an explicit updateProvider event;
reference the handleSetModelFromList, handleUpdateProvider, currentProvider,
modelsCache, sequenceTokenCounter, postConfig and modelSelectionsByProvider
symbols when making the change.
- Around line 1846-1892: The three early-return rejection paths in
handleSaveMixedProviders (malformed payload, enabled-but-missing-tiers, and
MixedProviderConfigSchema failure) leave the webview without a response; update
each rejection to post a message back to the webview just like handleStartProxy
does (use the same shape: type: 'proxyError', errorType: 'config', plus an
informative message or the schema error details), and then return; ensure you
call the same messaging helper used elsewhere (e.g., this.postMessage or the
existing panel/webview post method) so the webview UI can display the error and
stop the Save spinner instead of hanging.
In `@provider-router.js`:
- Around line 294-317: validate() currently treats a provider as covered if any
context with the same providerId has a key, which lets one tier (e.g.,
reasoning) satisfy another tier (e.g., completion); change validate() to check
keys per routed context instead of by providerId: iterate this.contexts (each
ctx), verify ctx.key exists when that context requires a key (use the same
condition routing uses), collect ctx identifiers (or providerId+tier) into
missing when a specific ctx lacks a key, and return valid based on that
per-context missing list; update references to uniqueProviders/providerToKey to
reflect per-context validation so downstream routing using ctx.key won’t fail.
In `@tests/mixed-provider-tabs.test.js`:
- Around line 148-160: Add an assertion to ensure mixedTierAssignments is
preserved when three-model mode is disabled: after calling
createMixedProviderHarness(), saving originalModelsByProvider, toggling
twoModelMode, addProviderTab(), and calling disableThreeModelMode(), capture the
original mixedTierAssignments (e.g.
JSON.stringify(harness.state.mixedTierAssignments)) and assert it equals the
post-action value (e.g.
expect(JSON.stringify(harness.state.mixedTierAssignments)).toBe(originalMixedTierAssignments));
reference the harness instance and its state.mixedTierAssignments alongside the
existing modelsByProvider checks so provider→tier bindings survive tab
deactivation.
---
Outside diff comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 35806-35825: The code opens a text block when receiving reasoning
deltas, causing thinking deltas to be applied to a text block; change the branch
that handles delta.reasoning or delta.reasoning_content to start a content block
with type "thinking" (use the same sendContentSSE("content_block_start", { type:
"content_block_start", content_block: { type: "thinking", text: "" } })) instead
of "text", and ensure the same start/delta/stop SSE order is preserved; update
the boolean used to guard block start (e.g., textBlockStarted) or add a separate
thinkingBlockStarted flag so accumulatedReasoning, the use of sendContentSSE for
"content_block_delta" and the sequence message_start → content_block_start →
content_block_delta → message_stop remain correct.
In `@index.js`:
- Around line 861-867: The Anthropic upstream payload is built with the original
request model instead of the routed provider-native model; when
isAnthropicNative is true, ensure buildAnthropicPayload (and the analogous
provider payload builders around the other noted block) receive
routedContext.model (or swap payload.model to routedContext.model) so the
forwarded request uses the provider-native model identifier rather than the
display/routing model.
- Around line 533-571: Change readiness logic in healthCheckHandler so it
considers mixed-provider routing: replace the simple isReady = !!key with a
check that returns true if a global key exists OR the mixed-provider router is
active and reports available/healthy providers (e.g., router &&
router.hasHealthyProviders() or router.isReady()). Use that combined boolean for
status and the 503 reply decision. Also, when key is missing include missing-key
diagnostics in healthResponse (keep detectionSource, endpointKind, lastProbedAt,
baseUrl fields) so downstream consumers can tell whether readiness came from the
single-provider key or the router.
- Around line 1749-1768: The code is sending an untyped reasoning delta
(delta.reasoning) which Anthropic SSE clients may ignore; update the
sendContentSSE call in the branch that handles
delta.reasoning/delta.reasoning_content (referencing sendContentSSE,
textBlockStarted, accumulatedReasoning, content_block_start,
content_block_delta) to emit a typed delta: either convert the reasoning into a
text delta (delta: { type: 'text', text: reasoningText }) so it fits the opened
text content_block, or if you intend a thinking block, start a
content_block_start with type: 'thinking' and then emit content_block_delta with
delta: { type: 'thinking', reasoning: reasoningText }; also ensure events
maintain correct streaming order (message_start → content_block_start →
content_block_delta → message_stop).
---
Duplicate comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 34073-34105: The constructor currently allows multiple tiers to
register the same model if their providerId/baseUrl/endpointKind/model match
(sameContext), which ignores differing auth keys and still leaves payload.model
ambiguous; modify the registration and resolution logic in the ProviderRouter:
when iterating this.contexts (use symbols: contexts, normalizedTierMap, tierMap,
sameContext, entry) treat any duplicate model across different tiers as an error
unless the incoming lookup explicitly includes the tier; either (A) reject
duplicates unconditionally by throwing if normalizedModel or ctx.model already
exists with a different tier, or (B) support tier-qualified lookups by storing
entries keyed by `${tier}:${model}` and update resolve(modelName) to parse an
optional `tier:` prefix to pick the exact tier entry (fall back to normalized
maps only when no tier specified and only one tier exists). Ensure tokens/keys
are considered by preventing silent routing when providerId/baseUrl/model match
but auth differs.
---
Nitpick comments:
In `@extensions/thronekeeper/out/views/PanelViewProvider.js`:
- Around line 1863-1892: The current save path constructs a partial fallbackTier
(providerId/endpointKind) so buildTier produces incomplete tier objects that get
persisted when enabled is false; later enabling causes schema failures in
handleStartProxy. Fix by either (A) when enabled === false, read the existing
mixedProviders config and only patch in tiers that were explicitly provided
(update buildTier to return undefined when no binding exists and merge with
existing settings before writing), or (B) always validate mixedConfig with
MixedProviderConfigSchema.safeParse and reject the save when any tier fields are
missing (i.e., move the safeParse check out of the enabled branch). Update
references to fallbackTier, buildTier and mixedConfig and the save/merge logic
in handleSaveMixedProviders accordingly so disabled saves never persist
partially populated tier objects.
In `@tests/provider-router.test.js`:
- Around line 295-303: The test's regex for the duplicate-model error is too
brittle; update the assertion in the 'rejects duplicate model IDs across tiers'
test so it matches a more resilient pattern or structured error: replace the
current toThrow(/assigned to multiple/) check with a case-insensitive broader
regex such as /duplicate|multiple/ or change to assert on an error property
thrown by new ProviderRouter (e.g., error.name or error.code) to detect the
duplicate-model failure; locate the test block using the `ProviderRouter`
constructor in this test and make the assertion change there.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eede7d5-2500-4a7c-bb5e-05378e1fe5c0
⛔ Files ignored due to path filters (3)
extensions/thronekeeper/out/schemas/config.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/Models.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/views/PanelViewProvider.js.mapis excluded by!**/*.map
📒 Files selected for processing (19)
extensions/thronekeeper/bundled/proxy/index.cjsextensions/thronekeeper/out/schemas/config.jsextensions/thronekeeper/out/services/Models.jsextensions/thronekeeper/out/views/PanelViewProvider.jsextensions/thronekeeper/package.jsonextensions/thronekeeper/src/schemas/config.tsextensions/thronekeeper/src/services/Models.tsextensions/thronekeeper/src/views/PanelViewProvider.tsextensions/thronekeeper/tests/vscode-shim.tsextensions/thronekeeper/webview/main.jsindex.jsprovider-router.jstests/mixed-provider-tabs.test.jstests/models.list.test.tstests/phase4-hydration.test.jstests/phase5-ui-optimization.test.jstests/provider-router.test.jstests/utils.jsvitest.config.js
✅ Files skipped from review due to trivial changes (2)
- vitest.config.js
- extensions/thronekeeper/tests/vscode-shim.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/phase5-ui-optimization.test.js
- extensions/thronekeeper/package.json
- tests/phase4-hydration.test.js
- extensions/thronekeeper/src/services/Models.ts
- extensions/thronekeeper/src/schemas/config.ts
- tests/models.list.test.ts
- extensions/thronekeeper/webview/main.js
- extensions/thronekeeper/src/views/PanelViewProvider.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
extensions/thronekeeper/src/schemas/config.ts (1)
307-357:⚠️ Potential issue | 🟡 MinorAvoid mixed-mode false positives in invariant checks.
When
mixedProviders.enabledis true, the tier bindings become the source of truth. The earlier active-provider checks still requiremodelSelectionsByProvider[activeProvider], so a valid mixed-only config can be reported invalid before the new mixed checks even matter.🧭 Proposed guard for single-provider invariants
// Check 2: Verify active provider has configuration const activeProvider = config.provider || 'openrouter' const activeProviderMap = config.modelSelectionsByProvider?.[activeProvider] + const mixedProviderMode = config.mixedProviders?.enabled === true - if (!activeProviderMap || (!activeProviderMap.reasoning && !config.reasoningModel)) { + if (!mixedProviderMode && (!activeProviderMap || (!activeProviderMap.reasoning && !config.reasoningModel))) { violations.push( `Active provider '${activeProvider}' has no model selections. ` + `This may cause proxy start failures.` ) } // Check 3: Verify two-model mode has all required models - if (config.twoModelMode) { + if (!mixedProviderMode && config.twoModelMode) { const normalized = normalizeProviderMap(activeProviderMap) if (!normalized.completion || !normalized.value) { violations.push( `Two-model mode enabled but provider '${activeProvider}' is missing completion or value models.` )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/schemas/config.ts` around lines 307 - 357, The active-provider invariant checks (using activeProvider, activeProviderMap, modelSelectionsByProvider and the two-model check via normalizeProviderMap) are producing false positives when mixedProviders.enabled is true because mixedProviders bindings are the source of truth; change the logic so that the Check 2 and Check 3 blocks are skipped when config.mixedProviders?.enabled is true (i.e., wrap or gate the existing activeProviderMap existence check and the twoModelMode/normalizeProviderMap check with a guard like if (!config.mixedProviders?.enabled) so mixed-only configurations rely solely on mixedProviders validation instead of modelSelectionsByProvider).extensions/thronekeeper/bundled/proxy/index.cjs (1)
35592-35830:⚠️ Potential issue | 🟠 MajorUnify tool-call content-block indices with the thinking/text counter to prevent collision.
Tool calls currently use raw
toolCall.indexfrom the upstream OpenAI response (typically 0, 1, 2…), while thinking and text blocks pull fromnextContentBlockIndex++. When a response contains thinking (index 0), text (index 1), and tool calls (indices 0, 1…), the SSE stream emits duplicate indices: e.g., both thinking and the first tool call claimindex: 0. Anthropic clients parse content blocks by index, so overlapping indices can cause tool calls to be lost or misattributed.Remap tool-call indices through the same counter as thinking and text:
Suggested fix (apply in index.js)
const toolCallAccumulators = {} + const toolCallIndexMap = {} // upstream index → assigned index // When a new tool call arrives: - const idx = toolCall.index - if (toolCallAccumulators[idx] === undefined) { - toolCallAccumulators[idx] = "" + const upstreamIdx = toolCall.index + if (toolCallAccumulators[upstreamIdx] === undefined) { + toolCallAccumulators[upstreamIdx] = "" + toolCallIndexMap[upstreamIdx] = nextContentBlockIndex++ await sendContentSSE('content_block_start', { type: 'content_block_start', - index: idx, + index: toolCallIndexMap[upstreamIdx], content_block: { type: 'tool_use', ... } }) } - const oldArgs = toolCallAccumulators[idx] + const oldArgs = toolCallAccumulators[upstreamIdx] if (newArgs.length > oldArgs.length) { await sendContentSSE('content_block_delta', { type: 'content_block_delta', - index: idx, + index: toolCallIndexMap[upstreamIdx], ... }) - toolCallAccumulators[idx] = newArgs + toolCallAccumulators[upstreamIdx] = newArgs } // In the message_stop section, update the tool-call stop loop: for (const upstreamIdx in toolCallAccumulators) { sendSSE(reply, 'content_block_stop', { type: 'content_block_stop', - index: parseInt(idx, 10) + index: toolCallIndexMap[parseInt(upstreamIdx, 10)] }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/bundled/proxy/index.cjs` around lines 35592 - 35830, Tool-call blocks are using upstream toolCall.index (raw indices) which can collide with text/thinking indices from nextContentBlockIndex; change handling in the delta.tool_calls branch (around toolCallAccumulators, toolCall.index, sendContentSSE, and encounteredToolCall logic) to assign a new unified index via nextContentBlockIndex++ for each new tool call and store a mapping from the original toolCall.index to this assignedIndex; use assignedIndex as the key into toolCallAccumulators and in all sendContentSSE calls (content_block_start, content_block_delta) and later when emitting content_block_stop for tool calls, iterate the mapped keys (or stored assigned indices) so tool-call blocks share the same index space as text/thinking blocks.extensions/thronekeeper/src/views/PanelViewProvider.ts (2)
1352-1516:⚠️ Potential issue | 🟠 MajorSkip active-provider model fallbacks once mixed routing is validated.
After
mixedProvidersis parsed, startup still reads/validates models frommodelSelectionsByProvider[this.runtimeProvider]. A valid mixed setup where reasoning/completion/value live on different tabs can be blocked by “missing completion/value for active provider” prompts, or worse, hydrate legacy globals from stale fallback models. When mixed mode is active, derivereasoningModel,completionModel, andvalueModelfrom the validated tier config and enforce the mixed-mode invariants there.🐛 Proposed direction
if (mixedProvidersRaw?.enabled) { const parsed = MixedProviderConfigSchema.safeParse(mixedProvidersRaw) if (!parsed.success) { @@ mixedProviders = parsed.data this.log.appendLine(`[handleStartProxy] Mixed provider config enabled and validated`) + + if (!twoModelMode) { + const errorMsg = `Mixed provider mode requires "3 Different Models" mode to be enabled.` + this.log.appendLine(`[handleStartProxy] ERROR: ${errorMsg}`) + vscode.window.showWarningMessage(errorMsg) + this.post({ + type: 'proxyError', + payload: { + provider: this.runtimeProvider || 'openrouter', + error: errorMsg, + errorType: 'config' + } + }) + return + } }Then branch the later model lookup so mixed mode uses:
reasoningModel = mixedProviders.reasoning.model completionModel = mixedProviders.completion.model valueModel = mixedProviders.value.modelinstead of the active-provider fallback path.
As per coding guidelines, “When implementing Start/Stop proxy operations, ensure atomic updates: save both legacy globals (reasoningModel, completionModel, valueModel) and provider-specific selections together to prevent partial state corruption”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 1352 - 1516, The startup still reads models from modelSelectionsByProvider[this.runtimeProvider] even when mixedProviders was validated, causing missing-model prompts or stale global fallbacks; update the handleStartProxy flow so that if mixedProviders (the parsed MixedProviderConfigSchema result) is enabled you set reasoningModel, completionModel, and valueModel directly from mixedProviders.reasoning.model / mixedProviders.completion.model / mixedProviders.value.model and skip the provider-specific normalization/fallback checks (normalizeProviderMap, modelSelectionsByProvider lookup and subsequent global fallback prompts), and ensure you still run the downstream presence checks/posting (configWarning/proxyError) against those mixed-derived values; additionally ensure atomic save semantics when starting/stopping proxy by saving legacy globals (reasoningModel/completionModel/valueModel) together with provider-specific selections so partial state cannot persist.
331-333:⚠️ Potential issue | 🟠 MajorPass the provider-aware payload into
handleSetModelFromList.The new handler can switch/save against
payload.provider, but the dispatcher strips that field by passing onlymodelIdandmodelType. In the tabbed UI, that’s like taking the address off an envelope before delivery: a selection from Provider 2 can be persisted under whichever provider happens to be current.🐛 Proposed fix
- case 'setModelFromList': - await this.handleSetModelFromList(msg.modelId, msg.modelType) + case 'setModelFromList': + await this.handleSetModelFromList(msg) breakAs per coding guidelines, “Maintain provider model selections with atomic structure: { reasoning, completion, value } per provider, never storing individual model keys without full context”.
Also applies to: 1866-1875
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/src/views/PanelViewProvider.ts` around lines 331 - 333, The dispatcher for case 'setModelFromList' is stripping provider context by calling handleSetModelFromList(msg.modelId, msg.modelType); change the call to pass the full provider-aware payload (e.g., msg or msg.payload) so handleSetModelFromList receives payload.provider as well; update any similar dispatches (same pattern around the block referenced at lines ~1866-1875) to pass the full payload so provider-specific storage (the { reasoning, completion, value } per provider model) is preserved.
♻️ Duplicate comments (2)
extensions/thronekeeper/out/views/PanelViewProvider.js (1)
322-324:⚠️ Potential issue | 🟠 MajorPass the provider-aware payload into
handleSetModelFromList.Line 322 drops
msg.provider, so the cleanup/switch logic on Lines 1699-1701 never runs for normalsetModelFromListmessages. In the tabbed UI, that can save a picked model under the currently active provider instead of the tab’s provider—like filing the card in the wrong drawer.Proposed fix
- case 'setModelFromList': - await this.handleSetModelFromList(msg.modelId, msg.modelType); + case 'setModelFromList': + await this.handleSetModelFromList(msg); break;Based on learnings, "In provider-aware model loading logic, validate request/response token matching and cache models by provider to prevent race conditions from concurrent provider switches".
Also applies to: 1695-1701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/thronekeeper/out/views/PanelViewProvider.js` around lines 322 - 324, The switch case for 'setModelFromList' is dropping msg.provider which prevents the provider-aware cleanup/switch logic in handleSetModelFromList from running; modify the case handling so it calls await this.handleSetModelFromList(msg.modelId, msg.modelType, msg.provider) and update the handleSetModelFromList function signature (and any internal calls) to accept and use the provider argument to run the existing cleanup/switch/token validation and provider-specific caching logic (the same fix should be applied to the other occurrence around the cleanup/switch block referenced by handleSetModelFromList and lines ~1695-1701) so models are saved/validated against the correct provider and race conditions are avoided.provider-router.js (1)
231-248:⚠️ Potential issue | 🟠 MajorAllow duplicate model IDs when they point to the same context.
This rejects the common “use one model for all tiers” setup even when every duplicate points to the same provider/baseUrl/endpointKind/key. Only duplicates that point to different upstream contexts are ambiguous; two signs pointing to the same door do not need to fail startup.
🐛 Proposed fix
const existing = this.normalizedTierMap.get(normalizedModel) if (existing) { - throw new Error( - `[ProviderRouter] Model "${ctx.model}" is assigned to multiple mixed-provider tiers (${existing.tier}, ${tier})` - ) + const existingContext = existing.context + const sameContext = + existingContext.providerId === ctx.providerId && + existingContext.baseUrl === ctx.baseUrl && + existingContext.endpointKind === ctx.endpointKind && + existingContext.key === ctx.key && + existingContext.model === ctx.model + + if (!sameContext) { + throw new Error( + `[ProviderRouter] Model "${ctx.model}" is assigned to multiple mixed-provider tiers (${existing.tier}, ${tier})` + ) + } + + continue }Also update the duplicate-model test that currently expects same-context reuse to throw.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider-router.js` around lines 231 - 248, The code currently throws on any duplicate normalized model name when building this.normalizedTierMap, which incorrectly rejects duplicates that refer to the exact same upstream context; change the check in the constructor loop around this.contexts so that when an existing entry is found for normalizedModel you only throw if the existing.context differs from the current ctx (compare either object identity or the unique upstream properties like provider, baseUrl, endpointKind, and key) and otherwise reuse the same entry (do not throw); update the duplicate-model test to expect same-context reuse (no throw) and still assert an error when duplicates point to different upstream contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extensions/thronekeeper/out/views/PanelViewProvider.js`:
- Around line 1253-1274: The startup validation still enforces single-provider
model checks even when a validated mixedProviders config is active; update the
startup guard logic in the same routine (handleStartProxy / the startup
validation blocks that reference this.runtimeProvider and single-provider model
slots) so that if mixedProviders?.enabled is true you skip the single-provider
checks and allow startup, and only run the existing this.runtimeProvider-based
guards when mixedProviders is null/disabled; change the conditional branches
that currently reject startup (the blocks that call this.post({ type:
'proxyError', ... }) and show warnings) to first check mixedProviders?.enabled
and bypass those errors when mixed routing is active.
In `@extensions/thronekeeper/src/schemas/config.ts`:
- Around line 55-61: The schema TierProviderBindingSchema currently allows alias
values ('openai-compatible', 'anthropic-native') for endpointKind which can be
forwarded by ProxyManager as binding.endpointKind into MIXED_PROVIDERS_CONFIG
and break runtime consumers; tighten the schema to only permit the concrete
runtime values ('openai' | 'anthropic') or add a normalization step in the code
path that sets binding.endpointKind (e.g., in ProxyManager) to map aliases to
their concrete equivalents before forwarding to MIXED_PROVIDERS_CONFIG (ensure
normalization handles 'openai-compatible' -> 'openai' and 'anthropic-native' ->
'anthropic').
In `@index.js`:
- Around line 791-816: Mixed-provider routing currently replaces the provider
context (routedContext) but does not swap the OpenAI-compatible outgoing model
ID, so openaiPayloads still use the incoming payload.model (display/lookup ID)
instead of the provider's real upstream model; update the code paths that build
OpenAI-compatible payloads (where openaiPayload.model or similar fields are set,
and analogous spots around the other noted locations) to use routedContext.model
when routedContext is present (i.e., set openaiPayload.model =
routedContext.model || payload.model), and apply the same substitution in the
other OpenAI-compatible builders referenced around the other locations (ensure
you use the routedContext-derived values consistently alongside
effectiveProvider/effectiveBaseUrl/effectiveKey).
In `@provider-router.js`:
- Around line 136-140: getUpstreamUrl is appending versioned paths to baseUrl
without stripping an existing "/v1" (ProviderContext.constructor only removes
trailing slashes), causing double "/v1/v1" URLs; modify ProviderContext (or the
normalization used by getUpstreamUrl) to strip any trailing "/v1" or other
version segment (e.g., remove /v\\d+(/.*)? at end) when normalizing baseUrl so
getUpstreamUrl can safely append "/v1/messages" or "/v1/chat/completions",
update getUpstreamUrl to join paths without duplicating slashes, and add unit
tests for pre-versioned baseUrls like "https://api.openai.com/v1" and
"https://api.together.xyz/v1/" to assert correct single "/v1" in resulting
upstream URLs.
---
Outside diff comments:
In `@extensions/thronekeeper/bundled/proxy/index.cjs`:
- Around line 35592-35830: Tool-call blocks are using upstream toolCall.index
(raw indices) which can collide with text/thinking indices from
nextContentBlockIndex; change handling in the delta.tool_calls branch (around
toolCallAccumulators, toolCall.index, sendContentSSE, and encounteredToolCall
logic) to assign a new unified index via nextContentBlockIndex++ for each new
tool call and store a mapping from the original toolCall.index to this
assignedIndex; use assignedIndex as the key into toolCallAccumulators and in all
sendContentSSE calls (content_block_start, content_block_delta) and later when
emitting content_block_stop for tool calls, iterate the mapped keys (or stored
assigned indices) so tool-call blocks share the same index space as
text/thinking blocks.
In `@extensions/thronekeeper/src/schemas/config.ts`:
- Around line 307-357: The active-provider invariant checks (using
activeProvider, activeProviderMap, modelSelectionsByProvider and the two-model
check via normalizeProviderMap) are producing false positives when
mixedProviders.enabled is true because mixedProviders bindings are the source of
truth; change the logic so that the Check 2 and Check 3 blocks are skipped when
config.mixedProviders?.enabled is true (i.e., wrap or gate the existing
activeProviderMap existence check and the twoModelMode/normalizeProviderMap
check with a guard like if (!config.mixedProviders?.enabled) so mixed-only
configurations rely solely on mixedProviders validation instead of
modelSelectionsByProvider).
In `@extensions/thronekeeper/src/views/PanelViewProvider.ts`:
- Around line 1352-1516: The startup still reads models from
modelSelectionsByProvider[this.runtimeProvider] even when mixedProviders was
validated, causing missing-model prompts or stale global fallbacks; update the
handleStartProxy flow so that if mixedProviders (the parsed
MixedProviderConfigSchema result) is enabled you set reasoningModel,
completionModel, and valueModel directly from mixedProviders.reasoning.model /
mixedProviders.completion.model / mixedProviders.value.model and skip the
provider-specific normalization/fallback checks (normalizeProviderMap,
modelSelectionsByProvider lookup and subsequent global fallback prompts), and
ensure you still run the downstream presence checks/posting
(configWarning/proxyError) against those mixed-derived values; additionally
ensure atomic save semantics when starting/stopping proxy by saving legacy
globals (reasoningModel/completionModel/valueModel) together with
provider-specific selections so partial state cannot persist.
- Around line 331-333: The dispatcher for case 'setModelFromList' is stripping
provider context by calling handleSetModelFromList(msg.modelId, msg.modelType);
change the call to pass the full provider-aware payload (e.g., msg or
msg.payload) so handleSetModelFromList receives payload.provider as well; update
any similar dispatches (same pattern around the block referenced at lines
~1866-1875) to pass the full payload so provider-specific storage (the {
reasoning, completion, value } per provider model) is preserved.
---
Duplicate comments:
In `@extensions/thronekeeper/out/views/PanelViewProvider.js`:
- Around line 322-324: The switch case for 'setModelFromList' is dropping
msg.provider which prevents the provider-aware cleanup/switch logic in
handleSetModelFromList from running; modify the case handling so it calls await
this.handleSetModelFromList(msg.modelId, msg.modelType, msg.provider) and update
the handleSetModelFromList function signature (and any internal calls) to accept
and use the provider argument to run the existing cleanup/switch/token
validation and provider-specific caching logic (the same fix should be applied
to the other occurrence around the cleanup/switch block referenced by
handleSetModelFromList and lines ~1695-1701) so models are saved/validated
against the correct provider and race conditions are avoided.
In `@provider-router.js`:
- Around line 231-248: The code currently throws on any duplicate normalized
model name when building this.normalizedTierMap, which incorrectly rejects
duplicates that refer to the exact same upstream context; change the check in
the constructor loop around this.contexts so that when an existing entry is
found for normalizedModel you only throw if the existing.context differs from
the current ctx (compare either object identity or the unique upstream
properties like provider, baseUrl, endpointKind, and key) and otherwise reuse
the same entry (do not throw); update the duplicate-model test to expect
same-context reuse (no throw) and still assert an error when duplicates point to
different upstream contexts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73ac0d62-476a-46d0-af36-9a0d6f28e811
⛔ Files ignored due to path filters (3)
extensions/thronekeeper/out/schemas/config.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/services/ProxyManager.js.mapis excluded by!**/*.mapextensions/thronekeeper/out/views/PanelViewProvider.js.mapis excluded by!**/*.map
📒 Files selected for processing (14)
extensions/thronekeeper/bundled/proxy/index.cjsextensions/thronekeeper/out/schemas/config.jsextensions/thronekeeper/out/services/ProxyManager.jsextensions/thronekeeper/out/views/PanelViewProvider.jsextensions/thronekeeper/src/schemas/config.tsextensions/thronekeeper/src/services/ProxyManager.tsextensions/thronekeeper/src/views/PanelViewProvider.tsindex.jskey-resolver.jsprovider-router.jsscripts/smoke.shtests/key-resolver.test.jstests/mixed-provider-tabs.test.jstests/provider-router.test.js
✅ Files skipped from review due to trivial changes (2)
- tests/key-resolver.test.js
- tests/mixed-provider-tabs.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/smoke.sh
- key-resolver.js
Owner
Author
|
Pushed CodeRabbit follow-up fixes in Addressed:
Verification:
|
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.
PR Description: Multi-Provider UI Tabbed Refactor (KHA-267)
Overview
This PR completes the transition from the legacy "Mix Providers" dropdown system to a modern, integrated tabbed interface. The refactor simplifies the configuration flow, reduces UI clutter, and ensures robust state persistence for multi-provider setups.
Key Changes
1. Integrated Tabbed UI
[+]button, replacing the complex manual toggle logic.[+]automatically enables "Mix Providers" and "Three Model Mode" if they aren't already active, streamlining the onboarding to advanced routing.2. State Management & Implicit Tiering
state.mixedTierAssignmentsto track provider-to-tier bindings independently of tab order, ensuring configuraiton remains stable across reloads.3. Layout & UX Fixes
content-gridlayout would break (pushing the model list below providers) due to incorrect div nesting.Invariants Touched
handleSaveMixedProvidersto accept the new tier-based payload.handleConfigLoadedto recreate secondary tabs and restore provider assignments on startup.Manual Testing Results
mixedProvidersconfig in VS Code settings.[+]enables mixed mode and spawns a new tab.Version Bump
1.5.71for final layout verification.Summary by CodeRabbit
New Features
Updates
Bug Fixes
Documentation
Tests