You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactoring (no functional changes)
Performance improvement
Problem
When an MCP server setup breaks — a stale config entry, missing credentials, a dead command path after a reinstall — there was no way to find out why beyond re-running install and hoping it fixes things. There was also no single command to update already-registered MCP servers without re-running the full interactive install.
Solution
Adds a new mcp command group:
mcp doctor inspects everything install may have set up (scanner/knowledge plugin files, credentials, editor config registrations, Claude Code's plugin registry, Codex's config.toml) and, unless --no-handshake is passed, spawns each MCP server and performs a live JSON-RPC initialize handshake to confirm it actually starts and responds. Supports --format json for machine-readable output and exits non-zero if any check fails.
mcp update reads the install manifest to find already-registered editors/targets and updates the MCP server and its registrations to the latest version, with --force and --with-knowledge flags.
install.go now points users at mcp doctor after install when credentials are already configured or when knowledge registration produced warnings. A follow-up fix commit corrects doctor status output and false-positive checks (e.g. sibling knowledge envs with bridge.py but no .venv were being reported as broken).
Testing
Automated Tests
Unit tests added/updated
Integration tests added/updated
All tests passing locally
Manual Testing
Reviewer Notes
internal/install/doctor.go re-execs the test binary itself (TestMain + ARMIS_TEST_MCP_HELPER) to fake an MCP stdio server for handshake tests — worth a look if unfamiliar with that pattern.
The handshake spawn/kill/Wait ordering in mcpHandshake closes stdout before cmd.Wait() to avoid racing os/exec's internal pipe close; flagged in code comments.
Users installing armis-cli's MCP servers had no way to debug a broken
setup beyond re-running install blindly. `mcp doctor` inspects plugin
files, credentials, editor registrations, Claude Code's plugin
registry, and Codex's config.toml, and optionally performs a live
initialize handshake against each server. `install` now points users
at it when credentials are configured or warnings occur.
Reads the install manifest to update the plugin and re-register it
across all previously configured editors, without requiring the user
to re-specify editor names.
- Fix statusSymbol to respect accessible/no-color mode with [OK]/[WARN]/[FAIL]
text instead of always styling with lipgloss
- Skip knowledge plugin sub-envs that were never installed (no .venv/),
avoiding false failures from sibling envs extracted by Fetch
- Report unsupported editor IDs during mcp update instead of silently
skipping them
- Bound config file reads (claude registry, codex config, env files) via a
shared readBoundedConfigFile helper to enforce regular-file and size checks
- Close stdout pipe before cmd.Wait() on the handshake timeout path to avoid
racing exec's internal pipe cleanup
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
A couple of mcp doctor diagnostics are misleading/inaccurate (notably error reporting and comment clarity), which undermines the command’s primary purpose as a troubleshooting tool.
Get a fresh assessment by requesting another Copilot review.
Adds a new armis-cli mcp command group to help diagnose and update MCP server installations created by armis-cli install, including optional live JSON-RPC initialize handshakes to verify servers actually start and respond.
Changes:
Introduces mcp doctor to run structural checks (plugin files, credentials, editor registrations, Claude/Codex registrations) and optional live handshakes with a configurable timeout and JSON output.
Introduces mcp update to update the installed MCP server and re-register it across the editors/targets recorded in the install manifest (optionally including Knowledge).
Updates install messaging to direct users to mcp doctor when credentials are already configured or Knowledge registration emitted warnings, and adds unit tests for the new behaviors.
File
Description
internal/install/doctor.go
Implements doctor report model, structural checks for scanner/knowledge/editor configs, and live stdio handshake logic.
internal/install/doctor_test.go
Adds focused unit tests for handshake behavior, env/config parsing, and key doctor checks.
internal/cmd/mcp.go
Adds the top-level mcp command group under the CLI root.
internal/cmd/mcp_doctor.go
Adds mcp doctor CLI wiring, output formatting (plain/JSON), and exit behavior on failures.
internal/cmd/mcp_update.go
Adds mcp update implementation to fetch updates and re-register editors/targets from the manifest.
internal/cmd/mcp_update_test.go
Adds basic tests for update command guardrails and flag/registration presence.
internal/cmd/install.go
Adds post-install guidance pointing users to armis-cli mcp doctor for verification/troubleshooting.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
internal/install/doctor.go contains a few concrete issues (misleading file-read comment, nondeterministic report ordering, and environment variable override handling) that should be corrected before approval.
Manifest editor checks emit nondeterministic order
internal/install/doctor.go:227
checkManifestEditors iterates over a map, so the order of checks (and therefore plain output and --format json output) is nondeterministic. Since doctor output is intended to be machine-readable, it should be stable across runs (e.g., sort editor IDs before emitting checks).
Handshake env vars can be shadowed by duplicates
internal/install/doctor.go:466
mcpHandshake appends env vars onto os.Environ() without de-duplicating existing keys. If the parent process already has (for example) ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET set, the child process may see the earlier value depending on platform/library lookup semantics for duplicate keys, making the handshake use the wrong credentials.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
mcp doctor currently produces misleading diagnostics in a couple of error paths (config-read boundary failures and env file read errors) and has a minor handshake success formatting issue that should be corrected before shipping.
checkCredentials treats any parseEnvFile error (missing/unreadable file, non-regular file, oversized, etc.) the same as “credentials not set”, which can mislead users when the real problem is that the env file can’t be read. It should surface the underlying read/parse error distinctly.
This issue also appears in the following locations of the same file:
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The new diagnostics can miss broken knowledge installs and can misdiagnose invalid/oversized/non-regular editor config files, undermining the reliability of mcp doctor.
Review effort: Lite Findings: None
Previously missed (2)
In code that hasn't changed since last review
Doctor misses broken knowledge installs with no .venv present
internal/install/doctor.go:162
If a knowledge plugin install is recorded in the manifest but none of the extracted env dirs (prod/stage/dev) has a .venv/ directory (e.g., venv creation failed or was deleted), this loop skips every env and mcp doctor reports no failure for knowledge. That can mask a broken knowledge install even though bridge.py is present. Consider tracking whether any env has a venv and emitting a failing (or at least warning) check when bridge.py exists but no .venv/ is found anywhere.
Editor config should reject non-regular or oversized files
internal/install/doctor.go:217
checkManifestEditors only checks that the config path exists. If the path is a device/FIFO or exceeds maxEditorConfigSize, lookupEntryCommand will silently treat it like an empty config and you'll report a misleading “entry not found” warning. Consider failing early when the config file is non-regular or oversized, matching the guards used by readJSONFileAsMap/readYAMLFileAsMap.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The new doctor handshake path has confirmed resource-handling and diagnostics issues (pipe FD lifecycle on error/cleanup and misleading “entry not found” vs invalid JSON configs) that should be corrected before approval.
Get a fresh assessment by requesting another Copilot review.
If cmd.Start() fails, stdin/stdout pipes were already opened but aren’t closed before returning, which can leak file descriptors in error scenarios (e.g., stale/broken command path). Close the pipes before returning the start error.
This issue also appears on line 498 of the same file.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The live-handshake implementation currently performs an unbounded stdout line read, which can lead to unbounded memory growth and should be bounded before approval.
communicateInitialize reads the server’s first stdout line with bufio.Reader.ReadBytes('\n'), which can grow without bound if a broken/malicious MCP process writes a very large line (or never emits a newline). Since mcp doctor may execute whatever command path is recorded in local configs, it’s safer to bound the read (e.g., with bufio.Scanner + a max token size) and fail with a clear error when exceeded.
Plain mcp doctor output should go to stderr
internal/cmd/mcp_doctor.go:85
mcp doctor plain output currently writes to stdout via cmd.OutOrStdout(). Most install/uninstall diagnostic output in this CLI is written to stderr (leaving stdout for machine output / pipelines). Consider printing the human-readable format to stderr (cmd.ErrOrStderr), while keeping --format json on stdout for scripting.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
mcp doctor currently reports OK even when it can’t determine a registered entry’s command path (and its JSON validation can still misdiagnose certain valid-but-wrong JSON shapes), which can produce misleading diagnostics.
Review effort: Lite Findings: None
Previously missed (4)
In code that hasn't changed since last review
JSON syntax check accepts non-object values
internal/install/doctor.go:242
json.Valid only checks JSON syntax; it will accept values like null or a JSON array, which still won’t decode into the object shape this code expects. In those cases lookupEntryCommand/readJSONFileAsMap will behave like the entry is missing, producing a misleading warning. Consider unmarshalling into a JSON object (map) here and failing if it’s not an object.
Empty command path is incorrectly reported as OK
internal/install/doctor.go:255
If lookupEntryCommand finds an entry but can’t extract a command path (command == ""), this falls through and reports StatusOK, even though the doctor couldn’t verify whether the command still exists on disk. Treat an empty command as a warning (unknown) instead of OK, and only mark OK once a non-empty command has been verified (or explicitly skipped).
Tests hardcode config format strings instead of constants
internal/install/doctor_test.go:141
These tests hardcode config format strings (e.g. "vscode-servers", "zed-context_servers"). Since this PR introduces configFormat* constants, using them here reduces drift and keeps the tests aligned if the identifiers ever change.
Use configFormatContinue constant in table tests
internal/install/doctor_test.go:158
Use configFormatContinue in these table cases to avoid repeating the literal "continue-yaml" and keep the test aligned with the editor format identifiers defined in editors.go.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
checkManifestEditors’ JSON “non-object” validation does not catch a null top-level config (it passes json.Unmarshal), so mcp doctor can misreport broken configs as “entry not found.”
Get a fresh assessment by requesting another Copilot review.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The new doctor handshake path leaks the stdin pipe FD and Continue YAML parse errors can be misreported as “entry not found” warnings instead of a failing diagnostic.
Get a fresh assessment by requesting another Copilot review.
mcpHandshake never closes the StdinPipe on the parent side after a successful Start(). os/exec does not close that pipe for you, so repeated handshakes can leak file descriptors in the doctor process; it also delays EOF delivery to the child if it relies on it to finish reading.
Empty version yields awkward response detail text
internal/install/doctor.go:487
If the server responds with a name but no version, the current detail string prints an awkward "v responded". It’d be clearer to only include the "v" segment when the version is non-empty.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
A verified bug in claudeRegistryStatus can cause it to read relative paths when the home directory cannot be resolved, leading to incorrect diagnostics.
Guard against empty claudeDir before joining plugin path
internal/install/doctor.go:323
claudeRegistryStatus assumes claudeDir is non-empty. If os.UserHomeDir() fails, homeDir(".claude") returns "", and filepath.Join will produce relative paths like "plugins/installed_plugins.json", which can read the wrong file and yield false positives/negatives. Add a guard for an empty claudeDir before joining paths.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟢 Approval recommended
The new commands integrate cleanly with existing install/manifest patterns, include bounded/defensive file reads plus handshake safety, and add focused unit test coverage for the introduced behaviors.
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
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.
shb7628
left a comment
Jira Ticket
Type of Change
Problem
When an MCP server setup breaks — a stale config entry, missing credentials, a dead command path after a reinstall — there was no way to find out why beyond re-running
installand hoping it fixes things. There was also no single command to update already-registered MCP servers without re-running the full interactive install.Solution
Adds a new
mcpcommand group:mcp doctorinspects everythinginstallmay have set up (scanner/knowledge plugin files, credentials, editor config registrations, Claude Code's plugin registry, Codex's config.toml) and, unless--no-handshakeis passed, spawns each MCP server and performs a live JSON-RPCinitializehandshake to confirm it actually starts and responds. Supports--format jsonfor machine-readable output and exits non-zero if any check fails.mcp updatereads the install manifest to find already-registered editors/targets and updates the MCP server and its registrations to the latest version, with--forceand--with-knowledgeflags.install.gonow points users atmcp doctorafter install when credentials are already configured or when knowledge registration produced warnings. A follow-up fix commit corrects doctor status output and false-positive checks (e.g. sibling knowledge envs withbridge.pybut no.venvwere being reported as broken).Testing
Automated Tests
Manual Testing
Reviewer Notes
internal/install/doctor.gore-execs the test binary itself (TestMain+ARMIS_TEST_MCP_HELPER) to fake an MCP stdio server for handshake tests — worth a look if unfamiliar with that pattern.mcpHandshakecloses stdout beforecmd.Wait()to avoid racing os/exec's internal pipe close; flagged in code comments.Checklist
Screenshots (if applicable)