fix(cli): surface Windows MCP launcher drift in doctor#2251
fix(cli): surface Windows MCP launcher drift in doctor#2251abundantbeing wants to merge 3 commits into
Conversation
Windows npm updates can leave older GitNexus MCP Node processes running from npx caches or stale package roots, which makes the next install/update look broken when native files stay locked. The doctor command now reports Windows MCP process sources so users can restart editors/agents or stop current MCP servers before retrying the update. Constraint: Windows package updates can leave node-based MCP servers holding native addon files open Rejected: Auto-stop GitNexus MCP processes from doctor | too side-effectful for a diagnostic command Confidence: medium Scope-risk: narrow Directive: Keep the Windows process scan best-effort and non-fatal; doctor must still work if PowerShell or CIM process lookup is unavailable Tested: npx vitest run test/unit/doctor-format.test.ts; npx tsc --noEmit; npm run build; npx eslint gitnexus/src/cli/doctor.ts gitnexus/test/unit/doctor-format.test.ts gitnexus/scripts/cross-platform-tests.ts; git diff --check; GitNexus detect_changes Not-tested: Full npm test passes on this Windows shell; broader unit and cross-platform suites have unrelated Windows/env failures noted in the PR
|
@abundantbeing is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
magyargergo
left a comment
There was a problem hiding this comment.
Automated tri-review — 7 Claude reviewer lanes + Codex (the one independent engine; Codex ran live and returned findings). Findings are weighted by cross-engine agreement: Codex and a Claude lane agreeing = strong; Claude-only agreement = "consistent across personas," weaker. This is a digest of automated tools — verify before acting.
What's solid (credit to the reviews). Tightly-scoped, well-built Windows-only diagnostic. Security reviewed clean (code-verified): the PowerShell script is a compile-time constant (no interpolation), execFile is called with an args array (no shell), parsed process command-lines are only classified — never executed, eval'd, or printed raw — and every failure path is caught. The non-Windows path is provably unchanged (win32 gate at both the function and the call site). The 3 new unit tests pass (9/9, run locally). Branch carries 2 merge-from-main commits but the net diff vs the merge-base is exactly the 3 intended files — no unrelated churn.
Headline findings (each reached by Codex AND ≥1 Claude lane → strong)
P2 · Sibling-install drift is mis-reported as "current" — defeats the feature's purpose (doctor.ts:168) · [reproduced]
command.includes(currentRoot) is a path-segment-unaware substring test and currentRoot carries no trailing separator, so a different install whose path shares the current root as a prefix is bucketed current. Reproduced by executing windowsGitNexusMcpDoctorStatus: currentRoot C:\work\gitnexus + processes at …\gitnexus2\… and …\gitnexus-old\gitnexus\… → 2 running (2 current) with the same-install note — i.e. genuine drift reported as none. Fix: match on a boundary, e.g. command.includes(currentRoot + '/') (appending '/', not path.sep, is correct because normalizePathForMatch already converted backslashes to /).
P2 · A failed/timed-out probe is indistinguishable from "none running" (doctor.ts:143 + 146-147) · [code-read]
Any spawn error (no powershell.exe / PS-Core-only box), AppLocker/ConstrainedLanguage block, maxBuffer overflow, or timeout (1500ms is tight for PowerShell cold-start) hits catch { return [] }, and the section prints Processes: none running with no Note — a false all-clear from a tool whose job is to surface drift. Fix: surface a distinct "detection unavailable" status from the catch path instead of collapsing to empty.
P3 · \bmcp\b over-matches incidental "mcp" tokens (doctor.ts:134) · [code-read]
The filter requires gitnexus AND \bmcp\b anywhere in the command line, so e.g. node …\gitnexus\scripts\mcp-smoke.js is counted as a running MCP server. Fix: anchor mcp as a trailing subcommand token rather than a bare word.
Test gaps — recommended, not merge-blocking
Claude lanes converged here; Codex classed these as coverage-only, not runtime bugs, so they don't block. Unexercised: the otherInstall bucket, the single-object parseWindowsProcessJson path (the most common real case — exactly one server running, where PowerShell emits an object not an array), the /npx-cli.js matcher arm, and currentPackageRootFromArgv (unexported, untested). A test in the first or last of these would have caught the headline bug.
Lower-priority (single Claude lane — optional)
Maintainability: the Windows logic could move to its own module; the npx markers (/_npx/, /npx-cli.js) and the two advisory-note strings could be named constants / t() i18n keys (the rest of doctor.ts uses the i18n helper).
Validated / refuted (validation is a feature)
- Confirmed safe (code-verified):
execFilewith args array → no shell injection; parsed command-lines never executed or printed; constant PowerShell script. - Consistent with PowerShell/JS semantics (not executed on a Windows host):
$null -matchaborting the PS pipeline (refuted — non-terminating no-op);JSON.parsechoking on quoted/unicode command-lines (refuted —ConvertTo-Jsonemits valid JSON and the parse is wrapped by the outer catch); single-object-vs-array shape (handled byArray.isArray ? … : [parsed]);doctorCommandhang / unhandled rejection (refuted — bounded by the 1500ms timeout and fully caught); a current-install path incidentally containing_npx(refuted — the current check runs before the npx check).
CI / coverage
PR is a draft; the only check is a Vercel deploy-auth gate (not functional CI), so no test suite has run on it yet. Coverage: full diff read (3 files, +180/-1); finding #1 reproduced via tsx; the two Windows-runtime findings are code-read (no Windows host available here). Automated multi-tool digest — verify before acting.
|
|
||
| for (const proc of opts.processes) { | ||
| const command = normalizePathForMatch(proc.commandLine); | ||
| if (currentRoot && command.includes(currentRoot)) { |
There was a problem hiding this comment.
[P2 · reproduced · Codex + 2 Claude lanes] Sibling-install drift is mis-reported as current, defeating this feature's purpose. command.includes(currentRoot) is a path-segment-unaware substring test, and currentRoot has no trailing separator — so any different install whose path shares the current root as a prefix is bucketed current.
Reproduced by executing windowsGitNexusMcpDoctorStatus: currentRoot C:\work\gitnexus + processes at …\gitnexus2\… and …\gitnexus-old\gitnexus\… → 2 running (2 current) with the same-install note — genuine drift reported as none.
Fix: match on a boundary, e.g. command.includes(currentRoot + '/'). Appending '/' (not path.sep) is correct because normalizePathForMatch has already converted backslashes to /. [reproduced]
| ); | ||
| return parseWindowsProcessJson(stdout); | ||
| } catch { | ||
| return []; |
There was a problem hiding this comment.
[P2 · code-read · Codex + 2 Claude lanes] A failed probe is indistinguishable from a clean machine. Any spawn error (no powershell.exe / PS-Core-only box), AppLocker/ConstrainedLanguage block, maxBuffer overflow, or timeout (the 1500ms at line 143 is tight for PowerShell cold-start) lands in this catch and returns []; the section then prints Processes: none running with no Note — a false all-clear from a tool whose entire job is to surface drift.
Fix: return a distinct detection unavailable status from the catch path so it isn't read as a verified-clean result. [code-read — not executed on a Windows host]
| const script = [ | ||
| "$ErrorActionPreference = 'Stop'", | ||
| 'Get-CimInstance Win32_Process -Filter "name = \'node.exe\'"', | ||
| " | Where-Object { $_.CommandLine -match 'gitnexus' -and $_.CommandLine -match '\\bmcp\\b' }", |
There was a problem hiding this comment.
[P3 · code-read · Codex + adversarial lane] \bmcp\b matches mcp as a word anywhere in the command line, and the filter only also requires the substring gitnexus. So e.g. node …\gitnexus\scripts\mcp-smoke.js (a dev script) is counted as a running MCP server.
Fix: anchor mcp as a trailing subcommand token (the CLI entry followed by mcp at end-of-args) rather than a bare word. Low severity — over-count only, same remediation note. [code-read]
Summary
gitnexus doctor.Why
On Windows,
gitnexus mcpcan keep running from an old package root or npx cache while a user updates the CLI. That makes npm cleanup/update failures look mysterious, especially when native addon files are still locked.doctornow points users toward the real recovery step: restart editors/agents after updating, or stop running MCP servers before retrying an update when locked native files are reported.Validation
npx prettier --write gitnexus/src/cli/doctor.ts gitnexus/test/unit/doctor-format.test.ts gitnexus/scripts/cross-platform-tests.tsgit diff --checknpx vitest run test/unit/doctor-format.test.ts(9 tests)npx tsc --noEmitnpx eslint gitnexus/src/cli/doctor.ts gitnexus/test/unit/doctor-format.test.ts gitnexus/scripts/cross-platform-tests.tsnpm run buildgitnexustypecheckdetect_changes: 3 changed files; impacted surface is thedoctorCommanddiagnostic flowKnown local verification limits
npm run test:cross-platformfails in this Windows shell on existinghooks-e2epnpm expectations: the tests expect--allow-build=@ladybugdb/core, while the current message isRun pnpm dlx gitnexus@latest analyze ....test/unit/git-utils.test.tswhereexecSync('git init -q')failed withspawnSync C:\WINDOWS\system32\cmd.exe ENOENT.Risk
Low runtime blast radius: the new lookup only runs on
win32, catches process-query failures, and leaves non-Windows doctor output untouched.