[PPSC-846] fix(install): separate Copilot CLI from VS Code target#201
Merged
Conversation
The install wizard previously only wrote pre-tool-use hooks to ~/.codex/hooks.json but never registered an MCP server entry in ~/.codex/config.toml, so Codex could not call the scanner tools. Add a text-based TOML section appender that writes/updates [mcp_servers.armis_scanner] in config.toml with the correct venv Python path. Codex is handled as a special case (like Claude Code) since its config format is TOML rather than JSON. - `armis-cli install codex` registers both MCP server and hooks - `armis-cli install` auto-detects and registers Codex when present - Interactive wizard shows "Codex CLI" in the editor picker - `armis-cli uninstall codex` cleanly removes the section
…rget The "copilot" target was incorrectly aliased to VS Code, writing MCP config to the wrong path. Add EditorCopilotCLI as a distinct editor with its own config at ~/.copilot/mcp-config.json, update detection to recognize the .copilot directory, and fix uninstall to clean up the correct config file.
There was a problem hiding this comment.
Pull request overview
This PR fixes MCP install/uninstall targeting by separating GitHub Copilot CLI from the VS Code Copilot Chat extension, and adds Codex CLI MCP registration by writing the Armis MCP server entry into Codex’s config.toml. It also updates agent detection and the install manifest to reflect these new targets.
Changes:
- Add a distinct
copiloteditor target for Copilot CLI (~/.copilot/mcp-config.json) instead of aliasing to VS Code. - Add Codex CLI MCP registration/deregistration via
~/.codex/config.tomland track it in the install manifest. - Update detection + tests to recognize Copilot CLI’s
.copilotdirectory and MCP config.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/install/manifest.go | Adds Codex registration info to the install manifest (ManifestCodex). |
| internal/install/editors.go | Introduces EditorCopilotCLI and routes its config to ~/.copilot/mcp-config.json. |
| internal/install/editors_test.go | Expands MCP servers format test coverage to include Copilot CLI editor target. |
| internal/install/codex.go | Implements Codex CLI config.toml section add/update/remove utilities. |
| internal/install/codex_test.go | Adds unit tests for Codex CLI config.toml registration/deregistration behavior. |
| internal/cmd/install.go | Updates CLI help + adds explicit codex target and correct copilot behavior. |
| internal/cmd/install_interactive.go | Updates interactive installer to allow selecting Codex CLI for MCP registration. |
| internal/cmd/uninstall.go | Updates uninstall flows to deregister Codex MCP and treat copilot as Copilot CLI. |
| internal/cmd/uninstall_test.go | Updates uninstall tests to reflect new copilot mapping behavior. |
| internal/agentdetect/detector.go | Detects Copilot CLI via ~/.copilot and checks its mcp-config.json. |
| internal/agentdetect/detector_test.go | Adds tests for Copilot CLI directory detection and MCP config presence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+170
to
+180
| // Calculate byte offsets | ||
| startByte := 0 | ||
| for i := 0; i < startLine; i++ { | ||
| startByte += len(lines[i]) + 1 // +1 for \n | ||
| } | ||
| endByte := 0 | ||
| for i := 0; i < endLine; i++ { | ||
| endByte += len(lines[i]) + 1 | ||
| } | ||
|
|
||
| return startByte, endByte |
Comment on lines
+148
to
+160
| if err := install.DeregisterCodexMCP(); err != nil { | ||
| if styled { | ||
| fmt.Fprintf(os.Stderr, " %s Codex CLI: %v\n", warnMark.Render("⚠"), err) | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, " ⚠ Codex CLI: %v\n", err) | ||
| } | ||
| } else if install.IsCodexDetected() { | ||
| if styled { | ||
| fmt.Fprintf(os.Stderr, " %s Removed from Codex CLI\n", successMark.Render("✓")) | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, " ✓ Removed from Codex CLI\n") | ||
| } | ||
| } |
Comment on lines
192
to
196
| const ( | ||
| targetClaude = "claude" | ||
| targetCodex = "codex" | ||
| targetCopilot = "copilot" | ||
| ) |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // RegisterCodexMCP adds or updates the [mcp_servers.armis_scanner] section | ||
| // in Codex CLI's config.toml. | ||
| func RegisterCodexMCP(pluginDir string) error { | ||
| if !filepath.IsAbs(pluginDir) { |
Add #nosec G703 annotations to os.Remove calls in writeFileAtomic (controlled temp paths from os.CreateTemp). Fix Codex MCP tests to use TOML-escaped paths for Windows compatibility and create the correct venv directory structure per platform.
Test Coverage Reporttotal: (statements) 71.8% Coverage by function |
- Fix findTOMLSectionBounds off-by-one when file lacks trailing newline - Sanitize pluginDir via filepath.Clean before writing to config (CWE-22) - DeregisterCodexMCP now returns bool indicating if removal occurred - Move target constants to shared targets.go (was coupling install/uninstall) - Add tests for no-trailing-newline and path traversal sanitization
Comment on lines
+174
to
+193
| // Calculate byte offsets. | ||
| // Only add +1 (for \n) between lines, not after the final line if there's | ||
| // no trailing newline in the original content. | ||
| hasTrailingNewline := strings.HasSuffix(content, "\n") | ||
| lastLine := len(lines) - 1 | ||
|
|
||
| startByte := 0 | ||
| for i := 0; i < startLine; i++ { | ||
| startByte += len(lines[i]) | ||
| if i < lastLine || hasTrailingNewline { | ||
| startByte++ | ||
| } | ||
| } | ||
| endByte := 0 | ||
| for i := 0; i < endLine; i++ { | ||
| endByte += len(lines[i]) | ||
| if i < lastLine || hasTrailingNewline { | ||
| endByte++ | ||
| } | ||
| } |
Comment on lines
+239
to
+244
| if err := os.Rename(tmpPath, path); err != nil { | ||
| _ = os.Remove(tmpPath) // #nosec G703 -- tmpPath from os.CreateTemp in controlled dir | ||
| return fmt.Errorf("renaming config: %w", err) | ||
| } | ||
| return nil | ||
| } |
Comment on lines
+25
to
+27
| codex Codex CLI (registers MCP server + hooks) | ||
| vscode VS Code / GitHub Copilot Chat extension | ||
| copilot GitHub Copilot CLI |
- Fix findTOMLSectionBounds to correctly handle trailing newlines by
only adding +1 for actual \n separators between lines (not the
phantom empty element from strings.Split)
- Install Codex native hooks in installAll (no-args) path to match
the documented behavior ("registers MCP server + hooks")
- Add test for section-at-EOF-with-trailing-newline scenario
Comment on lines
+217
to
+242
| func writeFileAtomic(path, content string) error { | ||
| dir := filepath.Dir(path) | ||
| f, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*") | ||
| if err != nil { | ||
| return fmt.Errorf("creating temp file: %w", err) | ||
| } | ||
| tmpPath := f.Name() | ||
|
|
||
| if _, err := f.WriteString(content); err != nil { | ||
| _ = f.Close() | ||
| _ = os.Remove(tmpPath) // #nosec G703 -- tmpPath from os.CreateTemp in controlled dir | ||
| return fmt.Errorf("writing config: %w", err) | ||
| } | ||
| if err := f.Chmod(0o600); err != nil { | ||
| _ = f.Close() | ||
| _ = os.Remove(tmpPath) // #nosec G703 -- tmpPath from os.CreateTemp in controlled dir | ||
| return fmt.Errorf("setting permissions: %w", err) | ||
| } | ||
| if err := f.Close(); err != nil { | ||
| _ = os.Remove(tmpPath) // #nosec G703 -- tmpPath from os.CreateTemp in controlled dir | ||
| return fmt.Errorf("closing temp file: %w", err) | ||
| } | ||
| if err := os.Rename(tmpPath, path); err != nil { // #nosec G703 -- atomic write to known config path | ||
| _ = os.Remove(tmpPath) // #nosec G703 -- tmpPath from os.CreateTemp in controlled dir | ||
| return fmt.Errorf("renaming config: %w", err) | ||
| } |
Comment on lines
14
to
23
| // Manifest records what was installed so uninstall is deterministic. | ||
| type Manifest struct { | ||
| SchemaVersion int `json:"schemaVersion"` | ||
| InstalledAt string `json:"installedAt"` | ||
| PluginVersion string `json:"pluginVersion"` | ||
| PluginDir string `json:"pluginDir"` | ||
| Editors map[EditorID]ManifestEntry `json:"editors,omitempty"` | ||
| Claude *ManifestClaude `json:"claude,omitempty"` | ||
| Codex *ManifestCodex `json:"codex,omitempty"` | ||
| } |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Type of Change
Problem
Two issues with MCP install targets:
Copilot CLI: The
armis-cli install copilotcommand treated "copilot" as an alias for VS Code, writing MCP config to~/.vscode/mcp.jsoninstead of Copilot CLI's own~/.copilot/mcp-config.json. Uninstall also removed the wrong config, and agent detection missed the.copilotdirectory entirely.Codex CLI: The
armis-cli install codexcommand did not register the MCP server in Codex CLI'sconfig.toml, so Codex couldn't discover the Armis scanner.Solution
Copilot CLI — Added
EditorCopilotCLIas a distinct editor target with config path~/.copilot/mcp-config.json. Updated detection to recognize the.copilotdirectory and its MCP config. Fixed uninstall to clean up the correct file and remove native hooks.Codex CLI — Added
RegisterCodexMCP()to write the MCP server entry into~/.codex/config.toml(TOML format), alongside the existing hook-based integration.Testing
Automated Tests
Manual Testing
armis-cli install copilotwrites to~/.copilot/mcp-config.jsonarmis-cli uninstall copilotremoves the correct configChecklist