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
mcp doctor could tell users something was wrong, but not fix it, and it missed the failures that actually cause most support requests: a stale VS Code/Copilot config, a broken proxy or TLS setup between the MCP server and the Armis API, or a venv left over from an uninstalled Python. There was also a parsing bug — VS Code's mcp.json/settings.json are JSONC (comments, trailing commas, a possible BOM), but the CLI parsed them as strict JSON, so a hand-edited file was treated as empty and install silently dropped the user's other MCP servers.
Solution
mcp doctor --fix re-registers editors, rebuilds a broken venv, and (when verified to work) writes the system proxy into the server's .env.
mcp doctor --bundle/--bundle-path writes a support zip (report, server stderr/logs, VS Code's MCP log) with credentials scrubbed.
The doctor now runs a full MCP session (initialize, tools/list, a diagnostic tool call) using each editor's own launch config, checks that client credentials are accepted by the API, and checks that the server's own Python runtime (not just the CLI) can reach the Armis API — which is where proxy/TLS-inspection problems actually show up.
Added VS Code/Copilot-specific checks: other VS Code builds (Insiders, VSCodium), per-profile and workspace configs, duplicate/stale entries, chat.mcp.*/chat.agent.enabled settings, Windows Group Policy, and VS Code's own MCP server log.
Added a JSONC parser (internal/install/jsonc.go) used everywhere VS Code config files are read/written, fixing the bug where comments or trailing commas caused install to treat the file as empty.
WriteEnvFromValues/new SetEnvFileVars now preserve other variables (e.g. HTTPS_PROXY, SSL_CERT_FILE) already in the plugin's .env instead of overwriting the file.
Testing
Automated Tests
Unit tests added/updated (internal/cmd/mcp_doctor_test.go, internal/install/doctor_checks_test.go, internal/install/doctor_probe.go helper coverage, internal/install/doctor_test.go updates)
Integration tests added/updated
All tests passing locally
Manual Testing
Reviewer Notes
checkServerNetwork/tryProxyFix in internal/install/doctor_probe.go spawn the server's Python interpreter to run live network probes — worth double-checking the command construction and timeouts.
Support bundle scrubbing (internal/install/doctor_bundle.go) is a second layer on top of never including credential values in the first place; worth confirming the secret-scrub list covers what's collected.
- Doctor now auto-repairs stale registrations/venvs (`--fix`) and writes
a credential-scrubbed support bundle (`--bundle`) for hand-off to
Armis support.
- Live checks upgraded from a bare initialize handshake to a full MCP
session (tools/list + a debug_config tool call), plus network/auth
probes run through the server's own Python runtime to catch
TLS-inspection and proxy issues the CLI's Go client wouldn't see.
- VS Code/Copilot gets dedicated checks (profiles, workspace configs,
duplicate entries, disabling settings, Group Policy, MCP logs) since
its MCP wiring has the most silent-failure modes; JSONC parsing
(`jsonc.go`) keeps hand-edited `mcp.json` comments from being
misread as invalid or wiping out other servers on re-registration.
- `mcp doctor --fix` now detects when the OS has a working system proxy
the server isn't using, and writes it to .env instead of just
re-registering/reinstalling.
- Writing credentials (install or doctor) no longer clobbers other
vars already in .env (HTTPS_PROXY, SSL_CERT_FILE), and parses
mcp.json/settings.json as JSONC so comments/BOM don't wipe them out.
- Network probe reports and reasons about which CA source (certifi vs
system store) the server's Python runtime is using.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The doctor auth-check path ignores the provided timeout/cancellation context (and there are a couple of user-facing message/docs mismatches) which can make diagnostics hang or mislead users.
Get a fresh assessment by requesting another Copilot review.
Adds a significantly more capable armis-cli mcp doctor workflow to diagnose—and optionally repair—editor MCP registrations and runtime connectivity issues, with special handling for VS Code/Copilot’s JSONC configs and logging.
Changes:
Extend mcp doctor to run a full MCP session (initialize/tools/list/tools/call), perform server-runtime network probes, and offer --fix + support --bundle.
Add VS Code/Copilot-specific diagnostics (variants, profiles/workspaces, settings/policy gating, and MCP server logs).
Parse VS Code MCP config files as JSONC and preserve non-credential .env variables when updating credentials.
File
Description
README.md
Documents MCP server install/update/doctor usage and troubleshooting options.
internal/install/plugin.go
Preserves existing .env vars via atomic in-place key updates and backup.
…ge and .env comment doc
Addresses Copilot review on PR #326: doctorAuthCheck ignored the caller's
context and could hang past the intended timeout; the MCP session timeout
error reported the per-call timeout instead of the actual remaining wait;
SetEnvFileVars's doc comment overclaimed comment preservation for replaced
assignment lines.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The review found concrete cases where diagnostics/fix behavior can miss or mishandle invalid VS Code workspace configs and where --fix should avoid destructive re-registration when configs are invalid.
Get a fresh assessment by requesting another Copilot review.
Ignore invalid workspace VS Code config parsing errors
internal/install/doctor_vscode.go:221
Workspace-level VS Code config files that exist but are invalid JSON/JSONC are silently ignored here (err != nil just continue). That misses a common failure mode: VS Code ignores an unparseable workspace config, so the user sees “server missing” with no explanation. This should be reported as a failing check (similar to the user/profile config parsing in checkVSCodeVariant).
… report unparsable workspace configs
Addresses Copilot review on PR #326: mcp doctor --fix re-registers every
manifest editor unconditionally, and the registration path reads an
unparsable config as an empty map before rewriting it — silently dropping
the user's other servers. Add FixBlocked so any unparsable editor config
vetoes FixReregister/FixReinstall for the whole run, and report (instead of
silently skip) unparsable workspace-level VS Code configs the same way
user-level ones already are.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Two functional gaps were found in the new repair/env-update logic (duplicate env assignments can persist; --fix won’t repair Knowledge installs), which should be addressed before approval.
applyDoctorFixes always calls performMCPUpdate with withKnowledge=false, so mcp doctor --fix will never repair Armis Knowledge even when the report indicates knowledge checks need FixReinstall/FixReregister (and even when Knowledge is already installed). This makes --fix incomplete for multi-plugin installs.
Duplicate env keys are left behind after replacement
internal/install/plugin.go:605
SetEnvFileVars replaces every occurrence of a key but does not remove duplicate assignments, so a file that already has the same key multiple times will still end up with multiple (now identical) lines. That can change which value downstream parsers pick and makes the file harder to reason about.
Addresses Copilot review on PR #326: SetEnvFileVars replaced every matching
KEY= line but didn't remove extras, so a file with a pre-existing duplicate
key (e.g. from a hand edit) stayed duplicated with the same value repeated.
Now only the first occurrence is kept.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
There are correctness/security gaps in newly added diagnostic utilities (JSONC empty/BOM handling and support-bundle secret scrubbing) that should be addressed before approval.
Review effort: Lite Findings: None
Previously missed (2)
In code that hasn't changed since last review
Short secrets may leak into support bundles
internal/install/doctor_bundle.go:42
WriteSupportBundle only scrubs secrets that are 4+ characters long. Since report.secrets is built from actual credential/proxy values, shorter secrets (e.g. short proxy passwords or test credentials) could still leak into the support bundle if they appear in captured stderr/log artifacts.
BOM-only JSONC files fail empty-file handling
internal/install/doctor_vscode.go:482
readJSONCObject checks for an empty file before stripping a UTF-8 BOM/comments/trailing commas. A file that contains only a BOM (or BOM+comments/whitespace) will skip the empty-file fast-path and then fail json.Unmarshal, causing VS Code config discovery to incorrectly report an error.
…handling
Addresses Copilot review on PR #326: WriteSupportBundle's scrub skipped
secrets under 4 characters (short proxy passwords, test credentials),
relying only on the generic pattern-based masker as a fallback.
readJSONCObject's empty-file fast path checked raw bytes with TrimSpace,
which doesn't strip the UTF-8 BOM rune, so a BOM-only (or BOM+comments-only)
config file fell through to json.Unmarshal on empty content and was
reported as an invalid config.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The live MCP probe reader can block on chatty server stdout (small buffered channel), risking hangs/timeouts during mcp doctor sessions.
Review effort: Lite Findings: None
Previously missed (2)
In code that hasn't changed since last review
Stdout log channel can block and stall MCP session draining
internal/install/doctor_probe.go:186
runMCPSession's stdout scanner goroutine writes every line into a small buffered channel (size 16). If a server is chatty (many notifications/log lines), this send can block, which can stall stdout draining and hang the probe until the timeout (and potentially leak the goroutine). Consider making the channel non-blocking by discarding the oldest entry when the buffer is full.
Error message omits '=' as an invalid environment key character
internal/install/plugin.go:569
SetEnvFileVars rejects keys containing '=' (via strings.ContainsAny(kv[0], "=\n\r")), but the returned error message only mentions newlines. This can mislead users/debugging when a key fails validation for including '='.
Addresses Copilot review on PR #326: runMCPSession's stdout reader goroutine
fed a 16-line buffered channel with nothing draining it between the
synchronous call()/notify() invocations in the session, so a server that
bursts many log/notification lines could momentarily block the reader.
Widen the buffer instead of dropping lines, since dropping could discard
the actual RPC response under load. Also mention '=' in SetEnvFileVars's
validation error, which previously only named newlines as invalid.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The doctor probe can incorrectly succeed when servers emit non-JSON stdout after initialize, and a couple of diagnostics/messages and scrubbing behaviors should be corrected to avoid misleading output and preserve useful bundle data.
Get a fresh assessment by requesting another Copilot review.
isSecretKey treats any env var containing "PROXY" as a secret. This causes support bundles to scrub non-credential proxy URLs like HTTPS_PROXY=http://proxy.corp:8080, which removes useful diagnostic detail while systemInfo/other code already masks only userinfo when present.
Report JSONC parse errors instead of invalid JSON
internal/install/doctor_vscode.go:138
These VS Code config files are JSONC (comments/trailing commas/BOM). When parsing fails, the error message currently says "not valid JSON", which is misleading for users troubleshooting JSONC syntax errors.
This issue also appears on line 224 of the same file.
…ng plain proxy URLs, fix JSONC error wording
Addresses Copilot review on PR #326:
- mcpSession.call only treated non-JSON stdout as fatal before the
initialize response; a server that corrupts its own stdout stream later
(during tools/list or tools/call) had those lines silently skipped,
letting the doctor report a healthy session for a broken transport.
- isSecretKey treated any PROXY-named env var as a secret, so a plain proxy
URL with no embedded credentials got scrubbed to "***" in support
bundles, destroying useful host:port detail. Now only proxy values that
actually carry userinfo are collected as secrets.
- readJSONCObject's config-file failures were reported as "not valid
JSON", which is misleading for JSONC syntax (comments, trailing commas).
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
It introduces substantial new diagnostic behavior (process spawning, network probing, and support bundle generation) that warrants careful human validation, and there are identified correctness concerns to address.
Clamp negative MCP session timeout before reporting
internal/install/doctor_probe.go:276
mcpSession.call uses wait := time.Until(s.deadline) directly for the timer and for the timeout error message. If earlier steps consumed the entire deadline, wait can go negative, which makes the error read like "after -123ms" and is confusing.
Clamping wait to >= 0 keeps the message sensible and avoids relying on negative timer semantics.
Addresses Copilot review on PR #326: mcpSession.call used time.Until's
result directly for both the timer and the timeout error message. If
earlier steps in the same session consumed the whole deadline, that value
goes negative, producing a confusing "after -123ms" message. Clamp to 0.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
checkManifestEditors can incorrectly report an editor registration as OK when the entry exists but has an empty/missing command, which can mask a broken configuration.
When the editor entry is found but its command field is missing/empty, this currently reports StatusOK and skips the live probe (because launch.Command == ""). That can mislead users into thinking the registration is healthy even though the editor can’t start the server. Treat an empty command as a failure (or at least a warning) with a remediation to re-register/fix the entry.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
There are a few concrete logic/concurrency issues in the new VS Code diagnostics and MCP session reader that can cause incorrect reporting and potential deadlock/leaked goroutines.
Get a fresh assessment by requesting another Copilot review.
The profile warning is triggered whenever a profile mcp.json has a "servers" key, even if it’s an empty object. In that case the message “has its own MCP servers” is inaccurate and will create noisy false positives. Only warn when the profile actually has at least one server configured.
Missing VS Code command should be a failure
internal/install/doctor_vscode.go:183
If the VS Code entry exists but has no command, VS Code cannot start the server, so this should be a failure (not a warning) and should include a remediation (delete/replace the entry with the generated snippet).
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
VS Code launches sourced from the install manifest can leave ${workspaceFolder} unexpanded, leading to incorrect path validation/probing and misleading diagnostics.
Expand VS Code variables before recording and probing launch
internal/install/doctor.go:747
For VS Code entries, the doctor currently builds the launch via lookupEntry → vscodeLaunch(entry, ""), which leaves ${workspaceFolder} unexpanded. That means the subsequent command/envFile existence checks and live probe may fail even though VS Code would expand the variable when running in a workspace (and the support-bundle launch artifact will capture the wrong command line). Consider expanding VS Code variables using DoctorOptions.WorkspaceDir (or cwd) before recording/probing the launch.
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.
Jira Ticket
Type of Change
Problem
mcp doctorcould tell users something was wrong, but not fix it, and it missed the failures that actually cause most support requests: a stale VS Code/Copilot config, a broken proxy or TLS setup between the MCP server and the Armis API, or a venv left over from an uninstalled Python. There was also a parsing bug — VS Code'smcp.json/settings.jsonare JSONC (comments, trailing commas, a possible BOM), but the CLI parsed them as strict JSON, so a hand-edited file was treated as empty andinstallsilently dropped the user's other MCP servers.Solution
mcp doctor --fixre-registers editors, rebuilds a broken venv, and (when verified to work) writes the system proxy into the server's.env.mcp doctor --bundle/--bundle-pathwrites a support zip (report, server stderr/logs, VS Code's MCP log) with credentials scrubbed.initialize,tools/list, a diagnostic tool call) using each editor's own launch config, checks that client credentials are accepted by the API, and checks that the server's own Python runtime (not just the CLI) can reach the Armis API — which is where proxy/TLS-inspection problems actually show up.chat.mcp.*/chat.agent.enabledsettings, Windows Group Policy, and VS Code's own MCP server log.internal/install/jsonc.go) used everywhere VS Code config files are read/written, fixing the bug where comments or trailing commas causedinstallto treat the file as empty.WriteEnvFromValues/newSetEnvFileVarsnow preserve other variables (e.g.HTTPS_PROXY,SSL_CERT_FILE) already in the plugin's.envinstead of overwriting the file.Testing
Automated Tests
internal/cmd/mcp_doctor_test.go,internal/install/doctor_checks_test.go,internal/install/doctor_probe.gohelper coverage,internal/install/doctor_test.goupdates)Manual Testing
Reviewer Notes
checkServerNetwork/tryProxyFixininternal/install/doctor_probe.gospawn the server's Python interpreter to run live network probes — worth double-checking the command construction and timeouts.internal/install/doctor_bundle.go) is a second layer on top of never including credential values in the first place; worth confirming the secret-scrub list covers what's collected.Checklist
Screenshots (if applicable)