Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an embedded Model Context Protocol (MCP) server to the SiFli CodeKit VS Code extension, exposing existing CodeKit automation capabilities (workflows, SDK/board/serial/build actions, etc.) via MCP while refactoring language model tools into a shared registry + automation layer.
Changes:
- Introduce an embedded MCP HTTP server + VS Code MCP server definition provider, with sidebar controls and new MCP commands.
- Refactor LM tool registration to use a new tool registry, and centralize tool implementations in a new
AutomationService. - Add automation-friendly SDK/project creation/clangd configuration APIs and wire them into MCP tools.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Adds MCP SDK and its transitive dependencies (including Express/Hono/Zod/etc.). |
| src/types/tooling.ts | Introduces shared tool/transport types for LM + MCP tool definitions. |
| src/types/index.ts | Re-exports the new tooling types. |
| src/services/toolRegistryService.ts | Central registry for LM + MCP tool metadata, schemas, and invocation plumbing. |
| src/services/sdkService.ts | Adds “detailed” SDK APIs returning structured results for automation/MCP use. |
| src/services/projectCreationService.ts | Adds non-UI project template listing + creation APIs for automation/MCP. |
| src/services/mcpServerService.ts | Implements embedded MCP HTTP server and tool registration against MCP SDK. |
| src/services/mcpServerDefinitionProviderService.ts | Provides/ resolves MCP server definitions to VS Code’s LM/MCP integration. |
| src/services/languageModelToolService.ts | Refactors LM tools to register from the centralized tool registry. |
| src/services/clangdService.ts | New service to write clangd settings non-interactively (automation/MCP friendly). |
| src/services/automationService.ts | New centralized implementation for automation actions used by LM tools + MCP tools. |
| src/providers/sifliSidebarProvider.ts | Adds an MCP Server group in the sidebar with quick actions (start/stop/config/copy). |
| src/extension.ts | Wires MCP services/providers/commands into extension activation + config change handling. |
| src/constants/index.ts | Adds MCP provider/label constants. |
| src/commands/mcpCommands.ts | Adds commands to manage MCP server lifecycle and copy connection info. |
| src/commands/configCommands.ts | Refactors clangd configuration flow to use ClangdService. |
| package.nls.zh-cn.json | Adds localized titles/descriptions for MCP commands and settings (zh-CN). |
| package.nls.json | Adds localized titles/descriptions for MCP commands and settings (en). |
| package.json | Contributes MCP provider + commands + configuration, and adds MCP SDK + zod deps. |
| l10n/bundle.l10n.zh-cn.json | Adds UI string localizations for MCP sidebar/actions (zh-CN). |
| l10n/bundle.l10n.json | Adds UI string localizations for MCP sidebar/actions (en). |
| README_EN.md | Documents how to start/use the embedded MCP server. |
| README.md | Documents how to start/use the embedded MCP server (Chinese). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const templates = await this.discoverTemplates(sdk, cancellationSource.token); | ||
| cancellationSource.dispose(); | ||
| return templates ?? []; |
There was a problem hiding this comment.
CancellationTokenSource is disposed only after discoverTemplates() resolves successfully. Since discoverTemplates() can throw (e.g. missing SDK example directory), cancellationSource.dispose() will be skipped on error. Consider wrapping this block in try/finally to always dispose the token source.
| const templates = await this.discoverTemplates(sdk, cancellationSource.token); | |
| cancellationSource.dispose(); | |
| return templates ?? []; | |
| try { | |
| const templates = await this.discoverTemplates(sdk, cancellationSource.token); | |
| return templates ?? []; | |
| } finally { | |
| cancellationSource.dispose(); | |
| } |
| const relativeExamplePath = path.relative(path.join(sdk.path, EXAMPLE_SUBFOLDER), options.templatePath); | ||
| return { | ||
| sdkPath: sdk.path, | ||
| sdkVersion: sdk.version, | ||
| templateRootPath: options.templatePath, |
There was a problem hiding this comment.
options.templatePath is accepted as-is and later used by copyTemplate(), which will recursively copy from that path. For the automation/MCP entrypoint this effectively allows a caller with MCP access to copy arbitrary local directories into an arbitrary targetPath (no check that templatePath is under ${sdk.path}/example). Consider validating that templatePath resolves under the SDK example root (or only allowing templates discovered by discoverTemplates() / relativeExamplePath) before proceeding.
| const relativeExamplePath = path.relative(path.join(sdk.path, EXAMPLE_SUBFOLDER), options.templatePath); | |
| return { | |
| sdkPath: sdk.path, | |
| sdkVersion: sdk.version, | |
| templateRootPath: options.templatePath, | |
| const examplesRoot = path.resolve(path.join(sdk.path, EXAMPLE_SUBFOLDER)); | |
| const templateRootPath = path.resolve(options.templatePath); | |
| // Ensure the requested template path is within the SDK examples directory. | |
| const isWithinExamplesRoot = | |
| templateRootPath === examplesRoot || | |
| templateRootPath.startsWith(examplesRoot + path.sep); | |
| if (!isWithinExamplesRoot) { | |
| // Reject paths outside the expected SDK example root. | |
| return undefined; | |
| } | |
| const relativeExamplePath = path.relative(examplesRoot, templateRootPath); | |
| return { | |
| sdkPath: sdk.path, | |
| sdkVersion: sdk.version, | |
| templateRootPath, |
| ok: false, | ||
| payload: { | ||
| success: false, | ||
| message: vscode.l10n.t('Language model tools are only available in a SiFli project.'), |
There was a problem hiding this comment.
ensureSiFliProject() returns a message saying "Language model tools are only available...", but this helper is now used by MCP/automation endpoints as well (e.g. runWorkflow/build actions). The message is misleading for MCP callers. Consider updating it to a transport-neutral message (e.g. "Automation tools are only available..." or "CodeKit tools are only available...") so it matches the expanded usage.
| message: vscode.l10n.t('Language model tools are only available in a SiFli project.'), | |
| message: vscode.l10n.t('Automation tools are only available in a SiFli project.'), |
| this.logService.info(`Resolving MCP server definition: ${server.label}`); | ||
| const connection = await this.mcpServerService.start(true); | ||
| if (!connection.running || !connection.url || !connection.token) { | ||
| const message = 'SiFli MCP server is unavailable.'; |
There was a problem hiding this comment.
The thrown error message 'SiFli MCP server is unavailable.' is user-facing but not localized via vscode.l10n.t(...), unlike most other strings in this PR. Consider localizing it (and adding it to the l10n bundles) so it matches the extension’s localization approach.
| const message = 'SiFli MCP server is unavailable.'; | |
| const message = vscode.l10n.t('SiFli MCP server is unavailable.'); |
| if (e.affectsConfiguration('sifli-sdk-codekit.workflows')) { | ||
| mcpServerService.notifyToolsListChanged(); | ||
| mcpServerDefinitionProviderService.notifyDefinitionsChanged(); | ||
| } |
There was a problem hiding this comment.
There are now two workspace.onDidChangeConfiguration listeners in activate(): one at lines 179-190 handling sifli-sdk-codekit.mcp / .workflows, and another at lines 224-242 handling any sifli-sdk-codekit change. When workflows changes, both listeners will run and can cause duplicated notifyToolsListChanged() / notifyDefinitionsChanged() calls (and extra work like updateConfiguration() + SDK discovery). Consider consolidating these into a single listener or adding a guard so workflow-related MCP notifications only run once per config change event.
| if (e.affectsConfiguration('sifli-sdk-codekit.workflows')) { | |
| mcpServerService.notifyToolsListChanged(); | |
| mcpServerDefinitionProviderService.notifyDefinitionsChanged(); | |
| } |
| "axios": "^1.10.0", | ||
| "serialport": "^13.0.0" | ||
| "serialport": "^13.0.0", | ||
| "zod": "^3.25.0" |
There was a problem hiding this comment.
@modelcontextprotocol/sdk pulls in zod@^3.25 || ^4.0, while this extension now depends on zod@^3.25.0. The lockfile shows this resolves to two different runtime copies (zod v4 for the MCP SDK and zod v3 for the extension). Passing Zod schemas/shapes across different Zod major versions can break runtime validation or type checks inside the MCP SDK. Consider forcing a single Zod version via Yarn resolutions (pick either v3.25.x or v4.x) and align the direct zod dependency accordingly to avoid duplicate installs.
| "zod": "^3.25.0" | |
| "zod": "^3.25.0" | |
| }, | |
| "resolutions": { | |
| "zod": "3.25.0" |
| public async activateSdk(sdk: SdkVersion): Promise<void> { | ||
| try { | ||
| this.logService.info(`Activating SDK: ${sdk.version} at ${sdk.path}`); | ||
| const result = await this.activateSdkVersion(sdk, true); | ||
| if (!result.success && result.message) { | ||
| vscode.window.showErrorMessage(result.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
activateSdkVersion(sdk, true) already shows an error notification when showNotifications is true. activateSdk() then shows the same error again when !result.success, which will produce duplicate error popups for the same failure. Consider either passing showNotifications=false here and letting activateSdk() own notifications, or keep showNotifications=true and remove the extra showErrorMessage in activateSdk().
| if (request.method === 'GET') { | ||
| this.sendStatus(response, 405, { Allow: 'POST, GET, DELETE' }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When sessionId is missing and the request is GET, this responds with 405 but the Allow header includes GET, which contradicts the status and can confuse clients. Either allow GET without a session id (if intended by the transport) or return an Allow header that excludes GET and/or respond with a clearer 400 indicating the missing session id.
| if (request.method === 'GET') { | |
| this.sendStatus(response, 405, { Allow: 'POST, GET, DELETE' }); | |
| return; | |
| } |
| public async startServer(): Promise<void> { | ||
| const info = await this.mcpServerService.start(true); | ||
| this.mcpServerDefinitionProviderService.notifyDefinitionsChanged(); | ||
| this.sidebarManager.refresh(); | ||
| await this.copyConnectionInfo(info); | ||
| } |
There was a problem hiding this comment.
startServer() uses start(true) which can start the server even when mcp.enabled is false, but then immediately calls copyConnectionInfo() which bails out when MCP is disabled. This results in a started server without any way for the user to retrieve connection info via this command path. Consider making startServer() enable MCP first (update mcp.enabled), or have copyConnectionInfo() allow copying when an explicit info is provided from a forced start.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 25 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| private async readJsonBody(request: http.IncomingMessage): Promise<unknown> { | ||
| const chunks: Buffer[] = []; | ||
|
|
||
| for await (const chunk of request) { | ||
| chunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); | ||
| } | ||
|
|
||
| const rawBody = Buffer.concat(chunks).toString('utf8').trim(); | ||
| if (!rawBody) { | ||
| throw new Error('Missing JSON request body.'); | ||
| } | ||
|
|
||
| try { | ||
| return JSON.parse(rawBody); | ||
| } catch { | ||
| const error = new Error('Invalid JSON request body.'); | ||
| (error as Error & { statusCode?: number }).statusCode = 400; | ||
| throw error; |
There was a problem hiding this comment.
readJsonBody reads the entire request body into memory with no size limit. A local client can send an arbitrarily large payload and potentially hang or crash the extension host. Consider enforcing a reasonable max body size (and/or checking Content-Length) and returning HTTP 413 when exceeded.
| try { | ||
| const url = new URL(request.url ?? '/', 'http://127.0.0.1'); | ||
| const sessionId = this.getSessionId(request); | ||
| this.logService.info( |
There was a problem hiding this comment.
Every MCP request is logged at info level. Since LogService defaults to INFO, this can flood the output channel and hurt performance when an MCP client polls frequently. Consider downgrading per-request logging to debug (and keeping info for start/stop and significant lifecycle events).
| this.logService.info( | |
| this.logService.debug( |
| } | ||
|
|
||
| private buildExampleId(relativeExamplePath: string): string { | ||
| return createHash('sha256').update(relativeExamplePath).digest('hex'); |
There was a problem hiding this comment.
buildExampleId hashes the platform-specific relativeExamplePath. On Windows this likely contains backslashes, while on macOS/Linux it contains forward slashes, making exampleId unstable across OSes for the same SDK template. Normalize the path separators (e.g., to POSIX '/') before hashing so automation/MCP clients can rely on stable IDs.
| return createHash('sha256').update(relativeExamplePath).digest('hex'); | |
| const normalizedPath = relativeExamplePath.replace(/\\/g, '/'); | |
| return createHash('sha256').update(normalizedPath).digest('hex'); |
| } | ||
| mcpServerDefinitionProviderService.notifyDefinitionsChanged(); | ||
| } | ||
| if (e.affectsConfiguration('sifli-sdk-codekit.workflows')) { |
There was a problem hiding this comment.
This adds a second onDidChangeConfiguration listener for workflow changes, but there is already another listener later that also calls mcpServerService.notifyToolsListChanged() and notifyDefinitionsChanged() when sifli-sdk-codekit.workflows changes. In a SiFli project this will cause duplicate notifications and extra work on every workflow edit. Consider consolidating into a single listener or gating the later one to avoid double-firing.
| if (e.affectsConfiguration('sifli-sdk-codekit.workflows')) { | |
| if (e.affectsConfiguration('sifli-sdk-codekit.workflows') && !isSiFliProject()) { |
| // workflowShellApprovals | ||
| public getWorkflowShellApprovals(): Record<string, string> { | ||
| public getWorkflowShellApprovals(): Record<string, true> { | ||
| return this.get('workflowShellApprovals') || {}; | ||
| } | ||
|
|
||
| public async setWorkflowShellApprovals(value: Record<string, string>): Promise<void> { | ||
| public async setWorkflowShellApprovals(value: Record<string, true>): Promise<void> { | ||
| await this.set('workflowShellApprovals', value); | ||
| } | ||
|
|
||
| public isWorkflowShellApproved(approvalKey: string, commandTemplate: string): boolean { | ||
| public buildWorkflowShellApprovalKey(workflowRef: string, stepIndex: number, commandTemplate: string): string { | ||
| const fingerprint = createHash('sha256').update(commandTemplate).digest('hex').slice(0, 16); | ||
| return `${workflowRef}:${stepIndex}:${fingerprint}`; | ||
| } | ||
|
|
||
| public isWorkflowShellApproved(approvalKey: string): boolean { | ||
| const approvals = this.getWorkflowShellApprovals(); | ||
| return approvals[approvalKey] === commandTemplate; | ||
| return approvals[approvalKey] === true; | ||
| } |
There was a problem hiding this comment.
workflowShellApprovals changed from Record<string, string> to Record<string, true>. Existing users may already have persisted string-valued approvals in workspaceState; after this change those entries will never match === true, forcing re-approval for previously-approved commands. If that’s not intended, consider a small migration/compatibility path (e.g., treating any truthy existing value as approved, or clearing/migrating stored approvals on first run).
| private asString(value: unknown): string { | ||
| if (typeof value !== 'string') { | ||
| throw new Error('Expected a string value.'); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Several input validation errors thrown here are too generic (e.g., "Expected a string value.") and don’t identify which argument/key failed. These errors can surface directly to LM/MCP callers, making debugging difficult. Consider including the parameter name/path in the error message (and localizing if it can surface in the VS Code UI).
No description provided.