feat(doctor): adapter-owned toolchain presentation via describeToolchain (#435, part 2) - #442
Merged
Merged
Conversation
…ain (#435, part 2) Each adapter factory now owns its doctor row through an optional IAdapterFactory.describeToolchain(validation, options?) that receives the just-computed validate() result, so the producer and consumer of every details key live in the same class - key renames are compiler-checked inside the package instead of silently blanking doctor output. - shared: ToolchainComponent/ToolchainDescription/DescribeToolchainOptions types, optional describeToolchain on IAdapterFactory, and toolchainComponent()/normalizeToolchainDescription() helpers carrying the omit-undetected and "(built-in)" stand-alone rendering rules - all nine adapter factories implement describeToolchain; dotnet/cpp absorb the former doctor-only extras probes (netcoredbg/SDK versions in parallel, compiler banner reusing the discovered command); the js-debug cell now renders only when the vendored payload was actually found (previously it showed "js-debug (vendored)" even when validate errored) - doctor: diagnose.ts calls probe.factory.describeToolchain under the remaining validate budget and normalizes the result; presenters.ts (the 19-key stringly-typed mapping + literal-specifier import switch) is deleted - no dynamic imports remain in the presentation path - JSON: schemaVersion stays 1; netcoredbgVersion/dotnetSdkVersion/ compilerVersion move from details into the typed runtime/backend cells; the never-rendered javacPath extra is dropped (java is now spawn-free) - docs: "wire the doctor command" removed from the new-adapter checklist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review findings on the adapter-owned presentation path, fixed:
- diagnose computes the verdict from a pre-call snapshot and hands
describeToolchain a defensive clone: a buggy or malicious plain-JS
factory can no longer mutate the validation object into flipping
broken->ok (and zeroing a gated run's exit code)
- withTimeout wraps in Promise.resolve: a sync non-thenable return from an
out-of-tree factory is now normalized instead of being discarded while a
leaked timer stalls the CLI
- new shared probeWithinBudget(): dotnet/cpp version probes race the
advisory options.timeoutMs (with headroom) and skip spawning entirely on
an exhausted budget, so a hung probe degrades to the detail-derived
cells instead of the caller's hard timeout blanking the whole row
- a factory without describeToolchain (older adapter package) now gets a
display-only version-skew warning instead of silently empty cells; the
verdict still stands on validate() alone
- MockAdapterFactory.describeToolchain declares the validation parameter
(strict-mode arity)
- '('-standalone label convention documented on ToolchainComponent.label
and the describeToolchain JSDoc
- cpp/dotnet factory-test mocks spread importOriginal so unrelated exports
stay real (missing-export trap)
- findJavacExecutable deleted (its only caller was the removed extras
path; unreleased export) along with its tests
- js vendored-badge gate simplified to truthiness (the shared helper
already enforces the non-empty-string rule)
- shared README documents the new doctor-presentation exports
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
Part 2 of #435 (part 1 landed in #437): doctor presentation moves out of the CLI and into the adapter packages.
What changed
New optional
IAdapterFactory.describeToolchain(validation, options?)(@debugmcp/shared): each adapter renders its own doctor runtime/backend row from thevalidate()result it just produced. The stringly-typeddetailskeys are now produced and consumed inside the same class in the same file, behind a package-local type alias — renaming a key is compiler-checked where it matters instead of silently blanking doctor output. The member is optional, so out-of-tree/older factories keep working (doctor shows empty cells),isAdapterFactoryand all plain-object test fakes stay valid.Shared rendering rules move to
toolchainComponent()/normalizeToolchainDescription()in@debugmcp/shared: a cell renders only when something was actually detected (a bare label would make an absent toolchain read as present);(built-in)style labels stand alone; the CLI defensively normalizes whatever a plain-JS factory returns.presenters.tsis deleted — the 19-keypresentLanguageswitch,collectDoctorExtras, and the literal-specifier dynamic-import switch (the esbuild bundling workaround) are all gone. Doctor reaches the presentation throughprobe.factory, which it already holds; no dynamic imports remain in the presentation path. The former extras probes now live in their own packages: dotnet runsgetNetcoredbgVersion+getDotnetSdkVersionin parallel; cpp reuses the validate()-discovered compiler command for the banner probe.describeToolchainruns under the same remaining-validate-budget timeout the extras used, withprobe.timedOutstill feeding the handler's force-exit containment.The "wire the doctor command" step is removed from the new-adapter checklist in CLAUDE.md — a new adapter's doctor row ships inside the adapter.
Deliberate output changes
js-debug (vendored)even whenvalidate()errored "js-debug adapter not found" (the CLI hardcoded it). It now renders only when the vendored payload was actually found.netcoredbgVersion/dotnetSdkVersion/compilerVersionmove from the--jsondetailsbag into the typedruntime/backendcells (detailsis now the rawvalidate()output, verbatim).schemaVersionstays 1 — the row shapes are supersets of the old ones.javacPathextra is dropped entirely: it was collected on every doctor run but rendered nowhere (only visible by spelunking--json). Java's presentation is now spawn-free;findJavacExecutableremains exported.Tests
presenters.test.tsinto each adapter package's factory tests (new test files for rust and cpp, which had none); dotnet pins "no probe without a debugger path", cpp pins command-reuse and banner-vs-bare-command.diagnose.test.ts: extras tests rewritten asdescribeToolchainequivalents (hang →probe.timedOut, slow-import budget exclusion, duration accounting, rows carried intoruntime/backend, throw keeps the verdict) plus new cases: factory without the method → empty cells; malformed return → normalized;valid:falsestill described.packages/shared/tests/unit/toolchain-description.test.ts.(built-in)cells — deterministic proof adapter-owned presentation survives the real built bundle.server-doctor-parity.test.ts) unchanged apart from dropping the removedcollectExtrasinjection.Verified locally: full build, lint, 222-file/4017-test unit suite, doctor e2e smoke, and a real
mcp-debugger doctorrun rendering all nine adapter-owned rows (including the relocated dotnet/cpp probes).Closes nothing on its own — #435 stays open for parts 3–4 (launch gate through the shared probe; typed registry surface), which follow in the next PR.
🤖 Generated with Claude Code