From 094ae190e5affde351a2f0a473ddaa0bc9291ceb Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 22:10:16 -0400 Subject: [PATCH 1/2] feat(doctor): adapter-owned toolchain presentation via describeToolchain (#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 --- CLAUDE.md | 7 +- .../adapter-cpp/src/cpp-adapter-factory.ts | 67 +++++- .../tests/unit/cpp-adapter-factory.test.ts | 114 ++++++++++ .../src/DotnetAdapterFactory.ts | 52 ++++- .../tests/unit/dotnet-adapter-factory.test.ts | 68 +++++- packages/adapter-go/src/go-adapter-factory.ts | 48 +++- .../adapter-java/src/java-adapter-factory.ts | 47 +++- .../src/javascript-adapter-factory.ts | 46 +++- ...avascript-adapter-factory.validate.test.ts | 45 ++++ .../adapter-mock/src/mock-adapter-factory.ts | 18 +- .../tests/unit/mock-adapter-factory.test.ts | 15 ++ .../src/python-adapter-factory.ts | 45 +++- .../tests/unit/python-adapter-factory.test.ts | 53 +++++ .../adapter-ruby/src/ruby-adapter-factory.ts | 47 +++- .../tests/unit/ruby-adapter-factory.test.ts | 40 ++++ .../adapter-rust/src/rust-adapter-factory.ts | 56 ++++- .../tests/unit/rust-adapter-factory.test.ts | 63 ++++++ packages/shared/src/index.ts | 9 +- .../shared/src/interfaces/adapter-registry.ts | 50 +++++ .../shared/src/utils/toolchain-description.ts | 73 ++++++ .../tests/unit/toolchain-description.test.ts | 97 ++++++++ src/cli/commands/doctor/diagnose.ts | 64 +++--- src/cli/commands/doctor/presenters.ts | 208 ------------------ src/utils/language-availability.ts | 8 +- .../go/unit/go-adapter-factory.test.ts | 47 +++- .../java/unit/java-adapter-factory.test.ts | 44 ++++ .../unit/server/server-doctor-parity.test.ts | 3 +- tests/e2e/doctor-smoke.test.ts | 4 + tests/unit/cli/doctor/diagnose.test.ts | 158 +++++++++---- tests/unit/cli/doctor/presenters.test.ts | 190 ---------------- 30 files changed, 1222 insertions(+), 564 deletions(-) create mode 100644 packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts create mode 100644 packages/adapter-rust/tests/unit/rust-adapter-factory.test.ts create mode 100644 packages/shared/src/utils/toolchain-description.ts create mode 100644 packages/shared/tests/unit/toolchain-description.test.ts delete mode 100644 src/cli/commands/doctor/presenters.ts delete mode 100644 tests/unit/cli/doctor/presenters.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 321b1a97..f394f821 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -309,14 +309,13 @@ When debugging issues: To add support for a new language: 1. **Create Package**: Add new package under `packages/adapter-{language}/` -2. **Implement Interfaces**: Implement `IAdapterFactory` and `IDebugAdapter` from `@debugmcp/shared` +2. **Implement Interfaces**: Implement `IAdapterFactory` and `IDebugAdapter` from `@debugmcp/shared`. Optionally implement `describeToolchain()` on the factory (using `toolchainComponent` from `@debugmcp/shared`) so `mcp-debugger doctor` renders the adapter's runtime/backend row — without it the doctor table shows empty cells 3. **Export Factory**: Export a factory class named `{Language}AdapterFactory` 4. **Register in root `package.json`**: Add `"@debugmcp/adapter-{language}": "workspace:*"` to `optionalDependencies` 5. **Add Vitest alias**: Add `{ find: '@debugmcp/adapter-{language}', replacement: path.resolve(__dirname, './packages/adapter-{language}/src/index.ts') }` to `resolve.alias` in `vitest.config.ts` 6. **Update adapter count**: Update hardcoded adapter counts in tests (`adapter-loader.test.ts`, `models.test.ts`, `tests/e2e/doctor-smoke.test.ts`) -7. **Wire the doctor command**: Add the language's runtime/backend column mapping to `presentLanguage` in `src/cli/commands/doctor/presenters.ts` (and `collectDoctorExtras` if it has doctor-only probes) -8. **Add Tests**: Include unit and integration tests in the package -9. **Run `pnpm install`**: To link the new workspace package +7. **Add Tests**: Include unit and integration tests in the package +8. **Run `pnpm install`**: To link the new workspace package Example structure: ``` diff --git a/packages/adapter-cpp/src/cpp-adapter-factory.ts b/packages/adapter-cpp/src/cpp-adapter-factory.ts index fd240642..9cb9de04 100644 --- a/packages/adapter-cpp/src/cpp-adapter-factory.ts +++ b/packages/adapter-cpp/src/cpp-adapter-factory.ts @@ -5,11 +5,27 @@ * Implements the adapter factory interface for dependency injection. */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { CppDebugAdapter } from './cpp-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { resolveCodeLLDBExecutableWithSource, getCodeLLDBVersion } from '@debugmcp/codelldb-common'; -import { findAnyCompiler } from './utils/compile-utils.js'; +import { findAnyCompiler, getCompilerInfo } from './utils/compile-utils.js'; + +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type CppToolchainDetails = { + codelldbPath?: string; + codelldbVersion?: string; + codelldbSource?: string; + compiler?: string; + platform: string; + arch: string; + timestamp: string; +}; /** * Factory for creating C/C++ debug adapters @@ -70,19 +86,48 @@ export class CppAdapterFactory implements IAdapterFactory { compiler = foundCompiler; } + const details: CppToolchainDetails = { + codelldbPath, + codelldbVersion, + codelldbSource, + compiler, + platform: process.platform, + arch: process.arch, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - codelldbPath, - codelldbVersion, - codelldbSource, - compiler, - platform: process.platform, - arch: process.arch, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): reuses the validate()-discovered compiler + * command instead of re-probing the whole candidate list; the --version + * banner already names the command, so the bare command only shows when no + * banner was captured. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + let compilerVersion: string | undefined; + if (details.compiler) { + const info = await getCompilerInfo(details.compiler).catch(() => null); + compilerVersion = info?.version ?? undefined; + } + return { + runtime: toolchainComponent({ + label: 'C/C++ compiler', + path: compilerVersion ? undefined : details.compiler, + version: compilerVersion + }), + backend: toolchainComponent({ + label: 'CodeLLDB', + path: details.codelldbPath, + version: details.codelldbVersion, + source: details.codelldbSource + }) }; } } diff --git a/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts b/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts new file mode 100644 index 00000000..d358894d --- /dev/null +++ b/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts @@ -0,0 +1,114 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { DebugLanguage } from '@debugmcp/shared'; +import { CppAdapterFactory } from '../../src/cpp-adapter-factory.js'; +import { getCompilerInfo } from '../../src/utils/compile-utils.js'; + +vi.mock('../../src/utils/compile-utils.js', () => ({ + findAnyCompiler: vi.fn(), + getCompilerInfo: vi.fn() +})); + +const getCompilerInfoMock = vi.mocked(getCompilerInfo); + +const validation = (details: Record) => ({ + valid: true, + errors: [], + warnings: [], + details +}); + +describe('CppAdapterFactory', () => { + it('returns accurate adapter metadata', () => { + const metadata = new CppAdapterFactory().getMetadata(); + + expect(metadata).toMatchObject({ + language: DebugLanguage.CPP, + displayName: 'C/C++', + modes: { launch: true, attach: 'spawn' } + }); + }); +}); + +describe('CppAdapterFactory.describeToolchain', () => { + beforeEach(() => { + vi.clearAllMocks(); + getCompilerInfoMock.mockReset(); + }); + + it('reuses the validate()-discovered compiler command and shows its version banner', async () => { + getCompilerInfoMock.mockResolvedValue({ command: 'g++', version: 'g++ (GCC) 13.2.0' }); + + const description = await new CppAdapterFactory().describeToolchain( + validation({ + codelldbPath: '/opt/codelldb/adapter/codelldb', + codelldbVersion: '1.11.5', + codelldbSource: 'platform-package', + compiler: 'g++', + platform: 'linux', + arch: 'x64', + timestamp: 'now' + }) + ); + + expect(getCompilerInfoMock).toHaveBeenCalledWith('g++'); + expect(description).toEqual({ + runtime: { label: 'C/C++ compiler', version: 'g++ (GCC) 13.2.0' }, + backend: { + label: 'CodeLLDB', + path: '/opt/codelldb/adapter/codelldb', + version: '1.11.5', + source: 'platform-package' + } + }); + }); + + it('falls back to the bare command when no version banner was captured', async () => { + getCompilerInfoMock.mockResolvedValue({ command: 'g++', version: null }); + + const description = await new CppAdapterFactory().describeToolchain( + validation({ compiler: 'g++' }) + ); + + expect(description).toEqual({ + runtime: { label: 'C/C++ compiler', path: 'g++' } + }); + }); + + it('falls back to the bare command when the banner probe fails outright', async () => { + getCompilerInfoMock.mockRejectedValue(new Error('spawn failed')); + + const description = await new CppAdapterFactory().describeToolchain( + validation({ compiler: 'g++' }) + ); + + expect(description).toEqual({ + runtime: { label: 'C/C++ compiler', path: 'g++' } + }); + }); + + it('does not probe at all when validate() found no compiler', async () => { + const description = await new CppAdapterFactory().describeToolchain( + validation({ + codelldbPath: '/opt/codelldb/adapter/codelldb', + codelldbVersion: '1.11.5', + codelldbSource: 'vendored' + }) + ); + + expect(getCompilerInfoMock).not.toHaveBeenCalled(); + expect(description).toEqual({ + backend: { + label: 'CodeLLDB', + path: '/opt/codelldb/adapter/codelldb', + version: '1.11.5', + source: 'vendored' + } + }); + }); + + it('renders empty cells when validate() produced no details', async () => { + expect( + await new CppAdapterFactory().describeToolchain({ valid: false, errors: [], warnings: [] }) + ).toEqual({}); + }); +}); diff --git a/packages/adapter-dotnet/src/DotnetAdapterFactory.ts b/packages/adapter-dotnet/src/DotnetAdapterFactory.ts index e9f244d7..d290c12e 100644 --- a/packages/adapter-dotnet/src/DotnetAdapterFactory.ts +++ b/packages/adapter-dotnet/src/DotnetAdapterFactory.ts @@ -7,10 +7,23 @@ * @since 0.2.0 */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { DotnetDebugAdapter } from './DotnetDebugAdapter.js'; import { DebugLanguage } from '@debugmcp/shared'; -import { findNetcoredbgExecutable } from './utils/dotnet-utils.js'; +import { findNetcoredbgExecutable, getNetcoredbgVersion, getDotnetSdkVersion } from './utils/dotnet-utils.js'; + +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type DotnetToolchainDetails = { + debuggerPath?: string; + backend: string; + platform: string; + timestamp: string; +}; /** * Factory for creating .NET debug adapters @@ -56,16 +69,39 @@ export class DotnetAdapterFactory implements IAdapterFactory { errors.push(error instanceof Error ? error.message : 'netcoredbg not found'); } + const details: DotnetToolchainDetails = { + debuggerPath, + backend: 'netcoredbg', + platform: process.platform, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - debuggerPath, - backend: 'netcoredbg', - platform: process.platform, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): the version probes that used to live in the + * doctor CLI's extras path run here instead, in parallel and best-effort — + * a failed probe just leaves its cell field empty. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + const debuggerPath = details.debuggerPath; + const [netcoredbgVersion, sdkVersion] = await Promise.all([ + debuggerPath ? getNetcoredbgVersion(debuggerPath).catch(() => null) : Promise.resolve(null), + getDotnetSdkVersion().catch(() => null) + ]); + return { + runtime: toolchainComponent({ label: '.NET SDK', version: sdkVersion ?? undefined }), + backend: toolchainComponent({ + label: 'netcoredbg', + path: debuggerPath, + version: netcoredbgVersion ?? undefined + }) }; } } diff --git a/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts b/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts index 85595053..3f7f74e7 100644 --- a/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts +++ b/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts @@ -3,15 +3,23 @@ import type { AdapterDependencies } from '@debugmcp/shared'; import { DebugLanguage } from '@debugmcp/shared'; import { DotnetAdapterFactory } from '../../src/DotnetAdapterFactory.js'; import { DotnetDebugAdapter } from '../../src/DotnetDebugAdapter.js'; -import { findNetcoredbgExecutable } from '../../src/utils/dotnet-utils.js'; +import { + findNetcoredbgExecutable, + getNetcoredbgVersion, + getDotnetSdkVersion +} from '../../src/utils/dotnet-utils.js'; vi.mock('../../src/utils/dotnet-utils.js', () => ({ findNetcoredbgExecutable: vi.fn(), findDotnetBackend: vi.fn(), - listDotnetProcesses: vi.fn() + listDotnetProcesses: vi.fn(), + getNetcoredbgVersion: vi.fn(), + getDotnetSdkVersion: vi.fn() })); const findNetcoredbgExecutableMock = vi.mocked(findNetcoredbgExecutable); +const getNetcoredbgVersionMock = vi.mocked(getNetcoredbgVersion); +const getDotnetSdkVersionMock = vi.mocked(getDotnetSdkVersion); const createDependencies = (): AdapterDependencies => ({ fileSystem: {} as unknown, @@ -80,3 +88,59 @@ describe('DotnetAdapterFactory', () => { expect(result.errors).toContain('netcoredbg not found'); }); }); + +describe('DotnetAdapterFactory.describeToolchain', () => { + beforeEach(() => { + vi.clearAllMocks(); + getNetcoredbgVersionMock.mockReset(); + getDotnetSdkVersionMock.mockReset(); + }); + + const validation = (details: Record) => ({ + valid: true, + errors: [], + warnings: [], + details + }); + + it('probes netcoredbg and SDK versions and renders both cells', async () => { + getNetcoredbgVersionMock.mockResolvedValue('3.1.2-1054'); + getDotnetSdkVersionMock.mockResolvedValue('8.0.401'); + + const description = await new DotnetAdapterFactory().describeToolchain( + validation({ debuggerPath: '/path/to/netcoredbg', backend: 'netcoredbg', platform: 'linux', timestamp: 'now' }) + ); + + expect(getNetcoredbgVersionMock).toHaveBeenCalledWith('/path/to/netcoredbg'); + expect(description).toEqual({ + runtime: { label: '.NET SDK', version: '8.0.401' }, + backend: { label: 'netcoredbg', path: '/path/to/netcoredbg', version: '3.1.2-1054' } + }); + }); + + it('skips the netcoredbg probe when validate() found no debugger path', async () => { + getDotnetSdkVersionMock.mockResolvedValue('8.0.401'); + + const description = await new DotnetAdapterFactory().describeToolchain( + validation({ backend: 'netcoredbg', platform: 'linux', timestamp: 'now' }) + ); + + expect(getNetcoredbgVersionMock).not.toHaveBeenCalled(); + expect(description).toEqual({ + runtime: { label: '.NET SDK', version: '8.0.401' } + }); + }); + + it('degrades gracefully when the probes fail', async () => { + getNetcoredbgVersionMock.mockRejectedValue(new Error('spawn failed')); + getDotnetSdkVersionMock.mockRejectedValue(new Error('spawn failed')); + + const description = await new DotnetAdapterFactory().describeToolchain( + validation({ debuggerPath: '/path/to/netcoredbg' }) + ); + + expect(description).toEqual({ + backend: { label: 'netcoredbg', path: '/path/to/netcoredbg' } + }); + }); +}); diff --git a/packages/adapter-go/src/go-adapter-factory.ts b/packages/adapter-go/src/go-adapter-factory.ts index 6d47624a..e412855c 100644 --- a/packages/adapter-go/src/go-adapter-factory.ts +++ b/packages/adapter-go/src/go-adapter-factory.ts @@ -7,11 +7,27 @@ * @since 0.1.0 */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { GoDebugAdapter } from './go-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { findGoExecutable, findDelveExecutable, getGoVersion, getDelveVersion, checkDelveDapSupport } from './utils/go-utils.js'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type GoToolchainDetails = { + goPath?: string; + goVersion?: string; + dlvPath?: string; + dlvVersion?: string; + platform: string; + arch: string; + timestamp: string; +}; + /** * Factory for creating Go debug adapters */ @@ -87,19 +103,31 @@ export class GoAdapterFactory implements IAdapterFactory { errors.push(error instanceof Error ? error.message : 'Go executable not found'); } + const details: GoToolchainDetails = { + goPath, + goVersion, + dlvPath, + dlvVersion, + platform: process.platform, + arch: process.arch, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - goPath, - goVersion, - dlvPath, - dlvVersion, - platform: process.platform, - arch: process.arch, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Go', path: details.goPath, version: details.goVersion }), + backend: toolchainComponent({ label: 'Delve', path: details.dlvPath, version: details.dlvVersion }) }; } } diff --git a/packages/adapter-java/src/java-adapter-factory.ts b/packages/adapter-java/src/java-adapter-factory.ts index 3d959ebf..f1f04665 100644 --- a/packages/adapter-java/src/java-adapter-factory.ts +++ b/packages/adapter-java/src/java-adapter-factory.ts @@ -5,12 +5,27 @@ * Uses JDI bridge (JdiDapServer) as the underlying DAP server. */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { JavaDebugAdapter } from './java-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { findJavaExecutable, getJavaVersion } from './utils/java-utils.js'; import { resolveJdiBridgeClassDir } from './utils/jdi-resolver.js'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type JavaToolchainDetails = { + javaPath?: string; + javaVersion?: string; + jdiBridgeDir?: string; + platform: string; + arch: string; + timestamp: string; +}; + /** * Factory for creating Java debug adapters */ @@ -78,18 +93,32 @@ export class JavaAdapterFactory implements IAdapterFactory { errors.push('Java not found. Install JDK 21+ from https://adoptium.net/'); } + const details: JavaToolchainDetails = { + javaPath, + javaVersion, + jdiBridgeDir, + platform: process.platform, + arch: process.arch, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - javaPath, - javaVersion, - jdiBridgeDir, - platform: process.platform, - arch: process.arch, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details — no + * extra probes (the old doctor-only javac lookup was never rendered and + * was dropped with the extras mechanism). + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Java', path: details.javaPath, version: details.javaVersion }), + backend: toolchainComponent({ label: 'JDI bridge', path: details.jdiBridgeDir }) }; } } diff --git a/packages/adapter-javascript/src/javascript-adapter-factory.ts b/packages/adapter-javascript/src/javascript-adapter-factory.ts index 13194280..e491adcd 100644 --- a/packages/adapter-javascript/src/javascript-adapter-factory.ts +++ b/packages/adapter-javascript/src/javascript-adapter-factory.ts @@ -9,9 +9,11 @@ import type { IDebugAdapter } from '@debugmcp/shared'; import { AdapterFactory as BaseAdapterFactory, + toolchainComponent, type AdapterDependencies, type AdapterMetadata, - type FactoryValidationResult + type FactoryValidationResult, + type ToolchainDescription } from '@debugmcp/shared'; import { JavascriptDebugAdapter } from './javascript-debug-adapter.js'; import { resolveJsDebugServer } from './utils/js-debug-resolver.js'; @@ -19,6 +21,18 @@ import fs from 'fs'; import path from 'path'; import { fileURLToPath } from 'url'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type JavascriptToolchainDetails = { + nodeVersion: string; + vendorPathChecked: string | null; + tsxFound: boolean; + tsNodeFound: boolean; +}; + const metadata: AdapterMetadata = { language: 'javascript', displayName: 'JavaScript/TypeScript', @@ -116,16 +130,34 @@ export class JavascriptAdapterFactory extends BaseAdapterFactory { warnings.push('No TypeScript runner found. Install tsx or ts-node for TS debugging'); } + const details: JavascriptToolchainDetails = { + nodeVersion, + vendorPathChecked: vendorPath, + tsxFound, + tsNodeFound + }; return { valid: errors.length === 0, errors, warnings, - details: { - nodeVersion, - vendorPathChecked: vendorPath, - tsxFound, - tsNodeFound - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details. The + * js-debug cell only renders when the vendored payload was actually found — + * a hardcoded 'vendored' badge used to show even when validate() errored + * "js-debug adapter not found". + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Node.js', version: details.nodeVersion }), + backend: toolchainComponent({ + label: 'js-debug', + source: typeof details.vendorPathChecked === 'string' && details.vendorPathChecked.length > 0 ? 'vendored' : undefined + }) }; } diff --git a/packages/adapter-javascript/tests/unit/javascript-adapter-factory.validate.test.ts b/packages/adapter-javascript/tests/unit/javascript-adapter-factory.validate.test.ts index b1bc3fa4..589394f9 100644 --- a/packages/adapter-javascript/tests/unit/javascript-adapter-factory.validate.test.ts +++ b/packages/adapter-javascript/tests/unit/javascript-adapter-factory.validate.test.ts @@ -137,3 +137,48 @@ describe('JavascriptAdapterFactory.validate', () => { expect(res.warnings).not.toContain('No TypeScript runner found. Install tsx or ts-node for TS debugging'); }); }); + +describe('JavascriptAdapterFactory.describeToolchain', () => { + const validation = (details: Record) => ({ + valid: true, + errors: [], + warnings: [], + details + }); + + it('renders Node.js and the vendored js-debug cells from its own validate() details', async () => { + const description = await new JavascriptAdapterFactory().describeToolchain( + validation({ + nodeVersion: 'v22.4.0', + vendorPathChecked: '/pkg/vendor/js-debug/vsDebugServer.js', + tsxFound: true, + tsNodeFound: false + }) + ); + + expect(description).toEqual({ + runtime: { label: 'Node.js', version: 'v22.4.0' }, + backend: { label: 'js-debug', source: 'vendored' } + }); + }); + + it('hides the js-debug cell when the vendored payload was not found (regression: cell rendered even when validate errored)', async () => { + const description = await new JavascriptAdapterFactory().describeToolchain( + validation({ nodeVersion: 'v22.4.0', vendorPathChecked: null, tsxFound: false, tsNodeFound: false }) + ); + + expect(description).toEqual({ + runtime: { label: 'Node.js', version: 'v22.4.0' } + }); + }); + + it('renders empty cells when validate() produced no details', async () => { + const description = await new JavascriptAdapterFactory().describeToolchain({ + valid: false, + errors: [], + warnings: [] + }); + + expect(description).toEqual({}); + }); +}); diff --git a/packages/adapter-mock/src/mock-adapter-factory.ts b/packages/adapter-mock/src/mock-adapter-factory.ts index 53631dad..c9491a19 100644 --- a/packages/adapter-mock/src/mock-adapter-factory.ts +++ b/packages/adapter-mock/src/mock-adapter-factory.ts @@ -5,12 +5,13 @@ * * @since 2.0.0 */ -import type { - IAdapterFactory, +import type { + IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, - IDebugAdapter + IDebugAdapter, + ToolchainDescription } from '@debugmcp/shared'; import { MockDebugAdapter, MockAdapterConfig } from './mock-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; @@ -70,6 +71,17 @@ export class MockAdapterFactory implements IAdapterFactory { } }; } + + /** + * Doctor row (issue #435): nothing external to detect — the standalone + * '(built-in)' labels render by design. + */ + async describeToolchain(): Promise { + return { + runtime: { label: '(built-in)' }, + backend: { label: '(built-in)' } + }; + } } /** diff --git a/packages/adapter-mock/tests/unit/mock-adapter-factory.test.ts b/packages/adapter-mock/tests/unit/mock-adapter-factory.test.ts index a6e83ad2..ad639b9a 100644 --- a/packages/adapter-mock/tests/unit/mock-adapter-factory.test.ts +++ b/packages/adapter-mock/tests/unit/mock-adapter-factory.test.ts @@ -64,3 +64,18 @@ describe('MockAdapterFactory', () => { expect(adapter.supportsFeature(DebugFeature.SET_VARIABLE)).toBe(true); }); }); + +describe('MockAdapterFactory.describeToolchain', () => { + it('renders standalone (built-in) cells regardless of details', async () => { + const description = await new MockAdapterFactory().describeToolchain({ + valid: true, + errors: [], + warnings: [] + }); + + expect(description).toEqual({ + runtime: { label: '(built-in)' }, + backend: { label: '(built-in)' } + }); + }); +}); diff --git a/packages/adapter-python/src/python-adapter-factory.ts b/packages/adapter-python/src/python-adapter-factory.ts index 2dae5ab8..78a7d16b 100644 --- a/packages/adapter-python/src/python-adapter-factory.ts +++ b/packages/adapter-python/src/python-adapter-factory.ts @@ -7,11 +7,26 @@ * @since 2.0.0 */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { PythonDebugAdapter } from './python-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { findPythonExecutable, getPythonVersion, getDebugpyVersion } from './utils/python-utils.js'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type PythonToolchainDetails = { + pythonPath?: string; + pythonVersion?: string; + debugpyVersion?: string; + pythonDetectionMethod: string; + platform: string; + timestamp: string; +}; + /** * Factory for creating Python debug adapters */ @@ -78,18 +93,30 @@ export class PythonAdapterFactory implements IAdapterFactory { errors.push(error instanceof Error ? error.message : 'Python executable not found'); } + const details: PythonToolchainDetails = { + pythonPath, + pythonVersion, + debugpyVersion, + pythonDetectionMethod: 'multi-strategy', + platform: process.platform, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - pythonPath, - pythonVersion, - debugpyVersion, - pythonDetectionMethod: 'multi-strategy', - platform: process.platform, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Python', path: details.pythonPath, version: details.pythonVersion }), + backend: toolchainComponent({ label: 'debugpy', version: details.debugpyVersion }) }; } } diff --git a/packages/adapter-python/tests/unit/python-adapter-factory.test.ts b/packages/adapter-python/tests/unit/python-adapter-factory.test.ts index ca053eca..012feb18 100644 --- a/packages/adapter-python/tests/unit/python-adapter-factory.test.ts +++ b/packages/adapter-python/tests/unit/python-adapter-factory.test.ts @@ -150,3 +150,56 @@ describe('PythonAdapterFactory', () => { }); }); }); + +describe('PythonAdapterFactory.describeToolchain', () => { + const validation = (details: Record) => ({ + valid: true, + errors: [], + warnings: [], + details + }); + + it('renders Python and debugpy cells from its own validate() details', async () => { + const factory = new PythonAdapterFactory(); + + const description = await factory.describeToolchain( + validation({ + pythonPath: '/usr/bin/python3', + pythonVersion: '3.12.1', + debugpyVersion: '1.8.14', + pythonDetectionMethod: 'multi-strategy', + platform: 'linux', + timestamp: 'now' + }) + ); + + expect(description).toEqual({ + runtime: { label: 'Python', path: '/usr/bin/python3', version: '3.12.1' }, + backend: { label: 'debugpy', version: '1.8.14' } + }); + }); + + it('omits a component that was not detected instead of naming it as if found', async () => { + const factory = new PythonAdapterFactory(); + + const description = await factory.describeToolchain( + validation({ pythonPath: '/usr/bin/python3' }) + ); + + expect(description).toEqual({ + runtime: { label: 'Python', path: '/usr/bin/python3' } + }); + }); + + it('renders empty cells when validate() produced no details', async () => { + const factory = new PythonAdapterFactory(); + + const description = await factory.describeToolchain({ + valid: false, + errors: ['Python executable not found'], + warnings: [] + }); + + expect(description).toEqual({}); + }); +}); diff --git a/packages/adapter-ruby/src/ruby-adapter-factory.ts b/packages/adapter-ruby/src/ruby-adapter-factory.ts index 6755e79e..b06257e6 100644 --- a/packages/adapter-ruby/src/ruby-adapter-factory.ts +++ b/packages/adapter-ruby/src/ruby-adapter-factory.ts @@ -3,9 +3,10 @@ import { IAdapterFactory, AdapterDependencies, AdapterMetadata, - FactoryValidationResult + FactoryValidationResult, + ToolchainDescription } from '@debugmcp/shared'; -import { DebugLanguage } from '@debugmcp/shared'; +import { DebugLanguage, toolchainComponent } from '@debugmcp/shared'; import { RubyDebugAdapter } from './ruby-debug-adapter.js'; import { findRubyExecutable, @@ -14,6 +15,20 @@ import { getRdbgVersion } from './utils/ruby-utils.js'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type RubyToolchainDetails = { + rubyPath?: string; + rubyVersion?: string; + rdbgPath?: string; + rdbgVersion?: string; + platform: string; + timestamp: string; +}; + export class RubyAdapterFactory implements IAdapterFactory { createAdapter(dependencies: AdapterDependencies): IDebugAdapter { return new RubyDebugAdapter(dependencies); @@ -68,18 +83,30 @@ export class RubyAdapterFactory implements IAdapterFactory { errors.push(error instanceof Error ? error.message : 'rdbg not found'); } + const details: RubyToolchainDetails = { + rubyPath, + rubyVersion, + rdbgPath, + rdbgVersion, + platform: process.platform, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - rubyPath, - rubyVersion, - rdbgPath, - rdbgVersion, - platform: process.platform, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Ruby', path: details.rubyPath, version: details.rubyVersion }), + backend: toolchainComponent({ label: 'rdbg', path: details.rdbgPath, version: details.rdbgVersion }) }; } } diff --git a/packages/adapter-ruby/tests/unit/ruby-adapter-factory.test.ts b/packages/adapter-ruby/tests/unit/ruby-adapter-factory.test.ts index 70c3e5c9..f433f890 100644 --- a/packages/adapter-ruby/tests/unit/ruby-adapter-factory.test.ts +++ b/packages/adapter-ruby/tests/unit/ruby-adapter-factory.test.ts @@ -91,3 +91,43 @@ describe('RubyAdapterFactory', () => { expect(result.warnings).toHaveLength(2); }); }); + +describe('RubyAdapterFactory.describeToolchain', () => { + it('renders Ruby and rdbg cells from its own validate() details', async () => { + const description = await new RubyAdapterFactory().describeToolchain({ + valid: true, + errors: [], + warnings: [], + details: { + rubyPath: 'C:\\Ruby34-x64\\bin\\ruby.exe', + rubyVersion: '3.4.1', + rdbgPath: 'C:\\Ruby34-x64\\bin\\rdbg', + rdbgVersion: '1.11.0', + platform: 'win32', + timestamp: 'now' + } + }); + + expect(description).toEqual({ + runtime: { label: 'Ruby', path: 'C:\\Ruby34-x64\\bin\\ruby.exe', version: '3.4.1' }, + backend: { label: 'rdbg', path: 'C:\\Ruby34-x64\\bin\\rdbg', version: '1.11.0' } + }); + }); + + it('omits undetected components and renders empty cells without details', async () => { + const factory = new RubyAdapterFactory(); + + expect( + await factory.describeToolchain({ + valid: false, + errors: ['Ruby not found'], + warnings: [], + details: { rdbgPath: '/usr/bin/rdbg' } + }) + ).toEqual({ backend: { label: 'rdbg', path: '/usr/bin/rdbg' } }); + + expect( + await factory.describeToolchain({ valid: false, errors: [], warnings: [] }) + ).toEqual({}); + }); +}); diff --git a/packages/adapter-rust/src/rust-adapter-factory.ts b/packages/adapter-rust/src/rust-adapter-factory.ts index f9e57415..8025f83e 100644 --- a/packages/adapter-rust/src/rust-adapter-factory.ts +++ b/packages/adapter-rust/src/rust-adapter-factory.ts @@ -5,12 +5,29 @@ * Implements the adapter factory interface for dependency injection. */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; +import { toolchainComponent } from '@debugmcp/shared'; import { RustDebugAdapter } from './rust-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { checkCargoInstallation, getCargoVersion, getRustHostTriple } from './utils/rust-utils.js'; import { resolveCodeLLDBExecutableWithSource, getCodeLLDBVersion } from './utils/codelldb-resolver.js'; +/** + * The details shape validate() emits and describeToolchain() reads — keeping + * producer and consumer on one alias makes key renames compiler-checked + * within this package (issue #435). + */ +type RustToolchainDetails = { + codelldbPath?: string; + codelldbVersion?: string; + codelldbSource?: string; + cargoVersion?: string; + hostTriple?: string; + platform: string; + arch: string; + timestamp: string; +}; + /** * Factory for creating Rust debug adapters */ @@ -79,20 +96,37 @@ export class RustAdapterFactory implements IAdapterFactory { } } + const details: RustToolchainDetails = { + codelldbPath, + codelldbVersion, + codelldbSource, + cargoVersion, + hostTriple, + platform: process.platform, + arch: process.arch, + timestamp: new Date().toISOString() + }; return { valid: errors.length === 0, errors, warnings, - details: { - codelldbPath, - codelldbVersion, - codelldbSource, - cargoVersion, - hostTriple, - platform: process.platform, - arch: process.arch, - timestamp: new Date().toISOString() - } + details + }; + } + + /** + * Doctor row (issue #435): rendered entirely from validate() details. + */ + async describeToolchain(validation: FactoryValidationResult): Promise { + const details = (validation.details ?? {}) as Partial; + return { + runtime: toolchainComponent({ label: 'Rust', version: details.cargoVersion }), + backend: toolchainComponent({ + label: 'CodeLLDB', + path: details.codelldbPath, + version: details.codelldbVersion, + source: details.codelldbSource + }) }; } } diff --git a/packages/adapter-rust/tests/unit/rust-adapter-factory.test.ts b/packages/adapter-rust/tests/unit/rust-adapter-factory.test.ts new file mode 100644 index 00000000..260c957b --- /dev/null +++ b/packages/adapter-rust/tests/unit/rust-adapter-factory.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect } from 'vitest'; +import { DebugLanguage } from '@debugmcp/shared'; +import { RustAdapterFactory } from '../../src/rust-adapter-factory.js'; + +describe('RustAdapterFactory', () => { + it('returns accurate adapter metadata', () => { + const metadata = new RustAdapterFactory().getMetadata(); + + expect(metadata).toMatchObject({ + language: DebugLanguage.RUST, + displayName: 'Rust', + fileExtensions: ['.rs'], + modes: { launch: true, attach: 'none' } + }); + }); +}); + +describe('RustAdapterFactory.describeToolchain', () => { + it('renders Rust and CodeLLDB cells from its own validate() details', async () => { + const description = await new RustAdapterFactory().describeToolchain({ + valid: true, + errors: [], + warnings: [], + details: { + codelldbPath: '/opt/codelldb/adapter/codelldb', + codelldbVersion: '1.11.5', + codelldbSource: 'vendored', + cargoVersion: 'cargo 1.82.0', + hostTriple: 'x86_64-unknown-linux-gnu', + platform: 'linux', + arch: 'x64', + timestamp: 'now' + } + }); + + expect(description).toEqual({ + runtime: { label: 'Rust', version: 'cargo 1.82.0' }, + backend: { + label: 'CodeLLDB', + path: '/opt/codelldb/adapter/codelldb', + version: '1.11.5', + source: 'vendored' + } + }); + }); + + it('omits undetected components and renders empty cells without details', async () => { + const factory = new RustAdapterFactory(); + + expect( + await factory.describeToolchain({ + valid: false, + errors: ['CodeLLDB not found'], + warnings: [], + details: { cargoVersion: 'cargo 1.82.0' } + }) + ).toEqual({ runtime: { label: 'Rust', version: 'cargo 1.82.0' } }); + + expect( + await factory.describeToolchain({ valid: false, errors: [], warnings: [] }) + ).toEqual({}); + }); +}); diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index cf4eb0b1..25e30658 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -70,7 +70,12 @@ export type { // Validation FactoryValidationResult, - + + // Doctor presentation (issue #435) + ToolchainComponent, + ToolchainDescription, + DescribeToolchainOptions, + // Utility types AdapterFactoryMap, ActiveAdapterMap @@ -233,6 +238,8 @@ export { } from './utils/secret-redaction.js'; export type { SecretRule, RedactionHit, RedactionResult } from './utils/secret-redaction.js'; export { LineBuffer } from './utils/line-buffer.js'; +// Doctor-row helpers for IAdapterFactory.describeToolchain (issue #435). +export { toolchainComponent, normalizeToolchainDescription } from './utils/toolchain-description.js'; export { toSourceBreakpoint, type BreakpointFields, toFunctionBreakpoint, type FunctionBreakpointFields } from './utils/to-source-breakpoint.js'; // Argv marker constants shared by spawn-time tagging and the startup orphan // reapers (issues #343, #431). diff --git a/packages/shared/src/interfaces/adapter-registry.ts b/packages/shared/src/interfaces/adapter-registry.ts index 74835e52..6e13fde4 100644 --- a/packages/shared/src/interfaces/adapter-registry.ts +++ b/packages/shared/src/interfaces/adapter-registry.ts @@ -104,6 +104,20 @@ export interface IAdapterFactory { * @returns Validation result with any warnings or errors */ validate(): Promise; + + /** + * Doctor-only presentation of the resolved toolchain (issue #435): the + * adapter owns its own runtime/backend row instead of the CLI restating + * adapter internals. Receives the just-computed validate() result so the + * producer and consumer of each `details` key live in the same class; may + * run additional best-effort probes (the caller enforces a hard timeout). + * Must not throw for a missing toolchain — omit the cell instead. Absent + * method → doctor renders empty cells. + */ + describeToolchain?( + validation: FactoryValidationResult, + options?: DescribeToolchainOptions + ): Promise; } /** @@ -208,6 +222,42 @@ export interface FactoryValidationResult { details?: Record; } +/** + * One resolved toolchain component (a doctor table cell): the runtime a + * debuggee needs (Python, Node.js, a C++ compiler) or the debug backend that + * drives it (debugpy, js-debug, CodeLLDB). + */ +export interface ToolchainComponent { + /** Display name, e.g. 'Python', 'CodeLLDB', '(built-in)' */ + label: string; + + /** Resolved executable/directory path, when detected */ + path?: string; + + /** Resolved version, when detected */ + version?: string; + + /** Where the component came from, e.g. 'vendored', 'env:CODELLDB_PATH' */ + source?: string; +} + +/** + * An adapter's own doctor row (issue #435). Absent cells render as empty — + * a component that was not detected must be omitted, not named as if found. + */ +export interface ToolchainDescription { + runtime?: ToolchainComponent; + backend?: ToolchainComponent; +} + +/** + * Options for IAdapterFactory.describeToolchain. + */ +export interface DescribeToolchainOptions { + /** Advisory budget for extra probes; the caller also enforces a hard timeout. */ + timeoutMs?: number; +} + // ===== Registry Implementation Helpers ===== /** diff --git a/packages/shared/src/utils/toolchain-description.ts b/packages/shared/src/utils/toolchain-description.ts new file mode 100644 index 00000000..7bbdd484 --- /dev/null +++ b/packages/shared/src/utils/toolchain-description.ts @@ -0,0 +1,73 @@ +/** + * Helpers for building and consuming IAdapterFactory.describeToolchain rows + * (issue #435). Adapters build cells with toolchainComponent(); the doctor CLI + * defends against out-of-tree factories with normalizeToolchainDescription(). + */ +import type { ToolchainComponent, ToolchainDescription } from '../interfaces/adapter-registry.js'; + +const FIELDS = ['path', 'version', 'source'] as const; + +const asDetected = (value: unknown): string | undefined => + typeof value === 'string' && value.length > 0 ? value : undefined; + +/** + * Build one doctor cell. A cell is only rendered when something was actually + * detected — a bare label would make an absent toolchain read as present. + * `(built-in)` style labels stand alone by design (mock). Field values are + * treated as detections only when they are non-empty strings, so adapters can + * pass raw `details` values without per-field guards. + */ +export function toolchainComponent(info: { + label: string; + path?: unknown; + version?: unknown; + source?: unknown; +}): ToolchainComponent | undefined { + const component: ToolchainComponent = { label: info.label }; + let detected = false; + for (const field of FIELDS) { + const value = asDetected(info[field]); + if (value !== undefined) { + component[field] = value; + detected = true; + } + } + if (info.label.startsWith('(')) { + return component; + } + return detected ? component : undefined; +} + +/** + * Defensive normalization of a describeToolchain() return value: an + * out-of-tree factory is plain JS, so anything can come back. Non-object + * values yield empty cells; each cell must carry a non-empty string label and + * is re-filtered through the toolchainComponent rules. + */ +export function normalizeToolchainDescription(value: unknown): ToolchainDescription { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return {}; + } + const description: ToolchainDescription = {}; + for (const cell of ['runtime', 'backend'] as const) { + const raw = (value as Record)[cell]; + if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { + continue; + } + const candidate = raw as Record; + const label = asDetected(candidate.label); + if (label === undefined) { + continue; + } + const component = toolchainComponent({ + label, + path: candidate.path, + version: candidate.version, + source: candidate.source + }); + if (component) { + description[cell] = component; + } + } + return description; +} diff --git a/packages/shared/tests/unit/toolchain-description.test.ts b/packages/shared/tests/unit/toolchain-description.test.ts new file mode 100644 index 00000000..aaccc0d2 --- /dev/null +++ b/packages/shared/tests/unit/toolchain-description.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect } from 'vitest'; +import { + toolchainComponent, + normalizeToolchainDescription +} from '../../src/utils/toolchain-description.js'; + +describe('toolchainComponent', () => { + it('returns the component when a path was detected', () => { + expect(toolchainComponent({ label: 'Python', path: '/usr/bin/python3' })).toEqual({ + label: 'Python', + path: '/usr/bin/python3' + }); + }); + + it('returns the component when only a version was detected', () => { + expect(toolchainComponent({ label: 'debugpy', version: '1.8.14' })).toEqual({ + label: 'debugpy', + version: '1.8.14' + }); + }); + + it('returns the component when only a source was detected', () => { + expect(toolchainComponent({ label: 'js-debug', source: 'vendored' })).toEqual({ + label: 'js-debug', + source: 'vendored' + }); + }); + + it('omits the component when nothing was detected — a bare label would read as present', () => { + expect(toolchainComponent({ label: 'Ruby' })).toBeUndefined(); + }); + + it('treats empty strings as absent, like the old presenter str() helper', () => { + expect(toolchainComponent({ label: 'Go', path: '', version: '' })).toBeUndefined(); + }); + + it('drops empty-string fields from a component that has a real detection', () => { + expect(toolchainComponent({ label: 'rdbg', path: '', version: '1.11.0' })).toEqual({ + label: 'rdbg', + version: '1.11.0' + }); + }); + + it('drops non-string field values (details come from an untyped bag)', () => { + expect( + toolchainComponent({ label: 'Delve', path: 42 as unknown as string, version: 'v1.26.3' }) + ).toEqual({ label: 'Delve', version: 'v1.26.3' }); + }); + + it('lets "(built-in)" style labels stand alone by design', () => { + expect(toolchainComponent({ label: '(built-in)' })).toEqual({ label: '(built-in)' }); + }); +}); + +describe('normalizeToolchainDescription', () => { + it('passes a well-formed description through', () => { + const description = { + runtime: { label: 'Python', path: '/usr/bin/python3', version: '3.12.1' }, + backend: { label: 'debugpy', version: '1.8.14' } + }; + expect(normalizeToolchainDescription(description)).toEqual(description); + }); + + it('returns empty cells for non-object values', () => { + expect(normalizeToolchainDescription(undefined)).toEqual({}); + expect(normalizeToolchainDescription(null)).toEqual({}); + expect(normalizeToolchainDescription('Python 3.12')).toEqual({}); + expect(normalizeToolchainDescription([{ label: 'x' }])).toEqual({}); + }); + + it('drops a component with a missing or non-string label', () => { + expect( + normalizeToolchainDescription({ + runtime: { path: '/usr/bin/python3' }, + backend: { label: 7, version: '1.0' } + }) + ).toEqual({}); + }); + + it('applies the omit-undetected rule to each component', () => { + expect( + normalizeToolchainDescription({ + runtime: { label: 'Ruby' }, + backend: { label: '(built-in)' } + }) + ).toEqual({ backend: { label: '(built-in)' } }); + }); + + it('strips non-string and unknown fields, then re-applies the omission rule', () => { + expect( + normalizeToolchainDescription({ + runtime: { label: 'Go', path: 42, extra: 'ignored' }, + backend: { label: 'Delve', version: 'v1.26.3', extra: 'ignored' } + }) + ).toEqual({ backend: { label: 'Delve', version: 'v1.26.3' } }); + }); +}); diff --git a/src/cli/commands/doctor/diagnose.ts b/src/cli/commands/doctor/diagnose.ts index cccb13cd..2b7913f2 100644 --- a/src/cli/commands/doctor/diagnose.ts +++ b/src/cli/commands/doctor/diagnose.ts @@ -6,17 +6,20 @@ * factory.validate() per installed language, computeModeAvailability() — * so doctor's launch/attach availability can never disagree with the * server. Doctor adds what the server deliberately omits: per-probe - * timeouts, doctor-only extras, host-platform checks, and an honest - * verdict where the server fails open (recorded via probe.failed / - * probe.timedOut so the divergence is visible). + * timeouts, adapter-owned presentation (describeToolchain), host-platform + * checks, and an honest verdict where the server fails open (recorded via + * probe.failed / probe.timedOut so the divergence is visible). */ import type { AttachMechanism, IAdapterFactory, IEnvironment, IFileSystem, - ILogger + ILogger, + ToolchainComponent, + ToolchainDescription } from '@debugmcp/shared'; +import { normalizeToolchainDescription } from '@debugmcp/shared'; import { probeLanguageEntry, type LanguageModes } from '../../../utils/language-availability.js'; import { getDisabledLanguages } from '../../../utils/language-config.js'; import { @@ -24,12 +27,6 @@ import { checkYamaPtraceScope, type PlatformCheckResult } from './platform-checks.js'; -import { - collectDoctorExtras, - presentLanguage, - type DoctorBackendInfo, - type DoctorRuntimeInfo -} from './presenters.js'; import { isContainerMode } from '../../../utils/container-path-utils.js'; export type DoctorVerdict = 'ok' | 'warn' | 'missing' | 'disabled' | 'broken'; @@ -42,11 +39,11 @@ export interface LanguageDiagnosis { verdict: DoctorVerdict; errors: string[]; warnings: string[]; - runtime?: DoctorRuntimeInfo; - backend?: DoctorBackendInfo; + runtime?: ToolchainComponent; + backend?: ToolchainComponent; /** Verbatim computeModeAvailability output — matches list_supported_languages */ modes?: LanguageModes; - /** Raw validate() details plus doctor-only extras */ + /** Raw validate() details, verbatim */ details?: Record; probe: { durationMs: number; timedOut: boolean; failed: boolean }; } @@ -86,8 +83,6 @@ export interface DiagnoseDeps { timeoutMs: number; version: string; logger?: ILogger; - /** Doctor-only extras collector; injectable for tests. Defaults to collectDoctorExtras. */ - collectExtras?: (language: string, details: Record) => Promise>; } class ProbeTimeoutError extends Error { @@ -118,8 +113,6 @@ const GATING_VERDICTS: ReadonlySet = new Set(['broken', 'missing' export async function diagnose(requested: string[], deps: DiagnoseDeps): Promise { const env = deps.env ?? process.env; const platform = deps.platform ?? process.platform; - const collectExtras = - deps.collectExtras ?? ((language, details) => collectDoctorExtras(language, details)); const entries = await deps.registry.listAvailableAdapters(); const knownNames = new Set(entries.map((entry) => entry.name)); @@ -128,7 +121,7 @@ export async function diagnose(requested: string[], deps: DiagnoseDeps): Promise const disabledSet = getDisabledLanguages(env); const languages = await Promise.all( - entries.map((entry) => diagnoseLanguage(entry, disabledSet, deps, collectExtras)) + entries.map((entry) => diagnoseLanguage(entry, disabledSet, deps)) ); const platformChecks: PlatformCheckResult[] = [ @@ -163,8 +156,7 @@ export async function diagnose(requested: string[], deps: DiagnoseDeps): Promise async function diagnoseLanguage( entry: RegistryAdapterEntry, disabledSet: Set, - deps: DiagnoseDeps, - collectExtras: (language: string, details: Record) => Promise> + deps: DiagnoseDeps ): Promise { const probeStarted = Date.now(); // The validate/extras budget clock starts when validate actually runs, so a @@ -280,26 +272,32 @@ async function diagnoseLanguage( const failed = probeError !== undefined && !timedOut; const modes = probe.modes; - let details = validation?.details ? { ...validation.details } : undefined; - if (validation) { - // Extras share the language's timeout budget: whatever validate() left - // over (clocked from validate start, so factory-import time is excluded). - // A hung extras child is flagged via probe.timedOut so the handler's - // force-exit containment covers it too. + const details = validation?.details ? { ...validation.details } : undefined; + // Adapter-owned presentation (issue #435): the factory renders its own + // runtime/backend row from the validate() result — including valid:false, + // so partially detected toolchains still show what IS there. It shares the + // language's timeout budget: whatever validate() left over (clocked from + // validate start, so factory-import time is excluded). A hung probe child + // is flagged via probe.timedOut so the handler's force-exit containment + // covers it too. + let view: ToolchainDescription = {}; + if (validation && typeof probe.factory.describeToolchain === 'function') { const remainingMs = Math.max(0, deps.timeoutMs - (Date.now() - validateStarted)); try { - const extras = await withTimeout(collectExtras(entry.name, details ?? {}), remainingMs); - if (extras && Object.keys(extras).length > 0) { - details = { ...(details ?? {}), ...extras }; - } + view = normalizeToolchainDescription( + await withTimeout( + probe.factory.describeToolchain(validation, { timeoutMs: remainingMs }), + remainingMs + ) + ); } catch (error) { - // Extras are best-effort; the verdict stands on validate() alone. + // Presentation is best-effort; the verdict stands on validate() alone. if (error instanceof ProbeTimeoutError) { timedOut = true; } } } - // Validate + extras — the toolchain's own cost, excluding the module import + // Validate + presentation — the toolchain's own cost, excluding the module import const durationMs = Date.now() - validateStarted; let verdict: DoctorVerdict; @@ -336,8 +334,6 @@ async function diagnoseLanguage( warnings = validation.warnings; } - const view = presentLanguage(entry.name, details); - return { ...base, verdict, diff --git a/src/cli/commands/doctor/presenters.ts b/src/cli/commands/doctor/presenters.ts deleted file mode 100644 index 1ccd1d25..00000000 --- a/src/cli/commands/doctor/presenters.ts +++ /dev/null @@ -1,208 +0,0 @@ -/** - * Per-language presentation for `mcp-debugger doctor` (issue #423). - * - * presentLanguage maps the raw factory-validate `details` (plus doctor-only - * extras) onto the table's runtime/backend columns; collectDoctorExtras runs - * the doctor-only probes that live in the adapter packages but are too - * expensive for the server's validate() path (netcoredbg/SDK versions, javac, - * compiler banner). Both are presentation-side: no probing logic lives here. - */ - -export interface DoctorRuntimeInfo { - label: string; - path?: string; - version?: string; -} - -export interface DoctorBackendInfo { - label: string; - path?: string; - version?: string; - source?: string; -} - -export interface LanguageView { - runtime?: DoctorRuntimeInfo; - backend?: DoctorBackendInfo; -} - -type Details = Record; - -const str = (details: Details | undefined, key: string): string | undefined => { - const value = details?.[key]; - return typeof value === 'string' && value.length > 0 ? value : undefined; -}; - -/** - * A cell is only rendered when something was actually detected — a bare label - * would make an absent toolchain read as present. `(built-in)` style labels - * stand alone by design (mock). - */ -function component(info: T): T | undefined { - const { label, ...rest } = info as DoctorBackendInfo; - if (label.startsWith('(')) { - return info; - } - return Object.values(rest).some((value) => value !== undefined) ? info : undefined; -} - -export function presentLanguage(language: string, details: Details | undefined): LanguageView { - if (details === undefined) { - // The probe failed or timed out before producing anything — an empty row - // is the honest rendering. - return {}; - } - switch (language) { - case 'python': - return { - runtime: component({ label: 'Python', path: str(details, 'pythonPath'), version: str(details, 'pythonVersion') }), - backend: component({ label: 'debugpy', version: str(details, 'debugpyVersion') }) - }; - case 'javascript': - return { - runtime: component({ label: 'Node.js', version: str(details, 'nodeVersion') }), - backend: { label: 'js-debug', source: 'vendored' } - }; - case 'ruby': - return { - runtime: component({ label: 'Ruby', path: str(details, 'rubyPath'), version: str(details, 'rubyVersion') }), - backend: component({ label: 'rdbg', path: str(details, 'rdbgPath'), version: str(details, 'rdbgVersion') }) - }; - case 'go': - return { - runtime: component({ label: 'Go', path: str(details, 'goPath'), version: str(details, 'goVersion') }), - backend: component({ label: 'Delve', path: str(details, 'dlvPath'), version: str(details, 'dlvVersion') }) - }; - case 'java': - return { - runtime: component({ label: 'Java', path: str(details, 'javaPath'), version: str(details, 'javaVersion') }), - backend: component({ label: 'JDI bridge', path: str(details, 'jdiBridgeDir') }) - }; - case 'dotnet': - return { - runtime: component({ label: '.NET SDK', version: str(details, 'dotnetSdkVersion') }), - backend: component({ - label: 'netcoredbg', - path: str(details, 'debuggerPath'), - version: str(details, 'netcoredbgVersion') - }) - }; - case 'rust': - return { - runtime: component({ label: 'Rust', version: str(details, 'cargoVersion') }), - backend: component({ - label: 'CodeLLDB', - path: str(details, 'codelldbPath'), - version: str(details, 'codelldbVersion'), - source: str(details, 'codelldbSource') - }) - }; - case 'cpp': { - // The --version banner already names the command; only show the bare - // command when no banner was captured. - const compilerVersion = str(details, 'compilerVersion'); - return { - runtime: component({ - label: 'C/C++ compiler', - path: compilerVersion ? undefined : str(details, 'compiler'), - version: compilerVersion - }), - backend: component({ - label: 'CodeLLDB', - path: str(details, 'codelldbPath'), - version: str(details, 'codelldbVersion'), - source: str(details, 'codelldbSource') - }) - }; - } - case 'mock': - return { - runtime: { label: '(built-in)' }, - backend: { label: '(built-in)' } - }; - default: - return {}; - } -} - -type ImportModule = (id: string) => Promise; - -/** - * Literal specifiers only: a variable `import(id)` cannot be inlined by the - * npx bundle's esbuild pass, so it would throw ERR_MODULE_NOT_FOUND in every - * npx/global install (the CLI package ships no adapter dependencies — they - * are bundled). With literals, esbuild bundles each target and the same code - * works in repo checkouts, Docker, and npx. - */ -const defaultImportModule: ImportModule = (id) => { - switch (id) { - case '@debugmcp/adapter-dotnet': - return import('@debugmcp/adapter-dotnet'); - case '@debugmcp/adapter-java': - return import('@debugmcp/adapter-java'); - case '@debugmcp/adapter-cpp': - return import('@debugmcp/adapter-cpp'); - default: - return Promise.reject(new Error(`No doctor extras module registered for '${id}'`)); - } -}; - -/** - * Doctor-only probes, run per installed language after validate(). Each is - * best-effort: an unimportable adapter package or a failing probe yields no - * extras rather than an error — the verdict already stands on validate(). - */ -export async function collectDoctorExtras( - language: string, - details: Details, - options: { importModule?: ImportModule } = {} -): Promise
{ - const importModule = options.importModule ?? defaultImportModule; - try { - switch (language) { - case 'dotnet': { - const mod = (await importModule('@debugmcp/adapter-dotnet')) as Partial<{ - getNetcoredbgVersion(path: string): Promise; - getDotnetSdkVersion(): Promise; - }>; - const extras: Details = {}; - const debuggerPath = str(details, 'debuggerPath'); - if (debuggerPath && typeof mod.getNetcoredbgVersion === 'function') { - const version = await mod.getNetcoredbgVersion(debuggerPath); - if (version) extras.netcoredbgVersion = version; - } - if (typeof mod.getDotnetSdkVersion === 'function') { - const sdk = await mod.getDotnetSdkVersion(); - if (sdk) extras.dotnetSdkVersion = sdk; - } - return extras; - } - case 'java': { - const mod = (await importModule('@debugmcp/adapter-java')) as Partial<{ - findJavacExecutable(javaPath?: string): Promise; - }>; - if (typeof mod.findJavacExecutable === 'function') { - const javacPath = await mod.findJavacExecutable(str(details, 'javaPath')); - if (javacPath) return { javacPath }; - } - return {}; - } - case 'cpp': { - const mod = (await importModule('@debugmcp/adapter-cpp')) as Partial<{ - getCompilerInfo(command?: string): Promise<{ command: string; version: string | null } | null>; - }>; - if (typeof mod.getCompilerInfo === 'function') { - // validate() already discovered the command — reuse it instead of - // re-probing the whole candidate list. - const info = await mod.getCompilerInfo(str(details, 'compiler')); - if (info?.version) return { compilerVersion: info.version }; - } - return {}; - } - default: - return {}; - } - } catch { - return {}; - } -} diff --git a/src/utils/language-availability.ts b/src/utils/language-availability.ts index 327eb7d6..880a0684 100644 --- a/src/utils/language-availability.ts +++ b/src/utils/language-availability.ts @@ -115,8 +115,12 @@ export interface LanguageAdapterEntry { attach?: AttachMechanism; } -/** The structural minimum the probe needs from a factory. */ -export type ProbeableAdapterFactory = Pick; +/** + * The structural minimum the probe needs from a factory, plus the optional + * adapter-owned doctor presentation (issue #435) so doctor can call it on + * probe.factory without a re-fetch. + */ +export type ProbeableAdapterFactory = Pick; export interface AvailabilityProbeOptions { /** Source of factories; absent getFactory means "cannot probe" (assume valid). */ diff --git a/tests/adapters/go/unit/go-adapter-factory.test.ts b/tests/adapters/go/unit/go-adapter-factory.test.ts index 38245495..2638c36d 100644 --- a/tests/adapters/go/unit/go-adapter-factory.test.ts +++ b/tests/adapters/go/unit/go-adapter-factory.test.ts @@ -245,10 +245,55 @@ describe('GoAdapterFactory', () => { }); const result = await factory.validate(); - + expect(result.details?.platform).toBe(process.platform); expect(result.details?.arch).toBe(process.arch); expect(result.details?.timestamp).toBeDefined(); }); }); + + describe('describeToolchain', () => { + it('renders Go and Delve cells from its own validate() details', async () => { + const factory = new GoAdapterFactory(); + + const description = await factory.describeToolchain({ + valid: true, + errors: [], + warnings: [], + details: { + goPath: '/usr/local/go/bin/go', + goVersion: '1.22.3', + dlvPath: '/home/user/go/bin/dlv', + dlvVersion: '1.26.3', + platform: 'linux', + arch: 'amd64', + timestamp: 'now' + } + }); + + expect(description).toEqual({ + runtime: { label: 'Go', path: '/usr/local/go/bin/go', version: '1.22.3' }, + backend: { label: 'Delve', path: '/home/user/go/bin/dlv', version: '1.26.3' } + }); + }); + + it('omits undetected components and renders empty cells without details', async () => { + const factory = new GoAdapterFactory(); + + expect( + await factory.describeToolchain({ + valid: false, + errors: ['dlv not found'], + warnings: [], + details: { goPath: '/usr/local/go/bin/go', goVersion: '1.22.3' } + }) + ).toEqual({ + runtime: { label: 'Go', path: '/usr/local/go/bin/go', version: '1.22.3' } + }); + + expect( + await factory.describeToolchain({ valid: false, errors: [], warnings: [] }) + ).toEqual({}); + }); + }); }); diff --git a/tests/adapters/java/unit/java-adapter-factory.test.ts b/tests/adapters/java/unit/java-adapter-factory.test.ts index d85845e1..87f6641b 100644 --- a/tests/adapters/java/unit/java-adapter-factory.test.ts +++ b/tests/adapters/java/unit/java-adapter-factory.test.ts @@ -206,4 +206,48 @@ describe('JavaAdapterFactory', () => { expect(result.warnings?.some(w => w.includes('Java 21+ recommended'))).toBeFalsy(); }); }); + + describe('describeToolchain', () => { + it('renders Java and JDI bridge cells from its own validate() details, with no extra probes', async () => { + const factory = new JavaAdapterFactory(); + + const description = await factory.describeToolchain({ + valid: true, + errors: [], + warnings: [], + details: { + javaPath: '/usr/lib/jvm/temurin-21/bin/java', + javaVersion: '21.0.4', + jdiBridgeDir: '/pkg/bridge/classes', + platform: 'linux', + arch: 'x64', + timestamp: 'now' + } + }); + + expect(description).toEqual({ + runtime: { label: 'Java', path: '/usr/lib/jvm/temurin-21/bin/java', version: '21.0.4' }, + backend: { label: 'JDI bridge', path: '/pkg/bridge/classes' } + }); + // The javac extras probe is gone (issue #435): no process is spawned. + expect(vi.mocked(spawn)).not.toHaveBeenCalled(); + }); + + it('omits undetected components and renders empty cells without details', async () => { + const factory = new JavaAdapterFactory(); + + expect( + await factory.describeToolchain({ + valid: false, + errors: ['Java not found. Install JDK 21+ from https://adoptium.net/'], + warnings: [], + details: { jdiBridgeDir: '/pkg/bridge/classes' } + }) + ).toEqual({ backend: { label: 'JDI bridge', path: '/pkg/bridge/classes' } }); + + expect( + await factory.describeToolchain({ valid: false, errors: [], warnings: [] }) + ).toEqual({}); + }); + }); }); diff --git a/tests/core/unit/server/server-doctor-parity.test.ts b/tests/core/unit/server/server-doctor-parity.test.ts index bcd144c6..d697a43d 100644 --- a/tests/core/unit/server/server-doctor-parity.test.ts +++ b/tests/core/unit/server/server-doctor-parity.test.ts @@ -131,8 +131,7 @@ describe('doctor / list_supported_languages availability parity (issue #435)', ( env: { DEBUG_MCP_DISABLE_LANGUAGES: 'mock' }, platform: 'linux', timeoutMs: 5000, - version: '0.0.0-test', - collectExtras: async () => ({}) + version: '0.0.0-test' }; const report = await diagnose([], doctorDeps); const doctorModes = new Map(report.languages.map((l) => [l.language, l.modes])); diff --git a/tests/e2e/doctor-smoke.test.ts b/tests/e2e/doctor-smoke.test.ts index 4587d09d..16e18849 100644 --- a/tests/e2e/doctor-smoke.test.ts +++ b/tests/e2e/doctor-smoke.test.ts @@ -52,6 +52,10 @@ describe('doctor e2e smoke', () => { expect(report.languages).toHaveLength(9); const mock = report.languages.find((l: { language: string }) => l.language === 'mock'); expect(mock.verdict).toBe('ok'); + // Adapter-owned presentation (issue #435) survives the real built bundle: + // mock's describeToolchain renders the standalone '(built-in)' cells. + expect(mock.runtime).toEqual({ label: '(built-in)' }); + expect(mock.backend).toEqual({ label: '(built-in)' }); const ids = report.platformChecks.map((c: { id: string }) => c.id); expect(ids).toEqual(expect.arrayContaining(['container-mode', 'workspace-mount', 'yama-ptrace-scope'])); }, 120_000); diff --git a/tests/unit/cli/doctor/diagnose.test.ts b/tests/unit/cli/doctor/diagnose.test.ts index 4c32408f..ed610646 100644 --- a/tests/unit/cli/doctor/diagnose.test.ts +++ b/tests/unit/cli/doctor/diagnose.test.ts @@ -2,7 +2,8 @@ * Unit tests for the doctor command's orchestration (issue #423). * * Everything is injected: a fake registry with fake factories, a fake - * environment/filesystem, a stubbed extras collector. No process is spawned. + * environment/filesystem. No process is spawned. Presentation comes from the + * factories' own describeToolchain (issue #435). */ import { describe, it, expect, vi, afterEach } from 'vitest'; import type { IEnvironment, IFileSystem } from '@debugmcp/shared'; @@ -26,6 +27,8 @@ interface FakeAdapterSpec { installed?: boolean; attach?: 'none' | 'direct-connect' | 'spawn'; validate?: () => Promise<{ valid: boolean; errors: string[]; warnings: string[]; details?: Record }>; + /** Adapter-owned doctor row (issue #435); absent models an older factory. */ + describeToolchain?: (validation: unknown, options?: unknown) => Promise; /** Return a loaded factory that lacks a validate function (version skew). */ factoryWithoutValidate?: boolean; /** Delay (ms) before getFactory resolves — models a slow dynamic import. */ @@ -58,6 +61,7 @@ function makeDeps(adapters: FakeAdapterSpec[], overrides: Partial } return { validate: spec.validate, + ...(spec.describeToolchain ? { describeToolchain: spec.describeToolchain } : {}), getMetadata: () => ({ modes: { launch: true, attach: spec.attach ?? 'none' } }), createAdapter: () => { throw new Error('doctor must never instantiate adapters'); @@ -74,7 +78,6 @@ function makeDeps(adapters: FakeAdapterSpec[], overrides: Partial platform: 'win32', timeoutMs: 5000, version: '0.0.0-test', - collectExtras: async () => ({}), ...overrides }; } @@ -248,21 +251,21 @@ describe('diagnose', () => { expect(python.modes?.launch.available).toBe(true); }); - it('does not bill a slow factory import against the validate/extras budget (no spurious timeout)', async () => { + it('does not bill a slow factory import against the validate/describe budget (no spurious timeout)', async () => { vi.useFakeTimers(); const deps = makeDeps( [ { name: 'dotnet', factoryLoadDelayMs: 900, // slow cold import eats most of a naive shared budget - validate: okValidate({ debuggerPath: '/x' }) + validate: okValidate({ debuggerPath: '/x' }), + describeToolchain: () => + new Promise((resolve) => + setTimeout(() => resolve({ runtime: { label: '.NET SDK', version: '8.0.301' } }), 500) + ) } ], - { - timeoutMs: 1000, - collectExtras: () => - new Promise((resolve) => setTimeout(() => resolve({ dotnetSdkVersion: '8.0.301' }), 500)) - } + { timeoutMs: 1000 } ); const reportPromise = diagnose([], deps); @@ -272,7 +275,7 @@ describe('diagnose', () => { const dotnet = report.languages[0]; expect(dotnet.verdict).toBe('ok'); expect(dotnet.probe.timedOut).toBe(false); - expect(dotnet.details).toMatchObject({ dotnetSdkVersion: '8.0.301' }); // extras survived + expect(dotnet.runtime).toEqual({ label: '.NET SDK', version: '8.0.301' }); // the slow row survived }); it('reports broken with probe.timedOut when getFactory itself hangs (wedged dynamic import)', async () => { @@ -319,14 +322,17 @@ describe('diagnose', () => { expect(report.exitCode).toBe(1); }); - it('sets probe.timedOut when the extras collector hangs, so the handler can force-exit', async () => { + it('sets probe.timedOut when describeToolchain hangs, so the handler can force-exit', async () => { vi.useFakeTimers(); const deps = makeDeps( - [{ name: 'dotnet', validate: okValidate({ debuggerPath: '/x' }) }], - { - timeoutMs: 1000, - collectExtras: () => new Promise(() => undefined) - } + [ + { + name: 'dotnet', + validate: okValidate({ debuggerPath: '/x' }), + describeToolchain: () => new Promise(() => undefined) + } + ], + { timeoutMs: 1000 } ); const reportPromise = diagnose([], deps); @@ -334,19 +340,25 @@ describe('diagnose', () => { const report = await reportPromise; const dotnet = report.languages[0]; - expect(dotnet.verdict).toBe('ok'); // extras are best-effort; the verdict stands on validate() + expect(dotnet.verdict).toBe('ok'); // presentation is best-effort; the verdict stands on validate() expect(dotnet.probe.timedOut).toBe(true); + expect(dotnet.runtime).toBeUndefined(); }); - it('counts the extras phase inside probe.durationMs', async () => { + it('counts the describeToolchain phase inside probe.durationMs', async () => { vi.useFakeTimers(); const deps = makeDeps( - [{ name: 'dotnet', validate: okValidate({ debuggerPath: '/x' }) }], - { - timeoutMs: 5000, - collectExtras: () => - new Promise((resolve) => setTimeout(() => resolve({ dotnetSdkVersion: '8.0.301' }), 300)) - } + [ + { + name: 'dotnet', + validate: okValidate({ debuggerPath: '/x' }), + describeToolchain: () => + new Promise((resolve) => + setTimeout(() => resolve({ runtime: { label: '.NET SDK', version: '8.0.301' } }), 300) + ) + } + ], + { timeoutMs: 5000 } ); const reportPromise = diagnose([], deps); @@ -374,37 +386,99 @@ describe('diagnose', () => { expect(report.languages[0].modes?.attach.available).toBe(true); // from entry.attach }); - it('merges collectExtras output into the reported details', async () => { - const deps = makeDeps( - [{ name: 'dotnet', validate: okValidate({ debuggerPath: '/opt/netcoredbg' }) }], + it('hands the validate() result to describeToolchain and carries its rows into runtime/backend', async () => { + const deps = makeDeps([ { - collectExtras: async (language, details) => { - expect(language).toBe('dotnet'); - expect(details).toMatchObject({ debuggerPath: '/opt/netcoredbg' }); - return { netcoredbgVersion: '3.1.2-1054', dotnetSdkVersion: '8.0.301' }; + name: 'dotnet', + validate: okValidate({ debuggerPath: '/opt/netcoredbg' }), + describeToolchain: async (validation) => { + expect(validation).toMatchObject({ valid: true, details: { debuggerPath: '/opt/netcoredbg' } }); + return { + runtime: { label: '.NET SDK', version: '8.0.301' }, + backend: { label: 'netcoredbg', path: '/opt/netcoredbg', version: '3.1.2-1054' } + }; } } - ); + ]); const report = await diagnose([], deps); - expect(report.languages[0].details).toMatchObject({ - debuggerPath: '/opt/netcoredbg', - netcoredbgVersion: '3.1.2-1054', - dotnetSdkVersion: '8.0.301' - }); + const dotnet = report.languages[0]; + expect(dotnet.runtime).toEqual({ label: '.NET SDK', version: '8.0.301' }); + expect(dotnet.backend).toEqual({ label: 'netcoredbg', path: '/opt/netcoredbg', version: '3.1.2-1054' }); + // details stay the raw validate() output — presentation no longer leaks into them + expect(dotnet.details).toEqual({ debuggerPath: '/opt/netcoredbg' }); }); - it('keeps the verdict when collectExtras itself fails', async () => { - const deps = makeDeps([{ name: 'cpp', validate: okValidate() }], { - collectExtras: async () => { - throw new Error('extras exploded'); + it('keeps the verdict and renders empty cells when describeToolchain itself fails', async () => { + const deps = makeDeps([ + { + name: 'cpp', + validate: okValidate(), + describeToolchain: async () => { + throw new Error('presentation exploded'); + } } - }); + ]); + + const report = await diagnose([], deps); + + expect(report.languages[0].verdict).toBe('ok'); + expect(report.languages[0].runtime).toBeUndefined(); + expect(report.languages[0].backend).toBeUndefined(); + }); + + it('renders empty cells for a factory without describeToolchain (older adapter package)', async () => { + const deps = makeDeps([ + { name: 'python', validate: okValidate({ pythonPath: '/usr/bin/python3' }) } + ]); const report = await diagnose([], deps); expect(report.languages[0].verdict).toBe('ok'); + expect(report.languages[0].runtime).toBeUndefined(); + expect(report.languages[0].backend).toBeUndefined(); + expect(report.languages[0].details).toEqual({ pythonPath: '/usr/bin/python3' }); + }); + + it('normalizes a malformed describeToolchain return (out-of-tree factory is plain JS)', async () => { + const deps = makeDeps([ + { + name: 'python', + validate: okValidate(), + describeToolchain: async () => ({ + runtime: { label: 'Python' }, // bare label — nothing detected, must not render + backend: 'not even an object' + }) + } + ]); + + const report = await diagnose([], deps); + + expect(report.languages[0].runtime).toBeUndefined(); + expect(report.languages[0].backend).toBeUndefined(); + }); + + it('still describes the toolchain when validate() reports invalid (partial rows stay honest)', async () => { + const deps = makeDeps([ + { + name: 'dotnet', + validate: async () => ({ + valid: false, + errors: ['dotnet SDK not found'], + warnings: [], + details: { debuggerPath: '/opt/netcoredbg' } + }), + describeToolchain: async () => ({ + backend: { label: 'netcoredbg', path: '/opt/netcoredbg' } + }) + } + ]); + + const report = await diagnose([], deps); + + expect(report.languages[0].verdict).toBe('broken'); + expect(report.languages[0].backend).toEqual({ label: 'netcoredbg', path: '/opt/netcoredbg' }); }); it('lists unknown requested languages and fails the run', async () => { diff --git a/tests/unit/cli/doctor/presenters.test.ts b/tests/unit/cli/doctor/presenters.test.ts deleted file mode 100644 index f16ca3c0..00000000 --- a/tests/unit/cli/doctor/presenters.test.ts +++ /dev/null @@ -1,190 +0,0 @@ -/** - * Unit tests for the doctor command's per-language presentation mapping and - * the doctor-only extras collector (issue #423). Module loading is injected — - * no adapter package is really imported and nothing is spawned. - */ -import { describe, it, expect, vi } from 'vitest'; -import { presentLanguage, collectDoctorExtras } from '../../../../src/cli/commands/doctor/presenters.js'; - -describe('presentLanguage', () => { - it('maps python details to runtime + debugpy backend columns', () => { - const view = presentLanguage('python', { - pythonPath: 'C:\\Python313\\python.exe', - pythonVersion: '3.13.2', - debugpyVersion: '1.8.14' - }); - - expect(view.runtime).toMatchObject({ label: 'Python', path: 'C:\\Python313\\python.exe', version: '3.13.2' }); - expect(view.backend).toMatchObject({ label: 'debugpy', version: '1.8.14' }); - }); - - it('maps ruby details to ruby + rdbg columns', () => { - const view = presentLanguage('ruby', { - rubyPath: '/usr/bin/ruby', - rubyVersion: '3.4.2', - rdbgPath: '/usr/bin/rdbg', - rdbgVersion: '1.11.0' - }); - - expect(view.runtime).toMatchObject({ label: 'Ruby', version: '3.4.2' }); - expect(view.backend).toMatchObject({ label: 'rdbg', path: '/usr/bin/rdbg', version: '1.11.0' }); - }); - - it('maps go details to go + delve columns', () => { - const view = presentLanguage('go', { - goPath: '/usr/local/go/bin/go', - goVersion: '1.24.1', - dlvPath: '/home/user/go/bin/dlv', - dlvVersion: '1.26.3' - }); - - expect(view.runtime).toMatchObject({ label: 'Go', version: '1.24.1' }); - expect(view.backend).toMatchObject({ label: 'Delve', version: '1.26.3' }); - }); - - it('maps rust details including the CodeLLDB source attribution', () => { - const view = presentLanguage('rust', { - cargoVersion: 'cargo 1.85.0', - codelldbPath: '/vendor/codelldb', - codelldbVersion: '1.11.8', - codelldbSource: 'vendored' - }); - - expect(view.runtime).toMatchObject({ label: 'Rust' }); - expect(view.backend).toMatchObject({ label: 'CodeLLDB', version: '1.11.8', source: 'vendored' }); - }); - - it('maps dotnet extras into SDK runtime and netcoredbg backend columns', () => { - const view = presentLanguage('dotnet', { - debuggerPath: '/opt/netcoredbg/netcoredbg', - netcoredbgVersion: '3.1.2-1054', - dotnetSdkVersion: '8.0.301' - }); - - expect(view.runtime).toMatchObject({ label: '.NET SDK', version: '8.0.301' }); - expect(view.backend).toMatchObject({ label: 'netcoredbg', path: '/opt/netcoredbg/netcoredbg', version: '3.1.2-1054' }); - }); - - it('maps java details to java + JDI bridge columns', () => { - const view = presentLanguage('java', { - javaPath: '/opt/jdk/bin/java', - javaVersion: '21.0.6', - jdiBridgeDir: '/opt/bridge' - }); - - expect(view.runtime).toMatchObject({ label: 'Java', version: '21.0.6' }); - expect(view.backend).toMatchObject({ label: 'JDI bridge', path: '/opt/bridge' }); - }); - - it('shows only the cpp compiler banner when a version was captured (the banner names the command)', () => { - const view = presentLanguage('cpp', { - compiler: 'g++', - compilerVersion: 'g++ (MinGW-w64) 13.2.0' - }); - - expect(view.runtime).toMatchObject({ label: 'C/C++ compiler', version: 'g++ (MinGW-w64) 13.2.0' }); - expect(view.runtime?.path).toBeUndefined(); - }); - - it('falls back to the bare cpp compiler command when no version banner was captured', () => { - const view = presentLanguage('cpp', { compiler: 'g++' }); - - expect(view.runtime).toMatchObject({ label: 'C/C++ compiler', path: 'g++' }); - }); - - it('marks mock as built-in', () => { - const view = presentLanguage('mock', {}); - - expect(view.runtime?.label).toContain('built-in'); - expect(view.backend?.label).toContain('built-in'); - }); - - it('returns empty cells when the probe produced no details (failed/timed-out probe)', () => { - const view = presentLanguage('python', undefined); - - expect(view.runtime).toBeUndefined(); - expect(view.backend).toBeUndefined(); - }); - - it('omits a component that was not detected instead of naming it as if found', () => { - // dotnet with netcoredbg resolved but no SDK: the runtime cell must read - // as absent, not ".NET SDK". - const view = presentLanguage('dotnet', { debuggerPath: '/opt/netcoredbg/netcoredbg' }); - - expect(view.runtime).toBeUndefined(); - expect(view.backend).toMatchObject({ label: 'netcoredbg', path: '/opt/netcoredbg/netcoredbg' }); - }); - - it('keeps the vendored js-debug backend visible without a version (source counts as detection)', () => { - const view = presentLanguage('javascript', { nodeVersion: 'v22.0.0' }); - - expect(view.backend).toMatchObject({ label: 'js-debug', source: 'vendored' }); - }); -}); - -describe('collectDoctorExtras', () => { - it('collects netcoredbg and SDK versions for dotnet via the adapter package', async () => { - const importModule = vi.fn().mockResolvedValue({ - getNetcoredbgVersion: vi.fn().mockResolvedValue('3.1.2-1054'), - getDotnetSdkVersion: vi.fn().mockResolvedValue('8.0.301') - }); - - const extras = await collectDoctorExtras('dotnet', { debuggerPath: '/opt/netcoredbg' }, { importModule }); - - expect(importModule).toHaveBeenCalledWith('@debugmcp/adapter-dotnet'); - expect(extras).toEqual({ netcoredbgVersion: '3.1.2-1054', dotnetSdkVersion: '8.0.301' }); - }); - - it('skips the netcoredbg version probe when no debugger path was resolved', async () => { - const getNetcoredbgVersion = vi.fn(); - const importModule = vi.fn().mockResolvedValue({ - getNetcoredbgVersion, - getDotnetSdkVersion: vi.fn().mockResolvedValue(null) - }); - - const extras = await collectDoctorExtras('dotnet', {}, { importModule }); - - expect(getNetcoredbgVersion).not.toHaveBeenCalled(); - expect(extras).toEqual({}); - }); - - it('collects the javac path for java', async () => { - const findJavacExecutable = vi.fn().mockResolvedValue('/opt/jdk/bin/javac'); - const importModule = vi.fn().mockResolvedValue({ findJavacExecutable }); - - const extras = await collectDoctorExtras('java', { javaPath: '/opt/jdk/bin/java' }, { importModule }); - - expect(importModule).toHaveBeenCalledWith('@debugmcp/adapter-java'); - expect(findJavacExecutable).toHaveBeenCalledWith('/opt/jdk/bin/java'); - expect(extras).toEqual({ javacPath: '/opt/jdk/bin/javac' }); - }); - - it('collects the compiler version banner for cpp, reusing the already-discovered command', async () => { - const getCompilerInfo = vi.fn().mockResolvedValue({ command: 'g++', version: 'g++ (MinGW-w64) 13.2.0' }); - const importModule = vi.fn().mockResolvedValue({ getCompilerInfo }); - - const extras = await collectDoctorExtras('cpp', { compiler: 'g++' }, { importModule }); - - // validate() already discovered the command — extras must not re-probe the - // whole candidate list. - expect(getCompilerInfo).toHaveBeenCalledWith('g++'); - expect(extras).toEqual({ compilerVersion: 'g++ (MinGW-w64) 13.2.0' }); - }); - - it('returns no extras for languages without doctor-only probes', async () => { - const importModule = vi.fn(); - - const extras = await collectDoctorExtras('python', { pythonPath: '/usr/bin/python3' }, { importModule }); - - expect(importModule).not.toHaveBeenCalled(); - expect(extras).toEqual({}); - }); - - it('returns no extras when the adapter package cannot be imported', async () => { - const importModule = vi.fn().mockRejectedValue(new Error('MODULE_NOT_FOUND')); - - const extras = await collectDoctorExtras('dotnet', { debuggerPath: '/x' }, { importModule }); - - expect(extras).toEqual({}); - }); -}); From bd91a79500b83fe6791b87e76a209bef1703be6d Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 22:49:41 -0400 Subject: [PATCH 2/2] fix(doctor): harden the describeToolchain seam per review (#442) 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 --- .../adapter-cpp/src/cpp-adapter-factory.ts | 18 +++-- .../tests/unit/cpp-adapter-factory.test.ts | 31 ++++++- .../src/DotnetAdapterFactory.ts | 19 +++-- .../tests/unit/dotnet-adapter-factory.test.ts | 34 +++++++- packages/adapter-java/src/index.ts | 2 +- packages/adapter-java/src/utils/java-utils.ts | 30 ------- .../src/javascript-adapter-factory.ts | 4 +- .../adapter-mock/src/mock-adapter-factory.ts | 2 +- packages/shared/README.md | 13 +++ packages/shared/src/index.ts | 2 +- .../shared/src/interfaces/adapter-registry.ts | 21 +++-- .../shared/src/utils/toolchain-description.ts | 39 +++++++++ .../tests/unit/toolchain-description.test.ts | 35 +++++++- src/cli/commands/doctor/diagnose.ts | 80 ++++++++++++++----- tests/adapters/java/unit/java-utils.test.ts | 53 ------------ tests/unit/cli/doctor/diagnose.test.ts | 70 ++++++++++++++-- 16 files changed, 317 insertions(+), 136 deletions(-) diff --git a/packages/adapter-cpp/src/cpp-adapter-factory.ts b/packages/adapter-cpp/src/cpp-adapter-factory.ts index 9cb9de04..181a38e2 100644 --- a/packages/adapter-cpp/src/cpp-adapter-factory.ts +++ b/packages/adapter-cpp/src/cpp-adapter-factory.ts @@ -5,8 +5,8 @@ * Implements the adapter factory interface for dependency injection. */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; -import { toolchainComponent } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription, DescribeToolchainOptions } from '@debugmcp/shared'; +import { toolchainComponent, probeWithinBudget } from '@debugmcp/shared'; import { CppDebugAdapter } from './cpp-debug-adapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { resolveCodeLLDBExecutableWithSource, getCodeLLDBVersion } from '@debugmcp/codelldb-common'; @@ -107,13 +107,19 @@ export class CppAdapterFactory implements IAdapterFactory { * Doctor row (issue #435): reuses the validate()-discovered compiler * command instead of re-probing the whole candidate list; the --version * banner already names the command, so the bare command only shows when no - * banner was captured. + * banner was captured (including when the probe fails or outlives the + * advisory budget — probeWithinBudget guarantees this method resolves + * before the caller's hard timeout would blank the row). */ - async describeToolchain(validation: FactoryValidationResult): Promise { + async describeToolchain( + validation: FactoryValidationResult, + options?: DescribeToolchainOptions + ): Promise { const details = (validation.details ?? {}) as Partial; + const compiler = details.compiler; let compilerVersion: string | undefined; - if (details.compiler) { - const info = await getCompilerInfo(details.compiler).catch(() => null); + if (compiler) { + const info = await probeWithinBudget(options?.timeoutMs, () => getCompilerInfo(compiler)); compilerVersion = info?.version ?? undefined; } return { diff --git a/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts b/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts index d358894d..fc198ef5 100644 --- a/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts +++ b/packages/adapter-cpp/tests/unit/cpp-adapter-factory.test.ts @@ -3,7 +3,10 @@ import { DebugLanguage } from '@debugmcp/shared'; import { CppAdapterFactory } from '../../src/cpp-adapter-factory.js'; import { getCompilerInfo } from '../../src/utils/compile-utils.js'; -vi.mock('../../src/utils/compile-utils.js', () => ({ +vi.mock('../../src/utils/compile-utils.js', async (importOriginal) => ({ + // Spread the real module so unrelated exports (used by CppDebugAdapter) + // stay defined if this file ever grows adapter-level tests. + ...(await importOriginal()), findAnyCompiler: vi.fn(), getCompilerInfo: vi.fn() })); @@ -111,4 +114,30 @@ describe('CppAdapterFactory.describeToolchain', () => { await new CppAdapterFactory().describeToolchain({ valid: false, errors: [], warnings: [] }) ).toEqual({}); }); + + it('still resolves with detail-derived cells when the banner probe hangs, inside the advisory budget', async () => { + getCompilerInfoMock.mockReturnValue(new Promise(() => undefined)); + + const description = await new CppAdapterFactory().describeToolchain( + validation({ compiler: 'g++', codelldbPath: '/opt/codelldb/adapter/codelldb', codelldbVersion: '1.11.5', codelldbSource: 'vendored' }), + { timeoutMs: 300 } + ); + + expect(description).toEqual({ + runtime: { label: 'C/C++ compiler', path: 'g++' }, + backend: { label: 'CodeLLDB', path: '/opt/codelldb/adapter/codelldb', version: '1.11.5', source: 'vendored' } + }); + }); + + it('skips the banner probe entirely when the advisory budget is exhausted', async () => { + const description = await new CppAdapterFactory().describeToolchain( + validation({ compiler: 'g++' }), + { timeoutMs: 50 } + ); + + expect(getCompilerInfoMock).not.toHaveBeenCalled(); + expect(description).toEqual({ + runtime: { label: 'C/C++ compiler', path: 'g++' } + }); + }); }); diff --git a/packages/adapter-dotnet/src/DotnetAdapterFactory.ts b/packages/adapter-dotnet/src/DotnetAdapterFactory.ts index d290c12e..0375c522 100644 --- a/packages/adapter-dotnet/src/DotnetAdapterFactory.ts +++ b/packages/adapter-dotnet/src/DotnetAdapterFactory.ts @@ -7,8 +7,8 @@ * @since 0.2.0 */ import { IDebugAdapter } from '@debugmcp/shared'; -import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription } from '@debugmcp/shared'; -import { toolchainComponent } from '@debugmcp/shared'; +import { IAdapterFactory, AdapterDependencies, AdapterMetadata, FactoryValidationResult, ToolchainDescription, DescribeToolchainOptions } from '@debugmcp/shared'; +import { toolchainComponent, probeWithinBudget } from '@debugmcp/shared'; import { DotnetDebugAdapter } from './DotnetDebugAdapter.js'; import { DebugLanguage } from '@debugmcp/shared'; import { findNetcoredbgExecutable, getNetcoredbgVersion, getDotnetSdkVersion } from './utils/dotnet-utils.js'; @@ -86,14 +86,21 @@ export class DotnetAdapterFactory implements IAdapterFactory { /** * Doctor row (issue #435): the version probes that used to live in the * doctor CLI's extras path run here instead, in parallel and best-effort — - * a failed probe just leaves its cell field empty. + * a failed or over-budget probe just leaves its cell field empty, and the + * detail-derived cells always render (probeWithinBudget guarantees this + * method resolves before the caller's hard timeout would blank the row). */ - async describeToolchain(validation: FactoryValidationResult): Promise { + async describeToolchain( + validation: FactoryValidationResult, + options?: DescribeToolchainOptions + ): Promise { const details = (validation.details ?? {}) as Partial; const debuggerPath = details.debuggerPath; const [netcoredbgVersion, sdkVersion] = await Promise.all([ - debuggerPath ? getNetcoredbgVersion(debuggerPath).catch(() => null) : Promise.resolve(null), - getDotnetSdkVersion().catch(() => null) + debuggerPath + ? probeWithinBudget(options?.timeoutMs, () => getNetcoredbgVersion(debuggerPath)) + : Promise.resolve(null), + probeWithinBudget(options?.timeoutMs, () => getDotnetSdkVersion()) ]); return { runtime: toolchainComponent({ label: '.NET SDK', version: sdkVersion ?? undefined }), diff --git a/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts b/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts index 3f7f74e7..2847978b 100644 --- a/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts +++ b/packages/adapter-dotnet/tests/unit/dotnet-adapter-factory.test.ts @@ -9,7 +9,10 @@ import { getDotnetSdkVersion } from '../../src/utils/dotnet-utils.js'; -vi.mock('../../src/utils/dotnet-utils.js', () => ({ +vi.mock('../../src/utils/dotnet-utils.js', async (importOriginal) => ({ + // Spread the real module so unrelated exports (used by DotnetDebugAdapter) + // stay defined if this file ever grows adapter-level tests. + ...(await importOriginal()), findNetcoredbgExecutable: vi.fn(), findDotnetBackend: vi.fn(), listDotnetProcesses: vi.fn(), @@ -143,4 +146,33 @@ describe('DotnetAdapterFactory.describeToolchain', () => { backend: { label: 'netcoredbg', path: '/path/to/netcoredbg' } }); }); + + it('still resolves with the detail-derived cells when a probe hangs, inside the advisory budget', async () => { + // The caller's hard timeout would otherwise blank the WHOLE row, losing + // the debuggerPath validate() already resolved (#435 review finding). + getNetcoredbgVersionMock.mockReturnValue(new Promise(() => undefined)); + getDotnetSdkVersionMock.mockReturnValue(new Promise(() => undefined)); + + const description = await new DotnetAdapterFactory().describeToolchain( + validation({ debuggerPath: '/path/to/netcoredbg' }), + { timeoutMs: 300 } + ); + + expect(description).toEqual({ + backend: { label: 'netcoredbg', path: '/path/to/netcoredbg' } + }); + }); + + it('skips the version probes entirely when the advisory budget is exhausted', async () => { + const description = await new DotnetAdapterFactory().describeToolchain( + validation({ debuggerPath: '/path/to/netcoredbg' }), + { timeoutMs: 50 } + ); + + expect(getNetcoredbgVersionMock).not.toHaveBeenCalled(); + expect(getDotnetSdkVersionMock).not.toHaveBeenCalled(); + expect(description).toEqual({ + backend: { label: 'netcoredbg', path: '/path/to/netcoredbg' } + }); + }); }); diff --git a/packages/adapter-java/src/index.ts b/packages/adapter-java/src/index.ts index 77a97df4..c4ef2467 100644 --- a/packages/adapter-java/src/index.ts +++ b/packages/adapter-java/src/index.ts @@ -10,7 +10,7 @@ export { JavaDebugAdapter } from './java-debug-adapter.js'; export { JavaAdapterFactory } from './java-adapter-factory.js'; -export { findJavaExecutable, findJavacExecutable, getJavaVersion, getJavaSearchPaths } from './utils/java-utils.js'; +export { findJavaExecutable, getJavaVersion, getJavaSearchPaths } from './utils/java-utils.js'; export { resolveJdiBridgeClassDir, ensureJdiBridgeCompiled } from './utils/jdi-resolver.js'; import { JavaAdapterFactory as _JavaAdapterFactory } from './java-adapter-factory.js'; diff --git a/packages/adapter-java/src/utils/java-utils.ts b/packages/adapter-java/src/utils/java-utils.ts index 6cd0288c..db303577 100644 --- a/packages/adapter-java/src/utils/java-utils.ts +++ b/packages/adapter-java/src/utils/java-utils.ts @@ -85,36 +85,6 @@ export async function validateJavaExecutable(javaPath: string): Promise }); } -/** - * Find a working javac executable, or null when none validates. - * - * Priority: sibling of the resolved java path > JAVA_HOME/bin/javac > 'javac' - * in PATH. Doctor-only probe (issue #423): javac is required to compile - * target code with -g for variable inspection, but the adapter itself only - * needs the JRE side, so this is never part of validate(). - */ -export async function findJavacExecutable(javaPath?: string): Promise { - /* istanbul ignore next -- platform-specific executable extension */ - const ext = process.platform === 'win32' ? '.exe' : ''; - const candidates: string[] = []; - - if (javaPath && path.dirname(javaPath) !== '.') { - candidates.push(path.join(path.dirname(javaPath), `javac${ext}`)); - } - if (process.env.JAVA_HOME) { - candidates.push(path.join(process.env.JAVA_HOME, 'bin', `javac${ext}`)); - } - candidates.push('javac'); - - for (const candidate of new Set(candidates)) { - // javac answers `-version` exactly like java — reuse the shared probe - if (await validateJavaExecutable(candidate)) { - return candidate; - } - } - return null; -} - /** * Get the Java version string. */ diff --git a/packages/adapter-javascript/src/javascript-adapter-factory.ts b/packages/adapter-javascript/src/javascript-adapter-factory.ts index e491adcd..380f27e5 100644 --- a/packages/adapter-javascript/src/javascript-adapter-factory.ts +++ b/packages/adapter-javascript/src/javascript-adapter-factory.ts @@ -156,7 +156,9 @@ export class JavascriptAdapterFactory extends BaseAdapterFactory { runtime: toolchainComponent({ label: 'Node.js', version: details.nodeVersion }), backend: toolchainComponent({ label: 'js-debug', - source: typeof details.vendorPathChecked === 'string' && details.vendorPathChecked.length > 0 ? 'vendored' : undefined + // vendorPathChecked is a resolved path or null — truthiness is the + // detection; toolchainComponent re-applies the non-empty-string rule. + source: details.vendorPathChecked ? 'vendored' : undefined }) }; } diff --git a/packages/adapter-mock/src/mock-adapter-factory.ts b/packages/adapter-mock/src/mock-adapter-factory.ts index c9491a19..2b35ff6e 100644 --- a/packages/adapter-mock/src/mock-adapter-factory.ts +++ b/packages/adapter-mock/src/mock-adapter-factory.ts @@ -76,7 +76,7 @@ export class MockAdapterFactory implements IAdapterFactory { * Doctor row (issue #435): nothing external to detect — the standalone * '(built-in)' labels render by design. */ - async describeToolchain(): Promise { + async describeToolchain(_validation: FactoryValidationResult): Promise { return { runtime: { label: '(built-in)' }, backend: { label: '(built-in)' } diff --git a/packages/shared/README.md b/packages/shared/README.md index dc48f0ad..4cb9ee96 100644 --- a/packages/shared/README.md +++ b/packages/shared/README.md @@ -60,6 +60,19 @@ Everything below is exported from the package root (`import { ... } from '@debug | `ActiveAdapterMap` | type | Map of language to active adapter | | `BaseAdapterFactory` | class | Abstract base for adapter factories | +### Doctor Presentation (issue #435) + +An adapter factory may implement the optional `IAdapterFactory.describeToolchain(validation, options?)` to own its `mcp-debugger doctor` runtime/backend row. + +| Export | Kind | Description | +|--------|------|-------------| +| `ToolchainComponent` | type | One doctor table cell (label + optional path/version/source) | +| `ToolchainDescription` | type | An adapter's doctor row: `{ runtime?, backend? }` | +| `DescribeToolchainOptions` | type | Advisory options for `describeToolchain` (`timeoutMs`) | +| `toolchainComponent` | function | Build one cell, omitting it unless something was actually detected (`(`-prefixed labels stand alone) | +| `normalizeToolchainDescription` | function | Defensively normalize an untrusted `describeToolchain` return value | +| `probeWithinBudget` | function | Run one best-effort probe inside the advisory budget; settles with the value or `null` | + ### Dependency Injection | Export | Kind | Description | diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 25e30658..d13011a2 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -239,7 +239,7 @@ export { export type { SecretRule, RedactionHit, RedactionResult } from './utils/secret-redaction.js'; export { LineBuffer } from './utils/line-buffer.js'; // Doctor-row helpers for IAdapterFactory.describeToolchain (issue #435). -export { toolchainComponent, normalizeToolchainDescription } from './utils/toolchain-description.js'; +export { toolchainComponent, normalizeToolchainDescription, probeWithinBudget } from './utils/toolchain-description.js'; export { toSourceBreakpoint, type BreakpointFields, toFunctionBreakpoint, type FunctionBreakpointFields } from './utils/to-source-breakpoint.js'; // Argv marker constants shared by spawn-time tagging and the startup orphan // reapers (issues #343, #431). diff --git a/packages/shared/src/interfaces/adapter-registry.ts b/packages/shared/src/interfaces/adapter-registry.ts index 6e13fde4..753579f1 100644 --- a/packages/shared/src/interfaces/adapter-registry.ts +++ b/packages/shared/src/interfaces/adapter-registry.ts @@ -108,11 +108,15 @@ export interface IAdapterFactory { /** * Doctor-only presentation of the resolved toolchain (issue #435): the * adapter owns its own runtime/backend row instead of the CLI restating - * adapter internals. Receives the just-computed validate() result so the - * producer and consumer of each `details` key live in the same class; may - * run additional best-effort probes (the caller enforces a hard timeout). - * Must not throw for a missing toolchain — omit the cell instead. Absent - * method → doctor renders empty cells. + * adapter internals. Receives a snapshot of the just-computed validate() + * result so the producer and consumer of each `details` key live in the + * same class; may run additional best-effort probes, bounded by the + * advisory options.timeoutMs (use probeWithinBudget so the method resolves + * before the caller's hard timeout — a hard timeout blanks the whole row). + * Must not throw for a missing toolchain — omit the cell instead + * (toolchainComponent enforces this; '('-prefixed labels are the one + * standalone exception, see ToolchainComponent.label). Absent method → + * doctor renders empty cells with a version-skew warning. */ describeToolchain?( validation: FactoryValidationResult, @@ -228,7 +232,12 @@ export interface FactoryValidationResult { * drives it (debugpy, js-debug, CodeLLDB). */ export interface ToolchainComponent { - /** Display name, e.g. 'Python', 'CodeLLDB', '(built-in)' */ + /** + * Display name, e.g. 'Python', 'CodeLLDB', '(built-in)'. A label starting + * with '(' is a standalone annotation: it renders by itself even when no + * path/version/source was detected (and the table renders ONLY the label), + * so use the prefix exclusively for cells that are not real detections. + */ label: string; /** Resolved executable/directory path, when detected */ diff --git a/packages/shared/src/utils/toolchain-description.ts b/packages/shared/src/utils/toolchain-description.ts index 7bbdd484..3ca00ca6 100644 --- a/packages/shared/src/utils/toolchain-description.ts +++ b/packages/shared/src/utils/toolchain-description.ts @@ -38,6 +38,45 @@ export function toolchainComponent(info: { return detected ? component : undefined; } +/** + * Headroom subtracted from the advisory describeToolchain budget so the + * method always resolves BEFORE the caller's hard timeout — a hard timeout + * blanks the whole row, losing cells validate() already resolved. + */ +const PROBE_BUDGET_HEADROOM_MS = 100; + +/** + * Run one best-effort probe inside the advisory describeToolchain budget: + * settles with the probe's value, or null when the probe rejects, outlives + * the budget, or the budget is already exhausted (then the probe is never + * started — no child is spawned that nothing will await). An undefined + * budget means "no limit". + */ +export async function probeWithinBudget( + budgetMs: number | undefined, + probe: () => Promise +): Promise { + const budget = + budgetMs === undefined + ? Number.POSITIVE_INFINITY + : Math.max(0, budgetMs - PROBE_BUDGET_HEADROOM_MS); + if (budget <= 0) { + return null; + } + const attempt = probe().catch(() => null); + if (!Number.isFinite(budget)) { + return attempt; + } + return Promise.race([ + attempt, + new Promise((resolve) => { + const timer = setTimeout(() => resolve(null), budget); + // Node returns a Timeout with unref(); browsers return a number. + (timer as { unref?: () => void }).unref?.(); + }) + ]); +} + /** * Defensive normalization of a describeToolchain() return value: an * out-of-tree factory is plain JS, so anything can come back. Non-object diff --git a/packages/shared/tests/unit/toolchain-description.test.ts b/packages/shared/tests/unit/toolchain-description.test.ts index aaccc0d2..83ea23bc 100644 --- a/packages/shared/tests/unit/toolchain-description.test.ts +++ b/packages/shared/tests/unit/toolchain-description.test.ts @@ -1,7 +1,8 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { toolchainComponent, - normalizeToolchainDescription + normalizeToolchainDescription, + probeWithinBudget } from '../../src/utils/toolchain-description.js'; describe('toolchainComponent', () => { @@ -95,3 +96,33 @@ describe('normalizeToolchainDescription', () => { ).toEqual({ backend: { label: 'Delve', version: 'v1.26.3' } }); }); }); + +describe('probeWithinBudget', () => { + it('returns the probe result when it settles inside the budget', async () => { + await expect(probeWithinBudget(5000, async () => '8.0.401')).resolves.toBe('8.0.401'); + }); + + it('returns null instead of hanging when the probe outlives the budget', async () => { + const result = await probeWithinBudget(150, () => new Promise(() => undefined)); + expect(result).toBeNull(); + }); + + it('does not run the probe at all when the budget is already exhausted', async () => { + const probe = vi.fn().mockResolvedValue('never'); + + await expect(probeWithinBudget(0, probe)).resolves.toBeNull(); + expect(probe).not.toHaveBeenCalled(); + }); + + it('swallows a probe rejection as null', async () => { + await expect( + probeWithinBudget(5000, async () => { + throw new Error('spawn failed'); + }) + ).resolves.toBeNull(); + }); + + it('awaits the probe fully when no budget was given', async () => { + await expect(probeWithinBudget(undefined, async () => 'value')).resolves.toBe('value'); + }); +}); diff --git a/src/cli/commands/doctor/diagnose.ts b/src/cli/commands/doctor/diagnose.ts index 2b7913f2..071719ee 100644 --- a/src/cli/commands/doctor/diagnose.ts +++ b/src/cli/commands/doctor/diagnose.ts @@ -95,7 +95,10 @@ class ProbeTimeoutError extends Error { function withTimeout(promise: Promise, timeoutMs: number): Promise { return new Promise((resolve, reject) => { const timer = setTimeout(() => reject(new ProbeTimeoutError(timeoutMs)), timeoutMs); - promise.then( + // Promise.resolve: a plain-JS factory can return a non-thenable from + // describeToolchain; `.then` on it would throw inside this executor, + // discarding the value and leaving the timer armed to stall the CLI. + Promise.resolve(promise).then( (value) => { clearTimeout(timer); resolve(value); @@ -273,6 +276,14 @@ async function diagnoseLanguage( const modes = probe.modes; const details = validation?.details ? { ...validation.details } : undefined; + // Snapshot the verdict inputs before any adapter-owned code runs: the + // factory is plain JS and receives only a defensive clone below, so a buggy + // (or malicious) describeToolchain cannot mutate its way past a broken + // verdict or blank the reported errors. + const verdictInputs = validation + ? { valid: validation.valid, errors: [...validation.errors], warnings: [...validation.warnings] } + : undefined; + // Adapter-owned presentation (issue #435): the factory renders its own // runtime/backend row from the validate() result — including valid:false, // so partially detected toolchains still show what IS there. It shares the @@ -281,20 +292,33 @@ async function diagnoseLanguage( // is flagged via probe.timedOut so the handler's force-exit containment // covers it too. let view: ToolchainDescription = {}; - if (validation && typeof probe.factory.describeToolchain === 'function') { - const remainingMs = Math.max(0, deps.timeoutMs - (Date.now() - validateStarted)); - try { - view = normalizeToolchainDescription( - await withTimeout( - probe.factory.describeToolchain(validation, { timeoutMs: remainingMs }), - remainingMs - ) - ); - } catch (error) { - // Presentation is best-effort; the verdict stands on validate() alone. - if (error instanceof ProbeTimeoutError) { - timedOut = true; + let describeMissing = false; + if (validation) { + if (typeof probe.factory.describeToolchain === 'function') { + const remainingMs = Math.max(0, deps.timeoutMs - (Date.now() - validateStarted)); + try { + view = normalizeToolchainDescription( + await withTimeout( + probe.factory.describeToolchain( + { + valid: validation.valid, + errors: [...validation.errors], + warnings: [...validation.warnings], + ...(validation.details ? { details: { ...validation.details } } : {}) + }, + { timeoutMs: remainingMs } + ), + remainingMs + ) + ); + } catch (error) { + // Presentation is best-effort; the verdict stands on validate() alone. + if (error instanceof ProbeTimeoutError) { + timedOut = true; + } } + } else { + describeMissing = true; } } // Validate + presentation — the toolchain's own cost, excluding the module import @@ -303,7 +327,7 @@ async function diagnoseLanguage( let verdict: DoctorVerdict; let errors: string[]; let warnings: string[]; - if (!validation) { + if (!verdictInputs) { verdict = 'broken'; errors = [ timedOut @@ -311,27 +335,39 @@ async function diagnoseLanguage( : `Toolchain probe failed: ${probeError instanceof Error ? probeError.message : String(probeError)}` ]; warnings = []; - } else if (!validation.valid) { + } else if (!verdictInputs.valid) { // A failed toolchain probe kills launch, but direct-connect attach runs // the debug engine inside the debuggee and needs nothing local (container // ruby is attach-only by design) — a partially usable adapter is a warn, // not broken, and must not fail a gated run. if (modes.attach.available) { verdict = 'warn'; - errors = validation.errors; + errors = verdictInputs.errors; warnings = [ - ...validation.warnings, + ...verdictInputs.warnings, `Launch is unavailable, but attach (direct-connect) still works — see the errors above for what launch would need.` ]; } else { verdict = 'broken'; - errors = validation.errors; - warnings = validation.warnings; + errors = verdictInputs.errors; + warnings = verdictInputs.warnings; } } else { - verdict = validation.warnings.length > 0 ? 'warn' : 'ok'; + verdict = verdictInputs.warnings.length > 0 ? 'warn' : 'ok'; errors = []; - warnings = validation.warnings; + warnings = verdictInputs.warnings; + } + + if (describeMissing) { + // Display-only breadcrumb (appended after the verdict is computed so an + // older adapter package never flips ok→warn): the cells are empty because + // the factory predates adapter-owned presentation, not because nothing + // was detected. + warnings = [ + ...warnings, + `The installed ${entry.packageName} predates adapter-owned doctor presentation ` + + `(describeToolchain) — runtime/backend cells are left empty; update the package to restore them.` + ]; } return { diff --git a/tests/adapters/java/unit/java-utils.test.ts b/tests/adapters/java/unit/java-utils.test.ts index 18585778..58dc1d71 100644 --- a/tests/adapters/java/unit/java-utils.test.ts +++ b/tests/adapters/java/unit/java-utils.test.ts @@ -4,7 +4,6 @@ import { EventEmitter } from 'events'; import path from 'path'; import { findJavaExecutable, - findJavacExecutable, getJavaVersion, getJavaSearchPaths } from '@debugmcp/adapter-java'; @@ -116,58 +115,6 @@ describe('java-utils', () => { }); }); - describe('findJavacExecutable (issue #423)', () => { - const ext = process.platform === 'win32' ? '.exe' : ''; - - const validatingSpawn = (validPaths: string[]) => { - mockSpawn.mockImplementation(((cmd: string) => { - const proc = new EventEmitter() as any; - proc.stdout = new EventEmitter(); - proc.stderr = new EventEmitter(); - process.nextTick(() => { - if (validPaths.includes(cmd)) { - proc.stdout.emit('data', Buffer.from('javac 21.0.6\n')); - proc.emit('exit', 0); proc.emit('close', 0); - } else { - proc.emit('error', new Error('ENOENT')); - } - }); - return proc; - }) as any); - }; - - it('returns the javac sibling of a resolved java path', async () => { - const javaPath = path.join(path.sep, 'jdk', 'bin', `java${ext}`); - const javacPath = path.join(path.sep, 'jdk', 'bin', `javac${ext}`); - validatingSpawn([javacPath]); - - await expect(findJavacExecutable(javaPath)).resolves.toBe(javacPath); - }); - - it('falls back to JAVA_HOME/bin/javac when the sibling does not validate', async () => { - const home = path.join(path.sep, 'opt', 'jdk21'); - vi.stubEnv('JAVA_HOME', home); - const javacHome = path.join(home, 'bin', `javac${ext}`); - validatingSpawn([javacHome]); - - await expect(findJavacExecutable(path.join(path.sep, 'elsewhere', 'java'))).resolves.toBe(javacHome); - }); - - it('falls back to bare javac on PATH', async () => { - vi.stubEnv('JAVA_HOME', ''); - validatingSpawn(['javac']); - - await expect(findJavacExecutable()).resolves.toBe('javac'); - }); - - it('returns null when no javac validates anywhere', async () => { - vi.stubEnv('JAVA_HOME', ''); - validatingSpawn([]); - - await expect(findJavacExecutable(path.join(path.sep, 'jdk', 'bin', 'java'))).resolves.toBeNull(); - }); - }); - describe('getJavaVersion', () => { it('should parse standard version string from stderr', async () => { mockSpawn.mockImplementation(() => { diff --git a/tests/unit/cli/doctor/diagnose.test.ts b/tests/unit/cli/doctor/diagnose.test.ts index ed610646..13b5d227 100644 --- a/tests/unit/cli/doctor/diagnose.test.ts +++ b/tests/unit/cli/doctor/diagnose.test.ts @@ -27,8 +27,11 @@ interface FakeAdapterSpec { installed?: boolean; attach?: 'none' | 'direct-connect' | 'spawn'; validate?: () => Promise<{ valid: boolean; errors: string[]; warnings: string[]; details?: Record }>; - /** Adapter-owned doctor row (issue #435); absent models an older factory. */ - describeToolchain?: (validation: unknown, options?: unknown) => Promise; + /** + * Adapter-owned doctor row (issue #435). Absent → a no-op modern factory; + * explicit null → a pre-describeToolchain factory (version skew). + */ + describeToolchain?: ((validation: unknown, options?: unknown) => Promise) | null; /** Return a loaded factory that lacks a validate function (version skew). */ factoryWithoutValidate?: boolean; /** Delay (ms) before getFactory resolves — models a slow dynamic import. */ @@ -61,7 +64,9 @@ function makeDeps(adapters: FakeAdapterSpec[], overrides: Partial } return { validate: spec.validate, - ...(spec.describeToolchain ? { describeToolchain: spec.describeToolchain } : {}), + ...(spec.describeToolchain === null + ? {} + : { describeToolchain: spec.describeToolchain ?? (async () => ({})) }), getMetadata: () => ({ modes: { launch: true, attach: spec.attach ?? 'none' } }), createAdapter: () => { throw new Error('doctor must never instantiate adapters'); @@ -428,17 +433,72 @@ describe('diagnose', () => { expect(report.languages[0].backend).toBeUndefined(); }); - it('renders empty cells for a factory without describeToolchain (older adapter package)', async () => { + it('renders empty cells with a version-skew breadcrumb for a factory without describeToolchain', async () => { const deps = makeDeps([ - { name: 'python', validate: okValidate({ pythonPath: '/usr/bin/python3' }) } + { name: 'python', validate: okValidate({ pythonPath: '/usr/bin/python3' }), describeToolchain: null } ]); const report = await diagnose([], deps); + // The breadcrumb is display-only: the verdict stands on validate() and + // must not flip to warn just because the adapter package is older. expect(report.languages[0].verdict).toBe('ok'); expect(report.languages[0].runtime).toBeUndefined(); expect(report.languages[0].backend).toBeUndefined(); expect(report.languages[0].details).toEqual({ pythonPath: '/usr/bin/python3' }); + expect( + report.languages[0].warnings.some( + (w) => w.includes('predates') && w.includes('@debugmcp/adapter-python') + ) + ).toBe(true); + }); + + it('computes the verdict from a snapshot — describeToolchain cannot mutate its way past a broken verdict', async () => { + // Adapter-owned presentation runs before the report is assembled; a buggy + // (or malicious) plain-JS factory that mutates the validation object it + // was handed must not be able to flip broken→ok or blank the errors. + const deps = makeDeps([ + { + name: 'go', + validate: async () => ({ + valid: false, + errors: ['Delve not found.'], + warnings: [], + details: { goPath: '/usr/local/go/bin/go' } + }), + describeToolchain: async (validation) => { + const v = validation as { valid: boolean; errors: string[] }; + v.valid = true; + v.errors.length = 0; + return {}; + } + } + ]); + + const report = await diagnose(['go'], deps); + + expect(report.languages[0].verdict).toBe('broken'); + expect(report.languages[0].errors).toEqual(['Delve not found.']); + expect(report.exitCode).toBe(1); + }); + + it('accepts a synchronous plain-object describeToolchain return without discarding it', async () => { + // Out-of-tree factories are plain JS: a sync (non-Promise) return must be + // normalized like any other, not lost to a thenable assumption. + const deps = makeDeps([ + { + name: 'python', + validate: okValidate(), + describeToolchain: (() => ({ + runtime: { label: 'Python', version: '3.12.1' } + })) as unknown as FakeAdapterSpec['describeToolchain'] + } + ]); + + const report = await diagnose([], deps); + + expect(report.languages[0].verdict).toBe('ok'); + expect(report.languages[0].runtime).toEqual({ label: 'Python', version: '3.12.1' }); }); it('normalizes a malformed describeToolchain return (out-of-tree factory is plain JS)', async () => {