feat: dynamic provider-aware fallback planning and install provider matrix#3
feat: dynamic provider-aware fallback planning and install provider matrix#3Daltonganger wants to merge 24 commits intomasterfrom
Conversation
…e-model-discovery
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Files Reviewed (12 files)
Note: The getEnv refactoring suggestion has already been addressed - the function is now in |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| --aa-key Artificial Analysis API key (optional) | ||
| --openrouter-key OpenRouter API key (optional) | ||
| --tmux=yes|no Enable tmux integration (yes/no) | ||
| --skills=yes|no Install recommended skills (yes/no) |
There was a problem hiding this comment.
CRITICAL: API keys should not be shown in plain text in documentation examples. Remove the dummy keys from the example command.
| ); | ||
| } | ||
| } else { | ||
| warnings.push( |
There was a problem hiding this comment.
SUGGESTION: Only add warnings to the warnings array when API keys are provided but the request fails. The current implementation adds warnings even when API keys are not set, which could be misleading.
src/cli/external-rankings.ts
Outdated
| return parsed * 1_000_000; | ||
| } | ||
|
|
||
| function getEnv(name: string): string | undefined { |
There was a problem hiding this comment.
SUGGESTION: Refactor the getEnv function to a shared utility file (like src/utils/index.ts) to avoid code duplication with install.ts.
src/cli/external-rankings.ts
Outdated
|
|
||
| for (const alias of baseAliases(key)) { | ||
| map[alias] = mergeSignal(map[alias], signal); | ||
| const chutesAlias = `chutes/${alias}`; |
There was a problem hiding this comment.
SUGGESTION: Add a check to avoid adding duplicate Chutes aliases in fetchOpenRouterSignals, similar to how it's done in fetchArtificialAnalysisSignals.
src/cli/external-rankings.ts
Outdated
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Artificial Analysis request failed (${response.status})`); |
There was a problem hiding this comment.
SUGGESTION: Improve error handling to include more detailed information about the failure, such as the response status text.
…alysis and OpenRouter
Prevent key leakage in installer prompts, isolate external ranking aliasing, and make free-model tails deterministic before Big Pickle fallback. Also relax fallback chain schema to preserve custom agent keys and align tests.
1 participant
Summary
Validation
bun test src/cli/dynamic-model-selection.test.ts src/cli/providers.test.ts src/cli/opencode-models.test.ts src/cli/config-io.test.ts src/background/background-manager.test.ts src/config/loader.test.tsNotes