fix(link-daintree): always register health_check + smoke-probe new slots#158
Conversation
Why: When /nf:link-daintree fanned out Daintree presets into new MCP slots, the resulting slots failed to expose `health_check` in tools/list because unified-mcp-server gated tool registration on `provider.health_check_args`, which is absent after install.js reconstructs providers.json from ~/.claude.json. Execution already defaults to ['--version'], so the gate caused silent capability loss with no execution-time fallback symptom. Changes: - bin/unified-mcp-server.mjs: drop the health_check_args registration gate for subprocess and ccr provider types — health_check is now always registered when type warrants it. - commands/nf/link-daintree.md: add Step 2e "Smoke-probe newly-added slots" that spawns unified-mcp-server out-of-band for each new slot, sends initialize + tools/list, and verifies identity + health_check are exposed. Per-slot pass/fail surfaces in the closing summary and Step 4 now tells the user explicitly to restart Claude Code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removing the `if (provider.health_check_args)` gate in bin/unified-mcp-server.mjs (2 lines for subprocess type + 2 lines for ccr type, indentation collapsed) shifted two pre-existing baselined hashes by 2 lines each. Hashes unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR unconditionally registers a ChangesHealth Check Registration and Validation
Sequence DiagramsequenceDiagram
participant Step2e as Step 2e (Probe)
participant Subprocess as Subprocess/CCR Provider
participant Tools as JSON-RPC tools/list
participant Step4 as Step 4 (Completion)
Step2e->>Subprocess: Spawn newly-added MCP server
Subprocess->>Tools: Handle JSON-RPC initialize
Tools->>Subprocess: Return initialize response
Subprocess->>Tools: Handle tools/list request
Tools->>Subprocess: Return tools list with identity, health_check
Subprocess->>Step2e: Respond with tool list
Step2e->>Step2e: Verify identity and health_check present
Step2e->>Step4: Store probe pass/fail counts
Step4->>Step4: Render Slot probe summary (pass/fail/repair guidance)
Step4->>Step4: Conditional restart instruction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/unified-mcp-server.mjs (2)
772-774:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the
health_check_argsgate to match unconditional registration.The
health_checktool is now always registered (lines 180-186), with documentation stating execution defaults to['--version']whenhealth_check_argsis missing. However, this dispatcher still gates execution with&& slotProvider.health_check_args, causing the tool to returnnull(unknown tool) when called on slots without that field. TherunSubprocessHealthCheckfunction already implements the default at line 601, so the gate should be removed.🐛 Proposed fix to remove the gate
- if (toolName === 'health_check' && slotProvider.health_check_args) { + if (toolName === 'health_check') { return runSubprocessHealthCheck(slotProvider); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/unified-mcp-server.mjs` around lines 772 - 774, The dispatcher currently checks "if (toolName === 'health_check' && slotProvider.health_check_args)" which prevents the always-registered "health_check" tool from executing when a slot lacks health_check_args; remove the extra gate so the condition reads only based on toolName (i.e., "if (toolName === 'health_check')") and let runSubprocessHealthCheck(handle) apply its built-in default args; update the conditional around toolName to call runSubprocessHealthCheck(slotProvider) unconditionally when toolName === 'health_check'.
788-795:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the
health_check_argsgate for CCR providers.Same issue as the subprocess branch:
health_checkis now always registered (lines 206-211) but execution is still gated. This breaks the contract that the tool defaults to['--version']whenhealth_check_argsis missing.🐛 Proposed fix to remove the gate
- if (toolName === 'health_check' && slotProvider.health_check_args) { + if (toolName === 'health_check') { const result = await runSubprocessHealthCheck(slotProvider); // Override type field to 'ccr' for clarity try { const parsed = JSON.parse(result); return JSON.stringify({ ...parsed, type: 'ccr' }); } catch (_) { return result; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/unified-mcp-server.mjs` around lines 788 - 795, The health_check execution is incorrectly gated by slotProvider.health_check_args; remove that gate so CCR providers run the health_check branch whenever toolName === 'health_check' (i.e., delete the "&& slotProvider.health_check_args" condition around the block that calls runSubprocessHealthCheck), allowing runSubprocessHealthCheck to use its default ['--version'] args when health_check_args is absent; keep the JSON parse/override logic (parsing result and forcing type: 'ccr') intact.
🧹 Nitpick comments (1)
commands/nf/link-daintree.md (1)
693-758: ⚡ Quick winProbe validates registration but not execution.
The smoke probe correctly verifies that newly-added MCP slots expose
identityandhealth_checkin their tools/list response. However, it doesn't invoke the tools—the code only sendsinitializeandtools/listmessages, nevertools/call. This means a slot can pass the probe yet fail at runtime if the tool implementation has conditional logic that rejects execution (e.g., missing configuration parameters likehealth_check_args).This scope is acceptable given the probe's stated goal (verify registration), but be aware of the gap between registration validation and execution testing.
Consider whether the probe should invoke
health_checkwith an empty args object to verify end-to-end functionality, or add a note to the success criteria documenting that the probe checks registration only (not execution).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commands/nf/link-daintree.md` around lines 693 - 758, The probe currently only sends initialize and tools/list (initReq, listReq) and therefore verifies registration but not execution; modify the probe (the PROBE_RESULT spawning block that uses spawnSync and parses stdout) to also send a tools/call request for health_check (e.g., add a callReq after listReq with jsonrpc id 3 and method 'tools/call' and params { name: 'health_check', args: {} }), parse the corresponding JSON response (id === 3) and treat a non-success or missing response as a failure for that slot; ensure the results array and downstream counts (probePassedCount/probeFailedCount) reflect failures from execution checks, and if you prefer not to execute, instead update the success message to explicitly state the probe only validates registration (not execution) and link to /nf:mcp-repair.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@bin/unified-mcp-server.mjs`:
- Around line 772-774: The dispatcher currently checks "if (toolName ===
'health_check' && slotProvider.health_check_args)" which prevents the
always-registered "health_check" tool from executing when a slot lacks
health_check_args; remove the extra gate so the condition reads only based on
toolName (i.e., "if (toolName === 'health_check')") and let
runSubprocessHealthCheck(handle) apply its built-in default args; update the
conditional around toolName to call runSubprocessHealthCheck(slotProvider)
unconditionally when toolName === 'health_check'.
- Around line 788-795: The health_check execution is incorrectly gated by
slotProvider.health_check_args; remove that gate so CCR providers run the
health_check branch whenever toolName === 'health_check' (i.e., delete the "&&
slotProvider.health_check_args" condition around the block that calls
runSubprocessHealthCheck), allowing runSubprocessHealthCheck to use its default
['--version'] args when health_check_args is absent; keep the JSON
parse/override logic (parsing result and forcing type: 'ccr') intact.
---
Nitpick comments:
In `@commands/nf/link-daintree.md`:
- Around line 693-758: The probe currently only sends initialize and tools/list
(initReq, listReq) and therefore verifies registration but not execution; modify
the probe (the PROBE_RESULT spawning block that uses spawnSync and parses
stdout) to also send a tools/call request for health_check (e.g., add a callReq
after listReq with jsonrpc id 3 and method 'tools/call' and params { name:
'health_check', args: {} }), parse the corresponding JSON response (id === 3)
and treat a non-success or missing response as a failure for that slot; ensure
the results array and downstream counts (probePassedCount/probeFailedCount)
reflect failures from execution checks, and if you prefer not to execute,
instead update the success message to explicitly state the probe only validates
registration (not execution) and link to /nf:mcp-repair.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e22226c-68e2-4e40-94ab-858887769f94
📒 Files selected for processing (3)
.secrets.baselinebin/unified-mcp-server.mjscommands/nf/link-daintree.md
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent Daintree-imported MCP slots from silently missing the health_check tool by (1) always registering health_check for subprocess and ccr slot mode in unified-mcp-server, and (2) adding a “smoke probe” step to /nf:link-daintree docs so newly-added slots are verified (via initialize + tools/list) before users restart Claude Code.
Changes:
- Always register
health_checkin slot-mode tool listings forsubprocessandccrproviders. - Add Step 2e documentation to out-of-band probe newly-added MCP slots for
identity+health_check. - Refresh
.secrets.baselineline-number references andgenerated_attimestamp.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
commands/nf/link-daintree.md |
Documents a new Step 2e smoke-probe workflow and updates the closing summary messaging. |
bin/unified-mcp-server.mjs |
Removes conditional registration gates so health_check is always listed for subprocess/ccr slot mode. |
.secrets.baseline |
Updates baseline metadata (line numbers + generated timestamp) after code shifts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tency
Copilot flagged the block-style `{if condition:}` placeholder as inconsistent
with the inline `{cond ? "..." : "..."}` pattern used throughout the rest of
the file. Rewriting inline so implementers read one consistent rendering form.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Closes a gap where
/nf:link-daintreecould write Daintree-imported slots to~/.claude.json/providers.jsonbut the resulting MCP slots silently failed to exposehealth_check, with no signal until the user manually ran/nf:mcp-statusafter a Claude Code restart.bin/unified-mcp-server.mjs— drop theif (provider.health_check_args)registration gate forsubprocessandccrprovider types. Execution already defaults to['--version']at line 603, so the gate was silently stripping the tool wheneverproviders.jsonwas reconstructed without that field (which is what happens afterinstall.jsregenerates it from~/.claude.jsonmcpServers).commands/nf/link-daintree.md— add Step 2e: Smoke-probe newly-added slots. For each new MCP slot, spawnunified-mcp-serverout-of-band with its exact command/args/env, sendinitialize+tools/list, and verify bothidentityandhealth_checkare exposed. Per-slot pass/fail surfaces inline and aggregates into the Step 4 closing summary; Step 4 now tells the user explicitly to restart Claude Code (since MCP tools register only at session start)..secrets.baseline— refresh line-number references for two pre-existing baselined hashes that shifted by 2 lines (486→484, 916→914) due to the gate removal. Hashes unchanged.Supersedes #157 (which had bundled unrelated stale commits from
feature/link-daintree-copilot).Test plan
npm run test:ci— 264 pass, 1 skip, 0 failnpm run check:assets— cleannpm run lint:isolation— cleandetect-secrets scan --baseline .secrets.baseline— clean against committed baselineunified-mcp-serverwithPROVIDER_SLOT=claude-1(vanilla) andPROVIDER_SLOT=claude-z-ai(Daintree-imported) —health_checkappears in bothtools/listoutputs/nf:link-daintreeto re-add Daintree slots; verify Step 2e probe reports✓per slot and Step 4 surfaces the restart banner🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation