feat(mcp): add managed MCP server profiles - #24
Conversation
📝 WalkthroughWalkthroughAdded workspace-scoped management for local STDIO and remote Streamable HTTP MCP servers. Profiles use workspace state and secret storage, while the provider and chat panel support add, connect, disconnect, remove, and status refresh operations. ChangesManaged MCP server management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Webview
participant ChatViewProvider
participant McpServerService
participant McpClient
Webview->>ChatViewProvider: Request add or connection operation
ChatViewProvider->>McpServerService: Execute MCP lifecycle method
McpServerService->>McpClient: Apply server mutation
McpClient-->>McpServerService: Return operation state
ChatViewProvider-->>Webview: Send result and refreshed MCP status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
webview/shared/src/chat/index.cssParsing error: Expression expected. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/services/McpServerService.ts (1)
185-188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing secrets become empty values without any signal.
readSecretssubstitutes""for a key that has no stored secret. On rehydration the service then registers the server with empty environment values or empty header values, so the MCP server fails with an opaque authentication error. Log a warning when a declared key has no stored secret, so the failure is diagnosable.♻️ Proposed change
private async readSecrets(id: string, kind: string, keys: string[]): Promise<Record<string, string>> { - const entries = await Promise.all(keys.map(async (key) => [key, await this.context.secrets.get(this.secretKey(id, kind, key)) ?? ""] as const)); + const entries = await Promise.all(keys.map(async (key) => { + const value = await this.context.secrets.get(this.secretKey(id, kind, key)); + if (value === undefined) this.log.warn("Managed MCP secret is missing", { profileId: id, kind, key }); + return [key, value ?? ""] as const; + })); return Object.fromEntries(entries); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/McpServerService.ts` around lines 185 - 188, Update readSecrets to detect when context.secrets.get returns no value for a declared key, log a warning identifying the missing secret, and still preserve the existing empty-string return behavior for that key.tests/services/mcp-server-service.test.mjs (1)
9-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd behavioral coverage for the new lifecycle service.
All three tests assert on source text, so they check formatting rather than behavior.
McpServerServicetakes injectedcontext,getWorkspaceDirectory,getClient, andlog, so it is directly testable with fakes. The paths that carry real risk have no coverage: rollback inaddwhenmcp.addrejects, secret keys written and deleted per profile, name validation and HTTPS enforcement, serialization inwithServerLock, and the retry loop instatus.Do you want me to generate a Vitest suite that instantiates
McpServerServicewith in-memoryworkspaceStateandsecretsfakes and covers these paths?As per coding guidelines: "Maintain regression and contract coverage for behavior changes; use Node's built-in runner for
.test.mjstests and Vitest for targeted unit tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/services/mcp-server-service.test.mjs` around lines 9 - 24, Replace the source-text assertions in the tests with behavioral coverage that instantiates McpServerService using in-memory context, workspaceState, secrets, getWorkspaceDirectory, getClient, and log fakes. Add tests for add rollback when mcp.add rejects, per-profile secret key creation and deletion, name validation and HTTPS enforcement, serialized execution through withServerLock, and status retry behavior, using the Node test runner for this .test.mjs suite.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/providers/ChatViewProvider.ts`:
- Line 4420: Update the MCP operation flow around the mcpOperationStarted
postMessage in ChatViewProvider so the message has a corresponding webview
consumer that updates McpServerInfo.operation and displays
add/connect/disconnect/remove operations as in progress; otherwise remove the
unused mcpOperationStarted message and operation payload. Keep the existing
mcpOperationResult handling consistent with whichever approach is chosen.
- Around line 4421-4442: Update the catch block in the MCP operation handler to
log the caught error through the existing host logging mechanism and forward its
user-safe message in the failed mcpOperationResult response. Preserve the
generic fallback only when the caught value does not provide a usable message,
while retaining the existing error notification.
In `@src/services/McpServerService.ts`:
- Around line 90-100: Update remove to use the resolved profile’s name for both
withServerLock and the mcp.disconnect scope, while continuing to remove the
profile selected by profileId or name. Replace the caller-supplied name in those
operations with profile.name to keep locking, disconnecting, and deletion
aligned.
In `@tests/services/mcp-server-service.test.mjs`:
- Around line 26-30: Update the status enrichment test around “status enrichment
exposes only safe managed metadata” to scope the forbidden environment and
headers checks to the actual enrichment block or, preferably, assert against the
constructed payload rather than scanning the entire provider source. Preserve
the existing managed, profileId, and kind assertions while preventing unrelated
later code from causing failures.
In `@webview/shared/src/chat/PanelComponents.tsx`:
- Line 4635: Update the repeated Inputs in the environmentRows mapping within
the MCP form section so their aria-label values include the row identifier and
use mode-correct terminology: “Environment variable name/value” for local mode
and “Header name/value” for remote mode. Keep the existing field names and row
behavior unchanged.
- Line 4539: Replace crypto.randomUUID with the existing local UUID generator
when creating request IDs for addMcpServer, connectMcpServer,
disconnectMcpServer, and removeMcpServer message calls, preserving the current
message payloads and request ID behavior.
---
Nitpick comments:
In `@src/services/McpServerService.ts`:
- Around line 185-188: Update readSecrets to detect when context.secrets.get
returns no value for a declared key, log a warning identifying the missing
secret, and still preserve the existing empty-string return behavior for that
key.
In `@tests/services/mcp-server-service.test.mjs`:
- Around line 9-24: Replace the source-text assertions in the tests with
behavioral coverage that instantiates McpServerService using in-memory context,
workspaceState, secrets, getWorkspaceDirectory, getClient, and log fakes. Add
tests for add rollback when mcp.add rejects, per-profile secret key creation and
deletion, name validation and HTTPS enforcement, serialized execution through
withServerLock, and status retry behavior, using the Node test runner for this
.test.mjs suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71c1fcd6-0044-4996-84fd-d32a454fd0d2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
README.mdpackage.jsonsrc/providers/ChatViewProvider.tssrc/services/McpServerService.tstests/services/mcp-server-service.test.mjswebview/shared/src/chat/PanelComponents.tsxwebview/shared/src/chat/index.csswebview/shared/src/chat/lib/messageHandler.tswebview/shared/src/chat/lib/types.ts
| ); | ||
| if (confirmation !== "Remove") break; | ||
| } | ||
| this.view?.webview.postMessage({ type: "mcpOperationStarted", requestID, operation, serverName, profileID }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether any webview code consumes mcpOperationStarted or McpServerInfo.operation.
set -euo pipefail
rg -n -C3 'mcpOperationStarted' --glob '!**/dist/**'
rg -n -C3 '\boperation\b' webview/shared/src/chat/lib/types.ts webview/shared/src/chat/PanelComponents.tsx --glob '!**/dist/**'Repository: chryzxc/vscode-opencode
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching ChatViewProvider/messageHandler/types =="
git ls-files | rg '(^|/)ChatViewProvider\.ts$|messageHandler\.ts$|types\.ts$' | head -200
echo "== locate provider/webview files =="
fd 'ChatViewProvider\.ts|messageHandler\.ts|types\.ts|PanelComponents\.tsx' . | sed -n '1,200p'
echo "== broad mcpOperationStarted search excluding dist if possible =="
rg -n -C3 'mcpOperationStarted|mcpOperationResult|McpServerInfo' . || trueRepository: chryzxc/vscode-opencode
Length of output: 8910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ChatViewProvider MCP postMessage block =="
sed -n '4380,4445p' src/providers/ChatViewProvider.ts | nl -ba -v 4380
echo "== messageHandler mcpOperationResult block =="
sed -n '15690,15800p' webview/shared/src/chat/lib/messageHandler.ts | nl -ba -v 15690
echo "== McpServerInfo type =="
sed -n '880,905p' webview/shared/src/chat/lib/types.ts | nl -ba -v 880
echo "== all non-backup references to mcpOperationStarted/operation in focused files =="
rg -n -C2 'mcpOperationStarted|case "mcpOperationStarted"|operation:' src/providers/ChatViewProvider.ts webview/shared/src/chat/lib/messageHandler.ts webview/shared/src/chat/lib/types.ts || true
echo "== SET_MCP_SERVERS implementation =="
rg -n -C3 'SET_MCP_SERVERS' webview/shared/src/chat/lib/store.ts webview/shared/src/chat/lib/messageHandler.ts src/providers/ChatViewProvider.ts || trueRepository: chryzxc/vscode-opencode
Length of output: 246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' "== ChatViewProvider MCP postMessage block =="
sed -n '4380,4445p' src/providers/ChatViewProvider.ts
printf '\n%s\n' "== messageHandler mcpOperationResult block =="
sed -n '15690,15800p' webview/shared/src/chat/lib/messageHandler.ts
printf '\n%s\n' "== McpServerInfo type =="
sed -n '880,905p' webview/shared/src/chat/lib/types.ts
printf '\n%s\n' "== all non-backup references to mcpOperationStarted/operation in focused files =="
rg -n -C2 'mcpOperationStarted|case "mcpOperationStarted"|operation:' src/providers/ChatViewProvider.ts webview/shared/src/chat/lib/messageHandler.ts webview/shared/src/chat/lib/types.ts || true
printf '\n%s\n' "== SET_MCP_SERVERS implementation =="
rg -n -C3 'SET_MCP_SERVERS' webview/shared/src/chat/lib/store.ts webview/shared/src/chat/lib/messageHandler.ts src/providers/ChatViewProvider.ts || trueRepository: chryzxc/vscode-opencode
Length of output: 11741
Add a consumer for mcpOperationStarted or remove it.
this.view?.webview.postMessage({ type: "mcpOperationStarted", ... }) posts start state, but the webview only handles mcpOperationResult and McpServerInfo.operation is never set, so MCP add/connect/disconnect/remove operations never show an in-progress state. Add the missing handler/reducer payload path, or drop this message and operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/ChatViewProvider.ts` at line 4420, Update the MCP operation
flow around the mcpOperationStarted postMessage in ChatViewProvider so the
message has a corresponding webview consumer that updates
McpServerInfo.operation and displays add/connect/disconnect/remove operations as
in progress; otherwise remove the unused mcpOperationStarted message and
operation payload. Keep the existing mcpOperationResult handling consistent with
whichever approach is chosen.
| try { | ||
| if (message.type === "addMcpServer") { | ||
| await this.mcpServerService.add(message.draft as ManagedMcpDraft); | ||
| } else if (message.type === "connectMcpServer") { | ||
| await this.mcpServerService.connect(serverName); | ||
| } else if (message.type === "disconnectMcpServer") { | ||
| await this.mcpServerService.disconnect(serverName); | ||
| } else { | ||
| await this.mcpServerService.remove(serverName, profileID); | ||
| void vscode.window.showInformationMessage("MCP profile removed. The current OpenCode process may list it until it restarts."); | ||
| } | ||
| this.view?.webview.postMessage({ type: "mcpOperationResult", requestID, operation, success: true }); | ||
| } catch (error) { | ||
| void vscode.window.showErrorMessage("OpenCode could not complete the MCP operation. Check the MCP status for details."); | ||
| this.view?.webview.postMessage({ | ||
| type: "mcpOperationResult", | ||
| requestID, | ||
| operation, | ||
| success: false, | ||
| error: "MCP operation failed. Refresh the MCP status and check the server details.", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not discard the operation error.
The catch block replaces every failure with one generic string and logs nothing. McpServerService throws precise, user-safe validation messages, for example An MCP server named "x" is already managed. and Remote MCP URL must use HTTPS. The user sees none of them, and no host log entry records the cause. Log the error and forward the service message.
🐛 Proposed fix
} catch (error) {
- void vscode.window.showErrorMessage("OpenCode could not complete the MCP operation. Check the MCP status for details.");
+ const reason = error instanceof Error ? error.message : "MCP operation failed.";
+ this.logger.error("MCP management operation failed", { operation, serverName, profileID }, error instanceof Error ? error : undefined);
+ void vscode.window.showErrorMessage(`OpenCode could not complete the MCP operation: ${reason}`);
this.view?.webview.postMessage({
type: "mcpOperationResult",
requestID,
operation,
success: false,
- error: "MCP operation failed. Refresh the MCP status and check the server details.",
+ error: reason,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| if (message.type === "addMcpServer") { | |
| await this.mcpServerService.add(message.draft as ManagedMcpDraft); | |
| } else if (message.type === "connectMcpServer") { | |
| await this.mcpServerService.connect(serverName); | |
| } else if (message.type === "disconnectMcpServer") { | |
| await this.mcpServerService.disconnect(serverName); | |
| } else { | |
| await this.mcpServerService.remove(serverName, profileID); | |
| void vscode.window.showInformationMessage("MCP profile removed. The current OpenCode process may list it until it restarts."); | |
| } | |
| this.view?.webview.postMessage({ type: "mcpOperationResult", requestID, operation, success: true }); | |
| } catch (error) { | |
| void vscode.window.showErrorMessage("OpenCode could not complete the MCP operation. Check the MCP status for details."); | |
| this.view?.webview.postMessage({ | |
| type: "mcpOperationResult", | |
| requestID, | |
| operation, | |
| success: false, | |
| error: "MCP operation failed. Refresh the MCP status and check the server details.", | |
| }); | |
| } | |
| try { | |
| if (message.type === "addMcpServer") { | |
| await this.mcpServerService.add(message.draft as ManagedMcpDraft); | |
| } else if (message.type === "connectMcpServer") { | |
| await this.mcpServerService.connect(serverName); | |
| } else if (message.type === "disconnectMcpServer") { | |
| await this.mcpServerService.disconnect(serverName); | |
| } else { | |
| await this.mcpServerService.remove(serverName, profileID); | |
| void vscode.window.showInformationMessage("MCP profile removed. The current OpenCode process may list it until it restarts."); | |
| } | |
| this.view?.webview.postMessage({ type: "mcpOperationResult", requestID, operation, success: true }); | |
| } catch (error) { | |
| const reason = error instanceof Error ? error.message : "MCP operation failed."; | |
| this.logger.error("MCP management operation failed", { operation, serverName, profileID }, error instanceof Error ? error : undefined); | |
| void vscode.window.showErrorMessage(`OpenCode could not complete the MCP operation: ${reason}`); | |
| this.view?.webview.postMessage({ | |
| type: "mcpOperationResult", | |
| requestID, | |
| operation, | |
| success: false, | |
| error: reason, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/ChatViewProvider.ts` around lines 4421 - 4442, Update the catch
block in the MCP operation handler to log the caught error through the existing
host logging mechanism and forward its user-safe message in the failed
mcpOperationResult response. Preserve the generic fallback only when the caught
value does not provide a usable message, while retaining the existing error
notification.
| public async remove(name: string, profileId?: string): Promise<void> { | ||
| const profile = this.profiles().find((item) => profileId ? item.id === profileId : item.name === name); | ||
| if (!profile) throw new Error("That MCP server is not managed by this extension."); | ||
| await this.withServerLock(name, async () => { | ||
| try { await (await this.getClient()).mcp.disconnect(this.scope({ name })); } catch (error) { | ||
| this.log.warn("MCP disconnect before removal failed", { name, error: this.safeError(error) }); | ||
| } | ||
| await this.removeProfileData(profile); | ||
| this.removedNames.add(profile.name); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the resolved profile name for lock and disconnect.
remove resolves the profile by profileId when it is supplied, but it then locks and disconnects with the caller-supplied name. If the two disagree, the code disconnects a different server and serializes on the wrong lock key while it deletes the resolved profile.
🐛 Proposed fix
public async remove(name: string, profileId?: string): Promise<void> {
const profile = this.profiles().find((item) => profileId ? item.id === profileId : item.name === name);
if (!profile) throw new Error("That MCP server is not managed by this extension.");
- await this.withServerLock(name, async () => {
- try { await (await this.getClient()).mcp.disconnect(this.scope({ name })); } catch (error) {
- this.log.warn("MCP disconnect before removal failed", { name, error: this.safeError(error) });
+ await this.withServerLock(profile.name, async () => {
+ try { await (await this.getClient()).mcp.disconnect(this.scope({ name: profile.name })); } catch (error) {
+ this.log.warn("MCP disconnect before removal failed", { name: profile.name, error: this.safeError(error) });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async remove(name: string, profileId?: string): Promise<void> { | |
| const profile = this.profiles().find((item) => profileId ? item.id === profileId : item.name === name); | |
| if (!profile) throw new Error("That MCP server is not managed by this extension."); | |
| await this.withServerLock(name, async () => { | |
| try { await (await this.getClient()).mcp.disconnect(this.scope({ name })); } catch (error) { | |
| this.log.warn("MCP disconnect before removal failed", { name, error: this.safeError(error) }); | |
| } | |
| await this.removeProfileData(profile); | |
| this.removedNames.add(profile.name); | |
| }); | |
| } | |
| public async remove(name: string, profileId?: string): Promise<void> { | |
| const profile = this.profiles().find((item) => profileId ? item.id === profileId : item.name === name); | |
| if (!profile) throw new Error("That MCP server is not managed by this extension."); | |
| await this.withServerLock(profile.name, async () => { | |
| try { await (await this.getClient()).mcp.disconnect(this.scope({ name: profile.name })); } catch (error) { | |
| this.log.warn("MCP disconnect before removal failed", { name: profile.name, error: this.safeError(error) }); | |
| } | |
| await this.removeProfileData(profile); | |
| this.removedNames.add(profile.name); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/McpServerService.ts` around lines 90 - 100, Update remove to use
the resolved profile’s name for both withServerLock and the mcp.disconnect
scope, while continuing to remove the profile selected by profileId or name.
Replace the caller-supplied name in those operations with profile.name to keep
locking, disconnecting, and deletion aligned.
| test("status enrichment exposes only safe managed metadata", () => { | ||
| assert.match(provider, /managed: true, profileId: profile\.id, kind: profile\.kind/); | ||
| assert.doesNotMatch(provider, /enrichedServers[\s\S]*environment/); | ||
| assert.doesNotMatch(provider, /enrichedServers[\s\S]*headers/); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
These assertions fail on unrelated edits.
doesNotMatch(provider, /enrichedServers[\s\S]*environment/) scans the remainder of src/providers/ChatViewProvider.ts after the first enrichedServers occurrence. Any later, unrelated use of the word environment or headers anywhere in that file fails the test, and it proves nothing about the posted payload. Scope the check to the enrichment block, or assert on a constructed payload instead.
💚 Proposed change
test("status enrichment exposes only safe managed metadata", () => {
assert.match(provider, /managed: true, profileId: profile\.id, kind: profile\.kind/);
- assert.doesNotMatch(provider, /enrichedServers[\s\S]*environment/);
- assert.doesNotMatch(provider, /enrichedServers[\s\S]*headers/);
+ const start = provider.indexOf("const enrichedServers");
+ const enrichmentBlock = provider.slice(start, provider.indexOf('type: "mcpStatus"', start));
+ assert.doesNotMatch(enrichmentBlock, /environment/);
+ assert.doesNotMatch(enrichmentBlock, /headers/);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("status enrichment exposes only safe managed metadata", () => { | |
| assert.match(provider, /managed: true, profileId: profile\.id, kind: profile\.kind/); | |
| assert.doesNotMatch(provider, /enrichedServers[\s\S]*environment/); | |
| assert.doesNotMatch(provider, /enrichedServers[\s\S]*headers/); | |
| }); | |
| test("status enrichment exposes only safe managed metadata", () => { | |
| assert.match(provider, /managed: true, profileId: profile\.id, kind: profile\.kind/); | |
| const start = provider.indexOf("const enrichedServers"); | |
| const enrichmentBlock = provider.slice(start, provider.indexOf('type: "mcpStatus"', start)); | |
| assert.doesNotMatch(enrichmentBlock, /environment/); | |
| assert.doesNotMatch(enrichmentBlock, /headers/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/services/mcp-server-service.test.mjs` around lines 26 - 30, Update the
status enrichment test around “status enrichment exposes only safe managed
metadata” to scope the forbidden environment and headers checks to the actual
enrichment block or, preferably, assert against the constructed payload rather
than scanning the entire provider source. Preserve the existing managed,
profileId, and kind assertions while preventing unrelated later code from
causing failures.
| const draft = kind === "local" | ||
| ? { name, kind, command: config.command, cwd: config.cwd, environment: config.environment ?? {}, timeout: config.timeout } | ||
| : { name, kind, url: config.url, headers: config.headers ?? {}, oauth: config.oauth, timeout: config.timeout }; | ||
| vscode.postMessage({ type: "addMcpServer", requestID: crypto.randomUUID(), draft }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is crypto.randomUUID available inside VS Code webviews secure context
💡 Result:
The crypto.randomUUID method is generally available in VS Code webviews, but its status as a "secure context" depends on the webview's implementation and environment [1][2]. While VS Code webviews are designed to be isolated, sandboxed environments, they do not automatically guarantee a secure context as defined by web standards [3][4]. Key considerations include: 1. Secure Context Requirements: Under web standards, the crypto.randomUUID method requires a secure context (typically served via HTTPS or as a local file URI within certain browser-based sandboxes) to function reliably [2]. Because VS Code webviews use specific URI schemes (often vscode-webview://) to load resources, the availability of crypto.randomUUID can vary based on the specific version of VS Code and the underlying Electron/browser environment [1][3]. 2. Implementation in VS Code: Official VS Code development practices acknowledge that crypto.randomUUID may not always be available or reliable in all webview contexts [1][5]. In fact, recent changes in the VS Code codebase have explicitly prioritized using crypto.getRandomValues as a fallback to ensure broader compatibility, treating randomUUID as optional [1][5]. 3. Security Best Practices: Regardless of the availability of crypto.randomUUID, webview security relies on robust Content Security Policies (CSP) and strict localResourceRoots configuration [3][6][4]. Developers are strongly encouraged to implement their own cryptographically secure nonces for CSPs, often using a custom random string generator if they cannot rely on the crypto API's availability [1][7]. For most robust cross-platform compatibility and security, it is recommended to check for the existence of crypto.randomUUID at runtime or use a fallback mechanism such as crypto.getRandomValues [1][5].
Citations:
- 1: microsoft/vscode@c47a56c
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
- 3: https://code.visualstudio.com/api/extension-guides/webview
- 4: https://blog.trailofbits.com/2023/02/21/vscode-extension-escape-vulnerability/
- 5: only rely on
crypto.getRandomValuesand treatrandomUUIDas being optional microsoft/vscode#241690 - 6: https://github.com/Microsoft/vscode-docs/blob/master/api/extension-guides/webview.md
- 7: https://github.com/microsoft/vscode-extension-samples/blob/main/webview-view-sample/src/extension.ts
🌐 Web query:
Microsoft VS Code crypto.randomUUID webview secure context getRandomValues fallback commit c47a56c7bc2f36b2a11cebbca42c6820cddeefc
💡 Result:
The commit c47a56c7bc2f36b2a11cebbca42c6820cddeefc (often referenced with a '4' suffix in full SHA notation) in the Microsoft VS Code repository is titled "Adopt uuids for generating webview nonces" [1]. This change implemented a generateUuid utility function specifically for webview components to handle the generation of secure nonces [1]. The implementation follows a standard pattern used throughout the VS Code codebase to ensure compatibility across different execution environments, particularly regarding the Web Crypto API [2][1][3]. The function logic is as follows: 1. Feature Detection: It first checks if crypto.randomUUID is available as a function [1][3]. If present, it uses this native method [1][3]. 2. Secure Context & Fallback: The code includes specific comments referencing MDN documentation, noting that while crypto might be available in various windows, it should ideally be used in secure contexts [2][1][3]. 3. Fallback Mechanism: If crypto.randomUUID is unavailable, the implementation falls back to using crypto.getRandomValues() [2][1][3]. It populates a Uint8Array(16), manually sets the UUID version bits (version 4) and variant bits (RFC 4122) required for a valid UUID, and converts the resulting byte array into the standard UUID string format [2][3]. This approach addresses the limitation where crypto.randomUUID() may not be available in all environments (e.g., certain older WebViews or non-secure contexts), ensuring that webview nonces can still be generated reliably using the underlying getRandomValues method [2][1][4].
Citations:
- 1: microsoft/vscode@c47a56c
- 2: https://github.com/microsoft/vscode/blob/99489178/src/vs/base/common/uuid.ts
- 3: https://github.com/microsoft/vscode/blob/main/extensions/markdown-language-features/src/util/uuid.ts
- 4: farion1231/cc-switch@bf92280
🌐 Web query:
VS Code webview crypto.randomUUID Node.js v19 secure context webview
💡 Result:
In VS Code webviews, the availability and usage of crypto.randomUUID depend on the browser context, as webviews run in isolated, browser-based environments [1]. 1. Secure Context Requirement: Like all modern browsers, the Web Crypto API, including crypto.randomUUID, is restricted to secure contexts (HTTPS) [2]. Because VS Code webviews are served in isolated contexts, ensure your webview is served over a secure origin to access this API [1][2]. 2. Node.js v19 Compatibility: While Node.js v19 includes the crypto.randomUUID method in its global crypto module (introduced in Node.js v16.7.0) [3][4], this is distinct from the browser's window.crypto API used within webviews. You cannot rely on Node.js-specific global crypto objects inside the webview's JavaScript environment; you must use the web-standard window.crypto API [2]. 3. Best Practices: VS Code has moved toward using crypto.randomUUID where available for generating nonces and IDs [5][6]. However, because support or security constraints can vary, the recommended approach is to check for the function's existence before calling it and implement a fallback—such as utilizing crypto.getRandomValues—to ensure robustness across all webview environments [5][7]. If you are developing a webview, treat crypto.randomUUID as an optional feature and verify that your content security policy (CSP) does not inadvertently block the necessary APIs [1][7].
Citations:
- 1: https://code.visualstudio.com/api/extension-guides/webview
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID
- 3: https://nodejs.org/download/release/v19.9.0/docs/api/webcrypto.html
- 4: https://nodejs.org/download/release/v19.7.0/docs/api/webcrypto.html
- 5: microsoft/vscode@c47a56c
- 6: https://code.visualstudio.com/updates/v1_74
- 7: only rely on
crypto.getRandomValuesand treatrandomUUIDas being optional microsoft/vscode#241690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package manifests =="
git ls-files | rg '(^|/)(package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock)$|package-lock\.json|pnpm-lock\.yaml|yarn\.lock' || true
echo
echo "== crypto.randomUUID usages =="
rg -n "crypto\.randomUUID" . || true
echo
echo "== Electron/Node/runtime hints =="
rg -n '"electron"|"nodeIntegration"|"webSecurity"|"sandbox"|"v8-compile-cache"|"type": "module"|nodeVersion|nodejs|node-args' package.json webview/shared/package.json 2>/dev/null || true
echo
echo "== Panel add/remove snippet =="
sed -n '4530,4548p;4668,4676p' webview/shared/src/chat/PanelComponents.tsx 2>/dev/null || trueRepository: chryzxc/vscode-opencode
Length of output: 5265
Use the existing UUID helper for MCP request IDs.
crypto.randomUUID requires a secure context and may be unavailable in some VS Code webview runs, while this file already has a local UUID generator. Reuse that helper for addMcpServer, connectMcpServer, disconnectMcpServer, and removeMcpServer request IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview/shared/src/chat/PanelComponents.tsx` at line 4539, Replace
crypto.randomUUID with the existing local UUID generator when creating request
IDs for addMcpServer, connectMcpServer, disconnectMcpServer, and removeMcpServer
message calls, preserving the current message payloads and request ID behavior.
| <section className="oc-mcp-form-card"><label>Command to launch</label><Input className="oc-mcp-add-field" name="command" aria-label="MCP executable" placeholder="Command to launch" /></section> | ||
| <section className="oc-mcp-form-card"><label>Arguments</label>{argumentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`argument-${row}`} aria-label={`MCP argument ${row + 1}`} placeholder="Argument" />{argumentRows.length > 1 && <Button type="button" variant="ghost" size="icon" aria-label="Remove argument" onClick={() => setArgumentRows((rows) => rows.filter((item) => item !== row))}><Trash2 className="h-3 w-3" /></Button>}</div>)}<Button type="button" variant="ghost" className="oc-mcp-add-row" onClick={() => setArgumentRows((rows) => [...rows, Math.max(...rows) + 1])}><Plus className="mr-1 h-3 w-3" /> Add argument</Button></section> | ||
| </> : <section className="oc-mcp-form-card"><label>Streamable HTTP URL</label><Input className="oc-mcp-add-field" name="url" aria-label="MCP URL" placeholder="https://example.com/mcp" /></section>} | ||
| <section className="oc-mcp-form-card"><label>{kind === "local" ? "Environment variables" : "Headers"}</label>{environmentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`env-key-${row}`} aria-label="Variable name" placeholder="Key" /><Input className="oc-mcp-add-field" name={`env-value-${row}`} aria-label="Variable value" placeholder="Value" />{environmentRows.length > 1 && <Button type="button" variant="ghost" size="icon" aria-label="Remove variable" onClick={() => setEnvironmentRows((rows) => rows.filter((item) => item !== row))}><Trash2 className="h-3 w-3" /></Button>}</div>)}<Button type="button" variant="ghost" className="oc-mcp-add-row" onClick={() => setEnvironmentRows((rows) => [...rows, Math.max(...rows) + 1])}><Plus className="mr-1 h-3 w-3" /> Add {kind === "local" ? "environment variable" : "header"}</Button></section> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the repeated field labels unique and mode-correct.
Every key input uses aria-label="Variable name" and every value input uses aria-label="Variable value". A screen reader user cannot tell the rows apart. The labels also stay "Variable" when kind === "remote", where the section renders headers.
♿ Proposed fix
-{environmentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`env-key-${row}`} aria-label="Variable name" placeholder="Key" /><Input className="oc-mcp-add-field" name={`env-value-${row}`} aria-label="Variable value" placeholder="Value" />
+{environmentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`env-key-${row}`} aria-label={`${kind === "local" ? "Environment variable" : "Header"} name ${row + 1}`} placeholder="Key" /><Input className="oc-mcp-add-field" name={`env-value-${row}`} aria-label={`${kind === "local" ? "Environment variable" : "Header"} value ${row + 1}`} placeholder="Value" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <section className="oc-mcp-form-card"><label>{kind === "local" ? "Environment variables" : "Headers"}</label>{environmentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`env-key-${row}`} aria-label="Variable name" placeholder="Key" /><Input className="oc-mcp-add-field" name={`env-value-${row}`} aria-label="Variable value" placeholder="Value" />{environmentRows.length > 1 && <Button type="button" variant="ghost" size="icon" aria-label="Remove variable" onClick={() => setEnvironmentRows((rows) => rows.filter((item) => item !== row))}><Trash2 className="h-3 w-3" /></Button>}</div>)}<Button type="button" variant="ghost" className="oc-mcp-add-row" onClick={() => setEnvironmentRows((rows) => [...rows, Math.max(...rows) + 1])}><Plus className="mr-1 h-3 w-3" /> Add {kind === "local" ? "environment variable" : "header"}</Button></section> | |
| <section className="oc-mcp-form-card"><label>{kind === "local" ? "Environment variables" : "Headers"}</label>{environmentRows.map((row) => <div className="oc-mcp-repeat-row" key={row}><Input className="oc-mcp-add-field" name={`env-key-${row}`} aria-label={`${kind === "local" ? "Environment variable" : "Header"} name ${row + 1}`} placeholder="Key" /><Input className="oc-mcp-add-field" name={`env-value-${row}`} aria-label={`${kind === "local" ? "Environment variable" : "Header"} value ${row + 1}`} placeholder="Value" />{environmentRows.length > 1 && <Button type="button" variant="ghost" size="icon" aria-label="Remove variable" onClick={() => setEnvironmentRows((rows) => rows.filter((item) => item !== row))}><Trash2 className="h-3 w-3" /></Button>}</div>)}<Button type="button" variant="ghost" className="oc-mcp-add-row" onClick={() => setEnvironmentRows((rows) => [...rows, Math.max(...rows) + 1])}><Plus className="mr-1 h-3 w-3" /> Add {kind === "local" ? "environment variable" : "header"}</Button></section> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview/shared/src/chat/PanelComponents.tsx` at line 4635, Update the
repeated Inputs in the environmentRows mapping within the MCP form section so
their aria-label values include the row identifier and use mode-correct
terminology: “Environment variable name/value” for local mode and “Header
name/value” for remote mode. Keep the existing field names and row behavior
unchanged.
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Documentation
Chores