Repository navigation
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The only incremental change since The prior inline findings remain as previously reconciled: the Files Reviewed (1 file)
Previous Review Summaries (16 snapshots, latest commit b5a8ddf)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b5a8ddf)Status: No Issues Found | Recommendation: Merge The incremental commit Files Reviewed (4 files)
Previous review (commit 4c042c3)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 549678d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit df0303b)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 4bdcc36)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit ff53f71)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit f79b13f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Resolved since the previous review (now at Fix these issues in Kilo Cloud Previous review (commit da039db)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Resolved since the previous review: the branch is now Fix these issues in Kilo Cloud Previous review (commit cc4f27f)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 0371a86)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous review (commit 6afac9e)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 7663853)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 79d5734)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit e874ff5)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Resolved since the previous review: the tool-less chat model gap is closed by the text-in/text-out predicate, and both previously missing test cases (family precedence over a cheaper non-family model, and skipping non-text models while a chat model is present) are now present. Fix these issues in Kilo Cloud Previous review (commit 9e04716)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Resolved since the previous review: the tool-less chat model gap is fixed by the new text-in/text-out predicate, and both previously missing test cases (family precedence over a cheaper non-family model, and skipping non-text models while a chat model is present) were added. Fix these issues in Kilo Cloud Previous review (commit eaceab3)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by deepseek-flash-latest · Input: 44.5K · Output: 3.6K · Cached: 227.5K Review guidance: REVIEW.md from base branch |
| */ | ||
| export function cheapestSmallModel(models: Provider.Model[]) { | ||
| return sortBy( | ||
| models.filter((model) => model.capabilities.input.text && model.capabilities.output.text), |
There was a problem hiding this comment.
WARNING: The new predicate admits embedding and rerank models, so the cheapest "text" model can be a non-generative one.
input.text && output.text matches models.dev embeddings/rerankers, which declare modalities: { input: ["text"], output: ["text"] } with tool_call: false (for example nomic-embed-text, qwen3-embedding-8b, text-embedding-3-small, voyage/rerank-2.5 in packages/opencode/test/tool/fixtures/models-api.json). Dropping the old toolcall filter removed the one signal that excluded them.
Concrete case: Tinfoil has no gemini-flash / gpt-nano / claude-haiku family, and its nomic-embed-text costs 0.05 + 0 while its cheapest chat model (gpt-oss-120b) costs 0.15 + 0.6, so cheapestSmallModel now returns the embedding model and session titles / commit messages / branch names are sent to a non-chat endpoint.
Text in/out is necessary but not sufficient for a chat model. Consider also skipping non-generative models (for example family/id containing embed/rerank) while still keeping tool-less chat models like sonar that motivated this change.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
26d9367 to
e874ff5
Compare
| // messages, branch names) on the active provider by picking its cheapest chat-capable | ||
| // model before considering Kilo's auto small model, which draws Kilo credits in BYOK | ||
| // setups (#4268). The Kilo fallback below then only applies when the provider has no | ||
| // usable model at all, and still only when the user has kilo credentials. |
There was a problem hiding this comment.
WARNING: This revision predates #13192 and currently conflicts with main (gh pr view reports CONFLICTING), so the Kilo fallback below is not credential-gated in this tree.
const kiloFallback = s.providers["kilo"] ?? s.catalog["kilo"] (line 2078) resolves even for users without Kilo credentials, and hasKiloCredentials does not exist in @/kilocode/provider/provider at this revision. So the claim in this comment ("still only when the user has kilo credentials") does not hold here, and if the conflict is resolved by taking this side, the gate added after this branch's base is dropped — the exact regression #13192 fixed.
Current main also already contains packages/opencode/test/kilocode/provider/provider.test.ts, which this branch re-creates as a new file (its afterEach also omits main's Env.use.remove / disposeAllInstances isolation). Rebase onto main and keep the hasKiloCredentials gate plus main's test cases.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import type { Config } from "@/config/config" | ||
| import { organization, token } from "./catalog" | ||
| import { | ||
| bedrockAuth, |
There was a problem hiding this comment.
SUGGESTION: These five cloud-auth helpers are imported but never referenced in this module; only the re-export at the end of the file (export { ... } from "./cloud-auth") is needed for provider/provider.ts to keep consuming them. Dropping the local import removes dead bindings and keeps this PR's diff focused on the small-model change.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
43b58dd to
0371a86
Compare
| // kilo credentials. The kilo provider is always autoloaded (anonymous key), so checking it | ||
| // unconditionally would route auxiliary tasks (session titles, commit messages, branch names) | ||
| // to the cloud for users without kilo access and break offline/local-only setups. | ||
| const kiloFallback = s.providers[ProviderV2.ID.make("kilo")] |
There was a problem hiding this comment.
WARNING: This revision sits on a stale base and conflicts with main; the credential gate re-added here already exists upstream.
gh pr view reports mergeable: CONFLICTING / mergeStateStatus: DIRTY, and this branch is based on release: v7.5.15. Current main already contains an identical hasKiloCredentials in packages/opencode/src/kilocode/provider/provider.ts (from #13192) plus this same kiloFallback gate in getSmallModel, so lines 2070-2083 are already upstream and cheapestSmallModel is the only genuinely new behavior in this diff.
Rebasing onto main would drop this duplicated block from the PR diff and avoid resolving the conflict by re-introducing an older copy of provider.ts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| it.instance( | ||
| // kilocode_change start - Kilo always has an auto-routed small-model fallback | ||
| "getSmallModel falls back to Kilo auto when model IDs lack family metadata", | ||
| "getSmallModel picks the provider's own model when model IDs lack family metadata", |
There was a problem hiding this comment.
SUGGESTION: cheapestSmallModel has no direct test in this revision, so the selection ordering it relies on is unpinned.
The commit message says "Add comprehensive tests for the new behavior", but the only test change here is one updated expectation in the shared upstream test, whose fixture has a single candidate model. The earlier revision of this branch (6afac9e8) carried 182 lines of tests for this helper in packages/opencode/test/kilocode/provider/provider.test.ts (multiple candidates, family-priority precedence, no-chat-capable case); none of that is in this diff.
Restoring those cases would pin cheapest-wins ordering plus the release_date/id tie-break before this lands.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
0371a86 to
cc4f27f
Compare
| if (candidates[0]) return candidates[0] | ||
| } | ||
|
|
||
| const cheap = cheapestSmallModel(Object.values(provider.models)) // kilocode_change |
There was a problem hiding this comment.
WARNING: This new fallback reverses the isolation that the existing guard tests in packages/opencode/test/kilocode/provider/provider.test.ts (from #13192) deliberately assert, and those two tests are still present unchanged on this branch, so they now fail.
Both use the same test-provider shape as the test added in packages/opencode/test/provider/provider.test.ts (a single gpt-5-nano with no family, so family falls back to "" at provider.ts:1565), and cheapestSmallModel filters only on text-in/text-out (capabilities default to true at provider.ts:1527/1534), so it always finds a candidate here:
getSmallModel returns undefined without kilo credentials when model IDs lack family metadata(configkilo: null) expectsundefined, but this line now returnstest-provider/gpt-5-nanobefore the credential gate is consulted.getSmallModel falls back to Kilo auto when the kilo provider is configuredexpectskilo/kilo-auto/small, but this line returns the provider's own model before the credentialed Kilo fallback below it.
The PR's "prefer same-vendor" intent and the #13192 contract currently contradict each other, and the new test asserts the opposite of the two tests it mirrors. Decide which behavior wins and update the other side: if routing auxiliary tasks to the provider's own cheapest model is intended, the two guard tests need to be rewritten (not left asserting the old behavior); if the Kilo fallback should still win for credentialled users, move this call below the Kilo-credentials block.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
cc4f27f to
da039db
Compare
| Effect.gen(function* () { | ||
| const model = yield* Provider.use.getSmallModel(ProviderV2.ID.make("test-provider")) | ||
| expect(model).toMatchObject({ providerID: "kilo", id: "kilo-auto/small" }) | ||
| expect(model).toMatchObject({ providerID: "test-provider", id: "gpt-5-nano" }) |
There was a problem hiding this comment.
SUGGESTION: The rewritten assertions now exercise the same branch in both tests, and the Kilo-credentials gate they used to guard is no longer covered.
Both fixtures contain a single text-capable gpt-5-nano, so cheapestSmallModel (provider.ts:2067) returns it before the hasKiloCredentials block at provider.ts:2076-2081 is ever consulted. The kilo provider added in the second fixture and the cleared KILO_* env in the first no longer change the result, and the final undefined return (provider.ts:2085) is unreachable from either fixture.
These two cases were the only tests pinning the #13192 contract (no credentials -> undefined, credentials -> kilo-auto/small). Consider keeping that coverage with a fixture that has no text-capable model (for example an embedding-only model) and asserting undefined without credentials and kilo-auto/small with them.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Thanks for the review — all three findings are addressed in f79b13f. WARNING — embedding/rerank models admitted ( SUGGESTION — #13192 contract no longer covered ( SUGGESTION — no direct Verification: 10/10 in |
| return sortBy( | ||
| models.filter((model) => { | ||
| if (!model.capabilities.input.text || !model.capabilities.output.text) return false | ||
| return !/(embed|rerank)/.test(`${model.family} ${model.id}`) |
There was a problem hiding this comment.
WARNING: The new /(embed|rerank)/ exclusion is case-sensitive and keyword-limited, so real embedding/rerank models still pass the filter and can win the price sort, routing session titles / commit messages / branch names to a non-generative endpoint.
The new test only covers lowercase ids/families, but catalog entries that slip through include:
Qwen/Qwen3-Reranker-0.6B(familyqwen, capitalR) - in thenearaicatalog this is0.01 + 0.01 = 0.02, cheaper thanopenai/gpt-5-nanoat0.05 + 0.4, so it is selected as the cheapest "chat" model for that provider.intfloat/e5-mistral-7b-instruct(familymistral, an embedding model, noembed/reranksubstring) - in thestackitcatalog0.02 + 0.02 = 0.04, cheaper thanopenai/gpt-oss-20bat0.18 + 0.29.bge-m3(familybge) has noembed/reranksubstring either.
Adding the i flag (/(embed|rerank)/i) at least covers the capitalized Qwen variants; the keyword match also needs a wider signal if the docstring claim that embedding/rerank models are skipped is meant to hold. Extending the direct test with a capitalized id would pin this.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1601323 to
ff53f71
Compare
github-actions
Bot
added
area:cli
|
The BYOK routing change looks sound, and targeted tests/typecheck pass. Before approval, please address two remaining gaps:
Reproduced through the actual Please add focused regression tests for these cases. These should be narrow fixes; no broader refactor is needed to move this PR toward approval. |
c4d2f29 to
df0303b
Compare
| function isLoopbackURL(raw: string) { | ||
| try { | ||
| const host = new URL(raw).hostname.toLowerCase() | ||
| return host === "localhost" || host === "127.0.0.1" || host === "::1" || host === "[::1]" |
There was a problem hiding this comment.
WARNING: The loopback guard recognizes only localhost, 127.0.0.1 and ::1/[::1]. A local model server reached over the LAN or via a host alias (http://192.168.x.x:1234, http://host.docker.internal, http://ollama.local, 0.0.0.0, [::]) is treated as remote, so cheapestSmallModel still runs and can name a model the server has not installed - the exact case this guard was added to prevent. Consider also treating private/link-local hosts and .local names as local, or gating the cheap pick on an explicit opt-out.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * pick is skipped for loopback providers. An explicit non-loopback baseURL | ||
| * means the provider points at a remote server and the cheap pick applies. | ||
| */ | ||
| export function isLoopbackProvider(provider: { options?: Record<string, any>; models: Record<string, Provider.Model> }) { |
There was a problem hiding this comment.
SUGGESTION: isLoopbackProvider types the injected provider as { options?: Record<string, any>; ... }. Only baseURL/baseUrl is read, and it is narrowed with typeof raw === "string", so Record<string, unknown> works here and avoids introducing a new any.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| expect(picked).toMatchObject({ id: "budget" }) | ||
| }) | ||
|
|
||
| test("breaks cost ties by release date, then id", () => { |
There was a problem hiding this comment.
SUGGESTION: This test is named "breaks cost ties by release date, then id", but the id key is never exercised: same (2026-01-01) and sameAsNewer (2026-06-01) differ in release_date, so release_date decides both assertions. Add two models with identical cost and identical release_date but different ids to pin the third sort key.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
github-actions
Bot
added
ci-failing
and removed
needs-triage
- Add cheapestSmallModel helper to pick the cheapest chat-capable model from the provider's own catalog - Update getSmallModel to use cheapestSmallModel before falling back to Kilo's auto small model - Add comprehensive tests for the new behavior - Update existing test to reflect new expected behavior
Embedding and rerank models pass the text-in/text-out filter but cannot serve auxiliary chat tasks, so a free nomic-embed-text would beat every real chat model on price. Also restores the Kilo-Org#13192 contract tests (undefined without kilo credentials, kilo-auto/small with credentials) and adds direct coverage for cheapestSmallModel ordering, tie-breaks and the family-preference precedence in getSmallModel.
The two awaitWithTimeout calls relied on the 2 second default, which is too tight on loaded CI runners: the readiness signal never arrived within the window and the test failed with 'processor.create did not start'. Pass an explicit 10 seconds, matching the rest of the file, and raise the test budget to 20 seconds so the inner timeout reports first.
df0303b to
20b7648
Compare
|
Counter-verified
Please preserve the safe fallback for known local-server providers regardless of hostname, unless model availability is verified, and add a regression test covering a LAN/custom-host endpoint. A remote address does not imply every catalog model is deployed. All 25 targeted tests and CLI typecheck pass; no broader refactor or unrelated cleanup is needed for this follow-up. |
github-actions
Bot
added
needs-triage
|
Both review suggestions addressed in b5a8ddf:
|
…o fix/small-model-same-vendor
Fixes the gap left by #13192 (itself the fix for the 7.4.22 regression in #13151). No linked issue: this was carved out of the review discussion on #13192 — happy to file a proper issue if maintainers prefer.
What this is in response to
PR #13192 stopped
getSmallModelfrom always falling back to the cloudkilo-auto/smallmodel. That fix had two side effects for BYOK (bring-your-own-key) users:kilo-auto/smalland silently drew Kilo credits even though the user pays their own provider.getSmallModelreturnedundefined, so auxiliary tasks ran on the session's full main model — an expensive way to name a session.Change
Before falling back to
kilo-auto/small,getSmallModelnow picks the cheapest chat-capable model from the provider the user actually configured:cheapestSmallModel()inpackages/opencode/src/kilocode/provider/provider.tsfilters to text-in/text-out models. Tool calling is deliberately not required (Perplexitysonarand morph chat models advertisetool_call: false). Models without text output (image/audio/video) are skipped, and embedding/rerank models are excluded by id/family match — they take text in and return text scores but cannot produce titles or commit messages (review: a freenomic-embed-textwould otherwise beat every chat model on price). Missing cost data sorts as free; ties break by release date then id, matching the existing catalog ordering.kilo-auto/smallfallback from fix(cli): only route auxiliary tasks to kilo-auto/small with kilo credentials #13192 is unchanged and remains the last resort when the user's provider has no chat-capable model.Review response (latest revision)
embed/rerankid/family matches; direct tests pin the Tinfoilnomic-embed-textvsgpt-oss-120bcase and keepsonarselectable.undefinedwithout kilo credentials andkilo-auto/smallwith credentials, both for a provider with no chat-capable model.cheapestSmallModeltests): added five — cheapest-wins,release_date/idtie-breaks, embedding/rerank exclusion, tool-less chat models, non-text-output exclusion — plus agetSmallModeltest pinning family-match precedence over a cheaper unlisted model.Verification
bun test ./test/kilocode/provider/provider.test.ts: 10 pass / 0 failbun test ./test/provider/provider.test.ts -t "getSmallModel": 9 pass / 0 failbun run typecheck(packages/opencode) andbun turbo typecheck --filter='!@kilocode/kilo-jetbrains': passenabled_providers/loader tests were confirmed to fail identically without these changes.