fix(mcp): guard unified-mcp-server spawn target against null CLI

[jobordu]

Problem

A user reported quorum running fail-open for an entire task: all 7 slots failed with TypeError: The "file" argument must be of type string. Received null, leaving plan review and verification review with no external voters.

Root cause for that incident was a stale local ~/.claude/nf-bin/ install (resolved by re-syncing). But investigating it surfaced a latent version of the same bug still in unified-mcp-server.mjs.

unified-mcp-server.mjs spawned providers via:

spawn(provider.resolvedCli ?? provider.cli, args, …)

— no mainTool fallback, no guard. providers.json carries cli: null for mainTool-only slots (the current schema after the providers.json consolidation, #167/#186). The startup resolution loop normally populates resolvedCli, but it only runs for type === 'subprocess' providers and skips any with no cli/mainTool. When it doesn't run, resolvedCli ?? cli is null → spawn(null) → opaque TypeError.

call-quorum-slot.cjs was already hardened against this exact case in #161 / #164 with a 3-tier fallback (resolvedCli → cli → mainTool) plus a guard throw. unified-mcp-server.mjs never got the matching fix — this PR closes that asymmetry.

Change

• Add a shared resolveSpawnTarget(provider) helper applying the same 3-tier fallback as call-quorum-slot.cjs, throwing a named error when nothing resolves.
• Both provider spawn sites (runProvider, runSubprocessWithArgs) use the helper. The throw lands inside their existing try/catch, so an unresolvable provider now surfaces as a diagnosable [spawn error: provider <name> has no resolvable CLI ...] instead of an opaque TypeError.
• The deep health-check binary lookup (runDeepHealthCheck) uses the same resolvedCli || cli || mainTool fallback so mainTool-only slots resolve there too.

No behavior change for correctly-resolved providers — resolveSpawnTarget returns exactly what resolvedCli ?? cli returned before. The only difference is a clear, named failure when a provider has no resolvable CLI.

Testing

• node --check bin/unified-mcp-server.mjs — passes.
• MCP server smoke test (initialize + tools/list + ping against PROVIDER_SLOT=codex-1) — boots, resolves CLIs, responds correctly.
• node --test test/resolve-cli-integration.test.cjs test/providers-path-consolidation.test.cjs — 43/45 pass. The 2 failures (providers.json empty) are pre-existing on main (repo ships bin/providers.json empty by design) and unrelated to this change.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Fixed subprocess executable resolution in the unified MCP server to prevent null values from being passed to spawned processes, improving stability for all provider configurations.
  • Improved error handling to provide clearer diagnostic error messages when provider executables cannot be resolved.
  • Enhanced health checks for subprocess providers to ensure comprehensive validation across all supported provider configuration options.

[coderabbitai]

Warning

Rate limit exceeded

@jobordu has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 15 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2f4331f0-a154-4aa7-9014-0cd0689c976b

📥 Commits

Reviewing files that changed from the base of the PR and between 183b881 and 542a397.

📒 Files selected for processing (1)

• .secrets.baseline

Walkthrough

This PR centralizes subprocess executable resolution in unified-mcp-server.mjs via a new resolveSpawnTarget helper that implements a consistent 3-tier fallback strategy (resolvedCli → cli → mainTool) and throws an explicit error when no executable can be determined. The change prevents passing null to child_process.spawn for mainTool-only provider slots and extends the same fallback logic to health-check binary path resolution.

Changes

Spawn target resolution and health check integration

Layer / File(s)	Summary
Spawn target resolution helper and call sites bin/unified-mcp-server.mjs, CHANGELOG.md	Added resolveSpawnTarget(provider) helper implementing the 3-tier fallback strategy with explicit error handling. Updated both runProvider and runSubprocessWithArgs spawn calls to use this helper instead of inline provider.resolvedCli ?? provider.cli expressions. Documented the fix in CHANGELOG.
Health check binary path resolution bin/unified-mcp-server.mjs	Updated deep health check binary existence lookup to compute binaryPath using the same 3-tier fallback as spawn target resolution, expanding coverage to mainTool-only slots.

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• nForma-AI/nForma#158: Both PRs modify bin/unified-mcp-server.mjs health check handling; #158 enables health check registration while this PR adjusts the executable resolution logic to support mainTool-only slots.
• nForma-AI/nForma#180: Both PRs implement the same 3-tier fallback strategy (resolvedCli → cli → mainTool) to prevent spawning with null/empty executables across different scripts.
• nForma-AI/nForma#161: Both PRs modify subprocess executable resolution to fall back to provider.mainTool when provider.cli is missing, preventing spawn failures with null targets.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding a guard against null CLI in unified-mcp-server spawn target resolution.
Description check	✅ Passed	The description covers all required template sections: Problem/Why, Change/What, Testing, and includes CHANGELOG update confirmation. However, the Checklist section is missing explicit check marks.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/unified-mcp-spawn-target-guard

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR hardens bin/unified-mcp-server.mjs against spawning subprocess providers with a null executable by aligning its spawn-target resolution behavior with the already-hardened quorum dispatch path.

Changes:

• Introduces a shared resolveSpawnTarget(provider) helper that resolves spawn executables via resolvedCli → cli → mainTool and throws a descriptive error if nothing resolves.
• Updates both subprocess spawn sites to use resolveSpawnTarget(...) instead of provider.resolvedCli ?? provider.cli.
• Updates deep health check binary lookup to apply the same 3-tier fallback and documents the fix in the changelog.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File	Description
CHANGELOG.md	Adds an Unreleased “Fixed” entry describing the spawn-target fallback/guard change.
bin/unified-mcp-server.mjs	Adds resolveSpawnTarget() and uses it at subprocess spawn sites; updates deep health check binary lookup fallback.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.