fix(amazonq): update mcp and persona config to agent config #1897
Conversation
| // Get agent paths | ||
| const wsAgentPaths = getWorkspaceAgentConfigPaths(wsUris) | ||
| const globalAgentPath = getGlobalAgentConfigPath(workspace.fs.getUserHomeDir()) | ||
| const allAgentPaths = [...wsAgentPaths, globalAgentPath] | ||
|
|
||
| const wsPersonaPaths = getWorkspacePersonaConfigPaths(wsUris) |
There was a problem hiding this comment.
I am a bit confused, why we still keep the persona? could you shared the final decision that Chay's aligned?
There was a problem hiding this comment.
Not keeping persona at all. removed this
| "report_issue", | ||
| "use_aws" | ||
| ], | ||
| "allowedTools": [ |
There was a problem hiding this comment.
this will only control ask/always right?
|
|
||
| // Initialize permissions for this server | ||
| // Check if server is enabled (either as @server or has specific tools like @server/tool) | ||
| const serverPrefix = `@${name}` |
There was a problem hiding this comment.
I thought we don't support enable/disable anymore? also, do we expect to have any tools in the allowedtools section if the server is missing?
There was a problem hiding this comment.
we don't. removed the isEnabled check in toolPerms
| const toolId = `${serverPrefix}/${toolName}` | ||
|
|
||
| if (permission === McpPermissionType.deny) { | ||
| // For deny: if server is enabled as a whole, we need to switch to individual tools |
There was a problem hiding this comment.
deny is pretty much the disable before, so currently we remove the server level enable/disable, but still keep the tool level ones right? is this aligned with Chay and Matt?
There was a problem hiding this comment.
No, deny is not disable at server level. we check deny for tool permissions, so we enable rest of the tools and deny/remove the denied tools.
There was a problem hiding this comment.
what I mean is deny is the tool level disable right? do we need to block it from been adding to the Agent and memory?
| @@ -825,7 +998,9 @@ export class McpManager { | |||
| * Check if a tool requires approval. | |||
There was a problem hiding this comment.
do we still need to two mutate method above? if yes, it means there are some changes needed right? all CRUD methods shouldn't touch both mcp and persona files anymore right?
There was a problem hiding this comment.
we are not touching mcp and persona files at all. i removed the mutate functions for both
|
|
||
| // Convert tools and permissions | ||
| if (persona.toolPerms) { | ||
| // Track which servers have specific tools enabled |
There was a problem hiding this comment.
We should handle * in persona as well.
There was a problem hiding this comment.
- for mcp servers right. since we don't have disbaled anymore. there are way to many edge cases, so far migration i'm keeping all tools default to
Askunless it italwaysAllowed.
There was a problem hiding this comment.
For the current customer, it could has persona that contains * right?, just want to make sure it will be handled.
There was a problem hiding this comment.
Doesn't matter if it *, we just enable all tools for all servers. Since there is no disable server anymore
|
|
||
| // Read MCP server configs directly from files | ||
| const serverConfigs: Record<string, MCPServerConfig> = {} | ||
| for (const configPath of configPaths) { |
There was a problem hiding this comment.
do we still care about handling the conflicts? like same server in all 4 files with different settings.
There was a problem hiding this comment.
No we don't....we should gloabl at global level.
All the changes are here in this PR.
Yes, want to test with a custom build to test before with team. can arrange one on friday |
Problem
We need to update the mcp config and persona config to the new agent config format for Agent building and parity with Q CLI
Solution
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.