fix(vscode): persist installed agent removal - #12542
Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The latest commit ( Files Reviewed (8 files changed since last review)
Previous Review Summaries (2 snapshots, latest commit bfeb3ae)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit bfeb3ae)Status: No Issues Found | Recommendation: Merge The latest commit removes the post-removal verification step ( Files Reviewed (1 file changed since last review)
Previous review (commit 5d713f7)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (14 files)
The core fix (reconciling removal across every editable config source reported by Reviewed by kimi-k3 · Input: 242K · Output: 10.6K · Cached: 2M Review guidance: REVIEW.md from base branch |
…oval-persistence # Conflicts: # packages/kilo-vscode/tests/unit/agent-behaviour-patches.test.ts # packages/kilo-vscode/webview-ui/src/components/settings/AgentBehaviourTab.tsx # packages/kilo-vscode/webview-ui/src/components/settings/agent-behaviour-patches.ts
Co-Authored-By: Thomas Brugman <thomas.brugman.teb3@gmail.com>
bagatao-anaconda
left a comment
There was a problem hiding this comment.
Thanks for tracking this down 🙂 Good catch on the root cause (removal only touching one file per scope while loading merges several). The fix itself is solid and the test coverage for the duplicate-source case is a nice addition.
I left a few comments inline, but the main one worth resolving before merge is the marketplace scope change (removing an agent now seems to wipe it from both project and global config regardless of which scope you pick, which is a behavior change from how MCPs/skills are removed). Want to double check that's intentional?
The others (lost error message on failure, the hardcoded source-kind list, a couple of small nits) aren't blockers but worth a look.
|
The description says the endpoint "reloads the instance and only reports success when no custom agent with that name remains," but I don't see that check in the diff (it just passes Right now it succeeds as long as removal happened in at least one file, not because it verified nothing's left. Might just be a wording thing, but wanted to flag it in case the stronger guarantee was meant to be there. |
|
This code is also relevant to the PR.
Since this PR adds a toast to actually show removal errors to the user, they'll just see a generic/blank message instead of the real reason. Can we pass the message through, e.g. new |
|
Thanks for the review. The issues should now be adressed. |
|
@Githubguy132010 Thanks for the great work on this, and for addressing all the review feedback so thoroughly. I've merged |
Issue
Fixes #11579
Context
Agent configuration loading merges every supported JSON/JSONC source, but removal only edited one preferred global file and one preferred project file. A legacy definition in another loaded source could therefore survive a successful removal response and reappear after state refreshed or VS Code restarted. Marketplace agent removal also used a separate file-only path, so it could uninstall the marketplace package while leaving the surviving legacy definition active.
Implementation
Agent removal now reconciles matching definitions across every existing editable config source in the requested scope, while continuing to reject organization-managed agents. Callers can select project or global scope; when scope is omitted, Settings-style removal reconciles all writable scopes. After a successful disk change, the endpoint disposes cached instance state so the next access reloads it.
Settings and Marketplace now share one extension-side CLI removal helper and surface backend failures. Helpful backend removal messages are preserved in HTTP 400 responses, and empty client errors use friendly fallbacks. The Settings UI also carries agent source metadata so organization-managed agents no longer show a removal action.
Screenshots / Video
How to Test
Manual/local verification
.kilo/kilo.jsoncand.kilo/opencode.jsonc; focused tests confirm duplicate definitions and staledefault_agentvalues are removed when scope is omitted.Reviewer test steps
.kilo/kilo.jsoncand.kilo/opencode.jsonc.Blocked checks and substitute verification
Checklist
Get in Touch
GitHub: @Githubguy132010