fix: warnings and errors for unresolved MCP secrets #8656
Conversation
dosubot
bot
added
the
size:M
|
✅ Review Complete Code Review Summary |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="core/context/mcp/MCPConnection.ts">
<violation number="1" location="core/context/mcp/MCPConnection.ts:172">
decodeSecretLocation expects a colon-delimited secret location (e.g. "user:slug/secret"), but this passes plain secret keys like "OPENAI_API_KEY". When unresolved secrets are present, decodeSecretLocation throws and connectClient crashes instead of collecting the warning.</violation>
</file>
<file name="extensions/cli/src/services/MCPService.ts">
<violation number="1" location="extensions/cli/src/services/MCPService.ts:257">
decodeSecretLocation is being invoked on every template variable, so any non-secret placeholder (e.g. ${{ inputs.* }}) makes rest undefined inside decodeSecretLocation and causes connectServer to throw before establishing the connection.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| const vars = getTemplateVariables(JSON.stringify(this.options)); | ||
| const unrendered = vars.map((v) => { | ||
| return decodeSecretLocation(v.replace("secrets.", "")).secretName; |
There was a problem hiding this comment.
decodeSecretLocation expects a colon-delimited secret location (e.g. "user:slug/secret"), but this passes plain secret keys like "OPENAI_API_KEY". When unresolved secrets are present, decodeSecretLocation throws and connectClient crashes instead of collecting the warning.
Prompt for AI agents
Address the following comment on core/context/mcp/MCPConnection.ts at line 172:
<comment>decodeSecretLocation expects a colon-delimited secret location (e.g. "user:slug/secret"), but this passes plain secret keys like "OPENAI_API_KEY". When unresolved secrets are present, decodeSecretLocation throws and connectClient crashes instead of collecting the warning.</comment>
<file context>
@@ -163,6 +167,19 @@ class MCPConnection {
+ const vars = getTemplateVariables(JSON.stringify(this.options));
+ const unrendered = vars.map((v) => {
+ return decodeSecretLocation(v.replace("secrets.", "")).secretName;
+ });
+
</file context>
| return decodeSecretLocation(v.replace("secrets.", "")).secretName; | |
| return v.replace("secrets.", ""); |
|
|
||
| const vars = getTemplateVariables(JSON.stringify(serverConfig)); | ||
| const unrendered = vars.map((v) => { | ||
| return decodeSecretLocation(v.replace("secrets.", "")).secretName; |
There was a problem hiding this comment.
decodeSecretLocation is being invoked on every template variable, so any non-secret placeholder (e.g. ${{ inputs.* }}) makes rest undefined inside decodeSecretLocation and causes connectServer to throw before establishing the connection.
Prompt for AI agents
Address the following comment on extensions/cli/src/services/MCPService.ts at line 257:
<comment>decodeSecretLocation is being invoked on every template variable, so any non-secret placeholder (e.g. ${{ inputs.* }}) makes rest undefined inside decodeSecretLocation and causes connectServer to throw before establishing the connection.</comment>
<file context>
@@ -248,7 +252,23 @@ export class MCPService
+ const vars = getTemplateVariables(JSON.stringify(serverConfig));
+ const unrendered = vars.map((v) => {
+ return decodeSecretLocation(v.replace("secrets.", "")).secretName;
+ });
+
</file context>
| sourceFile: doc.sourceFile, | ||
| })); | ||
|
|
||
| config.mcpServers?.forEach((mcpServer) => { |
There was a problem hiding this comment.
removed this since it only applied to args. Moved to mcp loading
|
|
||
| try { | ||
| if (unrendered.length > 0) { | ||
| const message = `${serverConfig.name} MCP Server has unresolved secrets: ${unrendered.join(", ")} |
There was a problem hiding this comment.
This seems duplicate but the message is different enough for CLI since process.env is supported that I decided to leave them separate.
Summary by cubic
Detect unresolved MCP secrets and surface clear warnings in extensions; in headless CLI, throw an error to prevent misconfigured runs. Secret checks are now done at connection time for more accurate feedback.
Bug Fixes
Refactors
Written for commit 86ed8e9. Summary will update automatically on new commits.