Make doctor prove a registered MCP command is a healthy CommitLore server - #636
Open
MongLong0214 wants to merge 20 commits into
Open
Make doctor prove a registered MCP command is a healthy CommitLore server#636MongLong0214 wants to merge 20 commits into
MongLong0214 wants to merge 20 commits into
Conversation
CommitLore — record lintTrailers: clean — 19 commits in Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
A registered MCP entry proves only that a host has a command string to launch. It does not prove that the command starts CommitLore or exposes its capture tools. Ruled-out: a separate MCP protocol probe | reuse the #595 implementation to prevent drift Blast: module Undo: easy Certainty: firm Record-Id: r-mcpid572 Provenance: authored Verified: npx tsc --noEmit; npx vitest run test/doctor.test.ts test/installer-hosts.test.ts; npx vitest run test/doctor.test.ts -t 'unattended capture initiator'; npx vitest run test/installer-hosts.test.ts; docker linux/amd64 npm ci && npm run build; find dist -type f | wc -l
The harness stub was a fake one-line hook response that the old presence-only check could not distinguish from a real command. The MCP probe caught it, so the test harness now delegates to the real CLI available to the test file. Ruled-out: retaining the fixed one-line stub | it is not an MCP server and the new probe correctly rejects it Blast: local Undo: easy Certainty: firm Record-Id: r-stub572 Provenance: authored Verified: npx tsc --noEmit Verified: npx vitest run test/init.test.ts Verified: npx vitest run test/doctor.test.ts test/installer-hosts.test.ts Verified: npx vitest run test/init.test.ts -t 'creates a portable registration|authorises unattended capture' Verified: git diff --stat -- src/
Merging main brought in the canonical-artifact check from #605, which this branch's macOS-built dist does not satisfy. Rebuilt with the documented builder and regenerated the manifest so the committed bytes are the ones that command produces. Provenance: authored Record-Id: r-canon572 Verified: docker linux/amd64 npm ci, npm run build, npm run artifact:manifest; artifact:verify reports 34e5f551; 277 files in dist; 101 cases across init and doctor
The synchronous wrapper was waiting for the helper to exit after its stdout verdict. The helper only sent SIGTERM to the MCP child, so a server that stayed alive kept the helper, and therefore spawnSync, alive until its outer timeout. Terminate the isolated probe process group with a bounded SIGTERM-to-SIGKILL sequence, then flush the verdict and exit the helper explicitly. The helper's protocol timeout now remains distinct from a failure to run the probe. Ruled-out: treating stdout as completion | spawnSync waits for helper exit after stdout Blast: module Undo: easy Certainty: firm Verified: container npm ci && npm run build && npm run artifact:manifest; container npm ci && npx vitest run test/init.test.ts test/doctor.test.ts; container npx vitest run test/init.test.ts; container npx vitest run test/doctor.test.ts; local npx tsc --noEmit; local npm run artifact:verify Record-Id: r-probesync572 Provenance: authored
Require the registered MCP server to advertise the three capture tools as well as its read tools. A read-only server remains a distinct healthy delivery result, while the doctor report names asset readiness as an unverified gap. Ruled-out: asset readiness preflight | F-002 and #633/#635 own that separate runtime check Blast: module Undo: easy Certainty: firm Verified: container npx vitest run test/doctor.test.ts (78 passed) Verified: container npx vitest run test/init.test.ts (24 passed) Verified: container npx vitest run test/doctor.test.ts test/init.test.ts (102 passed) Verified: container npx vitest run test/init.test.ts test/doctor.test.ts (102 passed) Verified: container npx vitest run test/installer-hosts.test.ts (12 passed) Verified: container npx tsc --noEmit; macOS npm run artifact:verify; dist file count 277 Record-Id: r-initiator636 Provenance: authored
The close-stdin case asserted that either command-closed-input or initialize-timed-out was acceptable, on the reasoning that scheduling decides which one the probe observes. Measured on linux/amd64, it does not: the shell reaches exec 0<&- before the parent's write lands, and the outcome was command-closed-input 10 times out of 10. An assertion that accepts either code stops distinguishing two different facts. "It hung up" and "it is listening and silent" are not the same diagnosis, and doctor exists to tell them apart. A regression that tolerates both would pass while the probe collapsed one into the other. Each fact now has its own fixture: a command that closes its input and stays alive, and a command that keeps its input open and never answers. The second takes the full probe budget, which is what makes it the timeout case. Limit: the close-stdin outcome is measured, not guaranteed by the shell -- a platform whose shell defers the redirect past the parent's first write would produce the timeout code instead, and this case would then need its own seam rather than a looser assertion Ruled-out: accepting either code in one case | it makes the regression pass whether or not the probe still separates the two facts, which is the property under test Blast: local Undo: easy Certainty: firm Provenance: authored Record-Id: r-probefacts572 Verified: 10/10 runs of the close-stdin fixture through dist/core/mcp-probe.js on linux/amd64 report command closed its input; five initiator cases pass in-container with one exact probe code each, the never-answers fixture taking 6.2s against a 5s budget; tsc clean
Declaring a single runtime asset and then executing a second file is a contract the product breaks against itself. Ruled-out: sidecar MCP helper | it is outside the declared single-file runtime Blast: module Undo: easy Certainty: firm Record-Id: r-bundleprobe636 Provenance: authored Verified: npx tsc --noEmit; npx vitest run test/doctor.test.ts; npx vitest run test/init.test.ts; npx vitest run test/doctor.test.ts test/init.test.ts; npx vitest run test/installer-hosts.test.ts; Linux node:24-bookworm doctor and init suite; npm run artifact:verify
A stat of one extensionless candidate is not a search of PATH on Windows. Terminating a shim is not terminating what the shim started. Ruled-out: process manager framework | taskkill is a bounded Windows helper Blast: module Undo: easy Certainty: firm Record-Id: r-winprobe640 Provenance: authored Verified: npx tsc --noEmit; npm run artifact:verify; canonical docker build; Windows CI 31763744143 and 31764253576
PATHEXT can select a cmd shim, but Node cannot spawn that shim with shell disabled. Let Windows run the shim through its command shell so the probe reaches its MCP child.
Use the launcher's .cmd command directly now that the probe dispatches command shims through cmd. This preserves the test's actual process-tree shape while keeping the protocol streams attached to the launched server.
timeout exits immediately when its stdin is redirected by the MCP probe. A bounded loopback ping keeps the cmd wrapper alive for the protocol exchange and task-tree cleanup assertion.
Temporarily use the direct-child termination path so install-ps1 proves that the launcher descendant survives without taskkill /T.
Restore taskkill /T after the Windows cleanup control. A probe now distinguishes a reclaimed child tree from one it could not reclaim.
The CI integrity guard fingerprints ci.yml. Refresh its reviewed digest after adding the bounded Windows MCP probe checks.
The close-stdin fixture was pinned to command-closed-input on the strength of a measurement that did not reproduce the conditions it runs in. Invoked on its own the probe reports that code every time -- 5/5 on macOS, 10/10 on linux/amd64. Under the suite's load it reports initialize-timed-out instead, because the parent's write can win the race to a pipe the shell has not closed yet. Both are correct readings of what the probe observed. Which one happens is the scheduler's choice, so pinning either makes the suite flaky and pinning both would stop the pair distinguishing "it hung up" from "it is listening and silent" -- the distinction the two fixtures exist to hold apart. The case now asserts what does not move: the check warns and the initiator is unhealthy. The timeout code stays pinned by the fixture that produces it deterministically, a command that keeps its input open and never answers. Limit: this leaves one outcome unpinned, so a regression that swapped closed-input for timed-out on that fixture would not be caught here -- the four other cases still pin their codes exactly Ruled-out: accepting either code in both fixtures | that is what makes the pair meaningless, since neither would then hold its own fact Ruled-out: forcing determinism by having the fixture signal readiness before closing stdin | the probe writes immediately on spawn, so there is no point at which a fixture could report ready first without changing the protocol Blast: local Undo: easy Certainty: firm Provenance: authored Record-Id: r-proberace572 Verified: 104 cases across doctor and init pass; the probe invoked directly reports command-closed-input 5/5 on macOS and 10/10 in a linux/amd64 container, while the same fixture under the suite reports initialize-timed-out, which is the contradiction this change records Unverified: whether the race resolves differently again on a machine with more or fewer cores -- the assertion no longer depends on it either way
Provenance: authored Record-Id: r-canon572reb Verified: docker linux/amd64 npm ci + build + artifact:manifest; artifact:verify reports 31b08e7c; 277 files in dist; spec/verify.sh 32 fixtures; check-readme-numbers matches bench/report.ts
MongLong0214
force-pushed
the
fix-572-mcp-identity
branch
from
August 14, 2026 06:14
4ff6e59 to
d53b92b
Compare
The step failed at setup, not at its assertion: the probe reported initialize-timed-out against the launcher, so `the wrapper MCP server was not healthy before cleanup` threw before any cleanup was exercised. That leaves two readings — the product cannot reclaim a cmd launcher's child tree, or the fixture cannot stand one up — and the log cannot separate them, because the precondition rested on the same Windows process handling the assertion is about. The wrapper used `start "" /b` and then slept. That detaches the child, so whether the probe's `initialize` ever reached it depended on console handle inheritance through `start`. Running node in the foreground makes the protocol path unambiguous while keeping the tree identical: cmd.exe is still the probe's direct child and node is still a grandchild, which is the shape #640 is about. This does not claim the product works. It makes the next Windows run able to say which of the two readings is true: a probe that now reports ok and a child that survives cleanup means the product is incomplete; both green means the fixture was. Blast: local Undo: easy Certainty: tentative Ruled-out: weaken or delete the failing step | it is the only coverage of Windows child-tree reclamation in the repository, and no unit test touches taskkill Limit: windows-latest is the only evidence for this path; nothing here was verified locally Provenance: authored Record-Id: r-childtreepre640 Verified: python yaml parse of the workflow confirms the step and that the wrapper now runs node in the foreground with no start /b and no ping sleep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #572.
This fixes the live MCP identity check used by
doctor. It verifies that theregistered command completes MCP initialization as CommitLore and advertises
the required read tools plus
commitlore_prepare_capture,commitlore_verify_capture, andcommitlore_stage_capture.A server that has only the two read tools is reported as a healthy
read-delivery server, but not as an unattended capture initiator. Launch,
identity, and incomplete tool sets remain distinct diagnostic outcomes.
Boundary
This verifies MCP identity and advertised tool membership only. It does not
verify asset readiness or prove that
prepare_capturewill succeed: requiredassets such as
SPEC.mdmay still be missing. Asset readiness is the separateF-002 finding tracked under #633 and broader runtime convergence #635; this PR
does not close #635.
Test plan
npx vitest run test/doctor.test.tsnpx vitest run test/init.test.tsnpx vitest run test/installer-hosts.test.tsnpx tsc --noEmitdist/rebuild and canonical-artifact manifest refresh