fix: fix for mcp delete to remove it from mcp config file (aws#1956)

* fix: fix for mcp delete to remove it from mcp config file

* fix: fix for unit test failure

Author: ashishrp-aws

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts

@@ -242,7 +242,7 @@ export class McpEventHandler {
      */
 
     async onMcpServerClick(params: McpServerClickParams) {
-        this.#features.logging.log(`[VSCode Server] onMcpServerClick event with params: ${JSON.stringify(params)}`)
+        this.#features.logging.log(`onMcpServerClick event with params: ${JSON.stringify(params)}`)
 
         // Use a map of handlers for different action types
         const handlers: Record<string, () => Promise<any>> = {
@@ -1143,7 +1143,6 @@ export class McpEventHandler {
             this.#pendingPermissionConfig = undefined
 
             this.#features.logging.info(`Applied permission changes for server: ${serverName}`)
-            this.#isProgrammaticChange = false
             return { id: params.id }
         } catch (error) {
             this.#features.logging.error(`Failed to save MCP permissions: ${error}`)

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpManager.test.ts

@@ -26,6 +26,7 @@ const fakeWorkspace = {
         mkdir: (_: string, __: any) => Promise.resolve(),
     },
     getUserHomeDir: () => '',
+    getAllWorkspaceFolders: () => [{ uri: '/fake/workspace' }],
 }
 const features = { logging: fakeLogging, workspace: fakeWorkspace, lsp: {} } as any
 
@@ -234,10 +235,26 @@ describe('addServer()', () => {
 describe('removeServer()', () => {
     let loadStub: sinon.SinonStub
     let saveAgentConfigStub: sinon.SinonStub
+    let existsStub: sinon.SinonStub
+    let readFileStub: sinon.SinonStub
+    let writeFileStub: sinon.SinonStub
+    let mkdirStub: sinon.SinonStub
+    let getWorkspaceMcpConfigPathsStub: sinon.SinonStub
+    let getGlobalMcpConfigPathStub: sinon.SinonStub
 
     beforeEach(() => {
         loadStub = stubAgentConfig()
         saveAgentConfigStub = sinon.stub(mcpUtils, 'saveAgentConfig').resolves()
+        existsStub = sinon.stub(fakeWorkspace.fs, 'exists').resolves(true)
+        readFileStub = sinon
+            .stub(fakeWorkspace.fs, 'readFile')
+            .resolves(Buffer.from(JSON.stringify({ mcpServers: { x: {} } })))
+        writeFileStub = sinon.stub(fakeWorkspace.fs, 'writeFile').resolves()
+        mkdirStub = sinon.stub(fakeWorkspace.fs, 'mkdir').resolves()
+        getWorkspaceMcpConfigPathsStub = sinon
+            .stub(mcpUtils, 'getWorkspaceMcpConfigPaths')
+            .returns(['ws1/config.json', 'ws2/config.json'])
+        getGlobalMcpConfigPathStub = sinon.stub(mcpUtils, 'getGlobalMcpConfigPath').returns('global/config.json')
     })
 
     afterEach(async () => {
@@ -275,6 +292,106 @@ describe('removeServer()', () => {
         expect(saveAgentConfigStub.calledOnce).to.be.true
         expect((mgr as any).clients.has('x')).to.be.false
     })
+
+    it('removes server from all config files', async () => {
+        const mgr = await McpManager.init([], features)
+        const dummy = new Client({ name: 'c', version: 'v' })
+        ;(mgr as any).clients.set('x', dummy)
+        ;(mgr as any).mcpServers.set('x', {
+            command: '',
+            args: [],
+            env: {},
+            timeout: 0,
+            __configPath__: 'c.json',
+        } as MCPServerConfig)
+        ;(mgr as any).serverNameMapping.set('x', 'x')
+        ;(mgr as any).agentConfig = {
+            name: 'test-agent',
+            version: '1.0.0',
+            description: 'Test agent',
+            mcpServers: { x: {} },
+            tools: ['@x'],
+            allowedTools: [],
+            toolsSettings: {},
+            includedFiles: [],
+            resources: [],
+        }
+
+        await mgr.removeServer('x')
+
+        // Verify that writeFile was called for each config path (2 workspace + 1 global)
+        expect(writeFileStub.callCount).to.equal(3)
+
+        // Verify the content of the writes (should have removed the server)
+        writeFileStub.getCalls().forEach(call => {
+            const content = JSON.parse(call.args[1])
+            expect(content.mcpServers).to.not.have.property('x')
+        })
+    })
+})
+
+describe('mutateConfigFile()', () => {
+    let existsStub: sinon.SinonStub
+    let readFileStub: sinon.SinonStub
+    let writeFileStub: sinon.SinonStub
+    let mkdirStub: sinon.SinonStub
+    let mgr: McpManager
+
+    beforeEach(async () => {
+        sinon.restore()
+        stubAgentConfig()
+        existsStub = sinon.stub(fakeWorkspace.fs, 'exists').resolves(true)
+        readFileStub = sinon
+            .stub(fakeWorkspace.fs, 'readFile')
+            .resolves(Buffer.from(JSON.stringify({ mcpServers: { test: {} } })))
+        writeFileStub = sinon.stub(fakeWorkspace.fs, 'writeFile').resolves()
+        mkdirStub = sinon.stub(fakeWorkspace.fs, 'mkdir').resolves()
+        mgr = await McpManager.init([], features)
+    })
+
+    afterEach(async () => {
+        sinon.restore()
+        try {
+            await McpManager.instance.close()
+        } catch {}
+    })
+
+    it('reads, mutates, and writes config file', async () => {
+        // Access the private method using type assertion
+        const mutateConfigFile = (mgr as any).mutateConfigFile.bind(mgr)
+
+        await mutateConfigFile('test/path.json', (json: any) => {
+            json.mcpServers.newServer = { command: 'test' }
+            delete json.mcpServers.test
+        })
+
+        expect(readFileStub.calledOnce).to.be.true
+        expect(writeFileStub.calledOnce).to.be.true
+
+        // Verify the content was modified correctly
+        const writtenContent = JSON.parse(writeFileStub.firstCall.args[1])
+        expect(writtenContent.mcpServers).to.have.property('newServer')
+        expect(writtenContent.mcpServers).to.not.have.property('test')
+    })
+
+    it('creates new config file if it does not exist', async () => {
+        existsStub.resolves(false)
+        readFileStub.rejects({ code: 'ENOENT' })
+
+        // Access the private method using type assertion
+        const mutateConfigFile = (mgr as any).mutateConfigFile.bind(mgr)
+
+        await mutateConfigFile('test/path.json', (json: any) => {
+            json.mcpServers.newServer = { command: 'test' }
+        })
+
+        expect(mkdirStub.calledOnce).to.be.true
+        expect(writeFileStub.calledOnce).to.be.true
+
+        // Verify the content was created correctly
+        const writtenContent = JSON.parse(writeFileStub.firstCall.args[1])
+        expect(writtenContent.mcpServers).to.have.property('newServer')
+    })
 })
 
 describe('updateServer()', () => {

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpManager.ts

@@ -14,11 +14,18 @@ import {
     McpServerRuntimeState,
     McpServerStatus,
     McpPermissionType,
-    PersonaModel,
     MCPServerPermission,
     AgentConfig,
 } from './mcpTypes'
-import { isEmptyEnv, loadAgentConfig, saveAgentConfig, sanitizeName, getGlobalAgentConfigPath } from './mcpUtils'
+import {
+    isEmptyEnv,
+    loadAgentConfig,
+    saveAgentConfig,
+    sanitizeName,
+    getGlobalAgentConfigPath,
+    getWorkspaceMcpConfigPaths,
+    getGlobalMcpConfigPath,
+} from './mcpUtils'
 import { AgenticChatError } from '../../errors'
 import { EventEmitter } from 'events'
 import { Mutex } from 'async-mutex'
@@ -658,6 +665,30 @@ export class McpManager {
 
             // Save agent config
             await saveAgentConfig(this.features.workspace, this.features.logging, this.agentConfig, cfg.__configPath__)
+
+            // Get all config paths and delete the server from each one
+            const wsUris = this.features.workspace.getAllWorkspaceFolders()?.map(f => f.uri) ?? []
+            const wsConfigPaths = getWorkspaceMcpConfigPaths(wsUris)
+            const globalConfigPath = getGlobalMcpConfigPath(this.features.workspace.fs.getUserHomeDir())
+            const allConfigPaths = [...wsConfigPaths, globalConfigPath]
+
+            // Delete the server from all config files
+            for (const configPath of allConfigPaths) {
+                try {
+                    await this.mutateConfigFile(configPath, json => {
+                        if (json.mcpServers && json.mcpServers[unsanitizedName]) {
+                            delete json.mcpServers[unsanitizedName]
+                            this.features.logging.info(
+                                `Deleted server '${unsanitizedName}' from config file: ${configPath}`
+                            )
+                        }
+                    })
+                } catch (err) {
+                    this.features.logging.warn(
+                        `Failed to delete server '${unsanitizedName}' from config file ${configPath}: ${err}`
+                    )
+                }
+            }
         }
 
         this.mcpServers.delete(serverName)
@@ -1047,6 +1078,46 @@ export class McpManager {
         }
     }
 
+    /**
+     * Read, mutate, and write the MCP JSON config at the given path.
+     * @private
+     */
+    private async mutateConfigFile(configPath: string, mutator: (json: any) => void): Promise<void> {
+        return McpManager.configMutex
+            .runExclusive(async () => {
+                let json: any = { mcpServers: {} }
+                try {
+                    const raw = await this.features.workspace.fs.readFile(configPath)
+                    this.features.logging.info(`Updating MCP config file: ${configPath}`)
+                    const existing = JSON.parse(raw.toString())
+                    json = { mcpServers: {}, ...existing }
+                } catch (err: any) {
+                    // ignore fire not exist error
+                    if (err?.code !== 'ENOENT') throw err
+                }
+                mutator(json)
+
+                let fsPath: string
+                try {
+                    const uri = URI.parse(configPath)
+                    fsPath = uri.scheme === 'file' ? uri.fsPath : configPath
+                } catch {
+                    fsPath = configPath
+                }
+                fsPath = path.normalize(fsPath)
+
+                const dir = path.dirname(fsPath)
+                await this.features.workspace.fs.mkdir(dir, { recursive: true })
+
+                await this.features.workspace.fs.writeFile(fsPath, JSON.stringify(json, null, 2))
+                this.features.logging.debug(`MCP config file write complete: ${configPath}`)
+            })
+            .catch((e: any) => {
+                this.features.logging.error(`MCP: failed to update config at ${configPath}: ${e.message}`)
+                throw e
+            })
+    }
+
     public getOriginalToolNames(namespacedName: string): { serverName: string; toolName: string } | undefined {
         return this.toolNameMapping.get(namespacedName)
     }