-
Notifications
You must be signed in to change notification settings - Fork 832
Adjust support for MCP roots #22016
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: master
Are you sure you want to change the base?
Adjust support for MCP roots #22016
Changes from 26 commits
dc4d59e
76616eb
57fa37e
61a3dda
0cf2f56
204ef05
46e8ad9
276d96e
f2640a2
bd6f7ab
d981ee1
14e92ec
bcc34a2
e1b4096
3674afb
aa833d3
fee5125
978fbcd
8d82b14
dc2dbb4
6b93777
73101a3
2f2be2b
f72bceb
7d9e37f
2b6056f
24dce25
4b4d93f
8e61601
75a4390
f0e7b38
b91a71d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||
| Set-PSDebug -Trace 1 | ||||||||||||||||||||||||||
| # Set-PSDebug -Trace 1 | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| # Set-PSDebug -Trace 1 |
Copilot
AI
Dec 4, 2025
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.
The command argument for the MCP registration uses --mcp-app, but this should match the actual flag name in the application. Verify this matches line 26-28 in Program.cs where the flag is documented. Also note there's a trace log level specified (-l trace) which may produce excessive output in CI logs.
| Invoke-CodexMcpAdd -Name "uno-app" -Arguments @("--", "dotnet", "uno-devserver", "--mcp-app", "-l", "trace") | |
| Invoke-CodexMcpAdd -Name "uno-app" -Arguments @("--", "dotnet", "uno-devserver", "--app", "-l", "info") |
Copilot
AI
Dec 5, 2025
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.
Model name appears incorrect: gpt-5-mini should likely be gpt-4o-mini (the actual OpenAI model name). GPT-5 has not been released as of the current date.
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-5-mini" } | |
| $model = if (-not [string]::IsNullOrWhiteSpace($env:CODEX_MODEL)) { $env:CODEX_MODEL } else { "gpt-4o-mini" } |
Copilot
AI
Dec 7, 2025
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 block builds a Codex CLI command using the CODEX_MODEL environment variable and, on Windows, executes it via cmd.exe /c codex ..., which allows shell metacharacters in CODEX_MODEL to be interpreted by cmd.exe. An attacker who can influence CODEX_MODEL (for example through CI variables or the process environment) can inject additional commands using characters like & or | and execute arbitrary code in the build agent context. To remediate this, avoid invoking codex via cmd.exe (execute the codex binary directly) or strictly whitelist/validate CODEX_MODEL and ensure it is passed as a single, safely quoted argument that cannot break out into the surrounding shell command.
| if ($IsWindows) { | |
| # npm exposes Codex CLI through a .cmd shim on Windows, so run it via cmd.exe | |
| $codexExecutable = "cmd.exe" | |
| $codexArguments = @("/c", "codex") + $codexArgs | |
| } | |
| # On Windows, PowerShell Start-Process can invoke .cmd shims directly, so we do not need to use cmd.exe /c. |
Copilot
AI
Dec 4, 2025
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.
The timeout is set to 300000ms (5 minutes), which is quite long for a CI test. Consider if this timeout is appropriate or if it could be reduced to fail faster when there are genuine issues. Document why such a long timeout is needed if it's intentional (e.g., for cold-start scenarios, model inference time, etc.).
| # Codex CLI may require up to 5 minutes for cold start/model inference in CI environments. | |
| # Do not reduce this timeout unless you are certain all scenarios complete faster. |
Copilot
AI
Dec 5, 2025
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.
The temporary files created with [System.IO.Path]::GetTempFileName() are cleaned up in lines 218-220, but this cleanup only happens if the process exits normally. If an exception occurs before reaching the cleanup code (e.g., at line 206 during process start), these temp files will leak. Consider moving the cleanup into a finally block or using a try-finally pattern to ensure cleanup always occurs.
Copilot
AI
Dec 4, 2025
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.
Missing space after else. Should be else { instead of else{ for consistency with PowerShell formatting conventions.
| else{ | |
| else { |
Copilot
AI
Dec 3, 2025
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.
Same environment variable naming issue: The variable name OPENAPI_TOKEN in the Azure DevOps configuration should match the corrected name (e.g., OPENAI_API_KEY or OPENAI_TOKEN) to maintain consistency and clarity.
Copilot
AI
Dec 4, 2025
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.
Inconsistent naming: variable is named $openApiToken but the environment variable is CODEX_API_KEY. The variable should be named $codexApiKey to match the environment variable it reads from.
| $openApiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $codexApiKey = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($codexApiKey)) { |
Copilot
AI
Dec 5, 2025
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.
Variable name typo: $openApiToken should likely be $openAiToken (AI, not API) since it refers to OpenAI's Codex API. The environment variable is correctly named CODEX_API_KEY.
| $openApiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $openAiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openAiToken)) { |
Copilot
AI
Dec 5, 2025
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.
Variable naming inconsistency: the variable is named $openApiToken but it stores the value from $env:CODEX_API_KEY, and it's used to access the Codex API. The variable should be named $codexApiKey to match its purpose and source. Also note that the CI configuration uses OPENAPI_TOKEN (line 300 in .azure-devops-tests-templates.yml) which should be CODEX_API_KEY for consistency.
| $openApiToken = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($openApiToken)) { | |
| $codexApiKey = $env:CODEX_API_KEY | |
| if ([string]::IsNullOrWhiteSpace($codexApiKey)) { |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,14 @@ This guide will walk you through the setup process for getting started with Code | |||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| ## Setting up Uno Platform MCPs | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Install [Codex CLI](https://developers.openai.com/codex/cli) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Install [Codex CLI](https://developers.openai.com/codex/cli) using the official commands, for example: | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| ```bash | ||||||||||||||||||||||||||||||||||||||||||||||||||
| npm i -g @openai/codex | ||||||||||||||||||||||||||||||||||||||||||||||||||
| # or on macOS | ||||||||||||||||||||||||||||||||||||||||||||||||||
| brew install --cask codex | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+22
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Install [Codex CLI](https://developers.openai.com/codex/cli) using the official commands, for example: | |
| ```bash | |
| npm i -g @openai/codex | |
| # or on macOS | |
| brew install --cask codex | |
| ``` | |
| 1. Install Codex CLI. The recommended installation method depends on your platform: | |
| - **All platforms (recommended):** | |
| Use [npm](https://www.npmjs.com/package/@openai/codex) (requires [Node.js](https://nodejs.org/)): | |
| ```bash | |
| npm i -g @openai/codex | |
| ``` | |
| - **macOS only (alternative):** | |
| You can also use [Homebrew](https://formulae.brew.sh/cask/codex): | |
| ```bash | |
| brew install --cask codex | |
| ``` | |
| For the latest installation instructions, see the [official Codex CLI documentation](https://platform.openai.com/docs/assistants/tools/codex-cli). |
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.
Commented-out debug code should be removed rather than left in the codebase. Either remove this line entirely or add a comment explaining why it's being kept for future debugging needs.