Skip to content

refactor(sync): remove unused model metadata logic in kilo provider#3235

Open
levdad wants to merge 10 commits into
anomalyco:devfrom
levdad:kilo-sync-cleanup
Open

refactor(sync): remove unused model metadata logic in kilo provider#3235
levdad wants to merge 10 commits into
anomalyco:devfrom
levdad:kilo-sync-cleanup

Conversation

@levdad

@levdad levdad commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Remove unused file system imports, constants, and helper functions (modelMetadataExists, baseModelOmit, baseModelOverrides, etc.) that were previously used for managing model metadata via local TOML files. This cleans up the Kilo sync provider by removing dead code and unnecessary dependencies on node:fs and node:path.

based on the review of #2997 comment

Remove unused file system imports, constants, and helper functions
(`modelMetadataExists`, `baseModelOmit`, `baseModelOverrides`, etc.)
that were previously used for managing model metadata via local TOML
files. This cleans up the Kilo sync provider by removing dead code
and unnecessary dependencies on `node:fs` and `node:path`.
@github-actions

Copy link
Copy Markdown
Contributor

No actionable findings.

levdad added 4 commits July 14, 2026 13:21
Apply consistent code formatting to the Kilo sync provider, including
multi-line type imports, Zod schema definitions, and improved line
wrapping for complex conditional expressions and function calls.
Ensure that `apiDescription` is explicitly converted to `null` if it is
falsy before falling back to `describeModel`. This prevents potential
issues with truthiness checks and ensures consistent object structure
when the description is missing.
Update the type assertion for `orderedEfforts` in the Kilo provider to
use a conditional type check against `SyncedFullModel`. This ensures
that the `values` property strictly adheres to the expected type
definition of the reasoning options, preventing type mismatches
during synchronization.
@github-actions

Copy link
Copy Markdown
Contributor

No actionable findings.

@levdad

levdad commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Added a fix for the error when the api returned an empty description for a model, like the error in GitHub Action.

@rekram1-node can you have a look at this?

levdad added 3 commits July 15, 2026 10:28
…hinking variants

Update Kilo and OpenRouter providers to recognize and correctly process
model IDs ending in `:discounted` and `:thinking`. This ensures these
specialized model variants are properly mapped to their canonical base
models during the synchronization process.
Adjust the priority of model description resolution to ensure that
`apiDescription` is preferred over `existing.description`, while
maintaining the fallback to `describeModel` if neither is available.
@github-actions

Copy link
Copy Markdown
Contributor

No actionable findings.

@levdad

levdad commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Reordering the description logic to prioritize the provider/API description.
This ensures we surface much richer details about how the provider uses the models (such as training data), especially regarding discounted or free models.

Verified in 61cf5c5

@github-actions

Copy link
Copy Markdown
Contributor

Action items

  • [medium] [possible mistake] packages/core/src/sync/providers/openrouter.ts:307 - Check: OpenRouter buildOpenRouterModel must preserve the display name for any model ID suffix that resolveCanonicalBaseModel strips. Why: This commit makes resolveCanonicalBaseModel strip :discounted and :thinking (in addition to :free), so future OpenRouter IDs like vendor/model:discounted or vendor/model:thinking will now resolve to a canonical base model and be factored via factorBaseModel. However, the name guard in buildOpenRouterModel at line 219 (baseModel !== undefined || model.id.endsWith(":free") || canonicalOverride === canonical) was not updated to include the new suffixes. The same commit did update the analogous name guard in buildKiloModel for :discounted/:thinking, so the OpenRouter side is asymmetric. When such a variant appears and matches a models/ entry, its name will be set to undefined and it will silently inherit the base model's name, losing the variant's distinct display name. Action: Add model.id.endsWith(":discounted") and model.id.endsWith(":thinking") to the name preservation condition in buildOpenRouterModel, or confirm and document that inheriting the base model name is intended for these OpenRouter variants (and explain why Kilo differs).

  • [medium] [possible mistake] packages/core/src/sync/providers/kilo.ts:217 (and the inline return at :251) - Check: The description fallback reorder must not silently overwrite preserved descriptions with unverified API text. Why: The new ordering (apiDescription ? apiDescription : null) ?? existing?.description ?? describeModel(...) reverses the previous existing?.description ?? apiDescription ?? describeModel(...) precedence. On the next Kilo sync, every model for which the Kilo API returns a non-empty description will have its existing TOML description overwritten with the API string, because the runner's preserveDescription only kicks in when the translated model has no description. This diverges from OpenRouter, which does not use an apiDescription at all. The existing Kilo TOMLs currently hold describeModel()-generated summaries, so this is a catalog-wide data change triggered by sync code. The PR body only describes dead-code removal and does not mention or justify this precedence change, and no evidence is provided about what Kilo's API returns for description or why it should be authoritative over preserved descriptions. Action: Either revert to existing?.description ?? (apiDescription ? apiDescription : null) ?? describeModel(...) (keeping the empty-string fix from patch 3), or justify in the PR body — with a citation to Kilo's API reference — that Kilo's API description is authoritative over preserved TOML descriptions, and confirm a dry-run sync diff is acceptable.

  • [low] [violation] PR body - Check: PRs that change sync behavior should document and source the behavior changes per AGENTS.md ("Citations"). Why: The PR body only describes the dead-code removal from commit 1, but the diff ships five additional commits that change behavior: empty-apiDescription normalization, the reasoning-effort type cast, :discounted/:thinking canonicalization in both Kilo and OpenRouter, and the description-fallback reorder. These are not pure refactors and will alter generated TOML content on the next sync. Action: Update the PR body to describe each behavior change and cite the Kilo/OpenRouter API documentation that supports the new canonicalization suffixes and the description-precedence change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant