From b18eae5a173c2e52af5eb42884502170ca576c88 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 13:24:11 +0300 Subject: [PATCH 01/12] feat(mcp): add `mcp doctor --fix`/`--bundle` and VS Code diagnostics - 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. --- internal/cmd/mcp.go | 2 + internal/cmd/mcp_doctor.go | 182 +++++- internal/cmd/mcp_update.go | 6 + internal/install/doctor.go | 746 +++++++++++++++++-------- internal/install/doctor_bundle.go | 84 +++ internal/install/doctor_checks_test.go | 522 +++++++++++++++++ internal/install/doctor_probe.go | 480 ++++++++++++++++ internal/install/doctor_test.go | 73 ++- internal/install/doctor_vscode.go | 634 +++++++++++++++++++++ internal/install/editors.go | 13 +- internal/install/jsonc.go | 91 +++ 11 files changed, 2567 insertions(+), 266 deletions(-) create mode 100644 internal/install/doctor_bundle.go create mode 100644 internal/install/doctor_checks_test.go create mode 100644 internal/install/doctor_probe.go create mode 100644 internal/install/doctor_vscode.go create mode 100644 internal/install/jsonc.go diff --git a/internal/cmd/mcp.go b/internal/cmd/mcp.go index 89833d3..6231a6d 100644 --- a/internal/cmd/mcp.go +++ b/internal/cmd/mcp.go @@ -11,6 +11,8 @@ var mcpCmd = &cobra.Command{ Use 'armis-cli mcp doctor' to check the scanner and knowledge MCP servers: plugin files, credentials, editor registrations, and a live handshake. +'armis-cli mcp doctor --fix' repairs what it can; '--bundle' writes a support +bundle. Use 'armis-cli mcp update' to update them to the latest version.`, } diff --git a/internal/cmd/mcp_doctor.go b/internal/cmd/mcp_doctor.go index 66b76ac..2cc4553 100644 --- a/internal/cmd/mcp_doctor.go +++ b/internal/cmd/mcp_doctor.go @@ -1,10 +1,14 @@ package cmd import ( + "context" "encoding/json" "fmt" + "io" + "strings" "time" + "github.com/ArmisSecurity/armis-cli/internal/auth" "github.com/ArmisSecurity/armis-cli/internal/cli" "github.com/ArmisSecurity/armis-cli/internal/cmd/cmdutil" "github.com/ArmisSecurity/armis-cli/internal/install" @@ -16,23 +20,47 @@ var ( mcpDoctorFormat string mcpDoctorNoHandshake bool mcpDoctorTimeout time.Duration + mcpDoctorFix bool + mcpDoctorBundle bool + mcpDoctorBundlePath string ) var mcpDoctorCmd = &cobra.Command{ Use: "doctor", Short: "Diagnose the installed MCP servers and their editor registrations", Long: `Diagnose everything 'armis-cli install' may have set up: the scanner and -knowledge MCP servers' plugin files and credentials, whether each registered -editor's config still contains the entry, Claude Code's plugin registry, and -Codex CLI's config.toml — then, unless --no-handshake is set, spawns each -server and performs a live MCP "initialize" handshake to confirm it actually -starts and responds. +knowledge MCP servers' plugin files, venv, and credentials; whether each +registered editor's config still contains the entry; Claude Code's plugin +registry; and Codex CLI's config.toml. + +Unless --no-handshake is set, it also: + - launches each server exactly as each editor's config says and runs a live + MCP session (initialize, tools/list, and a diagnostic tool call) + - checks that the client credentials are accepted by the Armis API + - checks that the server's own Python runtime can reach the Armis API, + which catches proxy and TLS-inspection problems the CLI itself doesn't hit + +For VS Code (GitHub Copilot) it additionally checks VS Code Insiders and +VSCodium, per-profile and workspace configs, duplicate entries, settings and +Windows Group Policy that disable MCP or Agent mode, and VS Code's own MCP log +for the server. + +Every failing check prints how to fix it. --fix repairs what the CLI can +(stale or missing registrations, a broken venv) and re-runs the checks. +--bundle writes a zip with the full diagnostics, credentials removed, to send +to support. Exits non-zero if any check fails.`, Example: ` # Full diagnostic, including live handshake armis-cli mcp doctor - # Structural checks only, skip spawning the servers + # Diagnose and repair what can be repaired automatically + armis-cli mcp doctor --fix + + # Write a support bundle (armis-mcp-doctor-.zip) + armis-cli mcp doctor --bundle + + # Structural checks only, skip spawning servers and network checks armis-cli mcp doctor --no-handshake # Machine-readable output @@ -44,8 +72,11 @@ Exits non-zero if any check fails.`, func init() { mcpCmd.AddCommand(mcpDoctorCmd) mcpDoctorCmd.Flags().StringVarP(&mcpDoctorFormat, "format", "f", agentFormatPlain, "Output format: plain, json") - mcpDoctorCmd.Flags().BoolVar(&mcpDoctorNoHandshake, "no-handshake", false, "Skip spawning MCP servers for a live handshake check") - mcpDoctorCmd.Flags().DurationVar(&mcpDoctorTimeout, "timeout", install.DefaultHandshakeTimeout, "Timeout for the live handshake check") + mcpDoctorCmd.Flags().BoolVar(&mcpDoctorNoHandshake, "no-handshake", false, "Skip spawning MCP servers and the network checks") + mcpDoctorCmd.Flags().DurationVar(&mcpDoctorTimeout, "timeout", install.DefaultHandshakeTimeout, "Timeout for each live handshake") + mcpDoctorCmd.Flags().BoolVar(&mcpDoctorFix, "fix", false, "Repair fixable problems (re-register editors, rebuild the venv), then re-check") + mcpDoctorCmd.Flags().BoolVar(&mcpDoctorBundle, "bundle", false, "Write a support bundle zip (credentials removed) for Armis support") + mcpDoctorCmd.Flags().StringVar(&mcpDoctorBundlePath, "bundle-path", "", "Where to write the support bundle (implies --bundle; default: ./armis-mcp-doctor-.zip)") } func runMCPDoctor(cmd *cobra.Command, _ []string) error { @@ -55,18 +86,47 @@ func runMCPDoctor(cmd *cobra.Command, _ []string) error { return fmt.Errorf("invalid --format value %q: must be plain or json", mcpDoctorFormat) } - report := install.RunDoctor(install.DoctorOptions{ + opts := install.DoctorOptions{ Handshake: !mcpDoctorNoHandshake, Timeout: mcpDoctorTimeout, - }) + AuthCheck: doctorAuthCheck, + } + stderr := cmd.ErrOrStderr() - switch mcpDoctorFormat { - case agentFormatJSON: + report := install.RunDoctor(opts) + if mcpDoctorFormat == agentFormatPlain { + printMCPDoctorPlain(stderr, report, !mcpDoctorFix) + } + + if mcpDoctorFix { + fixed, err := applyDoctorFixes(stderr, report) + if err != nil { + return err + } + if fixed { + _, _ = fmt.Fprintln(stderr, "\nRe-running checks...") + report = install.RunDoctor(opts) + if mcpDoctorFormat == agentFormatPlain { + printMCPDoctorPlain(stderr, report, false) + } + } + } + + if mcpDoctorFormat == agentFormatJSON { if err := printMCPDoctorJSON(cmd, report); err != nil { return err } - default: - printMCPDoctorPlain(cmd, report) + } + + if mcpDoctorBundle || mcpDoctorBundlePath != "" { + path := mcpDoctorBundlePath + if path == "" { + path = install.DefaultBundleName() + } + if err := install.WriteSupportBundle(report, path, version); err != nil { + return fmt.Errorf("writing support bundle: %w", err) + } + _, _ = fmt.Fprintf(stderr, "\nSupport bundle written to %s — attach it to your support request. It contains no credentials.\n", path) } if report.HasFailures() { @@ -75,28 +135,110 @@ func runMCPDoctor(cmd *cobra.Command, _ []string) error { return nil } +// doctorAuthCheck exchanges client credentials for a token against the same +// API base URL the rest of the CLI uses. +func doctorAuthCheck(_ context.Context, id, secret string) error { + _, err := auth.NewAuthProvider(auth.AuthConfig{ + ClientID: id, + ClientSecret: secret, + BaseURL: getAPIBaseURL(), + Region: region, + }) + return err +} + +// applyDoctorFixes performs the repairs the report calls for and reports +// whether anything was attempted. +func applyDoctorFixes(out io.Writer, report *install.DoctorReport) (bool, error) { + for _, c := range report.Checks { + if c.Component == install.ComponentInstall && c.Name == "manifest" && c.Status == install.StatusFail { + _, _ = fmt.Fprintln(out, "\nNothing is installed yet, so there is nothing to repair. Run: armis-cli install") + return false, nil + } + } + + fixes := report.Fixes() + if len(fixes) == 0 { + if report.HasProblems() { + _, _ = fmt.Fprintln(out, "\nNone of the remaining problems can be fixed automatically — follow the → hints above.") + } + return false, nil + } + + force := false + for _, f := range fixes { + if f == install.FixReinstall { + force = true + } + } + if force { + _, _ = fmt.Fprintln(out, "\nReinstalling the MCP server and re-registering editors...") + } else { + _, _ = fmt.Fprintln(out, "\nRe-registering editors...") + } + if err := performMCPUpdate(force, false); err != nil { + return true, fmt.Errorf("repair failed: %w", err) + } + return true, nil +} + func printMCPDoctorJSON(cmd *cobra.Command, report *install.DoctorReport) error { enc := json.NewEncoder(cmd.OutOrStdout()) enc.SetIndent("", " ") return enc.Encode(report) } -func printMCPDoctorPlain(cmd *cobra.Command, report *install.DoctorReport) { - out := cmd.ErrOrStderr() - +func printMCPDoctorPlain(out io.Writer, report *install.DoctorReport, suggestFix bool) { if len(report.Checks) == 0 { _, _ = fmt.Fprintln(out, "No checks produced any output.") return } accessible := !cli.ColorsEnabled() + width := 20 + for _, c := range report.Checks { + width = max(width, len(c.Name)) + } var lastComponent string + var passed, warned, failed int for _, c := range report.Checks { if c.Component != lastComponent { _, _ = fmt.Fprintf(out, "%s:\n", c.Component) lastComponent = c.Component } - _, _ = fmt.Fprintf(out, " %s %-20s %s\n", statusSymbol(c.Status, accessible), c.Name, c.Detail) + _, _ = fmt.Fprintf(out, " %s %-*s %s\n", statusSymbol(c.Status, accessible), width, c.Name, c.Detail) + if c.Remediation != "" && c.Status != install.StatusOK { + printRemediation(out, c.Remediation) + } + switch c.Status { + case install.StatusOK: + passed++ + case install.StatusWarn: + warned++ + case install.StatusFail: + failed++ + } + } + + _, _ = fmt.Fprintf(out, "\n%d passed, %d warnings, %d failed\n", passed, warned, failed) + if suggestFix && len(report.Fixes()) > 0 { + _, _ = fmt.Fprintln(out, "Some of these can be repaired automatically: armis-cli mcp doctor --fix") + } + if warned+failed > 0 { + _, _ = fmt.Fprintln(out, "Still stuck? Run 'armis-cli mcp doctor --bundle' and send the zip to Armis support.") + } +} + +// printRemediation prints a check's hint under it, indenting continuation +// lines so multi-line hints (steps, JSON snippets) stay readable. +func printRemediation(out io.Writer, remediation string) { + lines := strings.Split(strings.TrimRight(remediation, "\n"), "\n") + for i, line := range lines { + prefix := " " + if i == 0 { + prefix = " → " + } + _, _ = fmt.Fprintln(out, prefix+line) } } @@ -111,6 +253,8 @@ func statusSymbol(s install.CheckStatus, accessible bool) string { return "[OK]" case install.StatusWarn: return "[WARN]" + case install.StatusInfo: + return "[INFO]" default: return "[FAIL]" } @@ -120,6 +264,8 @@ func statusSymbol(s install.CheckStatus, accessible bool) string { return lipgloss.NewStyle().Foreground(cmdutil.BrandSuccess).Render("✓") case install.StatusWarn: return lipgloss.NewStyle().Foreground(cmdutil.BrandWarn).Render("⚠") + case install.StatusInfo: + return "ℹ" default: return lipgloss.NewStyle().Foreground(cmdutil.BrandError).Render("✗") } diff --git a/internal/cmd/mcp_update.go b/internal/cmd/mcp_update.go index 9960626..5cb336b 100644 --- a/internal/cmd/mcp_update.go +++ b/internal/cmd/mcp_update.go @@ -47,7 +47,13 @@ func runMCPUpdate(cmd *cobra.Command, _ []string) error { if err != nil { return fmt.Errorf("reading --with-knowledge flag: %w", err) } + return performMCPUpdate(force, withKnowledgeFlag) +} +// performMCPUpdate re-fetches the plugin (a full reinstall when force is set) +// and re-registers every editor recorded in the install manifest. Shared by +// `mcp update` and `mcp doctor --fix`. +func performMCPUpdate(force, withKnowledgeFlag bool) error { ei := install.NewEditorInstaller() manifest := install.ReadManifest(ei.PluginDir()) if manifest == nil { diff --git a/internal/install/doctor.go b/internal/install/doctor.go index 5f3019b..d57f0c5 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -1,16 +1,14 @@ package install import ( - "bufio" "bytes" + "context" "encoding/json" - "errors" "fmt" - "io" "os" - "os/exec" "path/filepath" "runtime" + "sort" "strings" "time" @@ -38,30 +36,84 @@ const ( // answer the initialize handshake before reporting it as unresponsive. const DefaultHandshakeTimeout = 10 * time.Second +// networkProbeTimeout bounds the server-runtime network probe. +const networkProbeTimeout = 30 * time.Second + // CheckStatus is the outcome of a single doctor check. type CheckStatus string +// ComponentInstall is the component of the check reporting a missing install +// manifest. +const ComponentInstall = "install" + const ( StatusOK CheckStatus = "ok" StatusWarn CheckStatus = "warn" StatusFail CheckStatus = "fail" + // StatusInfo carries guidance for things the doctor can't verify locally + // (e.g. organization-level Copilot policy). It never affects the exit code. + StatusInfo CheckStatus = "info" +) + +// FixAction names a repair `mcp doctor --fix` can perform for a check. +type FixAction string + +const ( + FixNone FixAction = "" + // FixReregister rewrites the editor registrations recorded in the manifest. + FixReregister FixAction = "reregister" + // FixReinstall re-downloads the plugin and rebuilds its venv, then + // re-registers every editor. + FixReinstall FixAction = "reinstall" ) // DoctorCheck is one diagnostic result reported by RunDoctor. type DoctorCheck struct { - Component string `json:"component"` - Name string `json:"name"` - Status CheckStatus `json:"status"` - Detail string `json:"detail"` + Component string `json:"component"` + Name string `json:"name"` + Status CheckStatus `json:"status"` + Detail string `json:"detail"` + Remediation string `json:"remediation,omitempty"` + Fix FixAction `json:"fix,omitempty"` +} + +// hint attaches remediation text a user can act on without support. +func (c *DoctorCheck) hint(remediation string) *DoctorCheck { + c.Remediation = remediation + return c +} + +// fix marks the check as repairable by `mcp doctor --fix`. +func (c *DoctorCheck) fix(action FixAction, remediation string) *DoctorCheck { + c.Fix = action + c.Remediation = remediation + return c } // DoctorReport is the full set of diagnostic results from RunDoctor. type DoctorReport struct { Checks []DoctorCheck `json:"checks"` + // Artifacts holds raw diagnostic material (full server stderr, config + // excerpts, log tails) for the support bundle. Keyed by bundle file name. + // Kept out of the JSON report, which stays a concise list of checks. + Artifacts map[string]string `json:"-"` + // secrets are credential values seen during the run, scrubbed verbatim + // from everything written to the support bundle. + secrets []string } -func (r *DoctorReport) add(component, name string, status CheckStatus, detail string) { +// add appends a check and returns it so a hint or fix can be attached. The +// pointer is only valid until the next add. +func (r *DoctorReport) add(component, name string, status CheckStatus, detail string) *DoctorCheck { r.Checks = append(r.Checks, DoctorCheck{Component: component, Name: name, Status: status, Detail: detail}) + return &r.Checks[len(r.Checks)-1] +} + +func (r *DoctorReport) artifact(name, content string) { + if r.Artifacts == nil { + r.Artifacts = make(map[string]string) + } + r.Artifacts[name] = content } // HasFailures reports whether any check in the report failed. @@ -74,78 +126,208 @@ func (r *DoctorReport) HasFailures() bool { return false } +// HasProblems reports whether any check failed or warned. +func (r *DoctorReport) HasProblems() bool { + for _, c := range r.Checks { + if c.Status == StatusFail || c.Status == StatusWarn { + return true + } + } + return false +} + +// Fixes returns the distinct repairs `--fix` can apply for failing or warning +// checks. FixReinstall subsumes FixReregister, so only one is returned. +func (r *DoctorReport) Fixes() []FixAction { + var reregister, reinstall bool + for _, c := range r.Checks { + if c.Status != StatusFail && c.Status != StatusWarn { + continue + } + switch c.Fix { + case FixReinstall: + reinstall = true + case FixReregister: + reregister = true + } + } + switch { + case reinstall: + return []FixAction{FixReinstall} + case reregister: + return []FixAction{FixReregister} + } + return nil +} + // DoctorOptions configures RunDoctor. type DoctorOptions struct { - // Handshake, when true, spawns each registered MCP server and performs a - // live JSON-RPC initialize handshake over stdio. + // Handshake, when true, spawns each registered MCP server and runs a live + // MCP session over stdio (initialize, tools/list, a diagnostic tool call), + // and runs the network checks. Handshake bool // Timeout bounds how long the handshake waits for a response. Defaults to // DefaultHandshakeTimeout when zero. Timeout time.Duration + // AuthCheck, when set and Handshake is true, verifies the scanner's client + // credentials against the Armis API. Injected by the caller so this + // package stays independent of the auth client. + AuthCheck func(ctx context.Context, clientID, clientSecret string) error + // WorkspaceDir is where a workspace-level .vscode/mcp.json is looked for. + // Defaults to the current directory. + WorkspaceDir string +} + +// doctorRun carries state shared by the checks of a single RunDoctor call. +type doctorRun struct { + report *DoctorReport + opts DoctorOptions + // probes caches live-session outcomes by launch, so an editor entry that + // launches exactly what an earlier check already ran isn't spawned twice. + probes map[string]*probeOutcome + // manifestConfigs are config files already covered by manifest checks, + // so VS Code discovery doesn't report them twice. + manifestConfigs map[string]bool +} + +type probeOutcome struct { + label string // component/name of the check that ran it + ok bool +} + +func newDoctorRun(opts DoctorOptions) *doctorRun { + return &doctorRun{ + report: &DoctorReport{}, + opts: opts, + probes: make(map[string]*probeOutcome), + manifestConfigs: make(map[string]bool), + } } // RunDoctor inspects everything armis-cli install may have registered — the // shared scanner plugin, the knowledge bridge, and every editor config // recorded in the install manifest — and, when requested, spawns each MCP -// server to confirm it actually answers a protocol handshake. +// server to confirm it actually answers a protocol handshake and serves tools. +// VS Code gets extra checks (all install variants and profiles, workspace +// configs, chat settings, Group Policy, and its MCP logs) since Copilot's MCP +// support has the most ways to silently not load a server. func RunDoctor(opts DoctorOptions) *DoctorReport { - report := &DoctorReport{} + d := newDoctorRun(opts) + report := d.report ei := NewEditorInstaller() + report.artifact("system.txt", systemInfo()) manifest := ReadManifest(ei.PluginDir()) if manifest == nil { - report.add("install", "manifest", StatusFail, - fmt.Sprintf("no install manifest found at %s — run: armis-cli install", ei.PluginDir())) + report.add(ComponentInstall, "manifest", StatusFail, + fmt.Sprintf("no install manifest found at %s", ei.PluginDir())). + hint("Run: armis-cli install") + checkVSCode(d, ei.PluginDir(), false) return report } + if b, err := json.MarshalIndent(manifest, "", " "); err == nil { + report.artifact("manifest.json", string(b)) + } + for _, e := range manifest.Editors { + d.manifestConfigs[filepath.Clean(e.ConfigFile)] = true + } - checkScannerPlugin(report, ei, opts) - checkManifestEditors(report, "scanner", mcpServerName, manifest.Editors) + checkScannerPlugin(d, ei) + checkManifestEditors(d, "scanner", mcpServerName, manifest.Editors) checkClaudeSection(report, "scanner", manifest.Claude, pluginName) checkCodexSection(report, "scanner", manifest.Codex, codexMCPServerName) if manifest.Knowledge != nil { - checkKnowledgePlugin(report, manifest.Knowledge, opts) - checkManifestEditors(report, "knowledge", knowledgeJSONIdentifier, manifest.Knowledge.Editors) + checkKnowledgePlugin(d, manifest.Knowledge) + checkManifestEditors(d, "knowledge", knowledgeJSONIdentifier, manifest.Knowledge.Editors) checkClaudeSection(report, "knowledge", manifest.Knowledge.Claude, knowledgeJSONIdentifier) checkCodexSection(report, "knowledge", manifest.Knowledge.Codex, knowledgeCodexIdentifier) } + _, hasVSCode := manifest.Editors[EditorVSCode] + checkVSCode(d, ei.PluginDir(), hasVSCode) + return report } -func checkScannerPlugin(report *DoctorReport, ei *EditorInstaller, opts DoctorOptions) { +// checkScannerPlugin verifies the scanner's files, venv, and credentials and, +// when enabled, runs the live session and network checks. +func checkScannerPlugin(d *doctorRun, ei *EditorInstaller) { const component = "scanner" + report := d.report + pythonPath := venvPython(ei.PluginDir()) + serverPy := filepath.Join(ei.PluginDir(), "server.py") if v := ei.GetInstalledVersion(); v == "" { - report.add(component, "plugin version", StatusWarn, "no installed version recorded") + report.add(component, "plugin version", StatusWarn, "no installed version recorded"). + fix(FixReinstall, "Reinstall the plugin: armis-cli mcp doctor --fix") } else { report.add(component, "plugin version", StatusOK, "v"+v) } - pythonPath := venvPython(ei.PluginDir()) if !isExecutableFile(pythonPath) { - report.add(component, "python venv", StatusFail, fmt.Sprintf("missing or not executable: %s", pythonPath)) + report.add(component, "python venv", StatusFail, fmt.Sprintf("missing or not executable: %s", pythonPath)). + fix(FixReinstall, "Rebuild the venv: armis-cli mcp doctor --fix") + return + } + if !checkVenvBase(report, component, filepath.Join(ei.PluginDir(), ".venv")) { return } report.add(component, "python venv", StatusOK, pythonPath) - serverPy := filepath.Join(ei.PluginDir(), "server.py") if _, err := os.Stat(serverPy); err != nil { - report.add(component, "server script", StatusFail, fmt.Sprintf("missing: %s", serverPy)) + report.add(component, "server script", StatusFail, fmt.Sprintf("missing: %s", serverPy)). + fix(FixReinstall, "Reinstall the plugin: armis-cli mcp doctor --fix") return } report.add(component, "server script", StatusOK, serverPy) env := checkCredentials(report, component, ei.EnvFilePath()) - if opts.Handshake { - runHandshakeCheck(report, component, pythonPath, []string{serverPy}, env, opts.Timeout) + if d.opts.Handshake { + // The canonical check launches with the .env merged in, which is what + // VS Code does via envFile; editors without envFile rely on the server + // loading .env itself, which it does from its own directory. + d.probe(component, "", serverLaunch{Command: pythonPath, Args: []string{serverPy}, EnvFile: ei.EnvFilePath(), Env: env}) + checkServerNetwork(d, component, pythonPath, env, ei.EnvFilePath()) + checkAuth(d, component, env) } } -func checkKnowledgePlugin(report *DoctorReport, k *ManifestKnowledge, opts DoctorOptions) { +// checkVenvBase verifies the interpreter a venv was created from still +// exists. A venv's python.exe on Windows is a thin launcher that execs the +// base interpreter recorded in pyvenv.cfg; uninstalling or upgrading that +// Python leaves a venv whose python.exe exists but can't start ("No Python +// at ..."). Returns false after reporting a failure. +func checkVenvBase(report *DoctorReport, component, venvDir string) bool { + cfgPath := filepath.Join(venvDir, "pyvenv.cfg") + b, err := readBoundedConfigFile(cfgPath) + if err != nil { + return true // older/unusual venvs may lack it; the handshake still catches a broken one + } + report.artifact(sanitizeArtifactName(component)+"/pyvenv.cfg", string(b)) + for _, line := range strings.Split(string(b), "\n") { + k, v, ok := strings.Cut(line, "=") + if !ok || strings.TrimSpace(k) != "home" { + continue + } + home := strings.TrimSpace(v) + if home == "" { + return true + } + if _, err := os.Stat(home); err != nil { + report.add(component, "python venv", StatusFail, + fmt.Sprintf("the venv's base Python (%s) no longer exists — Python was likely uninstalled or upgraded", home)). + fix(FixReinstall, "Rebuild the venv against a current Python: armis-cli mcp doctor --fix") + return false + } + } + return true +} + +func checkKnowledgePlugin(d *doctorRun, k *ManifestKnowledge) { const component = "knowledge" + report := d.report if k.SHA != "" { report.add(component, "bridge commit", StatusOK, k.SHA) @@ -175,42 +357,222 @@ func checkKnowledgePlugin(report *DoctorReport, k *ManifestKnowledge, opts Docto subComponent := component + " " + sub pythonPath := venvPython(envDir) if !isExecutableFile(pythonPath) { - report.add(subComponent, "python venv", StatusFail, fmt.Sprintf("missing or not executable: %s", pythonPath)) + report.add(subComponent, "python venv", StatusFail, fmt.Sprintf("missing or not executable: %s", pythonPath)). + fix(FixReinstall, "Rebuild the venv: armis-cli mcp doctor --fix") + continue + } + if !checkVenvBase(report, subComponent, filepath.Join(envDir, ".venv")) { continue } report.add(subComponent, "python venv", StatusOK, pythonPath) - env := checkCredentials(report, subComponent, filepath.Join(envDir, ".env")) + envFile := filepath.Join(envDir, ".env") + env := checkCredentials(report, subComponent, envFile) - if opts.Handshake { - runHandshakeCheck(report, subComponent, pythonPath, []string{bridge}, env, opts.Timeout) + if d.opts.Handshake { + d.probe(subComponent, "", serverLaunch{Command: pythonPath, Args: []string{bridge}, EnvFile: envFile, Env: env}) } } switch { case !found: - report.add(component, "bridge", StatusFail, fmt.Sprintf("no bridge.py found under %s", k.PluginDir)) + report.add(component, "bridge", StatusFail, fmt.Sprintf("no bridge.py found under %s", k.PluginDir)). + fix(FixReinstall, "Reinstall Armis Knowledge: armis-cli mcp doctor --fix") case !venvFound: - report.add(component, "python venv", StatusFail, fmt.Sprintf("bridge.py found under %s but no environment has a .venv — install may have failed", k.PluginDir)) + report.add(component, "python venv", StatusFail, fmt.Sprintf("bridge.py found under %s but no environment has a .venv — install may have failed", k.PluginDir)). + fix(FixReinstall, "Reinstall Armis Knowledge: armis-cli mcp doctor --fix") } } +// credentialsHint is how a user re-enters client credentials without support. +const credentialsHint = "Re-enter your client ID and secret with: armis-cli install --interactive " + + "(or set ARMIS_CLIENT_ID and ARMIS_CLIENT_SECRET in this file). " + + "If you sign in with SSO instead, you can ignore this." + // checkCredentials validates envFile carries both required credentials and // returns its contents for reuse by a following live handshake. func checkCredentials(report *DoctorReport, component, envFile string) map[string]string { env, err := parseEnvFile(envFile) if err != nil { - report.add(component, "credentials", StatusWarn, fmt.Sprintf("%s: %v", envFile, err)) + report.add(component, "credentials", StatusWarn, fmt.Sprintf("%s: %v", envFile, err)).hint(credentialsHint) return env } + keys := make([]string, 0, len(env)) + for k, v := range env { + keys = append(keys, k) + if isSecretKey(k) && v != "" { + report.secrets = append(report.secrets, v) + } + } + sort.Strings(keys) + report.artifact(sanitizeArtifactName(component)+"/env-keys.txt", + "Variables set in "+envFile+" (values omitted):\n"+strings.Join(keys, "\n")+"\n") + + if raw, rerr := readBoundedConfigFile(envFile); rerr == nil && bytes.HasPrefix(raw, utf8BOM) { + report.add(component, "credentials file", StatusWarn, + fmt.Sprintf("%s starts with a UTF-8 byte-order mark", envFile)). + hint("Some editors and the server's .env loader misread the first variable when a BOM is present. Re-save the file as \"UTF-8\" (not \"UTF-8 with BOM\").") + } if env["ARMIS_CLIENT_ID"] == "" || env["ARMIS_CLIENT_SECRET"] == "" { report.add(component, "credentials", StatusWarn, - fmt.Sprintf("ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET not set in %s", envFile)) + fmt.Sprintf("ARMIS_CLIENT_ID/ARMIS_CLIENT_SECRET not set in %s", envFile)).hint(credentialsHint) return env } report.add(component, "credentials", StatusOK, "configured") return env } +// checkAuth exchanges the scanner's client credentials for a token, proving +// they're valid for this tenant before the user ever reaches a tool call. +func checkAuth(d *doctorRun, component string, env map[string]string) { + if d.opts.AuthCheck == nil || env["ARMIS_CLIENT_ID"] == "" || env["ARMIS_CLIENT_SECRET"] == "" { + return + } + ctx, cancel := context.WithTimeout(context.Background(), networkProbeTimeout) + defer cancel() + if err := d.opts.AuthCheck(ctx, env["ARMIS_CLIENT_ID"], env["ARMIS_CLIENT_SECRET"]); err != nil { + msg := err.Error() + c := d.report.add(component, "authentication", StatusFail, msg) + lower := strings.ToLower(msg) + switch { + case strings.Contains(lower, "401") || strings.Contains(lower, "403") || + strings.Contains(lower, "invalid") || strings.Contains(lower, "unauthorized"): + c.hint("The client ID/secret were rejected. They may be revoked, expired, or for another tenant. " + credentialsHint) + case strings.Contains(lower, "x509") || strings.Contains(lower, "certificate"): + c.hint("TLS verification failed. Your network may be intercepting HTTPS; ask IT for the corporate root CA to be installed in the Windows certificate store.") + default: + c.hint("Check network access to the Armis API from this machine (proxy, firewall, VPN).") + } + return + } + d.report.add(component, "authentication", StatusOK, "client credentials accepted") +} + +// checkServerNetwork runs the network probe with the server's own Python +// runtime, which is where TLS-inspection and proxy problems actually bite. +func checkServerNetwork(d *doctorRun, component, python string, env map[string]string, envFile string) { + if caFile := firstNonEmpty(env["SSL_CERT_FILE"], os.Getenv("SSL_CERT_FILE")); caFile != "" { + // armis:ignore cwe:22 reason:stat-only existence check of the user's own SSL_CERT_FILE setting + if _, err := os.Stat(caFile); err != nil { //nolint:gosec // stat-only check of the user's own setting + d.report.add(component, "CA bundle", StatusFail, fmt.Sprintf("SSL_CERT_FILE points to a missing file: %s", caFile)). + hint("Fix the SSL_CERT_FILE path in " + envFile + " (or your environment) to point at your organization's root CA PEM file.") + } + } + url := serverAPIURL(env) + out, err := runNetworkProbe(python, env, url, networkProbeTimeout) + d.report.artifact(sanitizeArtifactName(component)+"/network-probe.txt", fmt.Sprintf("GET %s\n%s\n", url, out)) + if err != nil { + d.report.add(component, "server network", StatusFail, fmt.Sprintf("%s: %s", url, truncate(err.Error(), 300))). + hint(networkHint(err.Error(), envFile)) + return + } + d.report.add(component, "server network", StatusOK, fmt.Sprintf("%s reachable from the server's Python runtime (%s)", url, out)) +} + +// probe runs a live MCP session for launch and reports the handshake, tool +// listing, and diagnostic tool call as checks. label prefixes the check names +// ("" for the plugin's own launch, the editor name for an editor's entry). A +// launch identical to one already probed is reported by reference instead of +// being spawned again. +func (d *doctorRun) probe(component, label string, launch serverLaunch) { + report := d.report + named := func(n string) string { + if label == "" { + return n + } + return label + " " + n + } + + key := launch.key() + if prev, ok := d.probes[key]; ok { + status := StatusOK + if !prev.ok { + status = StatusFail + } + report.add(component, named("launch"), status, "same launch command as "+prev.label+" (see above)") + return + } + outcome := &probeOutcome{label: component + " / " + named("live handshake")} + d.probes[key] = outcome + + start := time.Now() + res, stderr, err := mcpHandshake(launch.Command, launch.Args, launch.Env, d.opts.Timeout) + elapsed := time.Since(start).Round(100 * time.Millisecond) + if stderr != "" { + report.artifact("stderr/"+sanitizeArtifactName(component+" "+named("live handshake"))+".txt", + "$ "+launch.commandLine()+"\n\n"+stderr+"\n") + } + if err != nil { + detail := err.Error() + if stderr != "" { + detail += " — stderr: " + tail(stderr, 300) + } + hint, action := launchHint(err, stderr, launch) + report.add(component, named("live handshake"), StatusFail, detail).fix(action, hint) + return + } + outcome.ok = true + + detail := "responded to initialize" + if res.ServerName != "" { + detail = res.ServerName + " responded" + if res.ServerVersion != "" { + detail = fmt.Sprintf("%s v%s responded", res.ServerName, res.ServerVersion) + } + } + detail += " in " + elapsed.String() + if elapsed > slowStartThreshold { + report.add(component, named("live handshake"), StatusWarn, detail+" (slow start)"). + hint("Slow starts are usually antivirus scanning the venv. If your editor gives up before the server is ready, ask IT to exclude " + filepath.Dir(filepath.Dir(filepath.Dir(launch.Command))) + " from real-time scanning.") + } else { + report.add(component, named("live handshake"), StatusOK, detail) + } + + switch { + case res.ToolsErr != nil: + outcome.ok = false + report.add(component, named("tools"), StatusFail, "tools/list failed: "+res.ToolsErr.Error()). + hint(launchHintText(res.ToolsErr, stderr, launch)) + case len(res.Tools) == 0: + outcome.ok = false + report.add(component, named("tools"), StatusFail, "server started but exposes no tools"). + fix(FixReinstall, "Reinstall the plugin: armis-cli mcp doctor --fix") + default: + report.add(component, named("tools"), StatusOK, fmt.Sprintf("%d tools: %s", len(res.Tools), strings.Join(res.Tools, ", "))) + } + + switch { + case res.DebugErr != nil: + outcome.ok = false + report.add(component, named("tool call"), StatusFail, debugConfigTool+" failed: "+truncate(res.DebugErr.Error(), 300)). + hint(launchHintText(res.DebugErr, stderr, launch)) + case res.DebugConfig != "": + report.artifact(sanitizeArtifactName(component)+"/debug_config.txt", res.DebugConfig+"\n") + report.add(component, named("tool call"), StatusOK, debugConfigTool+" succeeded — "+summarizeDebugConfig(res.DebugConfig)) + } +} + +func launchHintText(err error, stderr string, launch serverLaunch) string { + h, _ := launchHint(err, stderr, launch) + return h +} + +// summarizeDebugConfig pulls the lines of debug_config output most useful at +// a glance into a single line. +func summarizeDebugConfig(out string) string { + var parts []string + for _, line := range strings.Split(out, "\n") { + for _, p := range []string{"Auth method:", "API URL:", "Env:"} { + if strings.HasPrefix(strings.TrimSpace(line), p) { + parts = append(parts, strings.TrimSpace(line)) + } + } + } + if len(parts) == 0 { + return truncate(strings.ReplaceAll(strings.TrimSpace(out), "\n", "; "), 120) + } + return strings.Join(parts, "; ") +} + // checkManifestEditors verifies, for every editor the manifest recorded a // registration for, that the config file still exists, still contains an // entry matching identifier, and that the entry's command still exists on @@ -218,8 +580,20 @@ func checkCredentials(report *DoctorReport, component, envFile string) map[strin // drive-letter change, or a reinstall into a new plugin dir leaves editors // pointing at a command path that no longer resolves — the entry is still // present by name, so a name-only check would report this as healthy. -func checkManifestEditors(report *DoctorReport, component, identifier string, editors map[EditorID]ManifestEntry) { - for id, entry := range editors { +// +// With handshakes enabled it then launches exactly what the editor's config +// says (command, args, envFile, env) rather than what the manifest expects, +// since that's what the editor will actually run. +func checkManifestEditors(d *doctorRun, component, identifier string, editors map[EditorID]ManifestEntry) { + report := d.report + ids := make([]EditorID, 0, len(editors)) + for id := range editors { + ids = append(ids, id) + } + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + + for _, id := range ids { + entry := editors[id] name := string(id) if ed, ok := EditorByID(id); ok { name = ed.Name @@ -228,23 +602,25 @@ func checkManifestEditors(report *DoctorReport, component, identifier string, ed // readBoundedConfigFile applies the same regular-file and size guards as // readJSONFileAsMap/readYAMLFileAsMap, so a non-regular or oversized // config is reported here rather than silently read as empty by - // lookupEntryCommand below and misreported as "entry not found". + // lookupEntry below and misreported as "entry not found". content, err := readBoundedConfigFile(entry.ConfigFile) if err != nil { - report.add(component, name, StatusFail, fmt.Sprintf("config file %s: %v", entry.ConfigFile, err)) + report.add(component, name, StatusFail, fmt.Sprintf("config file %s: %v", entry.ConfigFile, err)). + fix(FixReregister, "Re-register the server: armis-cli mcp doctor --fix") continue } // readJSONFileAsMap/readYAMLFileAsMap also return an empty map on a // parse error, so a corrupted or non-object config (null, an array, // invalid YAML, ...) would otherwise fall through to the same "entry // not found" warning as a genuinely edited-out entry. Catch that case - // explicitly. + // explicitly. JSON configs are parsed as JSONC: VS Code's mcp.json + // allows comments and trailing commas. var obj map[string]interface{} var parseErr error if entry.Format == configFormatContinue { parseErr = yaml.Unmarshal(content, &obj) } else { - parseErr = json.Unmarshal(content, &obj) + parseErr = json.Unmarshal(stripJSONC(content), &obj) } // Both unmarshalers accept a top-level `null` without error (obj just // stays nil), so an err-only check would miss it — require a non-nil @@ -253,22 +629,40 @@ func checkManifestEditors(report *DoctorReport, component, identifier string, ed parseErr = fmt.Errorf("top-level value is not an object") } if parseErr != nil { - report.add(component, name, StatusFail, fmt.Sprintf("config file %s is not valid: %v", entry.ConfigFile, parseErr)) + // Not auto-fixable: re-registering would start from an empty map + // and drop every other server the user configured in this file. + report.add(component, name, StatusFail, fmt.Sprintf("config file %s is not valid: %v", entry.ConfigFile, parseErr)). + hint(name + " ignores the whole file when it can't be parsed, so no servers in it load. Fix the syntax (often a missing or extra comma), then re-run this doctor.") continue } - command, found := lookupEntryCommand(entry.ConfigFile, entry.Format, identifier) + launch, found := lookupEntry(entry.ConfigFile, entry.Format, identifier) if !found { report.add(component, name, StatusWarn, - fmt.Sprintf("registered at %s but entry not found — was it edited or removed?", entry.ConfigFile)) + fmt.Sprintf("registered at %s but entry not found — was it edited or removed?", entry.ConfigFile)). + fix(FixReregister, "Re-register the server: armis-cli mcp doctor --fix") continue } - if command != "" && !isExecutableFile(command) { + report.artifact(fmt.Sprintf("editors/%s-%s-entry.json", sanitizeArtifactName(component), id), launchArtifact(entry.ConfigFile, launch)) + if launch.Command != "" && !isExecutableFile(launch.Command) { report.add(component, name, StatusFail, - fmt.Sprintf("entry found in %s but its command does not exist: %s — likely stale after a reinstall or profile/home directory change; re-run armis-cli install", entry.ConfigFile, command)) + fmt.Sprintf("entry found in %s but its command does not exist: %s — likely stale after a reinstall or profile/home directory change", entry.ConfigFile, launch.Command)). + fix(FixReregister, "Point the entry at the current install: armis-cli mcp doctor --fix") continue } + if launch.EnvFile != "" { + if _, err := os.Stat(launch.EnvFile); err != nil { + report.add(component, name, StatusFail, + fmt.Sprintf("entry in %s references an envFile that does not exist: %s", entry.ConfigFile, launch.EnvFile)). + fix(FixReregister, "Point the entry at the current install: armis-cli mcp doctor --fix") + continue + } + } report.add(component, name, StatusOK, entry.ConfigFile) + + if d.opts.Handshake && launch.Command != "" { + d.probe(component, name, launch) + } } } @@ -277,16 +671,19 @@ func checkClaudeSection(report *DoctorReport, component string, claude *Manifest return } if _, err := os.Stat(claude.CacheDir); err != nil { - report.add(component, "Claude Code", StatusFail, fmt.Sprintf("cache dir missing: %s", claude.CacheDir)) + report.add(component, "Claude Code", StatusFail, fmt.Sprintf("cache dir missing: %s", claude.CacheDir)). + fix(FixReregister, "Reinstall the Claude Code plugin: armis-cli mcp doctor --fix") return } installed, enabled := claudeRegistryStatus(homeDir(".claude"), pluginKeyPrefix) switch { case !installed: - report.add(component, "Claude Code", StatusWarn, "not found in installed_plugins.json — re-run install") + report.add(component, "Claude Code", StatusWarn, "not found in installed_plugins.json"). + fix(FixReregister, "Reinstall the Claude Code plugin: armis-cli mcp doctor --fix") case !enabled: - report.add(component, "Claude Code", StatusWarn, "installed but not enabled in settings.json") + report.add(component, "Claude Code", StatusWarn, "installed but not enabled in settings.json"). + hint("Enable it in Claude Code with /plugin, or re-run: armis-cli mcp doctor --fix") default: report.add(component, "Claude Code", StatusOK, claude.CacheDir) } @@ -360,30 +757,44 @@ func checkCodexSection(report *DoctorReport, component string, codex *ManifestCo } content, err := readBoundedConfigFile(codex.ConfigFile) if err != nil { - report.add(component, "Codex CLI", StatusFail, fmt.Sprintf("config file %s: %v", codex.ConfigFile, err)) + report.add(component, "Codex CLI", StatusFail, fmt.Sprintf("config file %s: %v", codex.ConfigFile, err)). + fix(FixReregister, "Re-register the server: armis-cli mcp doctor --fix") return } if !strings.Contains(strings.ToLower(string(content)), strings.ToLower(identifier)) { report.add(component, "Codex CLI", StatusWarn, - fmt.Sprintf("registered at %s but entry not found — was it edited or removed?", codex.ConfigFile)) + fmt.Sprintf("registered at %s but entry not found — was it edited or removed?", codex.ConfigFile)). + fix(FixReregister, "Re-register the server: armis-cli mcp doctor --fix") return } report.add(component, "Codex CLI", StatusOK, codex.ConfigFile) } // lookupEntryCommand finds the server entry matching identifier in configFile -// (read per the manifest's recorded format) and returns the command path it -// declares. found is true as soon as a matching entry name exists, even when +// and returns the command path it declares. See lookupEntry. +func lookupEntryCommand(configFile, format, identifier string) (command string, found bool) { + l, found := lookupEntry(configFile, format, identifier) + return l.Command, found +} + +// lookupEntry finds the server entry matching identifier in configFile (read +// per the manifest's recorded format) and returns how it launches the server. +// found is true as soon as a matching entry name exists, even when the // command comes back empty because the format stores it somewhere this // function doesn't understand — callers must treat an empty command as // "unknown", not "missing". -func lookupEntryCommand(configFile, format, identifier string) (command string, found bool) { +func lookupEntry(configFile, format, identifier string) (serverLaunch, bool) { identifier = strings.ToLower(identifier) matchEntry := func(servers map[string]interface{}) (map[string]interface{}, bool) { - for k, v := range servers { + keys := make([]string, 0, len(servers)) + for k := range servers { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { if strings.Contains(strings.ToLower(k), identifier) { - m, _ := v.(map[string]interface{}) + m, _ := servers[k].(map[string]interface{}) return m, true } } @@ -395,19 +806,18 @@ func lookupEntryCommand(configFile, format, identifier string) (command string, servers, _ := readJSONFileAsMap(configFile)["servers"].(map[string]interface{}) entry, ok := matchEntry(servers) if !ok { - return "", false + return serverLaunch{}, false } - cmd, _ := entry[jsonKeyCommand].(string) - return cmd, true + return vscodeLaunch(entry, ""), true case configFormatZed: servers, _ := readJSONFileAsMap(configFile)["context_servers"].(map[string]interface{}) entry, ok := matchEntry(servers) if !ok { - return "", false + return serverLaunch{}, false } cmdObj, _ := entry[jsonKeyCommand].(map[string]interface{}) cmd, _ := cmdObj[jsonKeyPath].(string) - return cmd, true + return serverLaunch{Command: cmd, Args: stringSlice(cmdObj[jsonKeyArgs]), Env: stringMap(cmdObj["env"])}, true case configFormatContinue: list, _ := readYAMLFileAsMap(configFile)["mcpServers"].([]interface{}) for _, item := range list { @@ -417,19 +827,44 @@ func lookupEntryCommand(configFile, format, identifier string) (command string, } if n, _ := m["name"].(string); strings.Contains(strings.ToLower(n), identifier) { cmd, _ := m[jsonKeyCommand].(string) - return cmd, true + return serverLaunch{Command: cmd, Args: stringSlice(m[jsonKeyArgs]), Env: stringMap(m["env"])}, true } } - return "", false + return serverLaunch{}, false default: // "mcpServers" servers, _ := readJSONFileAsMap(configFile)["mcpServers"].(map[string]interface{}) entry, ok := matchEntry(servers) if !ok { - return "", false + return serverLaunch{}, false } cmd, _ := entry[jsonKeyCommand].(string) - return cmd, true + return serverLaunch{Command: cmd, Args: stringSlice(entry[jsonKeyArgs]), Env: stringMap(entry["env"])}, true + } +} + +func stringSlice(v interface{}) []string { + list, _ := v.([]interface{}) + out := make([]string, 0, len(list)) + for _, item := range list { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out +} + +func stringMap(v interface{}) map[string]string { + m, _ := v.(map[string]interface{}) + if len(m) == 0 { + return nil + } + out := make(map[string]string, len(m)) + for k, val := range m { + if s, ok := val.(string); ok { + out[k] = s + } } + return out } func isExecutableFile(path string) bool { @@ -444,12 +879,14 @@ func isExecutableFile(path string) bool { } // parseEnvFile reads a "KEY=VALUE" per line .env file, as written by -// writeEnvFromEnvironment/WriteEnvFromValues. +// writeEnvFromEnvironment/WriteEnvFromValues. A leading UTF-8 BOM is ignored +// so a file re-saved by a Windows editor still parses. func parseEnvFile(path string) (map[string]string, error) { b, err := readBoundedConfigFile(path) if err != nil { return nil, err } + b = bytes.TrimPrefix(b, utf8BOM) env := make(map[string]string) for _, line := range strings.Split(string(b), "\n") { line = strings.TrimSpace(line) @@ -465,173 +902,22 @@ func parseEnvFile(path string) (map[string]string, error) { return env, nil } -// handshakeResult carries the identity the MCP server reported in its -// initialize response. -type handshakeResult struct { - ServerName string - ServerVersion string -} - -type mcpInitResult struct { - ServerInfo struct { - Name string `json:"name"` - Version string `json:"version"` - } `json:"serverInfo"` -} - -func runHandshakeCheck(report *DoctorReport, component, command string, args []string, env map[string]string, timeout time.Duration) { - res, stderrTail, err := mcpHandshake(command, args, env, timeout) - if err != nil { - detail := err.Error() - if stderrTail != "" { - detail += " — stderr: " + stderrTail - } - report.add(component, "live handshake", StatusFail, detail) - return - } - detail := "responded to initialize" - if res.ServerName != "" { - detail = res.ServerName + " responded" - if res.ServerVersion != "" { - detail = fmt.Sprintf("%s v%s responded", res.ServerName, res.ServerVersion) +// isSecretKey reports whether an env var name likely holds a credential. +func isSecretKey(k string) bool { + k = strings.ToUpper(k) + for _, marker := range []string{"SECRET", "TOKEN", "PASSWORD", "CLIENT_ID", "API_KEY", "PROXY"} { + if strings.Contains(k, marker) { + return true } } - report.add(component, "live handshake", StatusOK, detail) + return false } -// mcpHandshake spawns command as an MCP stdio server, sends a single -// "initialize" JSON-RPC request, and waits up to timeout for a response line. -// The process is always killed and waited-on before returning, so stderr can -// be read back safely (os/exec only finishes copying stderr into the buffer -// once Wait returns). -func mcpHandshake(command string, args []string, env map[string]string, timeout time.Duration) (*handshakeResult, string, error) { - if timeout <= 0 { - timeout = DefaultHandshakeTimeout - } - - // armis:ignore cwe:78 cwe:88 reason:command/args come from the CLI's own recorded install paths (venv interpreter + server script), not user input - cmd := exec.Command(command, args...) //nolint:gosec // command/args are the CLI's own recorded install paths - cmd.Env = os.Environ() - for k, v := range env { - cmd.Env = append(cmd.Env, k+"="+v) - } - - stdin, err := cmd.StdinPipe() - if err != nil { - return nil, "", fmt.Errorf("opening stdin: %w", err) - } - stdout, err := cmd.StdoutPipe() - if err != nil { - _ = stdin.Close() - return nil, "", fmt.Errorf("opening stdout: %w", err) - } - var stderrBuf bytes.Buffer - cmd.Stderr = &stderrBuf - - // Start() failing means Wait() will never run to close these pipes for us - // (that cleanup is documented as conditional on a successful Start), so - // close them ourselves rather than leaking the file descriptors. - if err := cmd.Start(); err != nil { - _ = stdin.Close() - _ = stdout.Close() - return nil, "", fmt.Errorf("starting process: %w", err) - } - - result, opErr := communicateInitialize(stdin, stdout, timeout) - - // armis:ignore cwe:404 reason:best-effort cleanup of a short-lived diagnostic subprocess we just spawned - _ = cmd.Process.Kill() - // On the timeout path, communicateInitialize's reader goroutine may still - // be blocked reading stdout when we get here. os/exec's docs warn it is - // "incorrect to call Wait before all reads from the pipe have completed" - // because Wait closes this same pipe as part of its own cleanup — close - // it here first so the unblock is explicit and ordered rather than racing - // Wait's internal close. - _ = stdout.Close() - _ = stdin.Close() - _ = cmd.Wait() - - if opErr != nil { - return nil, stderrTail(&stderrBuf), opErr - } - return result, "", nil -} - -func communicateInitialize(stdin io.WriteCloser, stdout io.ReadCloser, timeout time.Duration) (*handshakeResult, error) { - req := map[string]interface{}{ - "jsonrpc": "2.0", - "id": 1, - "method": "initialize", - "params": map[string]interface{}{ - "protocolVersion": "2024-11-05", - "capabilities": map[string]interface{}{}, - "clientInfo": map[string]interface{}{"name": "armis-cli-doctor", "version": "1.0"}, - }, - } - line, err := json.Marshal(req) - if err != nil { - return nil, err - } - - type readOutcome struct { - line []byte - err error - } - lineCh := make(chan readOutcome, 1) - go func() { - scanner := bufio.NewScanner(stdout) - scanner.Buffer(make([]byte, 0, 64*1024), maxHandshakeLineSize) - if scanner.Scan() { - lineCh <- readOutcome{append([]byte(nil), scanner.Bytes()...), nil} - return - } - lineCh <- readOutcome{nil, scanner.Err()} - }() - - if _, err := stdin.Write(append(line, '\n')); err != nil { - return nil, fmt.Errorf("writing initialize request: %w", err) - } - - select { - case <-time.After(timeout): - return nil, fmt.Errorf("timed out waiting for response after %s", timeout) - case out := <-lineCh: - if len(out.line) == 0 { - if errors.Is(out.err, bufio.ErrTooLong) { - return nil, fmt.Errorf("response exceeded %d bytes", maxHandshakeLineSize) - } - if out.err != nil { - return nil, fmt.Errorf("no response: %w", out.err) - } - return nil, fmt.Errorf("no response") - } - var rpc struct { - Result *mcpInitResult `json:"result"` - Error *struct { - Message string `json:"message"` - } `json:"error"` - } - if err := json.Unmarshal(out.line, &rpc); err != nil { - return nil, fmt.Errorf("invalid response: %w", err) +func firstNonEmpty(vals ...string) string { + for _, v := range vals { + if v != "" { + return v } - if rpc.Error != nil { - return nil, fmt.Errorf("server returned error: %s", rpc.Error.Message) - } - if rpc.Result == nil { - return nil, fmt.Errorf("response missing result") - } - return &handshakeResult{ - ServerName: rpc.Result.ServerInfo.Name, - ServerVersion: rpc.Result.ServerInfo.Version, - }, nil - } -} - -func stderrTail(buf *bytes.Buffer) string { - s := strings.TrimSpace(buf.String()) - const maxLen = 300 - if len(s) > maxLen { - s = s[len(s)-maxLen:] } - return s + return "" } diff --git a/internal/install/doctor_bundle.go b/internal/install/doctor_bundle.go new file mode 100644 index 0000000..09b4183 --- /dev/null +++ b/internal/install/doctor_bundle.go @@ -0,0 +1,84 @@ +package install + +import ( + "archive/zip" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + "time" + + "github.com/ArmisSecurity/armis-cli/internal/util" +) + +// DefaultBundleName returns the file name used when --bundle is given without +// a path. +func DefaultBundleName() string { + return "armis-mcp-doctor-" + time.Now().Format("20060102-150405") + ".zip" +} + +// WriteSupportBundle writes report and its collected artifacts to a zip at +// path for the user to attach to a support request. Credentials are never +// included: .env files contribute only variable names, editor entries omit env +// values, every credential value seen during the run is scrubbed verbatim, +// and all text passes through the CLI's secret masker as a second layer. +func WriteSupportBundle(report *DoctorReport, path, cliVersion string) error { + // armis:ignore cwe:22 cwe:73 reason:path is the user's own --bundle argument for a file they are creating + f, err := os.OpenFile(filepath.Clean(path), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) //nolint:gosec // user-chosen output path + if err != nil { + return fmt.Errorf("creating bundle: %w", err) + } + zw := zip.NewWriter(f) + + scrub := func(s string) string { + for _, secret := range report.secrets { + if len(secret) >= 4 { + s = strings.ReplaceAll(s, secret, "***") + } + } + return util.MaskSecretInMultiLineString(s) + } + write := func(name, content string) error { + w, err := zw.Create(name) + if err != nil { + return err + } + _, err = w.Write([]byte(scrub(content))) + return err + } + + reportJSON, err := json.MarshalIndent(report, "", " ") + if err != nil { + _ = zw.Close() + _ = f.Close() + return fmt.Errorf("encoding report: %w", err) + } + files := map[string]string{ + "report.json": string(reportJSON) + "\n", + "README.txt": "Armis MCP doctor support bundle.\n" + + "armis-cli version: " + cliVersion + "\n" + + "Credentials are not included: .env files contribute only variable names.\n", + } + for k, v := range report.Artifacts { + files[k] = v + } + names := make([]string, 0, len(files)) + for k := range files { + names = append(names, k) + } + sort.Strings(names) + for _, name := range names { + if err := write(name, files[name]); err != nil { + _ = zw.Close() + _ = f.Close() + return fmt.Errorf("writing %s to bundle: %w", name, err) + } + } + if err := zw.Close(); err != nil { + _ = f.Close() + return fmt.Errorf("finalizing bundle: %w", err) + } + return f.Close() +} diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go new file mode 100644 index 0000000..1859b41 --- /dev/null +++ b/internal/install/doctor_checks_test.go @@ -0,0 +1,522 @@ +package install + +import ( + "archive/zip" + "encoding/json" + "errors" + "io" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// stubVSCode points the VS Code checks at variants (nil for none) and +// disables the Group Policy lookup for the duration of the test. +func stubVSCode(t *testing.T, variants []vscodeVariant) { + t.Helper() + origVariants, origPolicy := vscodeVariants, vscodePolicyQuery + vscodeVariants = func() []vscodeVariant { return variants } + vscodePolicyQuery = func(string) string { return "" } + t.Cleanup(func() { vscodeVariants, vscodePolicyQuery = origVariants, origPolicy }) +} + +// helperLaunch launches this test binary as a fake MCP server in mode. +func helperLaunch(mode string) serverLaunch { + return serverLaunch{Command: os.Args[0], Env: map[string]string{"ARMIS_TEST_MCP_HELPER": mode}} +} + +func mustWrite(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { //nolint:gosec // test temp dir + t.Fatal(err) + } +} + +// checkMap indexes checks by "component/name". Later checks with the same key +// overwrite earlier ones. +func checkMap(r *DoctorReport) map[string]DoctorCheck { + m := make(map[string]DoctorCheck) + for _, c := range r.Checks { + m[c.Component+"/"+c.Name] = c + } + return m +} + +func wantStatus(t *testing.T, checks map[string]DoctorCheck, key string, want CheckStatus) DoctorCheck { + t.Helper() + c, ok := checks[key] + if !ok { + t.Fatalf("no check %q; got %v", key, checkKeys(checks)) + } + if c.Status != want { + t.Errorf("check %q status = %s, want %s (detail: %s)", key, c.Status, want, c.Detail) + } + return c +} + +func checkKeys(checks map[string]DoctorCheck) []string { + keys := make([]string, 0, len(checks)) + for k := range checks { + keys = append(keys, k) + } + return keys +} + +func TestStripJSONC(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"line comment", "{\n// c\n\"a\": 1\n}", "{\n\n\"a\": 1\n}"}, + {"block comment", `{/* c */"a": 1}`, `{"a": 1}`}, + {"trailing comma object", `{"a": 1,}`, `{"a": 1}`}, + {"trailing comma array", `[1, 2, ]`, `[1, 2 ]`}, + {"trailing comma before comment", "{\"a\": 1, // c\n}", "{\"a\": 1 \n}"}, + {"bom", "\xEF\xBB\xBF{}", `{}`}, + {"url in string", `{"u": "http://x/*y*/"}`, `{"u": "http://x/*y*/"}`}, + {"escaped quote", `{"s": "a\"//b"}`, `{"s": "a\"//b"}`}, + {"comma in string", `{"s": ",}"}`, `{"s": ",}"}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := string(stripJSONC([]byte(tt.in))); got != tt.want { + t.Errorf("stripJSONC(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestRegisterVSCodePreservesJSONCServers pins the install bug: a hand-edited +// mcp.json with a comment used to parse as empty, so registering dropped every +// other server in it. +func TestRegisterVSCodePreservesJSONCServers(t *testing.T) { + path := filepath.Join(t.TempDir(), "mcp.json") + mustWrite(t, path, "\xEF\xBB\xBF{\n // my servers\n \"servers\": {\n \"other\": {\"command\": \"node\"},\n },\n}\n") + + if err := registerVSCodeFormat(path, scannerEntry("/plugin")); err != nil { + t.Fatalf("registerVSCodeFormat() error = %v", err) + } + servers, _ := readJSONFileAsMap(path)["servers"].(map[string]interface{}) + if _, ok := servers["other"]; !ok { + t.Errorf("servers = %v, want the existing \"other\" server preserved", servers) + } + if _, ok := servers[mcpServerName]; !ok { + t.Errorf("servers = %v, want %s added", servers, mcpServerName) + } +} + +func TestLookupEntryVSCodeLaunch(t *testing.T) { + dir := t.TempDir() + envFile := filepath.Join(dir, ".env") + mustWrite(t, envFile, "ARMIS_CLIENT_ID=id\nFROM_FILE=1\n") + path := filepath.Join(dir, "mcp.json") + mustWrite(t, path, `{ + // comment + "servers": { + "armis-appsec": { + "command": "/bin/python", + "args": ["${pathSeparator}server.py"], + "envFile": "`+filepath.ToSlash(envFile)+`", + "env": {"FROM_FILE": "2", "EXTRA": "x"}, + }, + }, +}`) + + l, ok := lookupEntry(path, configFormatVSCode, mcpServerName) + if !ok { + t.Fatal("lookupEntry() found = false") + } + if l.Command != "/bin/python" { + t.Errorf("Command = %q", l.Command) + } + if len(l.Args) != 1 || l.Args[0] != string(filepath.Separator)+"server.py" { + t.Errorf("Args = %v, want ${pathSeparator} expanded", l.Args) + } + if l.Env["ARMIS_CLIENT_ID"] != "id" || l.Env["FROM_FILE"] != "2" || l.Env["EXTRA"] != "x" { + t.Errorf("Env = %v, want envFile merged with inline env winning", l.Env) + } +} + +func TestExpandVSCodeVars(t *testing.T) { + home, _ := os.UserHomeDir() + t.Setenv("ARMIS_TEST_VAR", "val") + tests := map[string]string{ + "${userHome}/x": home + "/x", + "${workspaceFolder}/y": "/ws/y", + "${env:ARMIS_TEST_VAR}": "val", + "${unknown}": "${unknown}", + "plain": "plain", + } + for in, want := range tests { + if got := expandVSCodeVars(in, "/ws"); got != want { + t.Errorf("expandVSCodeVars(%q) = %q, want %q", in, got, want) + } + } +} + +func TestProbeReportsHandshakeToolsAndToolCall(t *testing.T) { + d := newDoctorRun(DoctorOptions{Handshake: true, Timeout: 10 * time.Second}) + d.probe("scanner", "", helperLaunch("ok")) + d.probe("scanner", "Cursor", helperLaunch("ok")) + + checks := checkMap(d.report) + wantStatus(t, checks, "scanner/live handshake", StatusOK) + tools := wantStatus(t, checks, "scanner/tools", StatusOK) + if !strings.Contains(tools.Detail, "debug_config") { + t.Errorf("tools detail = %q, want tool names", tools.Detail) + } + call := wantStatus(t, checks, "scanner/tool call", StatusOK) + if !strings.Contains(call.Detail, "Auth method: JWT") { + t.Errorf("tool call detail = %q, want debug_config summary", call.Detail) + } + // The identical second launch is reported by reference, not re-spawned. + dup := wantStatus(t, checks, "scanner/Cursor launch", StatusOK) + if !strings.Contains(dup.Detail, "same launch command") { + t.Errorf("dedup detail = %q", dup.Detail) + } + if _, ok := checks["scanner/Cursor live handshake"]; ok { + t.Error("identical launch was probed twice") + } + if d.report.Artifacts["scanner/debug_config.txt"] == "" { + t.Error("debug_config output not kept as an artifact") + } +} + +func TestProbeFailures(t *testing.T) { + tests := []struct { + mode string + key string + wantFix FixAction + }{ + {"notools", "scanner/tools", FixReinstall}, + {"toolerror", "scanner/tool call", FixNone}, + {"error", "scanner/live handshake", FixNone}, + } + for _, tt := range tests { + t.Run(tt.mode, func(t *testing.T) { + d := newDoctorRun(DoctorOptions{Handshake: true, Timeout: 10 * time.Second}) + d.probe("scanner", "", helperLaunch(tt.mode)) + c := wantStatus(t, checkMap(d.report), tt.key, StatusFail) + if c.Fix != tt.wantFix { + t.Errorf("fix = %q, want %q", c.Fix, tt.wantFix) + } + if c.Remediation == "" { + t.Error("failing check has no remediation") + } + }) + } +} + +func TestLaunchHint(t *testing.T) { + launch := serverLaunch{Command: `C:\Users\u\.armis\plugins\armis-appsec-mcp\.venv\Scripts\python.exe`, Args: []string{"server.py"}} + tests := []struct { + name string + err error + stderr string + wantFix FixAction + want string + }{ + {"venv base python gone", errors.New("no response to initialize: server exited"), `No Python at '"C:\Python311\python.exe'`, FixReinstall, "base Python"}, + {"missing module", errors.New("no response"), "ModuleNotFoundError: No module named 'mcp'", FixReinstall, "dependencies"}, + {"missing interpreter", errors.New("starting process: exec: file does not exist: The system cannot find the file specified."), "", FixReinstall, "missing"}, + {"antivirus", errors.New("starting process: Operation did not complete successfully because the file contains a virus"), "", FixNone, "Antivirus"}, + {"applocker", errors.New("starting process: This program is blocked by group policy."), "", FixNone, "AppLocker"}, + {"timeout", errors.New("timed out waiting for initialize response after 15s"), "", FixNone, "--timeout"}, + {"stdout noise", errors.New(`invalid response (non-JSON on stdout: "hello")`), "", FixNone, "stdout"}, + {"unknown", errors.New("weird"), "", FixNone, "server.py"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hint, fix := launchHint(tt.err, tt.stderr, launch) + if fix != tt.wantFix { + t.Errorf("fix = %q, want %q", fix, tt.wantFix) + } + if !strings.Contains(hint, tt.want) { + t.Errorf("hint = %q, want it to mention %q", hint, tt.want) + } + }) + } +} + +func TestNetworkHint(t *testing.T) { + tests := map[string]string{ + "ERR ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed": "SSL_CERT_FILE", + "ERR ProxyError 407 Proxy Authentication Required": "HTTPS_PROXY value", + "ERR ConnectError [Errno 11001] getaddrinfo failed": "PAC", + "ERR Something": "HTTPS_PROXY / SSL_CERT_FILE", + } + for out, want := range tests { + if got := networkHint(out, "/p/.env"); !strings.Contains(got, want) { + t.Errorf("networkHint(%q) = %q, want it to mention %q", out, got, want) + } + } +} + +func TestServerAPIURL(t *testing.T) { + t.Setenv("APPSEC_API_URL", "") + t.Setenv("APPSEC_ENV", "") + if got := serverAPIURL(nil); got != appsecProdURL { + t.Errorf("default = %q", got) + } + if got := serverAPIURL(map[string]string{"APPSEC_ENV": "dev"}); got != appsecDevURL { + t.Errorf("dev = %q", got) + } + if got := serverAPIURL(map[string]string{"APPSEC_ENV": "dev", "APPSEC_API_URL": "https://x"}); got != "https://x" { + t.Errorf("override = %q", got) + } +} + +func TestCheckVenvBase(t *testing.T) { + dir := t.TempDir() + report := &DoctorReport{} + if !checkVenvBase(report, "scanner", dir) { + t.Error("checkVenvBase() without pyvenv.cfg = false, want true") + } + + mustWrite(t, filepath.Join(dir, "pyvenv.cfg"), "home = "+dir+"\nversion = 3.12\n") + if !checkVenvBase(report, "scanner", dir) { + t.Error("checkVenvBase() with existing home = false, want true") + } + + mustWrite(t, filepath.Join(dir, "pyvenv.cfg"), "home = "+filepath.Join(dir, "gone")+"\n") + if checkVenvBase(report, "scanner", dir) { + t.Error("checkVenvBase() with missing home = true, want false") + } + if len(report.Checks) != 1 || report.Checks[0].Status != StatusFail || report.Checks[0].Fix != FixReinstall { + t.Errorf("checks = %+v, want one fail with FixReinstall", report.Checks) + } +} + +func TestCheckCredentialsBOMAndSecrets(t *testing.T) { + envFile := filepath.Join(t.TempDir(), ".env") + mustWrite(t, envFile, "\xEF\xBB\xBFARMIS_CLIENT_ID=the-id\nARMIS_CLIENT_SECRET=the-secret\n") + + report := &DoctorReport{} + env := checkCredentials(report, "scanner", envFile) + if env["ARMIS_CLIENT_ID"] != "the-id" { + t.Errorf("env = %v, want BOM stripped from the first key", env) + } + checks := checkMap(report) + wantStatus(t, checks, "scanner/credentials file", StatusWarn) + wantStatus(t, checks, "scanner/credentials", StatusOK) + if strings.Contains(report.Artifacts["scanner/env-keys.txt"], "the-secret") { + t.Error("env-keys artifact contains a secret value") + } + if len(report.secrets) != 2 { + t.Errorf("secrets = %d, want 2 collected for scrubbing", len(report.secrets)) + } +} + +func TestReportFixes(t *testing.T) { + r := &DoctorReport{} + if r.Fixes() != nil { + t.Error("empty report has fixes") + } + r.add("a", "ok", StatusOK, "").fix(FixReinstall, "") + if r.Fixes() != nil { + t.Error("fix on an ok check was counted") + } + r.add("a", "b", StatusWarn, "").fix(FixReregister, "") + if f := r.Fixes(); len(f) != 1 || f[0] != FixReregister { + t.Errorf("Fixes() = %v, want [reregister]", f) + } + r.add("a", "c", StatusFail, "").fix(FixReinstall, "") + if f := r.Fixes(); len(f) != 1 || f[0] != FixReinstall { + t.Errorf("Fixes() = %v, want [reinstall] to subsume reregister", f) + } +} + +// writeVSCodeFixture builds a VS Code user data dir that exhibits the +// problems the doctor should catch. +func writeVSCodeFixture(t *testing.T, root, goodCommand string) { + t.Helper() + user := filepath.Join(root, "User") + mustWrite(t, filepath.Join(user, "mcp.json"), `{ + // added by hand + "servers": { + "armis-appsec": {"type": "stdio", "command": "`+filepath.ToSlash(goodCommand)+`"}, + }, +}`) + mustWrite(t, filepath.Join(user, "settings.json"), `{ + "chat.mcp.enabled": false, + "chat.agent.enabled": false, + "mcp": {"servers": {"armis-appsec": {"command": "/no/such/python"}}}, +}`) + mustWrite(t, filepath.Join(user, "profiles", "abc123", "mcp.json"), `{"servers": {"other": {"command": "node"}}}`) + mustWrite(t, filepath.Join(user, "globalStorage", "storage.json"), + `{"userDataProfiles": [{"location": "abc123", "name": "Work"}]}`) +} + +func TestCheckVSCodeFindsConfigProblems(t *testing.T) { + root := t.TempDir() + workspace := t.TempDir() + writeVSCodeFixture(t, root, os.Args[0]) + mustWrite(t, filepath.Join(workspace, ".vscode", "mcp.json"), `{"servers": {"armis-appsec": {"command": "/no/such/python"}}}`) + stubVSCode(t, []vscodeVariant{{Name: "VS Code", Root: root}}) + + d := newDoctorRun(DoctorOptions{WorkspaceDir: workspace}) + checkVSCode(d, "/plugin", true) + checks := checkMap(d.report) + + wantStatus(t, checks, "vscode/VS Code (user mcp.json)", StatusOK) + wantStatus(t, checks, "vscode/VS Code (user settings.json)", StatusFail) + dup := wantStatus(t, checks, "vscode/VS Code duplicates", StatusWarn) + if !strings.Contains(dup.Detail, "2 times") { + t.Errorf("duplicates detail = %q", dup.Detail) + } + profile := wantStatus(t, checks, "vscode/VS Code profile", StatusWarn) + if !strings.Contains(profile.Detail, `"Work"`) { + t.Errorf("profile detail = %q, want the profile's display name", profile.Detail) + } + settings := wantStatus(t, checks, "vscode/VS Code settings", StatusFail) + if !strings.Contains(settings.Detail, "chat.mcp.enabled") || !strings.Contains(settings.Detail, "chat.agent.enabled") { + t.Errorf("settings detail = %q", settings.Detail) + } + wantStatus(t, checks, "vscode/VS Code log", StatusInfo) + wantStatus(t, checks, "vscode/workspace .vscode/mcp.json", StatusFail) + wantStatus(t, checks, "vscode/Copilot", StatusInfo) +} + +func TestCheckVSCodeNotRegistered(t *testing.T) { + stable, insiders := t.TempDir(), t.TempDir() + for _, root := range []string{stable, insiders} { + mustWrite(t, filepath.Join(root, "User", "settings.json"), `{}`) + } + stubVSCode(t, []vscodeVariant{{Name: "VS Code", Root: stable}, {Name: "VS Code Insiders", Root: insiders}}) + + d := newDoctorRun(DoctorOptions{WorkspaceDir: t.TempDir()}) + checkVSCode(d, "/plugin", false) + checks := checkMap(d.report) + + if c := wantStatus(t, checks, "vscode/VS Code", StatusWarn); !strings.Contains(c.Remediation, "armis-cli install") { + t.Errorf("stable hint = %q", c.Remediation) + } + if c := wantStatus(t, checks, "vscode/VS Code Insiders", StatusWarn); !strings.Contains(c.Remediation, `"envFile"`) { + t.Errorf("insiders hint = %q, want a paste-in snippet", c.Remediation) + } + wantStatus(t, checks, "vscode/VS Code settings", StatusOK) +} + +func TestCheckVSCodeSkipsManifestConfig(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "User", "mcp.json"), `{"servers": {"armis-appsec": {"command": "/no/such/python"}}}`) + stubVSCode(t, []vscodeVariant{{Name: "VS Code", Root: root}}) + + d := newDoctorRun(DoctorOptions{WorkspaceDir: t.TempDir()}) + d.manifestConfigs[filepath.Clean(filepath.Join(root, "User", "mcp.json"))] = true + checkVSCode(d, "/plugin", true) + if _, ok := checkMap(d.report)["vscode/VS Code (user mcp.json)"]; ok { + t.Error("config already covered by the manifest check was reported again") + } +} + +func TestCheckVSCodeNoVSCode(t *testing.T) { + stubVSCode(t, nil) + d := newDoctorRun(DoctorOptions{}) + checkVSCode(d, "/plugin", false) + if len(d.report.Checks) != 0 { + t.Errorf("checks = %+v, want none when VS Code isn't installed or registered", d.report.Checks) + } +} + +func TestCheckVSCodeLog(t *testing.T) { + logName := "mcpServer.mcp.config.usrlocal.armis-appsec.log" + tests := []struct { + name string + content string + want CheckStatus + detail string + }{ + {"error after start", "[info] Connection state: Running\n[error] Connection state: Error Process exited with code 1\n", StatusWarn, "exited with code 1"}, + {"recovered", "[error] spawn ENOENT\n[info] Connection state: Running\n", StatusOK, "no errors"}, + {"empty", "", StatusInfo, "empty"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "logs", "20260101T000000", "window1", logName), "stale") + mustWrite(t, filepath.Join(root, "logs", "20260102T000000", "window1", logName), tt.content) + old := time.Now().Add(-time.Hour) + _ = os.Chtimes(filepath.Join(root, "logs", "20260101T000000", "window1", logName), old, old) + + report := &DoctorReport{} + checkVSCodeLog(report, vscodeVariant{Name: "VS Code", Root: root}) + c := wantStatus(t, checkMap(report), "vscode/VS Code log", tt.want) + if !strings.Contains(c.Detail, tt.detail) { + t.Errorf("detail = %q, want %q", c.Detail, tt.detail) + } + }) + } +} + +func TestPolicyProblems(t *testing.T) { + out := "\r\nHKEY_LOCAL_MACHINE\\SOFTWARE\\Policies\\Microsoft\\VSCode\r\n" + + " ChatMCP REG_SZ registry\r\n" + + " ChatAgentMode REG_DWORD 0x0\r\n" + + " UpdateMode REG_SZ none\r\n" + values := parseRegQuery(out) + if values["ChatMCP"] != "registry" || values["ChatAgentMode"] != "0x0" { + t.Fatalf("parseRegQuery() = %v", values) + } + problems := policyProblems("HKLM", values) + if len(problems) != 2 { + t.Errorf("policyProblems() = %v, want ChatMCP and ChatAgentMode", problems) + } + if p := policyProblems("HKLM", map[string]string{"ChatMCP": "all", "ChatAgentMode": "0x1"}); len(p) != 0 { + t.Errorf("policyProblems() for permissive policy = %v", p) + } +} + +func TestWriteSupportBundleScrubsSecrets(t *testing.T) { + report := &DoctorReport{secrets: []string{"super-secret-value"}} + report.add("scanner", "live handshake", StatusFail, "auth failed for super-secret-value") + report.artifact("stderr/x.txt", "Traceback: token super-secret-value rejected\n") + report.artifact("system.txt", "os: windows/amd64\n") + + path := filepath.Join(t.TempDir(), "bundle.zip") + if err := WriteSupportBundle(report, path, "1.2.3"); err != nil { + t.Fatalf("WriteSupportBundle() error = %v", err) + } + zr, err := zip.OpenReader(path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = zr.Close() }() + + names := map[string]bool{} + for _, f := range zr.File { + names[f.Name] = true + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + b, _ := io.ReadAll(rc) + _ = rc.Close() + if strings.Contains(string(b), "super-secret-value") { + t.Errorf("%s contains the secret: %s", f.Name, b) + } + if f.Name == "report.json" { + var decoded DoctorReport + if err := json.Unmarshal(b, &decoded); err != nil || len(decoded.Checks) != 1 { + t.Errorf("report.json = %s (err %v)", b, err) + } + } + } + for _, want := range []string{"report.json", "README.txt", "stderr/x.txt", "system.txt"} { + if !names[want] { + t.Errorf("bundle missing %s; has %v", want, names) + } + } +} + +func TestMaskURLUserinfo(t *testing.T) { + if got := maskURLUserinfo("http://user:pw@proxy:8080"); got != "http://***@proxy:8080" { + t.Errorf("maskURLUserinfo() = %q", got) + } +} diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go new file mode 100644 index 0000000..6e78978 --- /dev/null +++ b/internal/install/doctor_probe.go @@ -0,0 +1,480 @@ +package install + +import ( + "bufio" + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "os" + "os/exec" + "sort" + "strings" + "time" +) + +// maxStderrCapture bounds how much of a spawned server's stderr the doctor +// keeps for the support bundle (CWE-770). +const maxStderrCapture = 64 << 10 // 64 KB + +// slowStartThreshold is the handshake latency above which the doctor warns +// that the server starts slowly — typically real-time antivirus scanning the +// venv on first launch. +const slowStartThreshold = 5 * time.Second + +// debugConfigTool is the scanner's diagnostic tool; calling it proves the +// server executes tool calls end to end, not just the handshake. +const debugConfigTool = "debug_config" + +// serverLaunch is everything needed to start an MCP stdio server the way an +// editor would: the command, its arguments, and the environment the editor +// adds on top of its own (envFile contents plus inline env). +type serverLaunch struct { + Command string + Args []string + EnvFile string + Env map[string]string +} + +// key identifies launches that would behave identically, so the doctor spawns +// each distinct configuration only once. +func (l serverLaunch) key() string { + keys := make([]string, 0, len(l.Env)) + for k := range l.Env { + keys = append(keys, k) + } + sort.Strings(keys) + var b strings.Builder + b.WriteString(l.Command) + for _, a := range l.Args { + b.WriteString("\x00" + a) + } + b.WriteString("\x00envFile=" + l.EnvFile) + for _, k := range keys { + b.WriteString("\x00" + k + "=" + l.Env[k]) + } + return b.String() +} + +// commandLine renders the launch as a command the user can paste into a +// terminal to reproduce the failure. +func (l serverLaunch) commandLine() string { + parts := make([]string, 0, len(l.Args)+1) + for _, p := range append([]string{l.Command}, l.Args...) { + if strings.ContainsAny(p, " \t") { + p = `"` + p + `"` + } + parts = append(parts, p) + } + return strings.Join(parts, " ") +} + +// probeResult is what a live MCP session with the server reported. +type probeResult struct { + ServerName string + ServerVersion string + Tools []string + ToolsErr error + DebugConfig string + DebugErr error +} + +type mcpInitResult struct { + ServerInfo struct { + Name string `json:"name"` + Version string `json:"version"` + } `json:"serverInfo"` +} + +// mcpHandshake spawns command as an MCP stdio server and runs a short session: +// initialize, then tools/list, then (when the server offers it) a +// debug_config tool call. Only the initialize step decides the returned +// error; later steps report their own errors on the result so the caller can +// tell "won't start" apart from "starts but tools are broken". +// +// The process is always killed and waited-on before returning, so stderr can +// be read back safely (os/exec only finishes copying stderr into the buffer +// once Wait returns). The returned string is the full (capped) stderr. +func mcpHandshake(command string, args []string, env map[string]string, timeout time.Duration) (*probeResult, string, error) { + if timeout <= 0 { + timeout = DefaultHandshakeTimeout + } + + // armis:ignore cwe:78 cwe:88 reason:command/args come from the CLI's own recorded install paths or the user's own editor config, not remote input + cmd := exec.Command(command, args...) //nolint:gosec // command/args are the CLI's own recorded install paths / user's editor config + cmd.Env = os.Environ() + for k, v := range env { + cmd.Env = append(cmd.Env, k+"="+v) + } + + stdin, err := cmd.StdinPipe() + if err != nil { + return nil, "", fmt.Errorf("opening stdin: %w", err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + _ = stdin.Close() + return nil, "", fmt.Errorf("opening stdout: %w", err) + } + stderrBuf := &cappedBuffer{max: maxStderrCapture} + cmd.Stderr = stderrBuf + + // Start() failing means Wait() will never run to close these pipes for us + // (that cleanup is documented as conditional on a successful Start), so + // close them ourselves rather than leaking the file descriptors. + if err := cmd.Start(); err != nil { + _ = stdin.Close() + _ = stdout.Close() + return nil, "", fmt.Errorf("starting process: %w", err) + } + + result, opErr := runMCPSession(stdin, stdout, timeout) + + // armis:ignore cwe:404 reason:best-effort cleanup of a short-lived diagnostic subprocess we just spawned + _ = cmd.Process.Kill() + // On the timeout path, the session's reader goroutine may still be + // blocked reading stdout when we get here. os/exec's docs warn it is + // "incorrect to call Wait before all reads from the pipe have completed" + // because Wait closes this same pipe as part of its own cleanup — close + // it here first so the unblock is explicit and ordered rather than racing + // Wait's internal close. + _ = stdout.Close() + _ = stdin.Close() + _ = cmd.Wait() + + stderr := strings.TrimSpace(stderrBuf.String()) + if opErr != nil { + return nil, stderr, opErr + } + return result, stderr, nil +} + +// rpcMessage is a JSON-RPC response or notification read from the server. +type rpcMessage struct { + ID *int `json:"id"` + Result json.RawMessage `json:"result"` + Error *struct { + Message string `json:"message"` + } `json:"error"` +} + +// mcpSession drives JSON-RPC over the server's stdio. Every read shares one +// deadline so a hung server can't stretch the doctor past its timeout. +type mcpSession struct { + stdin io.Writer + lines chan []byte + readErr chan error + deadline time.Time +} + +func runMCPSession(stdin io.WriteCloser, stdout io.ReadCloser, timeout time.Duration) (*probeResult, error) { + s := &mcpSession{ + stdin: stdin, + lines: make(chan []byte, 16), + readErr: make(chan error, 1), + deadline: time.Now().Add(timeout), + } + go func() { + scanner := bufio.NewScanner(stdout) + scanner.Buffer(make([]byte, 0, 64*1024), maxHandshakeLineSize) + for scanner.Scan() { + s.lines <- append([]byte(nil), scanner.Bytes()...) + } + s.readErr <- scanner.Err() + }() + + initRaw, err := s.call(1, "initialize", map[string]interface{}{ + "protocolVersion": "2024-11-05", + "capabilities": map[string]interface{}{}, + "clientInfo": map[string]interface{}{jsonKeyName: "armis-cli-doctor", jsonKeyVersion: "1.0"}, + }, timeout) + if err != nil { + return nil, err + } + var init mcpInitResult + if err := json.Unmarshal(initRaw, &init); err != nil { + return nil, fmt.Errorf("invalid initialize result: %w", err) + } + res := &probeResult{ServerName: init.ServerInfo.Name, ServerVersion: init.ServerInfo.Version} + + if err := s.notify("notifications/initialized"); err != nil { + res.ToolsErr = err + return res, nil + } + + toolsRaw, err := s.call(2, "tools/list", map[string]interface{}{}, timeout) + if err != nil { + res.ToolsErr = err + return res, nil + } + var tools struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + } + if err := json.Unmarshal(toolsRaw, &tools); err != nil { + res.ToolsErr = fmt.Errorf("invalid tools/list result: %w", err) + return res, nil + } + for _, t := range tools.Tools { + res.Tools = append(res.Tools, t.Name) + } + + for _, name := range res.Tools { + if name != debugConfigTool { + continue + } + callRaw, err := s.call(3, "tools/call", map[string]interface{}{ + jsonKeyName: debugConfigTool, + "arguments": map[string]interface{}{}, + }, timeout) + if err != nil { + res.DebugErr = err + break + } + text, isErr := toolCallText(callRaw) + if isErr { + res.DebugErr = fmt.Errorf("tool returned an error: %s", text) + } + res.DebugConfig = text + break + } + return res, nil +} + +func (s *mcpSession) notify(method string) error { + b, err := json.Marshal(map[string]interface{}{"jsonrpc": "2.0", "method": method}) + if err != nil { + return err + } + if _, err := s.stdin.Write(append(b, '\n')); err != nil { + return fmt.Errorf("writing %s: %w", method, err) + } + return nil +} + +// call sends a request and waits for the response carrying the same id, +// skipping notifications and log lines the server may interleave. +func (s *mcpSession) call(id int, method string, params interface{}, timeout time.Duration) (json.RawMessage, error) { + b, err := json.Marshal(map[string]interface{}{"jsonrpc": "2.0", "id": id, "method": method, "params": params}) + if err != nil { + return nil, err + } + if _, err := s.stdin.Write(append(b, '\n')); err != nil { + return nil, fmt.Errorf("writing %s request: %w", method, err) + } + + timer := time.NewTimer(time.Until(s.deadline)) + defer timer.Stop() + for { + select { + case <-timer.C: + return nil, fmt.Errorf("timed out waiting for %s response after %s", method, timeout) + case err := <-s.readErr: + s.readErr <- err // keep it for any later call + if errors.Is(err, bufio.ErrTooLong) { + return nil, fmt.Errorf("response exceeded %d bytes", maxHandshakeLineSize) + } + if err != nil { + return nil, fmt.Errorf("no response to %s: %w", method, err) + } + return nil, fmt.Errorf("no response to %s: server exited", method) + case line := <-s.lines: + var msg rpcMessage + if err := json.Unmarshal(line, &msg); err != nil { + if id == 1 { + // Anything but JSON on stdout before initialize means the + // server (or a wrapper script) is printing to stdout, + // which corrupts the MCP stream for every client. + return nil, fmt.Errorf("invalid response (non-JSON on stdout: %q): %w", truncate(string(line), 120), err) + } + continue + } + if msg.ID == nil || *msg.ID != id { + continue + } + if msg.Error != nil { + return nil, fmt.Errorf("server returned error: %s", msg.Error.Message) + } + if len(msg.Result) == 0 || string(msg.Result) == "null" { + return nil, fmt.Errorf("response missing result") + } + return msg.Result, nil + } + } +} + +// toolCallText joins the text content blocks of a tools/call result. +func toolCallText(raw json.RawMessage) (string, bool) { + var r struct { + Content []struct { + Type string `json:"type"` + Text string `json:"text"` + } `json:"content"` + IsError bool `json:"isError"` + } + if err := json.Unmarshal(raw, &r); err != nil { + return "", true + } + var parts []string + for _, c := range r.Content { + if c.Type == "text" { + parts = append(parts, c.Text) + } + } + return strings.Join(parts, "\n"), r.IsError +} + +// cappedBuffer keeps the last max bytes written to it. +type cappedBuffer struct { + buf bytes.Buffer + max int +} + +func (c *cappedBuffer) Write(p []byte) (int, error) { + n := len(p) + c.buf.Write(p) + if over := c.buf.Len() - c.max; over > 0 { + c.buf.Next(over) + } + return n, nil +} + +func (c *cappedBuffer) String() string { return c.buf.String() } + +func truncate(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] + "…" +} + +// tail returns the last n bytes of s. +func tail(s string, n int) string { + s = strings.TrimSpace(s) + if len(s) > n { + s = "…" + s[len(s)-n:] + } + return s +} + +// launchHint maps a failed launch to a remediation and, when the CLI can +// repair it itself, the fix action. It recognizes the failures most common on +// Windows, where they otherwise surface as an opaque process error. +func launchHint(err error, stderr string, launch serverLaunch) (string, FixAction) { + msg := strings.ToLower(err.Error()) + se := strings.ToLower(stderr) + manual := "To see the full error, run the server by hand: " + launch.commandLine() + + switch { + case strings.Contains(se, "no python at"): + return "The venv's base Python interpreter was removed or moved (e.g. Python was uninstalled or upgraded). The venv must be rebuilt.", FixReinstall + case strings.Contains(se, "modulenotfounderror") || strings.Contains(se, "importerror"): + return "The server's Python dependencies are missing or broken. The venv must be rebuilt.", FixReinstall + case strings.Contains(msg, "executable file not found") || strings.Contains(msg, "no such file") || + strings.Contains(msg, "cannot find the file") || strings.Contains(msg, "cannot find the path"): + return "The server's Python interpreter is missing.", FixReinstall + case strings.Contains(msg, "virus") || strings.Contains(msg, "potentially unwanted"): + return "Antivirus blocked or quarantined the server's python.exe. Ask IT to allow " + launch.Command + " and re-run with --fix.", FixNone + case strings.Contains(msg, "access is denied") || strings.Contains(msg, "permission denied") || + strings.Contains(msg, "blocked by group policy") || strings.Contains(msg, "operation did not complete"): + return "The OS refused to start " + launch.Command + ". On managed Windows machines this is usually AppLocker, WDAC, or antivirus blocking executables under your user profile — ask IT to allow it. " + manual, FixNone + case strings.Contains(msg, "timed out"): + return "The server didn't answer in time. First start after install or an antivirus scan can be slow: retry with --timeout 60s. If it's slow every time, ask IT to exclude the plugin directory from real-time scanning. " + manual, FixNone + case strings.Contains(msg, "non-json on stdout"): + return "Something prints to stdout before the MCP stream starts, which breaks every MCP client. " + manual, FixNone + } + return manual, FixNone +} + +// --- Server-runtime network probe --- + +// networkProbeScript makes one HTTPS request with the same library (httpx) and +// defaults (certifi CA bundle, HTTPS_PROXY/SSL_CERT_FILE from the environment) +// the MCP server uses. The CLI's own Go HTTP stack uses the OS certificate +// store and PAC proxy settings, so a Go-side check can pass while the server +// fails — this probe measures what the server will actually experience. +const networkProbeScript = `import sys +try: + import httpx + r = httpx.get(sys.argv[1], timeout=15) + print("HTTP", r.status_code) +except Exception as e: + print("ERR", type(e).__name__, str(e)[:500]) + sys.exit(1) +` + +const ( + appsecProdURL = "https://moose.armis.com/api/v1" + appsecDevURL = "https://moose-dev.armis.com/api/v1" +) + +// serverAPIURL resolves the API URL the scanner server will call, mirroring +// scanner_core.py: APPSEC_API_URL wins, else APPSEC_ENV picks dev or prod. +func serverAPIURL(env map[string]string) string { + get := func(k string) string { + if v := env[k]; v != "" { + return v + } + return os.Getenv(k) + } + if u := get("APPSEC_API_URL"); u != "" { + return u + } + if strings.EqualFold(get("APPSEC_ENV"), "dev") { + return appsecDevURL + } + return appsecProdURL +} + +// runNetworkProbe runs networkProbeScript with the server's interpreter and +// environment and returns its single line of output. +func runNetworkProbe(python string, env map[string]string, url string, timeout time.Duration) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + // armis:ignore cwe:78 cwe:88 reason:python is the CLI's own recorded venv interpreter; the script is a constant and url is passed as a separate argv element + cmd := exec.CommandContext(ctx, python, "-c", networkProbeScript, url) //nolint:gosec // constant script, venv interpreter + cmd.Env = os.Environ() + for k, v := range env { + cmd.Env = append(cmd.Env, k+"="+v) + } + stdout := &cappedBuffer{max: 4096} + stderr := &cappedBuffer{max: 4096} + cmd.Stdout = stdout + cmd.Stderr = stderr + err := cmd.Run() + result := strings.TrimSpace(stdout.String()) + if ctx.Err() != nil { + return result, fmt.Errorf("timed out after %s", timeout) + } + if err != nil { + if result == "" { + result = tail(stderr.String(), 500) + } + if result == "" { + result = err.Error() + } + return result, errors.New(result) + } + return result, nil +} + +// networkHint maps a failed network probe to a remediation. envFile is where +// the server reads extra environment variables from. +func networkHint(output, envFile string) string { + o := strings.ToLower(output) + switch { + case strings.Contains(o, "certificate_verify_failed") || strings.Contains(o, "certificate verify failed") || + strings.Contains(o, "self-signed") || strings.Contains(o, "unable to get local issuer"): + return "Your network re-signs HTTPS traffic (TLS inspection, e.g. Zscaler or Netskope) and the MCP server's Python runtime doesn't trust that certificate — it uses its own CA bundle, not the Windows certificate store. " + + "Export your organization's root CA as a PEM (Base-64 .cer) file, then add SSL_CERT_FILE= to " + envFile + " and restart VS Code." + case strings.Contains(o, "proxyerror") || strings.Contains(o, "407"): + return "The proxy rejected the request. Check the HTTPS_PROXY value in " + envFile + " (including credentials if your proxy requires them)." + case strings.Contains(o, "connecterror") || strings.Contains(o, "connecttimeout") || strings.Contains(o, "timed out") || + strings.Contains(o, "getaddrinfo") || strings.Contains(o, "name or service not known") || strings.Contains(o, "nodename"): + return "The server's Python runtime can't reach the Armis API. Python ignores Windows proxy/PAC settings: if you're behind a corporate proxy, add HTTPS_PROXY=http://: to " + envFile + " and restart VS Code." + } + return "The server's Python runtime couldn't reach the Armis API. Add HTTPS_PROXY / SSL_CERT_FILE to " + envFile + " if your network requires a proxy or TLS inspection." +} diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index 92e2f41..d4daab3 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -2,6 +2,7 @@ package install import ( "bufio" + "encoding/json" "fmt" "os" "path/filepath" @@ -22,19 +23,51 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +// runMCPHelperProcess acts as a fake MCP stdio server. It answers each +// request line by method so the doctor's full session (initialize, +// tools/list, tools/call) can be exercised. func runMCPHelperProcess(mode string) { - switch mode { - case "hang": + if mode == "hang" { select {} - case "garbage": - _, _ = bufio.NewReader(os.Stdin).ReadBytes('\n') - _, _ = fmt.Fprintln(os.Stdout, "not json") - case "error": - _, _ = bufio.NewReader(os.Stdin).ReadBytes('\n') - _, _ = fmt.Fprintln(os.Stdout, `{"jsonrpc":"2.0","id":1,"error":{"code":-1,"message":"boom"}}`) - default: // "ok" - _, _ = bufio.NewReader(os.Stdin).ReadBytes('\n') - _, _ = fmt.Fprintln(os.Stdout, `{"jsonrpc":"2.0","id":1,"result":{"serverInfo":{"name":"fake-mcp","version":"9.9.9"}}}`) + } + reader := bufio.NewReader(os.Stdin) + for { + line, err := reader.ReadBytes('\n') + if err != nil { + return + } + var req struct { + ID *int `json:"id"` + Method string `json:"method"` + } + if json.Unmarshal(line, &req) != nil || req.ID == nil { + continue // notification + } + reply := func(body string) { + _, _ = fmt.Fprintf(os.Stdout, `{"jsonrpc":"2.0","id":%d,%s}`+"\n", *req.ID, body) + } + switch { + case mode == "garbage": + _, _ = fmt.Fprintln(os.Stdout, "not json") + return + case mode == "error": + reply(`"error":{"code":-1,"message":"boom"}`) + return + case req.Method == "initialize": + // A log notification before the response must be skipped. + _, _ = fmt.Fprintln(os.Stdout, `{"jsonrpc":"2.0","method":"notifications/message","params":{}}`) + reply(`"result":{"serverInfo":{"name":"fake-mcp","version":"9.9.9"}}`) + case req.Method == "tools/list" && mode == "notools": + reply(`"result":{"tools":[]}`) + case req.Method == "tools/list": + reply(`"result":{"tools":[{"name":"scan_code"},{"name":"debug_config"}]}`) + case req.Method == "tools/call" && mode == "toolerror": + reply(`"result":{"content":[{"type":"text","text":"config broken"}],"isError":true}`) + case req.Method == "tools/call": + reply(`"result":{"content":[{"type":"text","text":"Auth: configured\nAuth method: JWT\nAPI URL: https://moose.armis.com/api/v1\nEnv: prod"}]}`) + default: + reply(`"error":{"code":-32601,"message":"method not found"}`) + } } } @@ -46,6 +79,12 @@ func TestMCPHandshakeSuccess(t *testing.T) { if res.ServerName != "fake-mcp" || res.ServerVersion != "9.9.9" { t.Errorf("mcpHandshake() result = %+v, want fake-mcp v9.9.9", res) } + if res.ToolsErr != nil || strings.Join(res.Tools, ",") != "scan_code,debug_config" { + t.Errorf("mcpHandshake() tools = %v (err %v), want scan_code,debug_config", res.Tools, res.ToolsErr) + } + if res.DebugErr != nil || !strings.Contains(res.DebugConfig, "Auth method: JWT") { + t.Errorf("mcpHandshake() debug_config = %q (err %v), want Auth method line", res.DebugConfig, res.DebugErr) + } } func TestMCPHandshakeServerError(t *testing.T) { @@ -211,8 +250,9 @@ func TestCheckManifestEditors(t *testing.T) { EditorVSCode: {ConfigFile: deadCommandFile, Format: "mcpServers"}, } - report := &DoctorReport{} - checkManifestEditors(report, "scanner", "armis-appsec", editors) + d := newDoctorRun(DoctorOptions{}) + checkManifestEditors(d, "scanner", "armis-appsec", editors) + report := d.report statuses := make(map[string]CheckStatus) for _, c := range report.Checks { @@ -298,6 +338,7 @@ func TestIsExecutableFile(t *testing.T) { } func TestRunDoctorNoManifest(t *testing.T) { + stubVSCode(t, nil) home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) @@ -312,6 +353,7 @@ func TestRunDoctorNoManifest(t *testing.T) { } func TestRunDoctorStructuralChecks(t *testing.T) { + stubVSCode(t, nil) home := t.TempDir() t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) @@ -363,8 +405,9 @@ func TestCheckKnowledgePluginSkipsUninstalledSiblingEnv(t *testing.T) { _ = os.MkdirAll(filepath.Join(dir, "dev"), 0o750) _ = os.WriteFile(filepath.Join(dir, "dev", "bridge.py"), []byte("# bridge"), 0o600) - report := &DoctorReport{} - checkKnowledgePlugin(report, &ManifestKnowledge{PluginDir: dir}, DoctorOptions{Handshake: false}) + d := newDoctorRun(DoctorOptions{Handshake: false}) + checkKnowledgePlugin(d, &ManifestKnowledge{PluginDir: dir}) + report := d.report if report.HasFailures() { t.Errorf("checkKnowledgePlugin() unexpected failures for uninstalled sibling env: %+v", report.Checks) diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go new file mode 100644 index 0000000..9a98a48 --- /dev/null +++ b/internal/install/doctor_vscode.go @@ -0,0 +1,634 @@ +package install + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "regexp" + "runtime" + "sort" + "strings" + "time" +) + +const componentVSCode = "vscode" + +// vscodeVariant is one VS Code build whose user data lives under Root +// (e.g. %APPDATA%\Code - Insiders). +type vscodeVariant struct { + Name string + Root string +} + +// vscodeVariants lists the VS Code builds to inspect. A var so tests can point +// it at a temp directory instead of the real %APPDATA%. +var vscodeVariants = func() []vscodeVariant { + var out []vscodeVariant + for _, v := range []struct{ dir, name string }{ + {"Code", editorNameVSCode}, + {"Code - Insiders", "VS Code Insiders"}, + {"VSCodium", "VSCodium"}, + } { + if root := appSupportPath(v.dir); root != "" { + out = append(out, vscodeVariant{Name: v.name, Root: root}) + } + } + return out +} + +// vscodePolicyQuery returns the output of `reg query` for a VS Code Group +// Policy key, or "" when the key doesn't exist. A var so tests can stub it. +var vscodePolicyQuery = func(key string) string { + if runtime.GOOS != osWindows { + return "" + } + // armis:ignore cwe:78 reason:fixed binary and fixed registry key constants + out, err := exec.Command("reg", "query", key).Output() //nolint:gosec // constant arguments + if err != nil { + return "" + } + return string(out) +} + +// vscodeSource is one VS Code config file that can declare MCP servers. +type vscodeSource struct { + Label string // "user", "profile Work", "workspace", ... + Path string + // ServersKey locates the servers map: mcp.json keeps it at "servers", + // settings.json nests it under "mcp". + InSettings bool +} + +// vscodeFound is an armis entry located in a vscodeSource. +type vscodeFound struct { + Source vscodeSource + Launch serverLaunch +} + +// checkVSCode runs the VS Code/Copilot-specific checks. It looks beyond the +// single config file the manifest records, because the common reasons Copilot +// doesn't show the server are all elsewhere: a different VS Code build +// (Insiders), a profile with its own mcp.json, a duplicate stale entry in a +// workspace or settings.json, MCP or agent mode being disabled by a setting +// or Group Policy, or organization Copilot policy. +func checkVSCode(d *doctorRun, pluginDir string, registered bool) { + report := d.report + workspace := d.opts.WorkspaceDir + if workspace == "" { + workspace, _ = os.Getwd() + } + + var detected []vscodeVariant + for _, v := range vscodeVariants() { + if info, err := os.Stat(filepath.Join(v.Root, "User")); err == nil && info.IsDir() { + detected = append(detected, v) + } + } + if len(detected) == 0 && !registered { + return + } + + snippet := vscodeSnippet(pluginDir) + for _, v := range detected { + checkVSCodeVariant(d, v, workspace, pluginDir, snippet) + } + checkVSCodeWorkspace(d, workspace) + checkVSCodePolicy(report) + + report.add(componentVSCode, "Copilot", StatusInfo, "can't be verified from this machine"). + hint(strings.Join([]string{ + "If every check above passes but Copilot Chat doesn't use the Armis tools:", + "1. Switch Copilot Chat to Agent mode (the mode picker under the chat input) — tools are only used in Agent mode.", + "2. Click the tools icon in the chat input and make sure armis-appsec and its tools are ticked.", + "3. Run \"MCP: List Servers\" from the Command Palette, select armis-appsec, and choose Start Server (accept the trust prompt if shown). \"Show Output\" there shows VS Code's own log for the server.", + "4. On a Copilot Business/Enterprise seat, the \"MCP servers in Copilot\" policy must be enabled by your GitHub organization admin (it is off by default). If it's off, VS Code shows the server but Copilot won't call it.", + }, "\n")) +} + +func checkVSCodeVariant(d *doctorRun, v vscodeVariant, workspace, pluginDir, snippet string) { + report := d.report + userDir := filepath.Join(v.Root, "User") + userMCP := filepath.Join(userDir, "mcp.json") + + sources := []vscodeSource{{Label: "user mcp.json", Path: userMCP}} + profileNames := vscodeProfileNames(v.Root) + profileDirs, _ := filepath.Glob(filepath.Join(userDir, "profiles", "*")) + sort.Strings(profileDirs) + for _, dir := range profileDirs { + id := filepath.Base(dir) + name := profileNames[id] + if name == "" { + name = id + } + sources = append(sources, vscodeSource{Label: "profile \"" + name + "\" mcp.json", Path: filepath.Join(dir, "mcp.json")}) + } + sources = append(sources, vscodeSource{Label: "user settings.json", Path: filepath.Join(userDir, "settings.json"), InSettings: true}) + + var found []vscodeFound + var profilesWithout []vscodeSource + for _, src := range sources { + obj, exists, err := readJSONCObject(src.Path) + if !exists { + continue + } + if err != nil { + if !d.manifestConfigs[filepath.Clean(src.Path)] { + report.add(componentVSCode, v.Name+" config", StatusFail, fmt.Sprintf("%s is not valid JSON: %v", src.Path, err)). + hint(v.Name + " ignores a file it can't parse, so no servers in it load. Fix the syntax (often a missing or extra comma), then re-run this doctor.") + } + continue + } + servers := vscodeServers(obj, src.InSettings) + name, entry, ok := findServer(servers, mcpServerName) + if !ok { + if strings.HasPrefix(src.Label, "profile") && servers != nil { + profilesWithout = append(profilesWithout, src) + } + continue + } + launch := vscodeLaunch(entry, workspace) + found = append(found, vscodeFound{Source: src, Launch: launch}) + report.artifact(fmt.Sprintf("vscode/%s/%s-entry.json", sanitizeArtifactName(v.Name), strings.TrimSuffix(sanitizeArtifactName(src.Label), ".json")), + launchArtifact(src.Path+" → "+name, launch)) + } + + switch { + case len(found) == 0: + c := report.add(componentVSCode, v.Name, StatusWarn, "installed, but armis-appsec isn't registered in any of its MCP configs") + if v.Name == editorNameVSCode { + c.hint("Run: armis-cli install (and select VS Code)") + } else { + c.hint("If you use " + v.Name + ", add this to the \"servers\" object in " + userMCP + ":\n" + snippet) + } + case len(found) > 1: + labels := make([]string, 0, len(found)) + for _, f := range found { + labels = append(labels, f.Source.Label) + } + report.add(componentVSCode, v.Name+" duplicates", StatusWarn, + fmt.Sprintf("armis-appsec is registered %d times (%s)", len(found), strings.Join(labels, ", "))). + hint(v.Name + " starts every copy; a stale one fails and Copilot shows an error or duplicate tools. Keep only the entry in " + userMCP + " and delete the others.") + } + + for _, f := range found { + if d.manifestConfigs[filepath.Clean(f.Source.Path)] { + continue // already checked and probed by checkManifestEditors + } + name := v.Name + " (" + f.Source.Label + ")" + if f.Launch.Command == "" { + report.add(componentVSCode, name, StatusWarn, f.Source.Path+": entry has no command") + continue + } + if !isExecutableFile(f.Launch.Command) { + report.add(componentVSCode, name, StatusFail, + fmt.Sprintf("%s: command does not exist: %s", f.Source.Path, f.Launch.Command)). + hint("This entry is stale. Delete it from " + f.Source.Path + ", or replace it with:\n" + snippet) + continue + } + report.add(componentVSCode, name, StatusOK, f.Source.Path) + if d.opts.Handshake { + d.probe(componentVSCode, name, f.Launch) + } + } + + if len(found) > 0 { + for _, p := range profilesWithout { + report.add(componentVSCode, v.Name+" profile", StatusWarn, + fmt.Sprintf("%s has its own MCP servers but not armis-appsec", p.Label)). + hint("If you use that VS Code profile, Copilot won't see the server there. Add this to the \"servers\" object in " + p.Path + ":\n" + snippet) + } + } + + checkVSCodeSettings(report, v, filepath.Join(userDir, "settings.json")) + checkVSCodeLog(report, v) +} + +// checkVSCodeWorkspace flags a workspace-level entry, which shadows or +// duplicates the user-level one only while that folder is open. +func checkVSCodeWorkspace(d *doctorRun, workspace string) { + if workspace == "" { + return + } + for _, src := range []vscodeSource{ + {Label: "workspace .vscode/mcp.json", Path: filepath.Join(workspace, ".vscode", "mcp.json")}, + {Label: "workspace .vscode/settings.json", Path: filepath.Join(workspace, ".vscode", "settings.json"), InSettings: true}, + } { + obj, exists, err := readJSONCObject(src.Path) + if !exists || err != nil { + continue + } + if _, entry, ok := findServer(vscodeServers(obj, src.InSettings), mcpServerName); ok { + launch := vscodeLaunch(entry, workspace) + status, detail := StatusWarn, src.Path+" also registers armis-appsec" + if launch.Command != "" && !isExecutableFile(launch.Command) { + status, detail = StatusFail, fmt.Sprintf("%s registers armis-appsec with a command that does not exist: %s", src.Path, launch.Command) + } + d.report.add(componentVSCode, src.Label, status, detail). + hint("Workspace entries apply only in this folder and run alongside the user-level one. Remove it from " + src.Path + " unless you need a per-project override.") + } + } +} + +// checkVSCodeSettings reports settings that stop Copilot from using MCP +// servers at all, independent of whether the server itself is healthy. +func checkVSCodeSettings(report *DoctorReport, v vscodeVariant, path string) { + obj, exists, err := readJSONCObject(path) + if !exists || err != nil { + return + } + relevant := make(map[string]interface{}) + for k, val := range obj { + if strings.HasPrefix(k, "chat.") || k == "mcp" || strings.HasPrefix(k, "github.copilot") { + relevant[k] = val + } + } + if b, err := json.MarshalIndent(relevant, "", " "); err == nil { + report.artifact("vscode/"+sanitizeArtifactName(v.Name)+"/settings-chat.json", string(b)) + } + + name := v.Name + " settings" + var problems []string + if enabled, ok := obj["chat.mcp.enabled"].(bool); ok && !enabled { + problems = append(problems, `"chat.mcp.enabled": false turns MCP support off`) + } + switch access, _ := obj["chat.mcp.access"].(string); access { + case "none": + problems = append(problems, `"chat.mcp.access": "none" blocks all MCP servers`) + case "registry": + problems = append(problems, `"chat.mcp.access": "registry" only allows servers from the MCP registry, which blocks locally installed servers like armis-appsec`) + } + if enabled, ok := obj["chat.agent.enabled"].(bool); ok && !enabled { + problems = append(problems, `"chat.agent.enabled": false disables Agent mode, and Copilot only calls MCP tools in Agent mode`) + } + if len(problems) > 0 { + report.add(componentVSCode, name, StatusFail, strings.Join(problems, "; ")). + hint("Remove or change these settings in " + path + " (or via Settings → search \"mcp\" / \"agent\"), then reload VS Code.") + return + } + report.add(componentVSCode, name, StatusOK, "MCP and Agent mode not disabled") +} + +// Windows Group Policy keys VS Code reads its policies from. +var vscodePolicyKeys = []string{ + `HKLM\SOFTWARE\Policies\Microsoft\VSCode`, + `HKCU\SOFTWARE\Policies\Microsoft\VSCode`, +} + +// checkVSCodePolicy reports Group Policy settings that disable MCP or Agent +// mode. Policy wins over user settings and is greyed out in the Settings UI, +// which is why users can't tell why the server never appears. +func checkVSCodePolicy(report *DoctorReport) { + if runtime.GOOS != osWindows { + return + } + var problems []string + anyPolicy := false + for _, key := range vscodePolicyKeys { + out := vscodePolicyQuery(key) + if out == "" { + continue + } + anyPolicy = true + report.artifact("vscode/group-policy.txt", report.Artifacts["vscode/group-policy.txt"]+out+"\n") + problems = append(problems, policyProblems(key, parseRegQuery(out))...) + } + switch { + case len(problems) > 0: + report.add(componentVSCode, "group policy", StatusFail, strings.Join(problems, "; ")). + hint("Your organization's Group Policy disables this, and it can't be overridden locally. Ask IT to allow MCP servers in VS Code (the ChatMCP / ChatAgentMode policies).") + case anyPolicy: + report.add(componentVSCode, "group policy", StatusOK, "VS Code policies present, none restrict MCP") + default: + report.add(componentVSCode, "group policy", StatusOK, "no VS Code Group Policy set") + } +} + +// parseRegQuery parses `reg query` output lines of the form +// " Name REG_TYPE Value" into a name → value map. +func parseRegQuery(out string) map[string]string { + values := make(map[string]string) + for _, line := range strings.Split(out, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 || !strings.HasPrefix(fields[1], "REG_") { + continue + } + values[fields[0]] = strings.Join(fields[2:], " ") + } + return values +} + +func policyProblems(key string, values map[string]string) []string { + var problems []string + disabled := func(v string) bool { + v = strings.ToLower(strings.TrimSpace(v)) + return v == "0x0" || v == "0" || v == "false" || v == "none" + } + for name, val := range values { + switch name { + case "ChatMCP": + if disabled(val) || strings.EqualFold(val, "registry") { + problems = append(problems, fmt.Sprintf("%s\\ChatMCP = %s blocks locally installed MCP servers", key, val)) + } + case "ChatAgentMode": + if disabled(val) { + problems = append(problems, fmt.Sprintf("%s\\ChatAgentMode = %s disables Agent mode", key, val)) + } + } + } + sort.Strings(problems) + return problems +} + +// vscodeLogRE matches VS Code's per-server MCP log files, e.g. +// mcpServer.mcp.config.usrlocal.armis-appsec.log. +var vscodeLogRE = regexp.MustCompile(`(?i)^mcpServer\..*` + regexp.QuoteMeta(mcpServerName) + `.*\.log$`) + +// checkVSCodeLog surfaces VS Code's own log for the server — what the user +// would otherwise have to find via "MCP: List Servers → Show Output". +func checkVSCodeLog(report *DoctorReport, v vscodeVariant) { + path, mod := latestVSCodeServerLog(filepath.Join(v.Root, "logs")) + name := v.Name + " log" + if path == "" { + report.add(componentVSCode, name, StatusInfo, "no VS Code log for armis-appsec yet — VS Code has not tried to start the server"). + hint("Open Copilot Chat in Agent mode, or run \"MCP: List Servers\" → armis-appsec → Start Server, then re-run this doctor.") + return + } + b, err := readBoundedConfigFile(path) + if err != nil { + return + } + content := tail(string(b), 32<<10) + if content == "" { + report.add(componentVSCode, name, StatusInfo, "VS Code created a log for armis-appsec but it's empty: "+path). + hint("VS Code registered the server but hasn't logged a start attempt. Run \"MCP: List Servers\" → armis-appsec → Start Server, then re-run this doctor.") + return + } + report.artifact("vscode/"+sanitizeArtifactName(v.Name)+"/mcp-server.log", content+"\n") + + lastErr := lastVSCodeError(content) + age := time.Since(mod).Round(time.Minute) + if lastErr != "" { + report.add(componentVSCode, name, StatusWarn, fmt.Sprintf("last error (log updated %s ago): %s", age, truncate(lastErr, 300))). + hint("Full log: " + path) + return + } + report.add(componentVSCode, name, StatusOK, fmt.Sprintf("no errors since the last start (log updated %s ago)", age)) +} + +// latestVSCodeServerLog finds the most recently modified armis MCP server log +// across VS Code's session log directories. +func latestVSCodeServerLog(logsDir string) (string, time.Time) { + sessions, err := os.ReadDir(logsDir) + if err != nil { + return "", time.Time{} + } + // Session dirs are timestamped (20260916T172015); the newest few are + // enough and bound the walk on machines with long log histories. + names := make([]string, 0, len(sessions)) + for _, s := range sessions { + if s.IsDir() { + names = append(names, s.Name()) + } + } + sort.Sort(sort.Reverse(sort.StringSlice(names))) + if len(names) > 5 { + names = names[:5] + } + + var best string + var bestMod time.Time + for _, n := range names { + windows, _ := filepath.Glob(filepath.Join(logsDir, n, "window*")) + for _, w := range windows { + entries, _ := os.ReadDir(w) + for _, e := range entries { + if e.IsDir() || !vscodeLogRE.MatchString(e.Name()) { + continue + } + info, err := e.Info() + if err != nil { + continue + } + if info.ModTime().After(bestMod) { + best, bestMod = filepath.Join(w, e.Name()), info.ModTime() + } + } + } + } + return best, bestMod +} + +// lastVSCodeError returns the last [error] line logged after the most recent +// successful start, or "" if the server was running cleanly. +func lastVSCodeError(log string) string { + lines := strings.Split(log, "\n") + lastErr := "" + for _, line := range lines { + l := strings.ToLower(line) + switch { + case strings.Contains(l, "connection state: running"): + lastErr = "" + case strings.Contains(l, "[error]"): + lastErr = strings.TrimSpace(line) + } + } + return lastErr +} + +// vscodeProfileNames maps profile directory IDs to their display names, read +// from VS Code's global storage. +func vscodeProfileNames(root string) map[string]string { + names := make(map[string]string) + obj, exists, err := readJSONCObject(filepath.Join(root, "User", "globalStorage", "storage.json")) + if !exists || err != nil { + return names + } + profiles, _ := obj["userDataProfiles"].([]interface{}) + for _, p := range profiles { + m, _ := p.(map[string]interface{}) + loc, _ := m["location"].(string) + name, _ := m[jsonKeyName].(string) + if loc != "" && name != "" { + names[filepath.Base(loc)] = name + } + } + return names +} + +// readJSONCObject reads a JSONC file into a map. exists is false when the file +// isn't there; err is set when it exists but can't be read or parsed. +func readJSONCObject(path string) (obj map[string]interface{}, exists bool, err error) { + b, err := readBoundedConfigFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil, false, nil + } + return nil, true, err + } + if len(strings.TrimSpace(string(b))) == 0 { + return map[string]interface{}{}, true, nil + } + if err := json.Unmarshal(stripJSONC(b), &obj); err != nil { + return nil, true, err + } + if obj == nil { + return nil, true, fmt.Errorf("top-level value is not an object") + } + return obj, true, nil +} + +// vscodeServers returns the servers map from an mcp.json object or, for +// settings.json, from its nested "mcp" object. +func vscodeServers(obj map[string]interface{}, inSettings bool) map[string]interface{} { + if inSettings { + mcp, _ := obj["mcp"].(map[string]interface{}) + obj = mcp + } + servers, _ := obj["servers"].(map[string]interface{}) + return servers +} + +func findServer(servers map[string]interface{}, identifier string) (string, map[string]interface{}, bool) { + keys := make([]string, 0, len(servers)) + for k := range servers { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + if strings.Contains(strings.ToLower(k), strings.ToLower(identifier)) { + m, _ := servers[k].(map[string]interface{}) + return k, m, true + } + } + return "", nil, false +} + +// vscodeLaunch converts a VS Code server entry into the launch VS Code +// performs: variables expanded, envFile loaded, and inline env layered on top. +func vscodeLaunch(entry map[string]interface{}, workspace string) serverLaunch { + cmd, _ := entry[jsonKeyCommand].(string) + envFile, _ := entry["envFile"].(string) + l := serverLaunch{ + Command: expandVSCodeVars(cmd, workspace), + EnvFile: expandVSCodeVars(envFile, workspace), + } + for _, a := range stringSlice(entry[jsonKeyArgs]) { + l.Args = append(l.Args, expandVSCodeVars(a, workspace)) + } + env := make(map[string]string) + if l.EnvFile != "" { + if fileEnv, err := parseEnvFile(l.EnvFile); err == nil { + for k, v := range fileEnv { + env[k] = v + } + } + } + for k, v := range stringMap(entry["env"]) { + env[k] = expandVSCodeVars(v, workspace) + } + if len(env) > 0 { + l.Env = env + } + return l +} + +var vscodeVarRE = regexp.MustCompile(`\$\{([^}]+)\}`) + +// expandVSCodeVars resolves the VS Code variables that commonly appear in MCP +// entries. Unknown variables are left as-is. +func expandVSCodeVars(s, workspace string) string { + if !strings.Contains(s, "${") { + return s + } + return vscodeVarRE.ReplaceAllStringFunc(s, func(m string) string { + name := m[2 : len(m)-1] + switch { + case name == "userHome": + if h, err := os.UserHomeDir(); err == nil { + return h + } + case name == "workspaceFolder" && workspace != "": + return workspace + case name == "pathSeparator" || name == "/": + return string(filepath.Separator) + case strings.HasPrefix(name, "env:"): + return os.Getenv(strings.TrimPrefix(name, "env:")) + } + return m + }) +} + +// vscodeSnippet renders the entry `armis-cli install` writes for VS Code, for +// users to paste into a config the CLI doesn't manage. +func vscodeSnippet(pluginDir string) string { + e := scannerEntry(pluginDir) + b, err := json.MarshalIndent(map[string]interface{}{ + e.name: map[string]interface{}{ + jsonKeyType: "stdio", + jsonKeyCommand: e.command, + jsonKeyArgs: e.args, + "envFile": e.envFile, + }, + }, "", " ") + if err != nil { + return "" + } + s := strings.TrimSpace(string(b)) + return strings.TrimSuffix(strings.TrimPrefix(s, "{"), "}") +} + +// launchArtifact records how an entry launches the server for the support +// bundle. Env values are omitted — only names are kept. +func launchArtifact(source string, l serverLaunch) string { + keys := make([]string, 0, len(l.Env)) + for k := range l.Env { + keys = append(keys, k) + } + sort.Strings(keys) + b, _ := json.MarshalIndent(map[string]interface{}{ + jsonKeySource: source, + jsonKeyCommand: l.Command, + jsonKeyArgs: l.Args, + "envFile": l.EnvFile, + "envKeys": keys, + }, "", " ") + return string(b) + "\n" +} + +var artifactNameRE = regexp.MustCompile(`[^A-Za-z0-9._-]+`) + +// sanitizeArtifactName makes s safe as a single zip path segment. +func sanitizeArtifactName(s string) string { + return strings.Trim(artifactNameRE.ReplaceAllString(s, "-"), "-") +} + +// systemInfo describes the machine for the support bundle. Proxy-related +// variables are included because they explain most network failures; any +// credentials embedded in a proxy URL are masked. +func systemInfo() string { + var b strings.Builder + fmt.Fprintf(&b, "os: %s/%s\n", runtime.GOOS, runtime.GOARCH) + fmt.Fprintf(&b, "time: %s\n", time.Now().UTC().Format(time.RFC3339)) + for _, k := range []string{"HTTPS_PROXY", "HTTP_PROXY", "NO_PROXY", "https_proxy", "http_proxy", "no_proxy", + "SSL_CERT_FILE", "REQUESTS_CA_BUNDLE", "APPSEC_ENV", "APPSEC_API_URL", "ARMIS_API_URL", "ARMIS_REGION"} { + if v := os.Getenv(k); v != "" { + fmt.Fprintf(&b, "%s=%s\n", k, maskURLUserinfo(v)) + } + } + for _, k := range []string{"ARMIS_CLIENT_ID", "ARMIS_CLIENT_SECRET", "ARMIS_API_TOKEN"} { + state := "not set" + if os.Getenv(k) != "" { + state = "set" + } + fmt.Fprintf(&b, "%s: %s (in this shell)\n", k, state) + } + return b.String() +} + +var urlUserinfoRE = regexp.MustCompile(`://[^/@\s]+@`) + +func maskURLUserinfo(s string) string { + return urlUserinfoRE.ReplaceAllString(s, "://***@") +} diff --git a/internal/install/editors.go b/internal/install/editors.go index 4c4890d..70efaab 100644 --- a/internal/install/editors.go +++ b/internal/install/editors.go @@ -28,6 +28,7 @@ const ( jsonKeyPath = "path" jsonKeyLastUpdated = "lastUpdated" jsonKeySource = "source" + jsonKeyName = "name" jsonTypeCommand = "command" ) @@ -57,6 +58,10 @@ const ( EditorCopilotCLI EditorID = "copilot" ) +// editorNameVSCode is VS Code's display name, shared by the editor list and +// the doctor's VS Code checks. +const editorNameVSCode = "VS Code" + // Editor represents a code editor with MCP server support. type Editor struct { ID EditorID @@ -65,7 +70,7 @@ type Editor struct { // AllEditors lists every editor that can be auto-configured. var AllEditors = []Editor{ - {EditorVSCode, "VS Code"}, + {EditorVSCode, editorNameVSCode}, {EditorCursor, "Cursor"}, {EditorWindsurf, "Windsurf"}, {EditorZed, "Zed"}, @@ -442,7 +447,7 @@ func registerContinueFormat(configFile string, entry mcpEntry) error { } server := map[string]interface{}{ - "name": entry.name, + jsonKeyName: entry.name, jsonKeyCommand: entry.command, } if len(entry.args) > 0 { @@ -517,8 +522,10 @@ func readJSONFileAsMap(path string) map[string]interface{} { } // armis:ignore cwe:22 cwe:253 reason:path from filepath.Join with known base dirs; filepath.Clean applied; ReadFile error handled by err == nil guard if b, err := os.ReadFile(clean); err == nil { //nolint:gosec + // VS Code's mcp.json is JSONC; strip comments/trailing commas/BOM so a + // hand-edited file keeps its other servers instead of parsing as empty. // armis:ignore cwe:502 cwe:770 reason:Go encoding/json into map[string]interface{} has no gadget/polymorphic deserialization; input is the user's own local editor config, size-bounded by the maxEditorConfigSize guard above - _ = json.Unmarshal(b, &data) + _ = json.Unmarshal(stripJSONC(b), &data) } return data } diff --git a/internal/install/jsonc.go b/internal/install/jsonc.go new file mode 100644 index 0000000..2c0988f --- /dev/null +++ b/internal/install/jsonc.go @@ -0,0 +1,91 @@ +package install + +import "bytes" + +// utf8BOM is the byte-order mark Windows Notepad (and some other editors) +// prepend when saving a file as UTF-8. encoding/json rejects it. +var utf8BOM = []byte{0xEF, 0xBB, 0xBF} + +// stripJSONC converts JSON-with-comments (the format VS Code uses for +// mcp.json and settings.json) into plain JSON: it removes a leading UTF-8 +// BOM, // line comments, /* block */ comments, and trailing commas before a +// closing } or ]. String contents are left untouched, including escaped +// quotes and comment-like sequences such as "http://...". +// +// Without this, a hand-edited mcp.json with a single comment fails to parse: +// the doctor misreports it as invalid, and a re-registration would start from +// an empty map and drop every other server in the file. +func stripJSONC(in []byte) []byte { + in = bytes.TrimPrefix(in, utf8BOM) + out := make([]byte, 0, len(in)) + + inString := false + for i := 0; i < len(in); i++ { + c := in[i] + if inString { + out = append(out, c) + switch c { + case '\\': + if i+1 < len(in) { + i++ + out = append(out, in[i]) + } + case '"': + inString = false + } + continue + } + + switch { + case c == '"': + inString = true + out = append(out, c) + case c == '/' && i+1 < len(in) && in[i+1] == '/': + for i < len(in) && in[i] != '\n' { + i++ + } + if i < len(in) { + out = append(out, '\n') + } + case c == '/' && i+1 < len(in) && in[i+1] == '*': + i += 2 + for i+1 < len(in) && (in[i] != '*' || in[i+1] != '/') { + i++ + } + i++ // skip the closing '/' + case c == ',': + // Drop a trailing comma: look past whitespace and comments for the + // next significant byte. + if next := nextSignificant(in, i+1); next == '}' || next == ']' { + continue + } + out = append(out, c) + default: + out = append(out, c) + } + } + return out +} + +// nextSignificant returns the first byte at or after start that is not +// whitespace or part of a comment, or 0 at end of input. +func nextSignificant(in []byte, start int) byte { + for i := start; i < len(in); i++ { + switch c := in[i]; { + case c == ' ' || c == '\t' || c == '\n' || c == '\r': + case c == '/' && i+1 < len(in) && in[i+1] == '/': + for i < len(in) && in[i] != '\n' { + i++ + } + case c == '/' && i+1 < len(in) && in[i+1] == '*': + i += 2 + for i+1 < len(in) && (in[i] != '*' || in[i+1] != '/') { + i++ + } + i++ + default: + return c + } + } + return 0 +} From 6fcd26152442df4c0c3473cf00d7dd7703d9287a Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 13:35:50 +0300 Subject: [PATCH 02/12] feat(mcp): add proxy auto-fix to doctor and preserve .env vars - `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. --- README.md | 38 ++++++ docs/CHANGELOG.md | 10 ++ internal/cmd/mcp_doctor.go | 34 +++-- internal/cmd/mcp_doctor_test.go | 176 +++++++++++++++++++++++++ internal/install/doctor.go | 89 +++++++++++-- internal/install/doctor_checks_test.go | 131 ++++++++++++++++++ internal/install/doctor_probe.go | 83 +++++++++++- internal/install/doctor_vscode.go | 2 +- internal/install/plugin.go | 49 ++++++- 9 files changed, 581 insertions(+), 31 deletions(-) create mode 100644 internal/cmd/mcp_doctor_test.go diff --git a/README.md b/README.md index b1a6cf4..fe813ee 100644 --- a/README.md +++ b/README.md @@ -679,6 +679,44 @@ armis-cli scan status --format json `scan status` reports every state the API can return: `PENDING_UPLOAD`, `UPLOADED`, `INITIATED`, `IN_PROGRESS`, `COMPLETED`, `FAILED`, `STOPPED`. +### MCP Server for AI Editors + +`armis-cli install` sets up the Armis AppSec MCP server and registers it with the AI editors it detects (VS Code / GitHub Copilot, Cursor, Claude Code, Windsurf, Codex CLI, and others). + +```bash +armis-cli install # install and register with detected editors +armis-cli mcp update # update the server and re-register editors +armis-cli mcp doctor # diagnose why an editor doesn't see or can't use the server +``` + +#### Troubleshooting with `mcp doctor` + +`mcp doctor` checks the whole path from install to a working tool call, and prints a `→` line with the fix under every check that fails: + +- plugin files, the Python venv (including a base Python that was uninstalled or upgraded), and credentials +- every editor config: the entry still exists, its command and envFile exist, and the server starts **exactly as that editor launches it** and answers `initialize`, `tools/list`, and a diagnostic tool call +- that the credentials are accepted, and that the server's own Python runtime can reach the Armis API (proxy and TLS-inspection problems often affect it and not the CLI) +- VS Code / Copilot: VS Code Insiders and VSCodium, per-profile and workspace configs, duplicate or stale entries, settings and Windows Group Policy that disable MCP or Agent mode, and the last error in VS Code's own MCP log for the server + +```bash +# Diagnose, then repair what can be repaired automatically: re-register +# editors, rebuild a broken venv, and configure the system proxy for the +# server when it works but the server isn't using it +armis-cli mcp doctor --fix + +# Write a zip for Armis support (report, server stderr and logs, VS Code MCP +# log, settings excerpts). Credential values are never included. +armis-cli mcp doctor --bundle +armis-cli mcp doctor --bundle-path C:\Users\me\Desktop\armis-doctor.zip + +# Structural checks only (don't start servers or make network calls) +armis-cli mcp doctor --no-handshake +``` + +Run it from your project folder to include that workspace's `.vscode/mcp.json` and `.vscode/settings.json`. If every check passes but Copilot still doesn't call the tools, the report ends with the remaining manual checks: Agent mode, the tools picker, **MCP: List Servers → Start Server**, and your GitHub organization's "MCP servers in Copilot" policy. + +The server reads extra environment variables from `~/.armis/plugins/armis-appsec-mcp/.env`. Behind a corporate proxy or TLS inspection, `HTTPS_PROXY` and `SSL_CERT_FILE` there apply to the server only; restart the editor after changing them. + ### Other Commands ```bash diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 45fbfa6..3937f8f 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,14 +9,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `mcp doctor` now runs a full MCP session (initialize, tools/list, a diagnostic tool call) using each editor's own config entry, checks credentials against the API, and checks that the server's Python runtime can reach the Armis API through the local proxy and TLS setup. Every failing check prints a remediation. +- `mcp doctor` VS Code / Copilot checks: VS Code Insiders and VSCodium, profile and workspace configs, duplicate and stale entries, `chat.mcp.*` / `chat.agent.enabled` settings, Windows Group Policy (ChatMCP, ChatAgentMode), and the last error from VS Code's MCP server log. +- `mcp doctor` Windows diagnostics: a venv whose base Python was removed, antivirus or AppLocker blocking the interpreter, slow starts, and output on stdout that corrupts the MCP stream. +- `mcp doctor --fix`: re-registers editors, rebuilds a broken venv, and writes the system proxy to the server's `.env` when the doctor has verified it works. +- `mcp doctor --bundle` / `--bundle-path`: writes a support zip with the report, server stderr and logs, and VS Code's MCP log. Credential values are excluded and scrubbed. + ### Changed +- Writing credentials to the MCP server's `.env` (interactive `install`) now keeps other variables in the file, such as `HTTPS_PROXY` and `SSL_CERT_FILE`. + ### Deprecated ### Removed ### Fixed +- `install` and `mcp doctor` parse VS Code's `mcp.json` / `settings.json` as JSONC. A file with comments, trailing commas, or a UTF-8 BOM was treated as empty, so `install` dropped the user's other MCP servers and `mcp doctor` reported the file as invalid. + ### Security --- diff --git a/internal/cmd/mcp_doctor.go b/internal/cmd/mcp_doctor.go index 2cc4553..8bf4065 100644 --- a/internal/cmd/mcp_doctor.go +++ b/internal/cmd/mcp_doctor.go @@ -46,7 +46,8 @@ Windows Group Policy that disable MCP or Agent mode, and VS Code's own MCP log for the server. Every failing check prints how to fix it. --fix repairs what the CLI can -(stale or missing registrations, a broken venv) and re-runs the checks. +(stale or missing registrations, a broken venv, a system proxy the server +isn't using) and re-runs the checks. --bundle writes a zip with the full diagnostics, credentials removed, to send to support. @@ -165,23 +166,32 @@ func applyDoctorFixes(out io.Writer, report *install.DoctorReport) (bool, error) return false, nil } - force := false for _, f := range fixes { - if f == install.FixReinstall { - force = true + switch f { + case install.FixSetProxy: + desc, err := report.ApplyEnvFix() + if err != nil { + return true, fmt.Errorf("repair failed: %w", err) + } + _, _ = fmt.Fprintf(out, "\nConfigured the MCP server's proxy: %s\nRestart VS Code (or your editor) so the server picks it up.\n", desc) + case install.FixReinstall, install.FixReregister: + force := f == install.FixReinstall + if force { + _, _ = fmt.Fprintln(out, "\nReinstalling the MCP server and re-registering editors...") + } else { + _, _ = fmt.Fprintln(out, "\nRe-registering editors...") + } + if err := mcpDoctorUpdate(force, false); err != nil { + return true, fmt.Errorf("repair failed: %w", err) + } } } - if force { - _, _ = fmt.Fprintln(out, "\nReinstalling the MCP server and re-registering editors...") - } else { - _, _ = fmt.Fprintln(out, "\nRe-registering editors...") - } - if err := performMCPUpdate(force, false); err != nil { - return true, fmt.Errorf("repair failed: %w", err) - } return true, nil } +// mcpDoctorUpdate is performMCPUpdate, swappable in tests. +var mcpDoctorUpdate = performMCPUpdate + func printMCPDoctorJSON(cmd *cobra.Command, report *install.DoctorReport) error { enc := json.NewEncoder(cmd.OutOrStdout()) enc.SetIndent("", " ") diff --git a/internal/cmd/mcp_doctor_test.go b/internal/cmd/mcp_doctor_test.go new file mode 100644 index 0000000..0730ca9 --- /dev/null +++ b/internal/cmd/mcp_doctor_test.go @@ -0,0 +1,176 @@ +package cmd + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/ArmisSecurity/armis-cli/internal/install" +) + +func TestPrintMCPDoctorPlain(t *testing.T) { + report := &install.DoctorReport{Checks: []install.DoctorCheck{ + {Component: "scanner", Name: "python venv", Status: install.StatusOK, Detail: "/venv/python", Remediation: "not shown for ok"}, + {Component: "scanner", Name: "VS Code live handshake", Status: install.StatusFail, Detail: "exited", + Remediation: "line one\nline two", Fix: install.FixReinstall}, + {Component: "vscode", Name: "Copilot", Status: install.StatusInfo, Detail: "manual"}, + }} + + var out bytes.Buffer + printMCPDoctorPlain(&out, report, true) + got := out.String() + + for _, want := range []string{ + "scanner:\n", + "vscode:\n", + " → line one\n line two\n", + "1 passed, 0 warnings, 1 failed", + "armis-cli mcp doctor --fix", + "armis-cli mcp doctor --bundle", + } { + if !strings.Contains(got, want) { + t.Errorf("output missing %q:\n%s", want, got) + } + } + if strings.Contains(got, "not shown for ok") { + t.Error("remediation printed for a passing check") + } + // Names longer than the default column still line up. + if !strings.Contains(got, "python venv /venv/python") { + t.Errorf("columns not aligned to the longest name:\n%s", got) + } + + out.Reset() + printMCPDoctorPlain(&out, report, false) + if strings.Contains(out.String(), "--fix") { + t.Error("--fix suggested after --fix already ran") + } +} + +func stubDoctorUpdate(t *testing.T) *[]bool { + t.Helper() + var calls []bool + orig := mcpDoctorUpdate + mcpDoctorUpdate = func(force, _ bool) error { + calls = append(calls, force) + return nil + } + t.Cleanup(func() { mcpDoctorUpdate = orig }) + return &calls +} + +func TestApplyDoctorFixes(t *testing.T) { + tests := []struct { + name string + checks []install.DoctorCheck + wantFixed bool + wantCalls []bool // force flag per update call + wantOut string + }{ + { + name: "nothing installed", + checks: []install.DoctorCheck{{Component: install.ComponentInstall, Name: "manifest", Status: install.StatusFail}}, + wantOut: "armis-cli install", + wantCalls: nil, + }, + { + name: "not fixable", + checks: []install.DoctorCheck{{Component: "vscode", Name: "settings", Status: install.StatusFail}}, + wantOut: "can be fixed automatically", + }, + { + name: "reregister", + checks: []install.DoctorCheck{{Component: "scanner", Name: "Cursor", Status: install.StatusWarn, Fix: install.FixReregister}}, + wantFixed: true, + wantCalls: []bool{false}, + wantOut: "Re-registering", + }, + { + name: "reinstall subsumes reregister", + checks: []install.DoctorCheck{ + {Component: "scanner", Name: "Cursor", Status: install.StatusWarn, Fix: install.FixReregister}, + {Component: "scanner", Name: "python venv", Status: install.StatusFail, Fix: install.FixReinstall}, + }, + wantFixed: true, + wantCalls: []bool{true}, + wantOut: "Reinstalling", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := stubDoctorUpdate(t) + var out bytes.Buffer + fixed, err := applyDoctorFixes(&out, &install.DoctorReport{Checks: tt.checks}) + if err != nil { + t.Fatalf("applyDoctorFixes() error = %v", err) + } + if fixed != tt.wantFixed { + t.Errorf("fixed = %v, want %v", fixed, tt.wantFixed) + } + if len(*calls) != len(tt.wantCalls) || (len(tt.wantCalls) > 0 && (*calls)[0] != tt.wantCalls[0]) { + t.Errorf("update calls = %v, want %v", *calls, tt.wantCalls) + } + if !strings.Contains(out.String(), tt.wantOut) { + t.Errorf("output = %q, want it to mention %q", out.String(), tt.wantOut) + } + }) + } +} + +func TestApplyDoctorFixesReportsUpdateError(t *testing.T) { + orig := mcpDoctorUpdate + mcpDoctorUpdate = func(bool, bool) error { return errors.New("download failed") } + t.Cleanup(func() { mcpDoctorUpdate = orig }) + + report := &install.DoctorReport{Checks: []install.DoctorCheck{{Component: "scanner", Name: "x", Status: install.StatusFail, Fix: install.FixReinstall}}} + if _, err := applyDoctorFixes(&bytes.Buffer{}, report); err == nil || !strings.Contains(err.Error(), "download failed") { + t.Errorf("applyDoctorFixes() error = %v, want the update error", err) + } +} + +// TestRunMCPDoctorWritesBundle runs the command end to end with nothing +// installed: it must still fail (no manifest) but write the bundle to +// --bundle-path. +func TestRunMCPDoctorWritesBundle(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("APPDATA", filepath.Join(home, "AppData")) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + + bundle := filepath.Join(t.TempDir(), "support.zip") + defer func(f, p string, h, fix, b bool) { + mcpDoctorFormat, mcpDoctorBundlePath, mcpDoctorNoHandshake, mcpDoctorFix, mcpDoctorBundle = f, p, h, fix, b + }(mcpDoctorFormat, mcpDoctorBundlePath, mcpDoctorNoHandshake, mcpDoctorFix, mcpDoctorBundle) + mcpDoctorFormat, mcpDoctorBundlePath, mcpDoctorNoHandshake, mcpDoctorFix, mcpDoctorBundle = agentFormatPlain, bundle, true, false, false + + var stderr bytes.Buffer + mcpDoctorCmd.SetErr(&stderr) + t.Cleanup(func() { mcpDoctorCmd.SetErr(nil) }) + + if err := runMCPDoctor(mcpDoctorCmd, nil); err == nil { + t.Error("runMCPDoctor() error = nil, want failure when nothing is installed") + } + if _, err := os.Stat(bundle); err != nil { + t.Fatalf("bundle not written to --bundle-path: %v\n%s", err, stderr.String()) + } + if !strings.Contains(stderr.String(), "Support bundle written to "+bundle) { + t.Errorf("stderr = %q", stderr.String()) + } +} + +func TestMCPDoctorFlags(t *testing.T) { + for name, def := range map[string]string{"fix": "false", "bundle": "false", "bundle-path": "", "no-handshake": "false"} { + f := mcpDoctorCmd.Flags().Lookup(name) + if f == nil { + t.Errorf("mcp doctor is missing --%s", name) + continue + } + if f.DefValue != def { + t.Errorf("--%s default = %q, want %q", name, f.DefValue, def) + } + } +} diff --git a/internal/install/doctor.go b/internal/install/doctor.go index d57f0c5..9ec7554 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -65,6 +65,9 @@ const ( // FixReinstall re-downloads the plugin and rebuilds its venv, then // re-registers every editor. FixReinstall FixAction = "reinstall" + // FixSetProxy writes a proxy the doctor verified works into the plugin's + // .env, so the server uses it on its next start. + FixSetProxy FixAction = "set-proxy" ) // DoctorCheck is one diagnostic result reported by RunDoctor. @@ -97,6 +100,8 @@ type DoctorReport struct { // excerpts, log tails) for the support bundle. Keyed by bundle file name. // Kept out of the JSON report, which stays a concise list of checks. Artifacts map[string]string `json:"-"` + // envFix holds the .env update FixSetProxy applies. + envFix *envFix // secrets are credential values seen during the run, scrubbed verbatim // from everything written to the support bundle. secrets []string @@ -137,9 +142,11 @@ func (r *DoctorReport) HasProblems() bool { } // Fixes returns the distinct repairs `--fix` can apply for failing or warning -// checks. FixReinstall subsumes FixReregister, so only one is returned. +// checks. FixReinstall subsumes FixReregister, so at most one of the two is +// returned; FixSetProxy is independent and comes first. func (r *DoctorReport) Fixes() []FixAction { - var reregister, reinstall bool + var out []FixAction + var reregister, reinstall, setProxy bool for _, c := range r.Checks { if c.Status != StatusFail && c.Status != StatusWarn { continue @@ -149,15 +156,43 @@ func (r *DoctorReport) Fixes() []FixAction { reinstall = true case FixReregister: reregister = true + case FixSetProxy: + setProxy = r.envFix != nil } } + if setProxy { + out = append(out, FixSetProxy) + } switch { case reinstall: - return []FixAction{FixReinstall} + out = append(out, FixReinstall) case reregister: - return []FixAction{FixReregister} + out = append(out, FixReregister) } - return nil + return out +} + +// envFix is a verified set of variables to add to a .env file. +type envFix struct { + EnvFile string + Vars [][2]string +} + +// ApplyEnvFix performs FixSetProxy: it writes the verified variables into the +// plugin's .env, keeping everything else in the file. It returns a +// description of the change with credentials masked. +func (r *DoctorReport) ApplyEnvFix() (string, error) { + if r.envFix == nil { + return "", nil + } + if err := SetEnvFileVars(r.envFix.EnvFile, r.envFix.Vars); err != nil { + return "", fmt.Errorf("updating %s: %w", r.envFix.EnvFile, err) + } + parts := make([]string, 0, len(r.envFix.Vars)) + for _, kv := range r.envFix.Vars { + parts = append(parts, kv[0]+"="+maskURLUserinfo(kv[1])) + } + return fmt.Sprintf("set %s in %s", strings.Join(parts, ", "), r.envFix.EnvFile), nil } // DoctorOptions configures RunDoctor. @@ -284,6 +319,12 @@ func checkScannerPlugin(d *doctorRun, ei *EditorInstaller) { env := checkCredentials(report, component, ei.EnvFilePath()) + // Plugin versions that keep their own log write it here; include the + // recent part in the support bundle. + if b, err := readBoundedConfigFile(filepath.Join(ei.PluginDir(), "logs", "server.log")); err == nil && len(b) > 0 { + report.artifact("scanner/server.log", tail(string(b), 64<<10)+"\n") + } + if d.opts.Handshake { // The canonical check launches with the .env merged in, which is what // VS Code does via envFile; editors without envFile rely on the server @@ -458,16 +499,48 @@ func checkServerNetwork(d *doctorRun, component, python string, env map[string]s } } url := serverAPIURL(env) - out, err := runNetworkProbe(python, env, url, networkProbeTimeout) + out, err := networkProbe(python, env, url, networkProbeTimeout) d.report.artifact(sanitizeArtifactName(component)+"/network-probe.txt", fmt.Sprintf("GET %s\n%s\n", url, out)) if err != nil { - d.report.add(component, "server network", StatusFail, fmt.Sprintf("%s: %s", url, truncate(err.Error(), 300))). - hint(networkHint(err.Error(), envFile)) + c := d.report.add(component, "server network", StatusFail, fmt.Sprintf("%s: %s", url, truncate(err.Error(), 300))) + c.hint(networkHint(err.Error(), envFile)) + if isConnectFailure(err.Error()) && !hasProxyEnv(env) { + tryProxyFix(d, c, python, env, envFile, url) + } return } d.report.add(component, "server network", StatusOK, fmt.Sprintf("%s reachable from the server's Python runtime (%s)", url, out)) } +// tryProxyFix handles the most common corporate-network failure: Python +// ignores the OS proxy settings the browser and CLI use. If the OS has a +// proxy configured and the probe succeeds through it, the check becomes +// auto-fixable by writing that proxy to .env. Nothing is written here. +func tryProxyFix(d *doctorRun, c *DoctorCheck, python string, env map[string]string, envFile, url string) { + proxy := systemProxyLookup(python) + if proxy == "" { + return + } + withProxy := make(map[string]string, len(env)+1) + for k, v := range env { + withProxy[k] = v + } + withProxy[envHTTPSProxy] = proxy + out, err := networkProbe(python, withProxy, url, networkProbeTimeout) + masked := maskURLUserinfo(proxy) + if masked != proxy { + d.report.secrets = append(d.report.secrets, proxy) + } + d.report.artifact("scanner/network-probe-system-proxy.txt", fmt.Sprintf("GET %s via %s\n%s\n", url, masked, out)) + if err != nil { + c.hint(c.Remediation + "\nThe system proxy " + masked + " was tried and also failed: " + truncate(err.Error(), 200)) + return + } + d.report.envFix = &envFix{EnvFile: envFile, Vars: [][2]string{{envHTTPSProxy, proxy}}} + c.fix(FixSetProxy, "Your system proxy "+masked+" works but the MCP server doesn't use it. "+ + "armis-cli mcp doctor --fix adds HTTPS_PROXY="+masked+" to "+envFile+"; then restart VS Code.") +} + // probe runs a live MCP session for launch and reports the handshake, tool // listing, and diagnostic tool call as checks. label prefixes the check names // ("" for the plugin's own launch, the editor name for an editor's entry). A diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index 1859b41..922dfb0 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -520,3 +520,134 @@ func TestMaskURLUserinfo(t *testing.T) { t.Errorf("maskURLUserinfo() = %q", got) } } + +func TestSetEnvFileVars(t *testing.T) { + path := filepath.Join(t.TempDir(), ".env") + mustWrite(t, path, "\xEF\xBB\xBF# creds\r\nARMIS_CLIENT_ID=old\r\nSSL_CERT_FILE=/ca.pem\r\n") + + if err := SetEnvFileVars(path, [][2]string{{"ARMIS_CLIENT_ID", "new"}, {"HTTPS_PROXY", "http://p:8080"}}); err != nil { + t.Fatalf("SetEnvFileVars() error = %v", err) + } + b, _ := os.ReadFile(path) //nolint:gosec // test temp dir + want := "# creds\nARMIS_CLIENT_ID=new\nSSL_CERT_FILE=/ca.pem\nHTTPS_PROXY=http://p:8080\n" + if string(b) != want { + t.Errorf("content = %q, want %q", b, want) + } + if _, err := os.Stat(path + ".bak"); err != nil { + t.Errorf("no backup written: %v", err) + } + if err := SetEnvFileVars(path, [][2]string{{"X", "a\nB=c"}}); err == nil { + t.Error("SetEnvFileVars() accepted a value with a newline") + } +} + +// TestWriteEnvFromValuesKeepsOtherVars pins that re-entering credentials +// doesn't drop a proxy or CA setting the doctor (or the user) added. +func TestWriteEnvFromValuesKeepsOtherVars(t *testing.T) { + path := filepath.Join(t.TempDir(), ".env") + mustWrite(t, path, "ARMIS_CLIENT_ID=a\nARMIS_CLIENT_SECRET=b\nHTTPS_PROXY=http://p:8080\n") + if err := WriteEnvFromValues(path, "c", "d"); err != nil { + t.Fatal(err) + } + env, _ := parseEnvFile(path) + if env["ARMIS_CLIENT_ID"] != "c" || env["ARMIS_CLIENT_SECRET"] != "d" || env["HTTPS_PROXY"] != "http://p:8080" { + t.Errorf("env = %v", env) + } +} + +// stubNetwork replaces the network probe and system proxy lookup. probe +// receives the env the probe would run with. +func stubNetwork(t *testing.T, proxy string, probe func(env map[string]string) (string, error)) { + t.Helper() + origProbe, origLookup := networkProbe, systemProxyLookup + networkProbe = func(_ string, env map[string]string, _ string, _ time.Duration) (string, error) { return probe(env) } + systemProxyLookup = func(string) string { return proxy } + t.Cleanup(func() { networkProbe, systemProxyLookup = origProbe, origLookup }) +} + +func TestCheckServerNetworkProxyFix(t *testing.T) { + for _, k := range []string{"HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy", "SSL_CERT_FILE"} { + t.Setenv(k, "") + } + connectErr := errors.New("ERR ConnectError [Errno 11001] getaddrinfo failed (CA: certifi)") + viaProxy := func(env map[string]string) (string, error) { + if env["HTTPS_PROXY"] != "" { + return "HTTP 401 (CA: certifi)", nil + } + return connectErr.Error(), connectErr + } + + t.Run("system proxy works", func(t *testing.T) { + stubNetwork(t, "http://user:pw@proxy.corp:8080", viaProxy) + envFile := filepath.Join(t.TempDir(), ".env") + mustWrite(t, envFile, "ARMIS_CLIENT_ID=id\n") + + d := newDoctorRun(DoctorOptions{}) + checkServerNetwork(d, "scanner", "python", map[string]string{}, envFile) + c := wantStatus(t, checkMap(d.report), "scanner/server network", StatusFail) + if c.Fix != FixSetProxy || strings.Contains(c.Remediation, "pw") { + t.Errorf("check = %+v, want FixSetProxy with the password masked", c) + } + if f := d.report.Fixes(); len(f) != 1 || f[0] != FixSetProxy { + t.Fatalf("Fixes() = %v", f) + } + desc, err := d.report.ApplyEnvFix() + if err != nil || strings.Contains(desc, "pw") { + t.Fatalf("ApplyEnvFix() = %q, %v", desc, err) + } + env, _ := parseEnvFile(envFile) + if env["HTTPS_PROXY"] != "http://user:pw@proxy.corp:8080" || env["ARMIS_CLIENT_ID"] != "id" { // #nosec G101 -- test fixture + t.Errorf(".env after fix = %v", env) + } + }) + + t.Run("system proxy also fails", func(t *testing.T) { + stubNetwork(t, "http://proxy.corp:8080", func(map[string]string) (string, error) { return connectErr.Error(), connectErr }) + d := newDoctorRun(DoctorOptions{}) + checkServerNetwork(d, "scanner", "python", map[string]string{}, "/p/.env") + c := wantStatus(t, checkMap(d.report), "scanner/server network", StatusFail) + if c.Fix != FixNone || !strings.Contains(c.Remediation, "also failed") { + t.Errorf("check = %+v", c) + } + }) + + t.Run("proxy already configured", func(t *testing.T) { + stubNetwork(t, "http://proxy.corp:8080", viaProxy) + d := newDoctorRun(DoctorOptions{}) + checkServerNetwork(d, "scanner", "python", map[string]string{"HTTPS_PROXY": "http://other:1"}, "/p/.env") + if c := checkMap(d.report)["scanner/server network"]; c.Fix != FixNone { + t.Errorf("proxy fix offered although HTTPS_PROXY is set: %+v", c) + } + }) + + t.Run("reachable", func(t *testing.T) { + stubNetwork(t, "", func(map[string]string) (string, error) { return "HTTP 401 (CA: system store)", nil }) + d := newDoctorRun(DoctorOptions{}) + checkServerNetwork(d, "scanner", "python", map[string]string{}, "/p/.env") + wantStatus(t, checkMap(d.report), "scanner/server network", StatusOK) + }) +} + +func TestNetworkHintTLSByCASource(t *testing.T) { + old := networkHint("ERR ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] (CA: certifi)", "/p/.env") + if !strings.Contains(old, "armis-cli mcp update") { + t.Errorf("certifi hint = %q, want update suggestion", old) + } + sys := networkHint("ERR ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] (CA: system store)", "/p/.env") + if strings.Contains(sys, "mcp update") || !strings.Contains(sys, "system certificate store") { + t.Errorf("system-store hint = %q", sys) + } +} + +func TestIsConnectFailure(t *testing.T) { + for out, want := range map[string]bool{ + "ERR ConnectError [Errno 11001] getaddrinfo failed": true, + "ERR ConnectTimeout timed out": true, + "ERR ConnectError [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed": false, + "ERR ProxyError 407": false, + } { + if got := isConnectFailure(out); got != want { + t.Errorf("isConnectFailure(%q) = %v, want %v", out, got, want) + } + } +} diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index 6e78978..f443dce 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -396,16 +396,86 @@ func launchHint(err error, stderr string, launch serverLaunch) (string, FixActio // the MCP server uses. The CLI's own Go HTTP stack uses the OS certificate // store and PAC proxy settings, so a Go-side check can pass while the server // fails — this probe measures what the server will actually experience. -const networkProbeScript = `import sys +// +// Newer plugin versions trust the OS certificate store via truststore unless +// SSL_CERT_FILE is set; the probe does the same when truststore is installed +// in the venv, and reports which CA source it used. +const networkProbeScript = `import os, sys +ca = "certifi" +if os.environ.get("SSL_CERT_FILE"): + ca = "SSL_CERT_FILE" +else: + try: + import truststore + truststore.inject_into_ssl() + ca = "system store" + except Exception: + pass try: import httpx r = httpx.get(sys.argv[1], timeout=15) - print("HTTP", r.status_code) + print("HTTP", r.status_code, "(CA: " + ca + ")") except Exception as e: - print("ERR", type(e).__name__, str(e)[:500]) + print("ERR", type(e).__name__, str(e)[:500], "(CA: " + ca + ")") sys.exit(1) ` +// systemProxyScript prints the HTTPS proxy the OS is configured with +// (Windows registry / macOS System Settings), as Python's urllib sees it. +// PAC-only configurations aren't visible this way. +const systemProxyScript = `import urllib.request +p = urllib.request.getproxies() +print(p.get("https") or p.get("http") or "") +` + +// networkProbe and systemProxyLookup are vars so tests can stub them. +var ( + networkProbe = runNetworkProbe + systemProxyLookup = lookupSystemProxy +) + +// lookupSystemProxy returns the OS-configured proxy URL, or "" if none. +func lookupSystemProxy(python string) string { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // armis:ignore cwe:78 cwe:88 reason:python is the CLI's own recorded venv interpreter; the script is a constant + out, err := exec.CommandContext(ctx, python, "-c", systemProxyScript).Output() //nolint:gosec // constant script, venv interpreter + if err != nil { + return "" + } + p := strings.TrimSpace(string(out)) + if p != "" && !strings.Contains(p, "://") { + p = "http://" + p + } + return p +} + +// envHTTPSProxy is the proxy variable the server's HTTP client reads. +const envHTTPSProxy = "HTTPS_PROXY" + +// hasProxyEnv reports whether a proxy is already configured for the server, +// either in its env file or the inherited environment. +func hasProxyEnv(env map[string]string) bool { + for _, k := range []string{envHTTPSProxy, "https_proxy", "HTTP_PROXY", "http_proxy", "ALL_PROXY", "all_proxy"} { + if env[k] != "" || os.Getenv(k) != "" { + return true + } + } + return false +} + +// isConnectFailure reports whether probe output is a connection-level +// failure (as opposed to TLS or HTTP errors) that a proxy could explain. +func isConnectFailure(output string) bool { + o := strings.ToLower(output) + for _, s := range []string{"connecterror", "connecttimeout", "timed out", "getaddrinfo", "name or service not known", "nodename", "network is unreachable", "connection refused"} { + if strings.Contains(o, s) && !strings.Contains(o, "certificate") { + return true + } + } + return false +} + const ( appsecProdURL = "https://moose.armis.com/api/v1" appsecDevURL = "https://moose-dev.armis.com/api/v1" @@ -468,8 +538,11 @@ func networkHint(output, envFile string) string { switch { case strings.Contains(o, "certificate_verify_failed") || strings.Contains(o, "certificate verify failed") || strings.Contains(o, "self-signed") || strings.Contains(o, "unable to get local issuer"): - return "Your network re-signs HTTPS traffic (TLS inspection, e.g. Zscaler or Netskope) and the MCP server's Python runtime doesn't trust that certificate — it uses its own CA bundle, not the Windows certificate store. " + - "Export your organization's root CA as a PEM (Base-64 .cer) file, then add SSL_CERT_FILE= to " + envFile + " and restart VS Code." + if strings.Contains(o, "(ca: certifi)") { + return "Your network re-signs HTTPS traffic (TLS inspection, e.g. Zscaler or Netskope) and this version of the MCP server doesn't trust that certificate — it uses its own CA bundle, not the Windows certificate store. " + + "Update the server (armis-cli mcp update) to a version that uses the system certificate store, or export your organization's root CA as a PEM (Base-64 .cer) file and add SSL_CERT_FILE= to " + envFile + ", then restart VS Code." + } + return "The server's Python runtime doesn't trust the certificate the Armis API presented, even with the system certificate store. If your network uses TLS inspection, ask IT to install the inspection root CA in the system certificate store, or export it as a PEM file and add SSL_CERT_FILE= to " + envFile + ", then restart VS Code." case strings.Contains(o, "proxyerror") || strings.Contains(o, "407"): return "The proxy rejected the request. Check the HTTPS_PROXY value in " + envFile + " (including credentials if your proxy requires them)." case strings.Contains(o, "connecterror") || strings.Contains(o, "connecttimeout") || strings.Contains(o, "timed out") || diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index 9a98a48..018c010 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -611,7 +611,7 @@ func systemInfo() string { var b strings.Builder fmt.Fprintf(&b, "os: %s/%s\n", runtime.GOOS, runtime.GOARCH) fmt.Fprintf(&b, "time: %s\n", time.Now().UTC().Format(time.RFC3339)) - for _, k := range []string{"HTTPS_PROXY", "HTTP_PROXY", "NO_PROXY", "https_proxy", "http_proxy", "no_proxy", + for _, k := range []string{envHTTPSProxy, "HTTP_PROXY", "NO_PROXY", "https_proxy", "http_proxy", "no_proxy", "SSL_CERT_FILE", "REQUESTS_CA_BUNDLE", "APPSEC_ENV", "APPSEC_API_URL", "ARMIS_API_URL", "ARMIS_REGION"} { if v := os.Getenv(k); v != "" { fmt.Fprintf(&b, "%s=%s\n", k, maskURLUserinfo(v)) diff --git a/internal/install/plugin.go b/internal/install/plugin.go index 914bcab..0fb518b 100644 --- a/internal/install/plugin.go +++ b/internal/install/plugin.go @@ -3,6 +3,7 @@ package install import ( "archive/tar" + "bytes" "compress/gzip" "encoding/json" "fmt" @@ -542,16 +543,32 @@ func writeEnvFromEnvironment(envPath string) error { } // WriteEnvFromValues writes client credentials to a .env file at envPath. -// If the file already exists, it is backed up to .env.bak before overwriting. -// The write is atomic (temp file + rename) to prevent corruption on interrupt. +// Other variables already in the file (HTTPS_PROXY, SSL_CERT_FILE, ...) are +// kept. If the file already exists, it is backed up to .env.bak first. The +// write is atomic (temp file + rename) to prevent corruption on interrupt. // armis:ignore cwe:73 reason:envPath derived from known plugin dir + ".env"; callers are internal install functions func WriteEnvFromValues(envPath, clientID, clientSecret string) error { - if strings.ContainsAny(clientID, "\n\r") || strings.ContainsAny(clientSecret, "\n\r") { - return fmt.Errorf("credentials must not contain newline characters") + return SetEnvFileVars(envPath, [][2]string{ + {"ARMIS_CLIENT_ID", clientID}, + {"ARMIS_CLIENT_SECRET", clientSecret}, + }) +} + +// SetEnvFileVars sets vars (in order) in the .env file at envPath, replacing +// existing assignments of the same keys in place and appending new ones. +// Comments, blank lines, and other variables are preserved; a UTF-8 BOM is +// dropped. An existing file is backed up to .env.bak, and the write is atomic. +// armis:ignore cwe:73 reason:envPath derived from known plugin dir + ".env"; callers are internal install/doctor functions +func SetEnvFileVars(envPath string, vars [][2]string) error { + for _, kv := range vars { + if kv[0] == "" || strings.ContainsAny(kv[0], "=\n\r") || strings.ContainsAny(kv[1], "\n\r") { + return fmt.Errorf("invalid env entry for %q: keys and values must not contain newlines", kv[0]) + } } cleanPath := filepath.Clean(envPath) + var lines []string // Back up existing file via copy (not rename) so the original remains if a later step fails if _, err := os.Stat(cleanPath); err == nil { bakPath := cleanPath + ".bak" @@ -559,16 +576,38 @@ func WriteEnvFromValues(envPath, clientID, clientSecret string) error { if err := copyFile(cleanPath, bakPath); err != nil { return fmt.Errorf("could not back up %s: %w", filepath.Base(cleanPath), err) } + existing, err := readBoundedConfigFile(cleanPath) + if err != nil { + return fmt.Errorf("reading existing env file: %w", err) + } + text := strings.TrimRight(string(bytes.TrimPrefix(existing, utf8BOM)), "\r\n") + if text != "" { + lines = strings.Split(strings.ReplaceAll(text, "\r\n", "\n"), "\n") + } } else if !os.IsNotExist(err) { return fmt.Errorf("checking existing env file: %w", err) } + for _, kv := range vars { + replaced := false + for i, line := range lines { + k, _, ok := strings.Cut(strings.TrimSpace(line), "=") + if ok && !strings.HasPrefix(strings.TrimSpace(line), "#") && strings.TrimSpace(k) == kv[0] { + lines[i] = kv[0] + "=" + kv[1] + replaced = true + } + } + if !replaced { + lines = append(lines, kv[0]+"="+kv[1]) + } + } + if err := os.MkdirAll(filepath.Dir(cleanPath), 0o750); err != nil { return fmt.Errorf("creating env directory: %w", err) } // armis:ignore cwe:522 reason:CLI writes credentials to .env file with 0600 permissions for local auth config - content := fmt.Sprintf("ARMIS_CLIENT_ID=%s\nARMIS_CLIENT_SECRET=%s\n", clientID, clientSecret) + content := strings.Join(lines, "\n") + "\n" // Atomic write: randomized temp file + rename // armis:ignore cwe:73 reason:temp file created in same directory as target, derived from known plugin dir From 2f9de6af4f6d0c02e3c6430b153fb2ef68cbbf82 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 15:37:54 +0300 Subject: [PATCH 03/12] fix(mcp): honor doctor timeout in auth check, fix probe timeout message 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. --- internal/auth/auth.go | 10 ++++++++-- internal/cmd/mcp_doctor.go | 4 ++-- internal/install/doctor_probe.go | 5 +++-- internal/install/plugin.go | 6 ++++-- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0bcaaf3..62a2f8c 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -144,6 +144,13 @@ func (p *AuthProvider) Expiry() time.Time { // If ClientID and ClientSecret are set, uses JWT auth with the specified base URL. // Otherwise falls back to legacy Basic auth with Token. func NewAuthProvider(config AuthConfig) (*AuthProvider, error) { + return NewAuthProviderWithContext(context.Background(), config) +} + +// NewAuthProviderWithContext creates an AuthProvider from configuration, +// using ctx for the initial JWT token exchange so callers can bound or +// cancel it. Otherwise behaves like NewAuthProvider. +func NewAuthProviderWithContext(ctx context.Context, config AuthConfig) (*AuthProvider, error) { p := &AuthProvider{ config: config, } @@ -169,8 +176,7 @@ func NewAuthProvider(config AuthConfig) (*AuthProvider, error) { } p.authClient = authClient - // Initial token exchange (use background context for initialization) - if err := p.exchangeCredentials(context.Background()); err != nil { + if err := p.exchangeCredentials(ctx); err != nil { return nil, fmt.Errorf("failed to authenticate: %w", err) } } else if config.Token != "" { diff --git a/internal/cmd/mcp_doctor.go b/internal/cmd/mcp_doctor.go index 8bf4065..19f6f90 100644 --- a/internal/cmd/mcp_doctor.go +++ b/internal/cmd/mcp_doctor.go @@ -138,8 +138,8 @@ func runMCPDoctor(cmd *cobra.Command, _ []string) error { // doctorAuthCheck exchanges client credentials for a token against the same // API base URL the rest of the CLI uses. -func doctorAuthCheck(_ context.Context, id, secret string) error { - _, err := auth.NewAuthProvider(auth.AuthConfig{ +func doctorAuthCheck(ctx context.Context, id, secret string) error { + _, err := auth.NewAuthProviderWithContext(ctx, auth.AuthConfig{ ClientID: id, ClientSecret: secret, BaseURL: getAPIBaseURL(), diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index f443dce..fbe3b32 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -266,12 +266,13 @@ func (s *mcpSession) call(id int, method string, params interface{}, timeout tim return nil, fmt.Errorf("writing %s request: %w", method, err) } - timer := time.NewTimer(time.Until(s.deadline)) + wait := time.Until(s.deadline) + timer := time.NewTimer(wait) defer timer.Stop() for { select { case <-timer.C: - return nil, fmt.Errorf("timed out waiting for %s response after %s", method, timeout) + return nil, fmt.Errorf("timed out waiting for %s response after %s", method, wait.Round(time.Millisecond)) case err := <-s.readErr: s.readErr <- err // keep it for any later call if errors.Is(err, bufio.ErrTooLong) { diff --git a/internal/install/plugin.go b/internal/install/plugin.go index 0fb518b..6b15206 100644 --- a/internal/install/plugin.go +++ b/internal/install/plugin.go @@ -556,8 +556,10 @@ func WriteEnvFromValues(envPath, clientID, clientSecret string) error { // SetEnvFileVars sets vars (in order) in the .env file at envPath, replacing // existing assignments of the same keys in place and appending new ones. -// Comments, blank lines, and other variables are preserved; a UTF-8 BOM is -// dropped. An existing file is backed up to .env.bak, and the write is atomic. +// Full-line comments, blank lines, and other variables are preserved; a +// UTF-8 BOM is dropped. An inline comment on a replaced assignment's line is +// not preserved, since the new value replaces the whole line. An existing +// file is backed up to .env.bak, and the write is atomic. // armis:ignore cwe:73 reason:envPath derived from known plugin dir + ".env"; callers are internal install/doctor functions func SetEnvFileVars(envPath string, vars [][2]string) error { for _, kv := range vars { From 5b986479515b61a21838f05dc36b8053703fb0d2 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 15:51:34 +0300 Subject: [PATCH 04/12] fix(mcp): block editor re-registration when a config can't be parsed, report unparsable workspace configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/cmd/mcp_doctor.go | 3 +++ internal/install/doctor.go | 34 +++++++++++++++++++----- internal/install/doctor_test.go | 43 +++++++++++++++++++++++++++++++ internal/install/doctor_vscode.go | 9 ++++++- 4 files changed, 82 insertions(+), 7 deletions(-) diff --git a/internal/cmd/mcp_doctor.go b/internal/cmd/mcp_doctor.go index 19f6f90..60c7ba5 100644 --- a/internal/cmd/mcp_doctor.go +++ b/internal/cmd/mcp_doctor.go @@ -159,6 +159,9 @@ func applyDoctorFixes(out io.Writer, report *install.DoctorReport) (bool, error) } fixes := report.Fixes() + if report.HasBlockedRegistration() { + _, _ = fmt.Fprintln(out, "\nSkipping editor re-registration: at least one editor's config file couldn't be parsed, and rewriting it would drop the other servers configured there. Fix the syntax (see the hint above), then re-run --fix.") + } if len(fixes) == 0 { if report.HasProblems() { _, _ = fmt.Fprintln(out, "\nNone of the remaining problems can be fixed automatically — follow the → hints above.") diff --git a/internal/install/doctor.go b/internal/install/doctor.go index 9ec7554..828255c 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -68,6 +68,12 @@ const ( // FixSetProxy writes a proxy the doctor verified works into the plugin's // .env, so the server uses it on its next start. FixSetProxy FixAction = "set-proxy" + // FixBlocked marks a check that --fix cannot safely repair and that + // blocks FixReregister/FixReinstall for every editor: re-registering + // reads the editor's config as a map first, and a config that fails to + // parse reads back as empty, so writing it out again would drop every + // other server the user configured in that file. + FixBlocked FixAction = "blocked" ) // DoctorCheck is one diagnostic result reported by RunDoctor. @@ -163,15 +169,31 @@ func (r *DoctorReport) Fixes() []FixAction { if setProxy { out = append(out, FixSetProxy) } - switch { - case reinstall: - out = append(out, FixReinstall) - case reregister: - out = append(out, FixReregister) + if !r.HasBlockedRegistration() { + switch { + case reinstall: + out = append(out, FixReinstall) + case reregister: + out = append(out, FixReregister) + } } return out } +// HasBlockedRegistration reports whether any check is marked FixBlocked, +// meaning at least one editor's config file failed to parse. Reregistering +// any editor goes through the same manifest-wide update, so this blocks +// FixReregister/FixReinstall entirely rather than risk rewriting that +// editor's config from an empty map. +func (r *DoctorReport) HasBlockedRegistration() bool { + for _, c := range r.Checks { + if (c.Status == StatusFail || c.Status == StatusWarn) && c.Fix == FixBlocked { + return true + } + } + return false +} + // envFix is a verified set of variables to add to a .env file. type envFix struct { EnvFile string @@ -705,7 +727,7 @@ func checkManifestEditors(d *doctorRun, component, identifier string, editors ma // Not auto-fixable: re-registering would start from an empty map // and drop every other server the user configured in this file. report.add(component, name, StatusFail, fmt.Sprintf("config file %s is not valid: %v", entry.ConfigFile, parseErr)). - hint(name + " ignores the whole file when it can't be parsed, so no servers in it load. Fix the syntax (often a missing or extra comma), then re-run this doctor.") + fix(FixBlocked, name+" ignores the whole file when it can't be parsed, so no servers in it load. Fix the syntax (often a missing or extra comma), then re-run this doctor.") continue } diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index d4daab3..d6c2b4c 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -243,11 +243,17 @@ func TestCheckManifestEditors(t *testing.T) { }, }) + // Malformed JSON: not auto-fixable, since re-registering reads this file + // as an empty map and would drop every other server it configures. + invalidFile := filepath.Join(dir, "invalid.json") + _ = os.WriteFile(invalidFile, []byte(`{"mcpServers": {`), 0o600) + editors := map[EditorID]ManifestEntry{ EditorCursor: {ConfigFile: presentFile, Format: "mcpServers"}, EditorWindsurf: {ConfigFile: staleFile, Format: "mcpServers"}, EditorZed: {ConfigFile: missingFile, Format: "mcpServers"}, EditorVSCode: {ConfigFile: deadCommandFile, Format: "mcpServers"}, + EditorCline: {ConfigFile: invalidFile, Format: "mcpServers"}, } d := newDoctorRun(DoctorOptions{}) @@ -255,8 +261,10 @@ func TestCheckManifestEditors(t *testing.T) { report := d.report statuses := make(map[string]CheckStatus) + fixes := make(map[string]FixAction) for _, c := range report.Checks { statuses[c.Name] = c.Status + fixes[c.Name] = c.Fix } if statuses["Cursor"] != StatusOK { @@ -271,6 +279,41 @@ func TestCheckManifestEditors(t *testing.T) { if statuses["VS Code"] != StatusFail { t.Errorf("VS Code status = %v, want fail (command path dead)", statuses["VS Code"]) } + if statuses["Cline"] != StatusFail || fixes["Cline"] != FixBlocked { + t.Errorf("Cline status/fix = %v/%v, want fail/blocked (invalid JSON)", statuses["Cline"], fixes["Cline"]) + } + + // Zed and VS Code alone would call for FixReregister, but the invalid + // Cline config must veto it for the whole report: reregistering goes + // through every manifest editor, including Cline's. + for _, f := range report.Fixes() { + if f == FixReregister || f == FixReinstall { + t.Errorf("Fixes() = %v, want FixReregister/FixReinstall withheld while a config is unparsable", report.Fixes()) + } + } +} + +func TestCheckVSCodeWorkspaceInvalidConfig(t *testing.T) { + workspace := t.TempDir() + vscodeDir := filepath.Join(workspace, ".vscode") + _ = os.MkdirAll(vscodeDir, 0o750) + // Malformed JSONC: VS Code ignores the whole file, so a real armis-appsec + // entry in here would silently stop loading. + _ = os.WriteFile(filepath.Join(vscodeDir, "mcp.json"), []byte(`{"servers": {`), 0o600) + + d := newDoctorRun(DoctorOptions{}) + checkVSCodeWorkspace(d, workspace) + + if len(d.report.Checks) != 1 { + t.Fatalf("checks = %+v, want exactly one failing check for the unparsable workspace config", d.report.Checks) + } + c := d.report.Checks[0] + if c.Status != StatusFail || c.Component != componentVSCode { + t.Errorf("check = %+v, want a StatusFail check in the vscode component", c) + } + if c.Remediation == "" { + t.Errorf("check has no remediation hint for the unparsable config") + } } func TestClaudeRegistryStatus(t *testing.T) { diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index 018c010..0792bfe 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -216,7 +216,14 @@ func checkVSCodeWorkspace(d *doctorRun, workspace string) { {Label: "workspace .vscode/settings.json", Path: filepath.Join(workspace, ".vscode", "settings.json"), InSettings: true}, } { obj, exists, err := readJSONCObject(src.Path) - if !exists || err != nil { + if !exists { + continue + } + if err != nil { + if !d.manifestConfigs[filepath.Clean(src.Path)] { + d.report.add(componentVSCode, src.Label, StatusFail, fmt.Sprintf("%s is not valid JSON: %v", src.Path, err)). + hint("VS Code ignores a workspace config it can't parse, so no servers in it load — including armis-appsec if it's registered there. Fix the syntax (often a missing or extra comma), then re-run this doctor.") + } continue } if _, entry, ok := findServer(vscodeServers(obj, src.InSettings), mcpServerName); ok { From 51f936c252ff62bf2ba57e177b31e31d4b0a81af Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 15:59:35 +0300 Subject: [PATCH 05/12] fix(mcp): collapse duplicate env keys instead of writing them twice 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. --- internal/install/doctor_checks_test.go | 17 +++++++++++++++++ internal/install/plugin.go | 14 +++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index 922dfb0..10be4a9 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -541,6 +541,23 @@ func TestSetEnvFileVars(t *testing.T) { } } +// TestSetEnvFileVarsDedupesExistingDuplicateKey pins that a key already +// duplicated in the file (e.g. from a hand edit) collapses to one line +// instead of ending up duplicated with the same value twice. +func TestSetEnvFileVarsDedupesExistingDuplicateKey(t *testing.T) { + path := filepath.Join(t.TempDir(), ".env") + mustWrite(t, path, "ARMIS_CLIENT_ID=old\nSSL_CERT_FILE=/ca.pem\nARMIS_CLIENT_ID=stale-dup\n") + + if err := SetEnvFileVars(path, [][2]string{{"ARMIS_CLIENT_ID", "new"}}); err != nil { + t.Fatalf("SetEnvFileVars() error = %v", err) + } + b, _ := os.ReadFile(path) //nolint:gosec // test temp dir + want := "ARMIS_CLIENT_ID=new\nSSL_CERT_FILE=/ca.pem\n" + if string(b) != want { + t.Errorf("content = %q, want %q (duplicate assignment collapsed)", b, want) + } +} + // TestWriteEnvFromValuesKeepsOtherVars pins that re-entering credentials // doesn't drop a proxy or CA setting the doctor (or the user) added. func TestWriteEnvFromValuesKeepsOtherVars(t *testing.T) { diff --git a/internal/install/plugin.go b/internal/install/plugin.go index 6b15206..b162e0c 100644 --- a/internal/install/plugin.go +++ b/internal/install/plugin.go @@ -592,13 +592,21 @@ func SetEnvFileVars(envPath string, vars [][2]string) error { for _, kv := range vars { replaced := false - for i, line := range lines { + kept := lines[:0] + for _, line := range lines { k, _, ok := strings.Cut(strings.TrimSpace(line), "=") - if ok && !strings.HasPrefix(strings.TrimSpace(line), "#") && strings.TrimSpace(k) == kv[0] { - lines[i] = kv[0] + "=" + kv[1] + isAssignment := ok && !strings.HasPrefix(strings.TrimSpace(line), "#") && strings.TrimSpace(k) == kv[0] + switch { + case !isAssignment: + kept = append(kept, line) + case !replaced: + // Keep the first assignment, updated in place; drop any + // further duplicate assignments of the same key below. + kept = append(kept, kv[0]+"="+kv[1]) replaced = true } } + lines = kept if !replaced { lines = append(lines, kv[0]+"="+kv[1]) } From cc1d5b9d8210514012ed111abc9c7795bc7a4588 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 16:07:14 +0300 Subject: [PATCH 06/12] fix(mcp): scrub short secrets in support bundles, fix BOM-only JSONC 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. --- internal/install/doctor_bundle.go | 2 +- internal/install/doctor_checks_test.go | 29 ++++++++++++++++++++++++++ internal/install/doctor_test.go | 18 ++++++++++++++++ internal/install/doctor_vscode.go | 5 +++-- 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/internal/install/doctor_bundle.go b/internal/install/doctor_bundle.go index 09b4183..d62e21d 100644 --- a/internal/install/doctor_bundle.go +++ b/internal/install/doctor_bundle.go @@ -34,7 +34,7 @@ func WriteSupportBundle(report *DoctorReport, path, cliVersion string) error { scrub := func(s string) string { for _, secret := range report.secrets { - if len(secret) >= 4 { + if secret != "" { s = strings.ReplaceAll(s, secret, "***") } } diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index 10be4a9..4e5f8e4 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -515,6 +515,35 @@ func TestWriteSupportBundleScrubsSecrets(t *testing.T) { } } +// TestWriteSupportBundleScrubsShortSecrets pins that a short proxy password +// or test credential is scrubbed too, not just secrets 4+ characters long. +func TestWriteSupportBundleScrubsShortSecrets(t *testing.T) { + report := &DoctorReport{secrets: []string{"pw1"}} + report.artifact("stderr/x.txt", "proxy auth failed with password pw1\n") + + path := filepath.Join(t.TempDir(), "bundle.zip") + if err := WriteSupportBundle(report, path, "1.2.3"); err != nil { + t.Fatalf("WriteSupportBundle() error = %v", err) + } + zr, err := zip.OpenReader(path) + if err != nil { + t.Fatal(err) + } + defer func() { _ = zr.Close() }() + + for _, f := range zr.File { + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + b, _ := io.ReadAll(rc) + _ = rc.Close() + if strings.Contains(string(b), "pw1") { + t.Errorf("%s contains the short secret: %s", f.Name, b) + } + } +} + func TestMaskURLUserinfo(t *testing.T) { if got := maskURLUserinfo("http://user:pw@proxy:8080"); got != "http://***@proxy:8080" { t.Errorf("maskURLUserinfo() = %q", got) diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index d6c2b4c..e07d419 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -316,6 +316,24 @@ func TestCheckVSCodeWorkspaceInvalidConfig(t *testing.T) { } } +// TestReadJSONCObjectBOMOnly pins that a file containing only a UTF-8 BOM (or +// BOM plus comments/whitespace) is treated as empty rather than a parse +// error: TrimSpace alone doesn't strip the BOM rune, so the emptiness check +// must run after stripJSONC removes it. +func TestReadJSONCObjectBOMOnly(t *testing.T) { + path := filepath.Join(t.TempDir(), "mcp.json") + + _ = os.WriteFile(path, []byte("\xEF\xBB\xBF"), 0o600) + if obj, exists, err := readJSONCObject(path); err != nil || !exists || len(obj) != 0 { + t.Errorf("readJSONCObject(BOM only) = (%v, %v, %v), want (empty map, true, nil)", obj, exists, err) + } + + _ = os.WriteFile(path, []byte("\xEF\xBB\xBF// just a comment\n"), 0o600) + if obj, exists, err := readJSONCObject(path); err != nil || !exists || len(obj) != 0 { + t.Errorf("readJSONCObject(BOM + comment) = (%v, %v, %v), want (empty map, true, nil)", obj, exists, err) + } +} + func TestClaudeRegistryStatus(t *testing.T) { dir := t.TempDir() pluginsDir := filepath.Join(dir, "plugins") diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index 0792bfe..feb8b31 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -474,10 +474,11 @@ func readJSONCObject(path string) (obj map[string]interface{}, exists bool, err } return nil, true, err } - if len(strings.TrimSpace(string(b))) == 0 { + stripped := stripJSONC(b) + if len(strings.TrimSpace(string(stripped))) == 0 { return map[string]interface{}{}, true, nil } - if err := json.Unmarshal(stripJSONC(b), &obj); err != nil { + if err := json.Unmarshal(stripped, &obj); err != nil { return nil, true, err } if obj == nil { From e41a30a7ae69d48783098227dd804942fda794d9 Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 16:14:58 +0300 Subject: [PATCH 07/12] fix(mcp): widen probe stdout buffer, clarify env key validation error 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. --- internal/install/doctor_probe.go | 9 +++++++-- internal/install/plugin.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index fbe3b32..543b9f3 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -171,8 +171,13 @@ type mcpSession struct { func runMCPSession(stdin io.WriteCloser, stdout io.ReadCloser, timeout time.Duration) (*probeResult, error) { s := &mcpSession{ - stdin: stdin, - lines: make(chan []byte, 16), + stdin: stdin, + // Buffered generously: nothing drains this channel between the + // synchronous call()/notify() invocations below, so a server that + // bursts many log/notification lines right after answering one + // call could otherwise block the reader goroutine until the next + // call starts draining. + lines: make(chan []byte, 256), readErr: make(chan error, 1), deadline: time.Now().Add(timeout), } diff --git a/internal/install/plugin.go b/internal/install/plugin.go index b162e0c..3cab9c6 100644 --- a/internal/install/plugin.go +++ b/internal/install/plugin.go @@ -564,7 +564,7 @@ func WriteEnvFromValues(envPath, clientID, clientSecret string) error { func SetEnvFileVars(envPath string, vars [][2]string) error { for _, kv := range vars { if kv[0] == "" || strings.ContainsAny(kv[0], "=\n\r") || strings.ContainsAny(kv[1], "\n\r") { - return fmt.Errorf("invalid env entry for %q: keys and values must not contain newlines", kv[0]) + return fmt.Errorf("invalid env entry for %q: keys must not contain '=' or newlines, and values must not contain newlines", kv[0]) } } From 723783617491ed1ac4e3c2cc8aeca7815549bb3a Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 16:26:26 +0300 Subject: [PATCH 08/12] fix(mcp): catch stdout corruption after initialize, stop over-scrubbing 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). --- internal/install/doctor.go | 10 +++++++-- internal/install/doctor_checks_test.go | 30 ++++++++++++++++++++++++++ internal/install/doctor_probe.go | 12 +++++------ internal/install/doctor_test.go | 17 +++++++++++++++ internal/install/doctor_vscode.go | 4 ++-- 5 files changed, 62 insertions(+), 11 deletions(-) diff --git a/internal/install/doctor.go b/internal/install/doctor.go index 828255c..dc88030 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -462,7 +462,13 @@ func checkCredentials(report *DoctorReport, component, envFile string) map[strin keys := make([]string, 0, len(env)) for k, v := range env { keys = append(keys, k) - if isSecretKey(k) && v != "" { + switch { + case isSecretKey(k) && v != "": + report.secrets = append(report.secrets, v) + case strings.Contains(strings.ToUpper(k), "PROXY") && v != "" && maskURLUserinfo(v) != v: + // Only the credentials embedded in a proxy URL are secret; the + // host/port on their own are useful diagnostic detail worth + // keeping readable in the bundle. report.secrets = append(report.secrets, v) } } @@ -1000,7 +1006,7 @@ func parseEnvFile(path string) (map[string]string, error) { // isSecretKey reports whether an env var name likely holds a credential. func isSecretKey(k string) bool { k = strings.ToUpper(k) - for _, marker := range []string{"SECRET", "TOKEN", "PASSWORD", "CLIENT_ID", "API_KEY", "PROXY"} { + for _, marker := range []string{"SECRET", "TOKEN", "PASSWORD", "CLIENT_ID", "API_KEY"} { if strings.Contains(k, marker) { return true } diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index 4e5f8e4..c3b80db 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -313,6 +313,36 @@ func TestCheckCredentialsBOMAndSecrets(t *testing.T) { } } +// TestCheckCredentialsProxyOnlyScrubsCredentials pins that a plain proxy URL +// with no embedded userinfo isn't collected as a secret — scrubbing it would +// blank out the host:port, which is useful diagnostic detail, not a +// credential. A proxy URL that does carry userinfo is still collected. +func TestCheckCredentialsProxyOnlyScrubsCredentials(t *testing.T) { + envFile := filepath.Join(t.TempDir(), ".env") + mustWrite(t, envFile, "ARMIS_CLIENT_ID=the-id\nARMIS_CLIENT_SECRET=the-secret\n"+ + "HTTPS_PROXY=http://proxy.corp:8080\nHTTP_PROXY=http://user:pw@proxy.corp:8080\n") + + report := &DoctorReport{} + checkCredentials(report, "scanner", envFile) + + for _, want := range []string{"the-secret", "user:pw"} { + found := false + for _, s := range report.secrets { + if s == want || strings.Contains(s, want) { + found = true + } + } + if !found { + t.Errorf("secrets = %v, want an entry covering %q", report.secrets, want) + } + } + for _, s := range report.secrets { + if s == "http://proxy.corp:8080" { + t.Errorf("secrets = %v, plain non-credential proxy URL should not be collected", report.secrets) + } + } +} + func TestReportFixes(t *testing.T) { r := &DoctorReport{} if r.Fixes() != nil { diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index 543b9f3..e6f7ba8 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -290,13 +290,11 @@ func (s *mcpSession) call(id int, method string, params interface{}, timeout tim case line := <-s.lines: var msg rpcMessage if err := json.Unmarshal(line, &msg); err != nil { - if id == 1 { - // Anything but JSON on stdout before initialize means the - // server (or a wrapper script) is printing to stdout, - // which corrupts the MCP stream for every client. - return nil, fmt.Errorf("invalid response (non-JSON on stdout: %q): %w", truncate(string(line), 120), err) - } - continue + // Anything but JSON on stdout means the server (or a wrapper + // script) is printing to stdout, which corrupts the MCP + // stream for every client — not just while waiting on + // initialize. + return nil, fmt.Errorf("invalid response (non-JSON on stdout: %q): %w", truncate(string(line), 120), err) } if msg.ID == nil || *msg.ID != id { continue diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index e07d419..d7c83d2 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -57,6 +57,9 @@ func runMCPHelperProcess(mode string) { // A log notification before the response must be skipped. _, _ = fmt.Fprintln(os.Stdout, `{"jsonrpc":"2.0","method":"notifications/message","params":{}}`) reply(`"result":{"serverInfo":{"name":"fake-mcp","version":"9.9.9"}}`) + case req.Method == "tools/list" && mode == "garbage-after-init": + _, _ = fmt.Fprintln(os.Stdout, "not json") + return case req.Method == "tools/list" && mode == "notools": reply(`"result":{"tools":[]}`) case req.Method == "tools/list": @@ -101,6 +104,20 @@ func TestMCPHandshakeInvalidResponse(t *testing.T) { } } +// TestMCPHandshakeInvalidResponseAfterInit pins that non-JSON stdout is +// caught even once the session is past initialize, not just before it: a +// server that starts clean but later corrupts its own stdout stream should +// surface as a tools error rather than being silently skipped forever. +func TestMCPHandshakeInvalidResponseAfterInit(t *testing.T) { + res, _, err := mcpHandshake(os.Args[0], nil, map[string]string{"ARMIS_TEST_MCP_HELPER": "garbage-after-init"}, 5*time.Second) + if err != nil { + t.Fatalf("mcpHandshake() error = %v, want initialize to still succeed", err) + } + if res.ToolsErr == nil { + t.Fatal("mcpHandshake() ToolsErr = nil, want error from invalid JSON on stdout during tools/list") + } +} + func TestMCPHandshakeTimeout(t *testing.T) { _, _, err := mcpHandshake(os.Args[0], nil, map[string]string{"ARMIS_TEST_MCP_HELPER": "hang"}, 300*time.Millisecond) if err == nil { diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index feb8b31..30d401d 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -135,7 +135,7 @@ func checkVSCodeVariant(d *doctorRun, v vscodeVariant, workspace, pluginDir, sni } if err != nil { if !d.manifestConfigs[filepath.Clean(src.Path)] { - report.add(componentVSCode, v.Name+" config", StatusFail, fmt.Sprintf("%s is not valid JSON: %v", src.Path, err)). + report.add(componentVSCode, v.Name+" config", StatusFail, fmt.Sprintf("%s is not valid JSONC: %v", src.Path, err)). hint(v.Name + " ignores a file it can't parse, so no servers in it load. Fix the syntax (often a missing or extra comma), then re-run this doctor.") } continue @@ -221,7 +221,7 @@ func checkVSCodeWorkspace(d *doctorRun, workspace string) { } if err != nil { if !d.manifestConfigs[filepath.Clean(src.Path)] { - d.report.add(componentVSCode, src.Label, StatusFail, fmt.Sprintf("%s is not valid JSON: %v", src.Path, err)). + d.report.add(componentVSCode, src.Label, StatusFail, fmt.Sprintf("%s is not valid JSONC: %v", src.Path, err)). hint("VS Code ignores a workspace config it can't parse, so no servers in it load — including armis-appsec if it's registered there. Fix the syntax (often a missing or extra comma), then re-run this doctor.") } continue From 21cec3bc97be7b69ab0df065ad72d27cfd3c851a Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 16:47:58 +0300 Subject: [PATCH 09/12] fix(mcp): clamp negative session timeout in probe error message 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. --- internal/install/doctor_probe.go | 3 +++ internal/install/doctor_test.go | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index e6f7ba8..710b0dd 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -272,6 +272,9 @@ func (s *mcpSession) call(id int, method string, params interface{}, timeout tim } wait := time.Until(s.deadline) + if wait < 0 { + wait = 0 + } timer := time.NewTimer(wait) defer timer.Stop() for { diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index d7c83d2..0366e9b 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -4,6 +4,7 @@ import ( "bufio" "encoding/json" "fmt" + "io" "os" "path/filepath" "runtime" @@ -118,6 +119,26 @@ func TestMCPHandshakeInvalidResponseAfterInit(t *testing.T) { } } +// TestMCPSessionCallClampsNegativeWait pins that a deadline already in the +// past (because earlier steps in the same session consumed the whole +// timeout) produces a sensible "after 0s"-style message, not a confusing +// negative duration. +func TestMCPSessionCallClampsNegativeWait(t *testing.T) { + s := &mcpSession{ + stdin: io.Discard, + lines: make(chan []byte, 1), + readErr: make(chan error, 1), + deadline: time.Now().Add(-time.Second), + } + _, err := s.call(1, "initialize", map[string]interface{}{}, time.Second) + if err == nil { + t.Fatal("call() error = nil, want a timeout error") + } + if strings.Contains(err.Error(), "-") { + t.Errorf("call() error = %q, want no negative duration in the message", err.Error()) + } +} + func TestMCPHandshakeTimeout(t *testing.T) { _, _, err := mcpHandshake(os.Args[0], nil, map[string]string{"ARMIS_TEST_MCP_HELPER": "hang"}, 300*time.Millisecond) if err == nil { From 6a12e36161d24f926a0a9b770fa3239411af54da Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 17:01:53 +0300 Subject: [PATCH 10/12] fix(mcp): report editor entries with no command as failures in doctor --- internal/install/doctor.go | 12 ++++++++++-- internal/install/doctor_test.go | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/internal/install/doctor.go b/internal/install/doctor.go index dc88030..1cbe111 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -745,7 +745,15 @@ func checkManifestEditors(d *doctorRun, component, identifier string, editors ma continue } report.artifact(fmt.Sprintf("editors/%s-%s-entry.json", sanitizeArtifactName(component), id), launchArtifact(entry.ConfigFile, launch)) - if launch.Command != "" && !isExecutableFile(launch.Command) { + // lookupEntry understands every format the installer writes, so an + // entry with no command here can't be started by the editor. + if launch.Command == "" { + report.add(component, name, StatusFail, + fmt.Sprintf("entry found in %s but it has no command", entry.ConfigFile)). + fix(FixReregister, "Re-register the server: armis-cli mcp doctor --fix") + continue + } + if !isExecutableFile(launch.Command) { report.add(component, name, StatusFail, fmt.Sprintf("entry found in %s but its command does not exist: %s — likely stale after a reinstall or profile/home directory change", entry.ConfigFile, launch.Command)). fix(FixReregister, "Point the entry at the current install: armis-cli mcp doctor --fix") @@ -761,7 +769,7 @@ func checkManifestEditors(d *doctorRun, component, identifier string, editors ma } report.add(component, name, StatusOK, entry.ConfigFile) - if d.opts.Handshake && launch.Command != "" { + if d.opts.Handshake { d.probe(component, name, launch) } } diff --git a/internal/install/doctor_test.go b/internal/install/doctor_test.go index 0366e9b..a556f11 100644 --- a/internal/install/doctor_test.go +++ b/internal/install/doctor_test.go @@ -286,12 +286,19 @@ func TestCheckManifestEditors(t *testing.T) { invalidFile := filepath.Join(dir, "invalid.json") _ = os.WriteFile(invalidFile, []byte(`{"mcpServers": {`), 0o600) + // Entry present by name but with no command: the editor can't start it. + noCommandFile := filepath.Join(dir, "no-command.json") + mustWriteJSON(t, noCommandFile, map[string]interface{}{ + "mcpServers": map[string]interface{}{"armis-appsec": map[string]interface{}{"args": []string{"-m", "x"}}}, + }) + editors := map[EditorID]ManifestEntry{ EditorCursor: {ConfigFile: presentFile, Format: "mcpServers"}, EditorWindsurf: {ConfigFile: staleFile, Format: "mcpServers"}, EditorZed: {ConfigFile: missingFile, Format: "mcpServers"}, EditorVSCode: {ConfigFile: deadCommandFile, Format: "mcpServers"}, EditorCline: {ConfigFile: invalidFile, Format: "mcpServers"}, + EditorAmazonQ: {ConfigFile: noCommandFile, Format: "mcpServers"}, } d := newDoctorRun(DoctorOptions{}) @@ -317,6 +324,10 @@ func TestCheckManifestEditors(t *testing.T) { if statuses["VS Code"] != StatusFail { t.Errorf("VS Code status = %v, want fail (command path dead)", statuses["VS Code"]) } + amazonQ, _ := EditorByID(EditorAmazonQ) + if statuses[amazonQ.Name] != StatusFail || fixes[amazonQ.Name] != FixReregister { + t.Errorf("%s status/fix = %v/%v, want fail/reregister (empty command)", amazonQ.Name, statuses[amazonQ.Name], fixes[amazonQ.Name]) + } if statuses["Cline"] != StatusFail || fixes["Cline"] != FixBlocked { t.Errorf("Cline status/fix = %v/%v, want fail/blocked (invalid JSON)", statuses["Cline"], fixes["Cline"]) } @@ -464,7 +475,9 @@ func TestRunDoctorStructuralChecks(t *testing.T) { []byte("ARMIS_CLIENT_ID=id\nARMIS_CLIENT_SECRET=secret\n"), 0o600) editorConfig := filepath.Join(home, "editor-mcp.json") - _ = os.WriteFile(editorConfig, []byte(`{"mcpServers":{"armis-appsec":{}}}`), 0o600) + mustWriteJSON(t, editorConfig, map[string]interface{}{ + "mcpServers": map[string]interface{}{"armis-appsec": map[string]interface{}{"command": venvPython(pluginDir)}}, + }) manifest := NewManifest(pluginDir, "1.2.3") manifest.AddEditor(EditorCursor, editorConfig, "mcpServers") From 36450be7421bf8ff848f0f4faf8d496f1579aa5c Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 17:12:29 +0300 Subject: [PATCH 11/12] fix(mcp): unblock probe stdout reader on session end, fail VS Code entries with no command, skip empty-profile warning --- internal/install/doctor_checks_test.go | 19 +++++++++++++++++++ internal/install/doctor_probe.go | 11 ++++++++++- internal/install/doctor_vscode.go | 5 +++-- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index c3b80db..2509275 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -413,6 +413,25 @@ func TestCheckVSCodeFindsConfigProblems(t *testing.T) { wantStatus(t, checks, "vscode/Copilot", StatusInfo) } +func TestCheckVSCodeEmptyProfileAndMissingCommand(t *testing.T) { + root := t.TempDir() + user := filepath.Join(root, "User") + mustWrite(t, filepath.Join(user, "mcp.json"), `{"servers": {"armis-appsec": {"type": "stdio"}}}`) + mustWrite(t, filepath.Join(user, "profiles", "abc123", "mcp.json"), `{"servers": {}}`) + stubVSCode(t, []vscodeVariant{{Name: "VS Code", Root: root}}) + + d := newDoctorRun(DoctorOptions{WorkspaceDir: t.TempDir()}) + checkVSCode(d, "/plugin", false) + checks := checkMap(d.report) + + if c := wantStatus(t, checks, "vscode/VS Code (user mcp.json)", StatusFail); c.Remediation == "" { + t.Error("entry with no command has no remediation") + } + if c, ok := checks["vscode/VS Code profile"]; ok { + t.Errorf("profile check = %+v, want none for a profile with an empty servers object", c) + } +} + func TestCheckVSCodeNotRegistered(t *testing.T) { stable, insiders := t.TempDir(), t.TempDir() for _, root := range []string{stable, insiders} { diff --git a/internal/install/doctor_probe.go b/internal/install/doctor_probe.go index 710b0dd..21a2438 100644 --- a/internal/install/doctor_probe.go +++ b/internal/install/doctor_probe.go @@ -181,11 +181,20 @@ func runMCPSession(stdin io.WriteCloser, stdout io.ReadCloser, timeout time.Dura readErr: make(chan error, 1), deadline: time.Now().Add(timeout), } + // done unblocks the reader once the session is over, so a server that + // keeps writing after the last call can't park the goroutine on a full + // channel forever. Dropping lines instead could drop the response itself. + done := make(chan struct{}) + defer close(done) go func() { scanner := bufio.NewScanner(stdout) scanner.Buffer(make([]byte, 0, 64*1024), maxHandshakeLineSize) for scanner.Scan() { - s.lines <- append([]byte(nil), scanner.Bytes()...) + select { + case s.lines <- append([]byte(nil), scanner.Bytes()...): + case <-done: + return + } } s.readErr <- scanner.Err() }() diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index 30d401d..7861f41 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -143,7 +143,7 @@ func checkVSCodeVariant(d *doctorRun, v vscodeVariant, workspace, pluginDir, sni servers := vscodeServers(obj, src.InSettings) name, entry, ok := findServer(servers, mcpServerName) if !ok { - if strings.HasPrefix(src.Label, "profile") && servers != nil { + if strings.HasPrefix(src.Label, "profile") && len(servers) > 0 { profilesWithout = append(profilesWithout, src) } continue @@ -178,7 +178,8 @@ func checkVSCodeVariant(d *doctorRun, v vscodeVariant, workspace, pluginDir, sni } name := v.Name + " (" + f.Source.Label + ")" if f.Launch.Command == "" { - report.add(componentVSCode, name, StatusWarn, f.Source.Path+": entry has no command") + report.add(componentVSCode, name, StatusFail, f.Source.Path+": entry has no command"). + hint(v.Name + " can't start an entry with no command. Delete it from " + f.Source.Path + ", or replace it with:\n" + snippet) continue } if !isExecutableFile(f.Launch.Command) { From 23aa1fbcbf1e6c853edffc57667e3fc38de3285a Mon Sep 17 00:00:00 2001 From: Yiftach Cohen Date: Wed, 23 Sep 2026 17:19:04 +0300 Subject: [PATCH 12/12] fix(mcp): expand ${workspaceFolder} in VS Code manifest entries before checking and probing --- internal/install/doctor.go | 23 +++++++++++++++-------- internal/install/doctor_checks_test.go | 12 +++++++++++- internal/install/doctor_vscode.go | 5 +---- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/internal/install/doctor.go b/internal/install/doctor.go index 1cbe111..87d6840 100644 --- a/internal/install/doctor.go +++ b/internal/install/doctor.go @@ -247,6 +247,15 @@ type doctorRun struct { manifestConfigs map[string]bool } +// workspaceDir returns opts.WorkspaceDir, defaulting to the current directory. +func (d *doctorRun) workspaceDir() string { + if d.opts.WorkspaceDir != "" { + return d.opts.WorkspaceDir + } + wd, _ := os.Getwd() + return wd +} + type probeOutcome struct { label string // component/name of the check that ran it ok bool @@ -737,7 +746,7 @@ func checkManifestEditors(d *doctorRun, component, identifier string, editors ma continue } - launch, found := lookupEntry(entry.ConfigFile, entry.Format, identifier) + launch, found := lookupEntry(entry.ConfigFile, entry.Format, identifier, d.workspaceDir()) if !found { report.add(component, name, StatusWarn, fmt.Sprintf("registered at %s but entry not found — was it edited or removed?", entry.ConfigFile)). @@ -882,17 +891,15 @@ func checkCodexSection(report *DoctorReport, component string, codex *ManifestCo // lookupEntryCommand finds the server entry matching identifier in configFile // and returns the command path it declares. See lookupEntry. func lookupEntryCommand(configFile, format, identifier string) (command string, found bool) { - l, found := lookupEntry(configFile, format, identifier) + l, found := lookupEntry(configFile, format, identifier, "") return l.Command, found } // lookupEntry finds the server entry matching identifier in configFile (read // per the manifest's recorded format) and returns how it launches the server. -// found is true as soon as a matching entry name exists, even when the -// command comes back empty because the format stores it somewhere this -// function doesn't understand — callers must treat an empty command as -// "unknown", not "missing". -func lookupEntry(configFile, format, identifier string) (serverLaunch, bool) { +// found is true as soon as a matching entry name exists, even when the entry +// has no command. workspace resolves ${workspaceFolder} in VS Code entries. +func lookupEntry(configFile, format, identifier, workspace string) (serverLaunch, bool) { identifier = strings.ToLower(identifier) matchEntry := func(servers map[string]interface{}) (map[string]interface{}, bool) { @@ -917,7 +924,7 @@ func lookupEntry(configFile, format, identifier string) (serverLaunch, bool) { if !ok { return serverLaunch{}, false } - return vscodeLaunch(entry, ""), true + return vscodeLaunch(entry, workspace), true case configFormatZed: servers, _ := readJSONFileAsMap(configFile)["context_servers"].(map[string]interface{}) entry, ok := matchEntry(servers) diff --git a/internal/install/doctor_checks_test.go b/internal/install/doctor_checks_test.go index 2509275..75b22b0 100644 --- a/internal/install/doctor_checks_test.go +++ b/internal/install/doctor_checks_test.go @@ -128,7 +128,7 @@ func TestLookupEntryVSCodeLaunch(t *testing.T) { }, }`) - l, ok := lookupEntry(path, configFormatVSCode, mcpServerName) + l, ok := lookupEntry(path, configFormatVSCode, mcpServerName, "") if !ok { t.Fatal("lookupEntry() found = false") } @@ -143,6 +143,16 @@ func TestLookupEntryVSCodeLaunch(t *testing.T) { } } +func TestLookupEntryVSCodeExpandsWorkspaceFolder(t *testing.T) { + path := filepath.Join(t.TempDir(), "mcp.json") + mustWrite(t, path, `{"servers": {"armis-appsec": {"command": "${workspaceFolder}/python"}}}`) + + l, ok := lookupEntry(path, configFormatVSCode, mcpServerName, "/ws") + if !ok || l.Command != "/ws/python" { + t.Errorf("lookupEntry() = %q, %v, want ${workspaceFolder} expanded to /ws", l.Command, ok) + } +} + func TestExpandVSCodeVars(t *testing.T) { home, _ := os.UserHomeDir() t.Setenv("ARMIS_TEST_VAR", "val") diff --git a/internal/install/doctor_vscode.go b/internal/install/doctor_vscode.go index 7861f41..3ebac0a 100644 --- a/internal/install/doctor_vscode.go +++ b/internal/install/doctor_vscode.go @@ -75,10 +75,7 @@ type vscodeFound struct { // or Group Policy, or organization Copilot policy. func checkVSCode(d *doctorRun, pluginDir string, registered bool) { report := d.report - workspace := d.opts.WorkspaceDir - if workspace == "" { - workspace, _ = os.Getwd() - } + workspace := d.workspaceDir() var detected []vscodeVariant for _, v := range vscodeVariants() {