-
Notifications
You must be signed in to change notification settings - Fork 4k
[MCP] Async -> Sync Execution for server commands. #28133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev/mcp-server
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Model Context Protocol (MCP) server under tools/Mcp
to support synchronous execution of server commands for Azure PowerShell code generation using AutoRest.
- Sets up the TypeScript project (
tsconfig.json
,package.json
) and adds necessary dependencies. - Implements the core MCP server (
CodegenServer.ts
,index.ts
) with tool registration based on JSON specs. - Adds utility functions (
utils.ts
), tool service handlers (toolServices.ts
), and tool specifications/responses (specs.json
,responses.json
).
Reviewed Changes
Copilot reviewed 11 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tools/Mcp/tsconfig.json | Configures compiler options for the TypeScript project. |
tools/Mcp/package.json | Defines project metadata, dependencies, and build scripts. |
tools/Mcp/src/CodegenServer.ts | Implements MCP server initialization and tool registration. |
tools/Mcp/src/index.ts | Entry point that starts and connects the MCP server. |
tools/Mcp/src/services/utils.ts | Provides file, YAML, and HTTP utilities for tool operations. |
tools/Mcp/src/services/toolServices.ts | Contains implementations of MCP tool handlers. |
tools/Mcp/src/specs/specs.json | Defines tool schemas and callback names used by the server. |
tools/Mcp/src/specs/responses.json | Provides response templates for each tool. |
tools/Mcp/src/types.ts | Declares TypeScript types for schemas and directives. |
tools/Mcp/README.md | Documents installation, usage, and features of the MCP server. |
Files not reviewed (1)
- tools/Mcp/package-lock.json: Language not supported
Comments suppressed due to low confidence (7)
tools/Mcp/src/services/utils.ts:7
- [nitpick] The parameter name
path
shadows the importedpath
module. Consider renaming it todirectory
orcwd
to avoid confusion.
const _pwshCD = (path: string): string => { return `pwsh -Command "$path = resolve-path ${path} | Set-Location"` }
tools/Mcp/src/specs/responses.json:24
- Spelling error: "Fulfil" should be "Fulfill".
"type": "tool",
tools/Mcp/src/services/utils.ts:24
- There's an extraneous trailing double quote at the end of the command string, which will break execution. Remove the final
"
.
const genBuildCommand = `${_pwshCD(workingDirectory)}; ${_autorest}; ${_pwshBuild};"`;
tools/Mcp/src/services/utils.ts:11
- The
testYaml
function appears to be a leftover utility and is not used elsewhere. Consider removing it or moving it to a dedicated test file.
function testYaml() {
tools/Mcp/src/services/utils.ts:197
- The
testCase
function is hardcoded to a local path and unused. Remove it or refactor it into a proper test suite.
export async function testCase() {
tools/Mcp/src/services/toolServices.ts:5
- The
get
import from 'http' is unused. Consider removing this import.
import { get } from 'http';
tools/Mcp/src/CodegenServer.ts:106
- Typo in the description: "such a formal" should read "such as formal" for clarity.
{ name: z.string().describe("Name of the person to greet"), style: z.string().describe("The style of greeting, such a formal, excited, or casual. If not specified casual will be used")},
@@ -105,14 +105,14 @@ export const createExamplesFromSpecs = async <Args extends ZodRawShape>(args: Ar | |||
const workingDirectory = z.string().parse(Object.values(args)[0]); | |||
const examplePath = path.join(workingDirectory, "examples"); | |||
const exampleSpecsPath = await utils.getExamplesFromSpecs(workingDirectory); | |||
return [examplePath, exampleSpecsPath]; | |||
return [exampleSpecsPath, examplePath]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason reversing these 2 responses? There is a certain order in the response string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,92 @@ | |||
# Execution rules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prompt doesn't look like part of this mcp server, this is a VSCode prompt. We can make it some kind of test case but not part of this product
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is just for reference not a part of the tool...can be removed later if required.
@@ -16,11 +16,11 @@ const responses = JSON.parse(readFileSync(path.join(srcPath, "specs/responses.js | |||
|
|||
export class CodegenServer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all changes in this file looks ai generated and haven't really done anything, please revert them all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Havent changed anything here,
- Renamed _mcp -> _server (which sounds correct...)
- moved initTools & initPrompts to the bottom of the file (since they are using other helper fuctions)
- renamed initResponses -> readResponses (since its not intializing anything, just importing Responses...)
- Added some comments
const exampleFileName = path.join(exampleSpecsPath, `${ex.name}.json`); | ||
fs.writeFileSync(exampleFileName, JSON.stringify(exJson, null, 2), 'utf8'); | ||
const exampleFileName = path.join(exampleSpecsPath, `${ex.name}`); | ||
await fs.promises.writeFile(exampleFileName, JSON.stringify(exJson, null, 2), 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of this change? does sync method not working in some scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change, control returns to LLM instantly.
The LLM tries to create examples/tests while the module still hasn't even generated.
This change ensures control execution stays on the same thread.
So the execution progresses in an orderly way.
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.