Skip to content

fix(vscode): allow removing provider reasoning - #11238

Merged
marius-kilocode merged 3 commits into
mainfrom
fix-11193-remove-reasoning
Jun 15, 2026
Merged

fix(vscode): allow removing provider reasoning#11238
marius-kilocode merged 3 commits into
mainfrom
fix-11193-remove-reasoning

Conversation

@marius-kilocode

Copy link
Copy Markdown
Collaborator

Edit Provider omits a model's reasoning field when it is disabled, but provider updates are deep-merged with the existing global config. The previously saved reasoning: true therefore survived and reappeared when the provider was reloaded.

Emit the same explicit null deletion sentinel already used for removed models and variants when reasoning is cleared, and accept that sentinel through the config update schema. Existing provider fields remain intact, including sibling models and model variants, while the persisted reasoning override is removed.

Regression coverage exercises the extension save path and the real global config add, persist, remove, and reload lifecycle.

Fixes #11193

@marius-kilocode
marius-kilocode enabled auto-merge (squash) June 15, 2026 09:13
@kilo-code-bot

kilo-code-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The only change since the previous review at d3ce281 is a cosmetic line-wrap in a test assertion (custom-provider.test.ts). No logic, types, or behavior changed.

Files Reviewed (6 files)
  • .changeset/calm-models-forget.md
  • packages/kilo-vscode/src/shared/custom-provider.ts
  • packages/kilo-vscode/tests/unit/custom-provider.test.ts
  • packages/kilo-vscode/tests/unit/provider-actions-save.test.ts
  • packages/opencode/src/config/provider.ts
  • packages/opencode/test/kilocode/config/config.test.ts
Previous Review Summary (commit d3ce281)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit d3ce281)

Status: No Issues Found | Recommendation: Merge

The fix is correct and well-scoped. The deep-merge/stripNulls pattern is already in use for model and variant deletions; this PR extends it consistently to the reasoning field.

Key observations:

  • ProviderPatch type correctly widens reasoning to true | null for the intermediate patch, and the as SanitizedProviderConfig cast at line 200 is intentional — callers receive null sentinels that stripNulls will eliminate on the CLI side.
  • The guard oldModel.reasoning !== undefined && newModel.reasoning === undefined correctly captures the "was set, now removed" case, including when the prior value was null.
  • The Schema.NullOr(Schema.Boolean) change in provider.ts is the minimal schema change needed to let the null sentinel pass through the CLI's update endpoint without being rejected.
  • Test coverage exercises the extension save path (unit), the withCustomProviderDeletions helper directly (unit), and the real global config add/persist/remove/reload lifecycle (integration) — solid regression coverage.
Files Reviewed (5 files)
  • .changeset/calm-models-forget.md
  • packages/kilo-vscode/src/shared/custom-provider.ts
  • packages/kilo-vscode/tests/unit/custom-provider.test.ts
  • packages/kilo-vscode/tests/unit/provider-actions-save.test.ts
  • packages/opencode/src/config/provider.ts
  • packages/opencode/test/kilocode/config/config.test.ts

Reviewed by claude-4.6-sonnet-20260217 · 131,810 tokens

Review guidance: REVIEW.md from base branch main

@marius-kilocode
marius-kilocode merged commit fb2db2e into main Jun 15, 2026
21 checks passed
@marius-kilocode
marius-kilocode deleted the fix-11193-remove-reasoning branch June 15, 2026 10:00
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 2026
* fix(vscode): allow removing provider reasoning

* style(vscode): format provider regression test

---------

Co-authored-by: Christiaan Arnoldus <christiaan.arnoldus@outlook.com>
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.

[Bug] Edit Provider Broken — Reasoning Removal Fails

2 participants