From a74abfe190b911558e5fa418ba754b52dbefdeb3 Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 20:30:17 -0400 Subject: [PATCH 1/2] refactor(availability): one shared per-entry probe for server + doctor (#435, part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list_supported_languages and doctor previously hand-mirrored the same per-entry loop (disabled gate, installed gate, getFactory fail-open, metadata-over-entry attach preference) with only computeModeAvailability shared — so the "doctor can never disagree with the server" guarantee rested on a mirroring convention. probeLanguageEntry (language-availability.ts) is now that loop, consumed by both: the server injects its TTL validation cache via the runValidate wrapper; doctor injects its per-probe timeout and records the raw outcome through the same seam. A parity test runs both paths against one registry fixture covering every availability shape (valid, invalid, throwing validate, metadata-attach override, not-installed, disabled) and asserts identical modes — the fence that keeps them from drifting apart again. Behavioral deltas, both strict improvements the doctor path already had: the server's loop now fault-isolates a throwing factory.getMetadata (previously it would crash the whole list_supported_languages response) and no longer swallows getFactory load errors invisibly (recorded on the probe result; server behavior unchanged — it still fails open). Co-Authored-By: Claude Fable 5 --- src/cli/commands/doctor/diagnose.ts | 115 ++++++------- src/server.ts | 39 +++-- src/utils/language-availability.ts | 102 +++++++++++- .../unit/server/server-doctor-parity.test.ts | 150 +++++++++++++++++ .../unit/utils/language-availability.test.ts | 155 ++++++++++++++++++ 5 files changed, 473 insertions(+), 88 deletions(-) create mode 100644 tests/core/unit/server/server-doctor-parity.test.ts diff --git a/src/cli/commands/doctor/diagnose.ts b/src/cli/commands/doctor/diagnose.ts index 827c80ee..0ba2d4d9 100644 --- a/src/cli/commands/doctor/diagnose.ts +++ b/src/cli/commands/doctor/diagnose.ts @@ -18,7 +18,7 @@ import type { IFileSystem, ILogger } from '@debugmcp/shared'; -import { computeModeAvailability, type LanguageModes } from '../../../utils/language-availability.js'; +import { probeLanguageEntry, type LanguageModes } from '../../../utils/language-availability.js'; import { getDisabledLanguages } from '../../../utils/language-config.js'; import { checkContainerWorkspace, @@ -129,9 +129,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.has(entry.name), deps, collectExtras) - ) + entries.map((entry) => diagnoseLanguage(entry, disabledSet, deps, collectExtras)) ); const platformChecks: PlatformCheckResult[] = [ @@ -165,57 +163,69 @@ export async function diagnose(requested: string[], deps: DiagnoseDeps): Promise async function diagnoseLanguage( entry: RegistryAdapterEntry, - disabled: boolean, + disabledSet: Set, deps: DiagnoseDeps, collectExtras: (language: string, details: Record) => Promise> ): Promise { + const started = Date.now(); + let validation: FactoryValidationResult | undefined; + let probeError: unknown; + let timedOut = false; + + // The shared probe (issue #435) — the same function the server's + // list_supported_languages runs, so the two views cannot drift apart. + // Doctor's wrapper adds what the server deliberately omits: a per-probe + // timeout and a record of the raw outcome; the rethrow lets + // computeModeAvailability fail open exactly as it does for the server. + const probe = await probeLanguageEntry( + { + language: entry.name, + packageName: entry.packageName, + installed: entry.installed, + attach: entry.attach + }, + { + registry: deps.registry, + disabledSet, + runValidate: async (_language, validate) => { + try { + validation = await withTimeout(validate(), deps.timeoutMs); + return validation; + } catch (error) { + probeError = error; + timedOut = error instanceof ProbeTimeoutError; + throw error; + } + }, + logger: deps.logger + } + ); + const base = { language: entry.name, package: entry.packageName, installed: entry.installed, - disabled + disabled: probe.disabled }; - if (disabled || !entry.installed) { - const modes = await computeModeAvailability({ - language: entry.name, - packageName: entry.packageName, - installed: entry.installed, - disabled, - attach: entry.attach ?? 'none', - logger: deps.logger - }); + if (probe.disabled || !entry.installed) { return { ...base, - verdict: disabled ? 'disabled' : 'missing', + verdict: probe.disabled ? 'disabled' : 'missing', errors: [], warnings: [], - modes, + modes: probe.modes, probe: { durationMs: 0, timedOut: false, failed: false } }; } - let factoryLoadError: unknown; - const factory = await deps.registry.getFactory(entry.name).catch((error: unknown) => { - factoryLoadError = error; - return undefined; - }); - - if (!factory || typeof factory.validate !== 'function') { + if (!probe.factory || typeof probe.factory.validate !== 'function') { // An installed adapter whose factory cannot even be loaded cannot start // any session — that is broken, and a gated run must fail. (The server - // fails open here; the modes below reflect that so the divergence stays + // fails open here; probe.modes reflects that so the divergence stays // visible rather than silent.) - const modes = await computeModeAvailability({ - language: entry.name, - packageName: entry.packageName, - installed: true, - disabled: false, - attach: entry.attach ?? 'none', - logger: deps.logger - }); const loadDetail = - factoryLoadError instanceof Error ? `: ${factoryLoadError.message}` : ''; + probe.factoryLoadError instanceof Error ? `: ${probe.factoryLoadError.message}` : ''; return { ...base, verdict: 'broken', @@ -225,46 +235,13 @@ async function diagnoseLanguage( `(The server assumes availability when it cannot probe.)` ], warnings: [], - modes, + modes: probe.modes, probe: { durationMs: 0, timedOut: false, failed: true } }; } - const started = Date.now(); - let validation: FactoryValidationResult | undefined; - let probeError: unknown; - let timedOut = false; - try { - validation = await withTimeout(factory.validate(), deps.timeoutMs); - } catch (error) { - probeError = error; - timedOut = error instanceof ProbeTimeoutError; - } const failed = probeError !== undefined && !timedOut; - - // Feed computeModeAvailability the same outcome the server would see: the - // memoized result, or a throwing probe so its fail-open path runs. A - // throwing getMetadata (malformed third-party factory) must not take the - // other languages down with it. - let metadataAttach: AttachMechanism | undefined; - try { - metadataAttach = factory.getMetadata().modes?.attach; - } catch { - metadataAttach = undefined; - } - const modes = await computeModeAvailability({ - language: entry.name, - packageName: entry.packageName, - installed: true, - disabled: false, - attach: metadataAttach ?? entry.attach ?? 'none', - validate: validation - ? async () => validation - : async () => { - throw probeError; - }, - logger: deps.logger - }); + const modes = probe.modes; let details = validation?.details ? { ...validation.details } : undefined; if (validation) { diff --git a/src/server.ts b/src/server.ts index 479697fe..c568adab 100644 --- a/src/server.ts +++ b/src/server.ts @@ -47,10 +47,11 @@ import { LineReader, createLineReader } from './utils/line-reader.js'; import { getDisabledLanguages, isLanguageDisabled } from './utils/language-config.js'; import { ErrorMessages } from './utils/error-messages.js'; import { - computeModeAvailability, + probeLanguageEntry, checkLaunchToolchain, ValidationResultCache, - LanguageModes + LanguageModes, + ProbeableAdapterFactory } from './utils/language-availability.js'; import { isContainerMode, getWorkspaceRoot } from './utils/container-path-utils.js'; import { @@ -2811,7 +2812,7 @@ export class DebugMcpServer { const dyn = adapterRegistry as unknown as { listAvailableAdapters?: () => Promise>; - getFactory?: (language: string) => Promise<{ validate: () => Promise<{ valid: boolean; errors: string[]; warnings: string[] }>; getMetadata: () => { modes?: { attach: 'none' | 'direct-connect' | 'spawn' } } } | undefined>; + getFactory?: (language: string) => Promise; } | undefined; if (adapterRegistry && typeof dyn?.listAvailableAdapters === 'function') { @@ -2829,29 +2830,31 @@ export class DebugMcpServer { } } + // Shared per-entry probe (issue #435): doctor consumes the same + // function, so the two views cannot drift apart. const disabledSet = getDisabledLanguages(); const available: AvailableLanguage[] = []; for (const entry of baseEntries) { - const disabled = disabledSet.has(entry.language); - // Only probe toolchains for installed, enabled adapters - const factory = !disabled && entry.installed && typeof dyn?.getFactory === 'function' - ? await dyn.getFactory(entry.language).catch(() => undefined) - : undefined; - const modes = await computeModeAvailability({ - language: entry.language, - packageName: entry.package, - installed: entry.installed, - disabled, - attach: factory?.getMetadata().modes?.attach ?? entry.attach, - validate: factory ? () => this.validationCache.get(entry.language, () => factory.validate()) : undefined, - logger: this.logger - }); + const probe = await probeLanguageEntry( + { + language: entry.language, + packageName: entry.package, + installed: entry.installed, + attach: entry.attach + }, + { + registry: dyn, + disabledSet, + runValidate: (language, validate) => this.validationCache.get(language, validate), + logger: this.logger + } + ); available.push({ language: entry.language, package: entry.package, installed: entry.installed, description: entry.description, - modes + modes: probe.modes }); } diff --git a/src/utils/language-availability.ts b/src/utils/language-availability.ts index 5bb82217..03458283 100644 --- a/src/utils/language-availability.ts +++ b/src/utils/language-availability.ts @@ -6,7 +6,7 @@ * can't assess the toolchain — issue #360) and authoritative for attach * 'none' (enforced in SessionManagerOperations.attachToProcess). */ -import type { AttachMechanism, FactoryValidationResult } from '@debugmcp/shared'; +import type { AttachMechanism, FactoryValidationResult, IAdapterFactory } from '@debugmcp/shared'; import { ErrorMessages } from './error-messages.js'; export interface ModeAvailability { @@ -107,6 +107,106 @@ export async function checkLaunchToolchain( } } +/** A registry adapter entry as reported by listAvailableAdapters(). */ +export interface LanguageAdapterEntry { + language: string; + packageName: string; + installed: boolean; + attach?: AttachMechanism; +} + +/** The structural minimum the probe needs from a factory. */ +export type ProbeableAdapterFactory = Pick; + +export interface AvailabilityProbeOptions { + /** Source of factories; absent getFactory means "cannot probe" (assume valid). */ + registry?: { getFactory?: (language: string) => Promise }; + disabledSet: Set; + /** + * Wraps each factory.validate call — the injection point for the server's + * TTL cache and doctor's per-probe timeout. Default: call straight through. + */ + runValidate?: ( + language: string, + validate: () => Promise + ) => Promise; + logger?: { warn?: (message: string) => void }; +} + +export interface LanguageAvailabilityProbe { + disabled: boolean; + installed: boolean; + /** The loaded factory, when one was loaded (installed, enabled, load succeeded). */ + factory?: ProbeableAdapterFactory; + /** Set when registry.getFactory threw (doctor reports it; the server fails open). */ + factoryLoadError?: unknown; + /** Metadata-preferred attach mechanism (entry attach when metadata is unavailable). */ + attach: AttachMechanism; + modes: LanguageModes; +} + +/** + * The one per-entry availability probe shared by list_supported_languages and + * `mcp-debugger doctor` (issue #435): disabled gate → installed gate → + * factory load (fail-open on error) → metadata-over-entry attach preference → + * computeModeAvailability. Keeping both consumers on this function is what + * makes "doctor can never disagree with the server" structural rather than a + * mirroring convention. + */ +export async function probeLanguageEntry( + entry: LanguageAdapterEntry, + options: AvailabilityProbeOptions +): Promise { + const disabled = options.disabledSet.has(entry.language); + const entryAttach = entry.attach ?? 'none'; + + let factory: ProbeableAdapterFactory | undefined; + let factoryLoadError: unknown; + if (!disabled && entry.installed && typeof options.registry?.getFactory === 'function') { + try { + factory = await options.registry.getFactory(entry.language); + } catch (error) { + factoryLoadError = error; + } + } + + // A malformed factory's getMetadata must not take the probe down — fall + // back to the registry entry's attach declaration. + let attach = entryAttach; + if (factory) { + try { + attach = factory.getMetadata().modes?.attach ?? entryAttach; + } catch { + attach = entryAttach; + } + } + + const runValidate = + options.runValidate ?? ((_language, validate) => validate()); + const probeable = factory !== undefined && typeof factory.validate === 'function'; + + const modes = await computeModeAvailability({ + language: entry.language, + packageName: entry.packageName, + installed: entry.installed, + disabled, + attach, + validate: probeable + ? () => runValidate(entry.language, () => factory!.validate()) + : undefined, + logger: options.logger + }); + + return { + disabled, + installed: entry.installed, + factory, + factoryLoadError, + attach, + modes + }; +} + export async function computeModeAvailability(input: ModeAvailabilityInput): Promise { const { language, packageName, installed, disabled, attach } = input; diff --git a/tests/core/unit/server/server-doctor-parity.test.ts b/tests/core/unit/server/server-doctor-parity.test.ts new file mode 100644 index 00000000..b203fbe8 --- /dev/null +++ b/tests/core/unit/server/server-doctor-parity.test.ts @@ -0,0 +1,150 @@ +/** + * Parity test for issue #435: list_supported_languages (the server) and + * `mcp-debugger doctor` must report identical per-language launch/attach + * availability for the same registry state. Both now consume the shared + * probeLanguageEntry — this test is the fence that keeps them from drifting + * back into hand-mirrored loops. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { Server } from '@modelcontextprotocol/sdk/server/index.js'; +import { DebugMcpServer } from '../../../../src/server.js'; +import { SessionManager } from '../../../../src/session/session-manager.js'; +import { createProductionDependencies } from '../../../../src/container/dependencies.js'; +import { diagnose, type DiagnoseDeps } from '../../../../src/cli/commands/doctor/diagnose.js'; +import type { IEnvironment, IFileSystem } from '@debugmcp/shared'; +import { + createMockDependencies, + createMockServer, + createMockSessionManager, + getToolHandlers +} from './server-test-helpers.js'; + +vi.mock('@modelcontextprotocol/sdk/server/index.js'); +vi.mock('@modelcontextprotocol/sdk/server/stdio.js'); +vi.mock('../../../../src/session/session-manager.js'); +vi.mock('../../../../src/container/dependencies.js'); + +/** + * One registry covering every availability shape at once: + * - python: valid toolchain, direct-connect attach + * - go: invalid toolchain, no attach + * - ruby: validate() throws (fail-open territory), direct-connect + * - java: valid with warnings, spawn attach, metadata attach differs from entry + * - dotnet: not installed + * - mock: disabled via DEBUG_MCP_DISABLE_LANGUAGES + */ +function buildSharedRegistry() { + const entries = [ + { name: 'python', packageName: '@debugmcp/adapter-python', installed: true, attach: 'direct-connect' as const }, + { name: 'go', packageName: '@debugmcp/adapter-go', installed: true, attach: 'none' as const }, + { name: 'ruby', packageName: '@debugmcp/adapter-ruby', installed: true, attach: 'direct-connect' as const }, + { name: 'java', packageName: '@debugmcp/adapter-java', installed: true, attach: 'none' as const }, + { name: 'dotnet', packageName: '@debugmcp/adapter-dotnet', installed: false, attach: 'spawn' as const }, + { name: 'mock', packageName: '@debugmcp/adapter-mock', installed: true, attach: 'none' as const } + ]; + + const factories: Record = { + python: { + validate: async () => ({ valid: true, errors: [], warnings: [], details: {} }), + getMetadata: () => ({ modes: { launch: true, attach: 'direct-connect' } }) + }, + go: { + validate: async () => ({ valid: false, errors: ['Delve not found.'], warnings: [] }), + getMetadata: () => ({ modes: { launch: true, attach: 'none' } }) + }, + ruby: { + validate: async () => { + throw new Error('probe exploded'); + }, + getMetadata: () => ({ modes: { launch: true, attach: 'direct-connect' } }) + }, + java: { + validate: async () => ({ valid: true, errors: [], warnings: ['JDK below 21'], details: {} }), + // Metadata says spawn even though the entry says none — metadata must win in both paths + getMetadata: () => ({ modes: { launch: true, attach: 'spawn' } }) + } + }; + + return { + listLanguages: vi.fn().mockResolvedValue(entries.filter((e) => e.installed).map((e) => e.name)), + getSupportedLanguages: vi.fn().mockReturnValue(entries.filter((e) => e.installed).map((e) => e.name)), + listAvailableAdapters: vi.fn().mockResolvedValue(entries), + getFactory: vi.fn(async (language: string) => factories[language]), + isLanguageSupported: vi.fn().mockReturnValue(true), + create: vi.fn(), + register: vi.fn() + }; +} + +describe('doctor / list_supported_languages availability parity (issue #435)', () => { + let mockServer: ReturnType; + + beforeEach(() => { + vi.stubEnv('DEBUG_MCP_DISABLE_LANGUAGES', 'mock'); + + const mockDependencies = createMockDependencies(); + vi.mocked(createProductionDependencies).mockReturnValue(mockDependencies as never); + + mockServer = createMockServer(); + vi.mocked(Server).mockImplementation(function () { + return mockServer as never; + }); + + const registry = buildSharedRegistry(); + (mockDependencies as { adapterRegistry: unknown }).adapterRegistry = registry; + const mockSessionManager = createMockSessionManager(registry); + vi.mocked(SessionManager).mockImplementation(function () { + return mockSessionManager as never; + }); + }); + + afterEach(() => { + vi.clearAllMocks(); + vi.unstubAllEnvs(); + }); + + it('reports identical per-language modes from both paths against the same registry', async () => { + // Server path + new DebugMcpServer(); + const { callToolHandler } = getToolHandlers(mockServer); + const serverResult = await callToolHandler({ + method: 'tools/call', + params: { name: 'list_supported_languages', arguments: {} } + }); + const serverPayload = JSON.parse(serverResult.content[0].text); + const serverModes = new Map( + serverPayload.available.map((a: { language: string; modes: unknown }) => [a.language, a.modes]) + ); + + // Doctor path, against a fresh identical registry (no shared validation cache) + const doctorDeps: DiagnoseDeps = { + registry: buildSharedRegistry() as unknown as DiagnoseDeps['registry'], + environment: { + get: () => undefined, + getAll: () => ({}), + getCurrentWorkingDirectory: () => process.cwd() + } as IEnvironment, + fileSystem: { + readFile: vi.fn().mockRejectedValue(new Error('ENOENT')), + stat: vi.fn().mockRejectedValue(new Error('ENOENT')), + readdir: vi.fn().mockRejectedValue(new Error('ENOENT')) + } as unknown as IFileSystem, + env: { DEBUG_MCP_DISABLE_LANGUAGES: 'mock' }, + platform: 'linux', + timeoutMs: 5000, + version: '0.0.0-test', + collectExtras: async () => ({}) + }; + const report = await diagnose([], doctorDeps); + const doctorModes = new Map(report.languages.map((l) => [l.language, l.modes])); + + expect([...doctorModes.keys()].sort()).toEqual([...serverModes.keys()].sort()); + for (const [language, modes] of serverModes) { + expect(doctorModes.get(language), `modes for ${language}`).toEqual(modes); + } + // Sanity: the fixture really exercised the interesting shapes + expect((serverModes.get('java') as { attach: { supported: boolean } }).attach.supported).toBe(true); // metadata attach won + expect((serverModes.get('ruby') as { launch: { available: boolean } }).launch.available).toBe(true); // fail-open + expect((serverModes.get('go') as { launch: { available: boolean } }).launch.available).toBe(false); + }); +}); diff --git a/tests/unit/utils/language-availability.test.ts b/tests/unit/utils/language-availability.test.ts index 595bef55..8775c765 100644 --- a/tests/unit/utils/language-availability.test.ts +++ b/tests/unit/utils/language-availability.test.ts @@ -1,9 +1,11 @@ /** * Unit tests for per-mode language availability computation (issue #331) + * and the shared per-entry availability probe (issue #435). */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { computeModeAvailability, + probeLanguageEntry, ValidationResultCache } from '../../../src/utils/language-availability.js'; @@ -136,6 +138,159 @@ describe('computeModeAvailability', () => { }); }); +describe('probeLanguageEntry (issue #435)', () => { + const entry = (overrides: Record = {}) => ({ + language: 'python', + packageName: '@debugmcp/adapter-python', + installed: true, + attach: 'direct-connect' as const, + ...overrides + }); + + const makeFactory = (overrides: Record = {}) => ({ + validate: vi.fn().mockResolvedValue(ok), + getMetadata: vi.fn().mockReturnValue({ modes: { launch: true, attach: 'spawn' } }), + createAdapter: vi.fn(), + ...overrides + }); + + it('does not load the factory for a disabled language and reports disabled modes', async () => { + const getFactory = vi.fn(); + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory }, + disabledSet: new Set(['python']) + }); + + expect(probe.disabled).toBe(true); + expect(getFactory).not.toHaveBeenCalled(); + expect(probe.factory).toBeUndefined(); + expect(probe.modes.launch.available).toBe(false); + expect(probe.modes.launch.reason).toContain('disabled in this runtime'); + }); + + it('does not load the factory for a not-installed package and reports not-installed modes', async () => { + const getFactory = vi.fn(); + + const probe = await probeLanguageEntry(entry({ installed: false }), { + registry: { getFactory }, + disabledSet: new Set() + }); + + expect(getFactory).not.toHaveBeenCalled(); + expect(probe.modes.launch.reason).toContain('@debugmcp/adapter-python'); + }); + + it('prefers the loaded factory metadata attach over the registry entry attach', async () => { + const factory = makeFactory(); // metadata declares 'spawn' + + const probe = await probeLanguageEntry(entry({ attach: 'none' }), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set() + }); + + expect(probe.attach).toBe('spawn'); + expect(probe.modes.attach.supported).toBe(true); + }); + + it('falls back to the entry attach when getMetadata throws, without failing the probe', async () => { + const factory = makeFactory({ + getMetadata: vi.fn(() => { + throw new Error('metadata exploded'); + }) + }); + + const probe = await probeLanguageEntry(entry({ attach: 'direct-connect' }), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set() + }); + + expect(probe.attach).toBe('direct-connect'); + expect(probe.modes.launch.available).toBe(true); + }); + + it('records a factory load failure and fails open like the server', async () => { + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory: vi.fn().mockRejectedValue(new Error('import exploded')) }, + disabledSet: new Set() + }); + + expect(probe.factory).toBeUndefined(); + expect(probe.factoryLoadError).toBeInstanceOf(Error); + // No factory means no probe: availability is assumed (issue #360 contract) + expect(probe.modes.launch.available).toBe(true); + }); + + it('assumes availability when the registry has no getFactory at all', async () => { + const probe = await probeLanguageEntry(entry(), { + registry: undefined, + disabledSet: new Set() + }); + + expect(probe.modes.launch.available).toBe(true); + }); + + it('routes validate through the runValidate wrapper (cache/timeout injection point)', async () => { + const factory = makeFactory(); + const runValidate = vi.fn(async (_language: string, validate: () => Promise) => { + return validate(); + }); + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set(), + runValidate + }); + + expect(runValidate).toHaveBeenCalledWith('python', expect.any(Function)); + expect(factory.validate).toHaveBeenCalledTimes(1); + expect(probe.modes.launch.available).toBe(true); + }); + + it('drives modes from the wrapper result, not a second validate call', async () => { + const factory = makeFactory(); + const runValidate = vi.fn().mockResolvedValue(bad('toolchain gone')); + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set(), + runValidate + }); + + expect(probe.modes.launch).toEqual({ + supported: true, + available: false, + reason: 'toolchain gone' + }); + expect(factory.validate).not.toHaveBeenCalled(); + }); + + it('fails open when the wrapped validate throws, mirroring computeModeAvailability', async () => { + const warn = vi.fn(); + const factory = makeFactory({ validate: vi.fn().mockRejectedValue(new Error('probe exploded')) }); + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set(), + logger: { warn } + }); + + expect(probe.modes.launch.available).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('probe exploded')); + }); + + it('treats a factory without a validate function as unprobeable (assume valid)', async () => { + const factory = { getMetadata: vi.fn().mockReturnValue({}) }; + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set() + }); + + expect(probe.modes.launch.available).toBe(true); + }); +}); + describe('ValidationResultCache', () => { beforeEach(() => { vi.useFakeTimers(); From d7f9427a6bc85a801dbb07f77096053818724e8c Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 20:49:11 -0400 Subject: [PATCH 2/2] fix(availability): review follow-ups on the shared probe (#435, part 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review batch on PR #437 (13 of 15 findings addressed; the remaining two — routing checkLaunchToolchain through the shared probe, and a typed getFactory surface instead of the duck-typed cast — noted on #435): - Timing regression: hoisting the probe clock made a slow factory dynamic import eat the validate/extras budget, manufacturing a spurious probe.timedOut that escalated to the handler's forced exit on healthy languages. The budget clock now starts when validate runs; factory-import time is excluded (regression-tested with a 900ms import + 500ms extras under a 1s budget). - A wedged getFactory (hung dynamic import) previously hung doctor forever — the whole probe now runs under a 2x-timeout envelope and reports broken + probe.timedOut, wiring it into the force-exit containment. - probeLanguageEntry now carries the validate outcome itself (validation / validationError / probeable) instead of doctor smuggling it out via mutated closure variables; dead result fields (installed, attach) dropped. - Unknown attach strings from third-party factory metadata are normalized (metadata → entry → 'none') so computeModeAvailability's non-exhaustive switch can no longer yield modes.attach undefined and crash a consumer. - getMetadata throws and validate-less factories now leave a warn-level breadcrumb (main's server previously surfaced the former as a hard error; the graceful fallback kept, the silence not). - Doctor's broken diagnoses are now accurate per shape: load failure (with the underlying error when the registry propagates one — the concrete AdapterRegistry swallows loader errors today, noted on #435), registry-returned-nothing, and loaded-but-no-validate (version skew), each with honest durationMs instead of 0. - list_supported_languages probes all languages in parallel like the doctor path (cold-cache cost = max, not sum). - Parity test reuses the shared createMockEnvironment / createMockFileSystem helpers. Co-Authored-By: Claude Fable 5 --- src/cli/commands/doctor/diagnose.ts | 121 ++++++++++++------ src/server.ts | 50 ++++---- src/utils/language-availability.ts | 53 ++++++-- .../unit/server/server-doctor-parity.test.ts | 21 ++- tests/unit/cli/doctor/diagnose.test.ts | 86 ++++++++++++- .../unit/utils/language-availability.test.ts | 65 ++++++++-- 6 files changed, 302 insertions(+), 94 deletions(-) diff --git a/src/cli/commands/doctor/diagnose.ts b/src/cli/commands/doctor/diagnose.ts index 0ba2d4d9..cccb13cd 100644 --- a/src/cli/commands/doctor/diagnose.ts +++ b/src/cli/commands/doctor/diagnose.ts @@ -12,7 +12,6 @@ */ import type { AttachMechanism, - FactoryValidationResult, IAdapterFactory, IEnvironment, IFileSystem, @@ -167,47 +166,63 @@ async function diagnoseLanguage( deps: DiagnoseDeps, collectExtras: (language: string, details: Record) => Promise> ): Promise { - const started = Date.now(); - let validation: FactoryValidationResult | undefined; - let probeError: unknown; - let timedOut = false; - - // The shared probe (issue #435) — the same function the server's - // list_supported_languages runs, so the two views cannot drift apart. - // Doctor's wrapper adds what the server deliberately omits: a per-probe - // timeout and a record of the raw outcome; the rethrow lets - // computeModeAvailability fail open exactly as it does for the server. - const probe = await probeLanguageEntry( - { - language: entry.name, - packageName: entry.packageName, - installed: entry.installed, - attach: entry.attach - }, - { - registry: deps.registry, - disabledSet, - runValidate: async (_language, validate) => { - try { - validation = await withTimeout(validate(), deps.timeoutMs); - return validation; - } catch (error) { - probeError = error; - timedOut = error instanceof ProbeTimeoutError; - throw error; - } - }, - logger: deps.logger - } - ); + const probeStarted = Date.now(); + // The validate/extras budget clock starts when validate actually runs, so a + // slow factory import cannot eat the budget and manufacture timeouts. + let validateStarted = probeStarted; const base = { language: entry.name, package: entry.packageName, installed: entry.installed, - disabled: probe.disabled + disabled: disabledSet.has(entry.name) }; + // The shared probe (issue #435) — the same function the server's + // list_supported_languages runs, so the two views cannot drift apart. + // Doctor's wrapper adds what the server deliberately omits: a per-probe + // timeout (the probe carries the outcome back; computeModeAvailability + // fails open on the rethrow exactly as it does for the server). The outer + // timeout bounds everything else — chiefly a wedged getFactory dynamic + // import, which validate's inner timeout can never see. + let probe; + try { + probe = await withTimeout( + probeLanguageEntry( + { + language: entry.name, + packageName: entry.packageName, + installed: entry.installed, + attach: entry.attach + }, + { + registry: deps.registry, + disabledSet, + runValidate: (_language, validate) => { + validateStarted = Date.now(); + return withTimeout(validate(), deps.timeoutMs); + }, + logger: deps.logger + } + ), + deps.timeoutMs * 2 + ); + } catch (error) { + const outerTimedOut = error instanceof ProbeTimeoutError; + return { + ...base, + verdict: 'broken', + errors: [ + outerTimedOut + ? `Adapter probe timed out after ${deps.timeoutMs * 2}ms while loading the adapter — ` + + `the ${entry.packageName} import may be wedged` + : `Adapter probe failed: ${error instanceof Error ? error.message : String(error)}` + ], + warnings: [], + probe: { durationMs: Date.now() - probeStarted, timedOut: outerTimedOut, failed: !outerTimedOut } + }; + } + if (probe.disabled || !entry.installed) { return { ...base, @@ -219,13 +234,15 @@ async function diagnoseLanguage( }; } - if (!probe.factory || typeof probe.factory.validate !== 'function') { + if (!probe.factory) { // An installed adapter whose factory cannot even be loaded cannot start // any session — that is broken, and a gated run must fail. (The server // fails open here; probe.modes reflects that so the divergence stays // visible rather than silent.) const loadDetail = - probe.factoryLoadError instanceof Error ? `: ${probe.factoryLoadError.message}` : ''; + probe.factoryLoadError instanceof Error + ? `: ${probe.factoryLoadError.message}` + : ' (the registry returned no factory)'; return { ...base, verdict: 'broken', @@ -236,19 +253,40 @@ async function diagnoseLanguage( ], warnings: [], modes: probe.modes, - probe: { durationMs: 0, timedOut: false, failed: true } + probe: { durationMs: Date.now() - probeStarted, timedOut: false, failed: true } + }; + } + + if (!probe.probeable) { + // The factory loaded but is not probeable — a different defect than a + // load failure, and 'reinstall' phrasing would misdiagnose it. + return { + ...base, + verdict: 'broken', + errors: [ + `Adapter factory loaded but exposes no validate() function — the installed ${entry.packageName} ` + + `is likely version-skewed relative to this server. ` + + `(The server assumes availability when it cannot probe.)` + ], + warnings: [], + modes: probe.modes, + probe: { durationMs: Date.now() - probeStarted, timedOut: false, failed: true } }; } + const validation = probe.validation; + const probeError = probe.validationError; + let timedOut = probeError instanceof ProbeTimeoutError; 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. A hung extras child is flagged via probe.timedOut so the handler's + // 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 remainingMs = Math.max(0, deps.timeoutMs - (Date.now() - started)); + 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) { @@ -261,7 +299,8 @@ async function diagnoseLanguage( } } } - const durationMs = Date.now() - started; + // Validate + extras — the toolchain's own cost, excluding the module import + const durationMs = Date.now() - validateStarted; let verdict: DoctorVerdict; let errors: string[]; diff --git a/src/server.ts b/src/server.ts index c568adab..daec6fd7 100644 --- a/src/server.ts +++ b/src/server.ts @@ -2831,32 +2831,36 @@ export class DebugMcpServer { } // Shared per-entry probe (issue #435): doctor consumes the same - // function, so the two views cannot drift apart. + // function, so the two views cannot drift apart. Probes run in + // parallel — on a cold cache each may import an adapter package and + // spawn a toolchain check, and this call should pay the max, not the + // sum (the doctor path already runs them concurrently). const disabledSet = getDisabledLanguages(); - const available: AvailableLanguage[] = []; - for (const entry of baseEntries) { - const probe = await probeLanguageEntry( - { + const available: AvailableLanguage[] = await Promise.all( + baseEntries.map(async (entry) => { + const probe = await probeLanguageEntry( + { + language: entry.language, + packageName: entry.package, + installed: entry.installed, + attach: entry.attach + }, + { + registry: dyn, + disabledSet, + runValidate: (language, validate) => this.validationCache.get(language, validate), + logger: this.logger + } + ); + return { language: entry.language, - packageName: entry.package, + package: entry.package, installed: entry.installed, - attach: entry.attach - }, - { - registry: dyn, - disabledSet, - runValidate: (language, validate) => this.validationCache.get(language, validate), - logger: this.logger - } - ); - available.push({ - language: entry.language, - package: entry.package, - installed: entry.installed, - description: entry.description, - modes: probe.modes - }); - } + description: entry.description, + modes: probe.modes + }; + }) + ); // Also build simple metadata array for backward compatibility with previous payload shape const languageMetadata = await this.getLanguageMetadata(); diff --git a/src/utils/language-availability.ts b/src/utils/language-availability.ts index 03458283..327eb7d6 100644 --- a/src/utils/language-availability.ts +++ b/src/utils/language-availability.ts @@ -135,16 +135,29 @@ export interface AvailabilityProbeOptions { export interface LanguageAvailabilityProbe { disabled: boolean; - installed: boolean; /** The loaded factory, when one was loaded (installed, enabled, load succeeded). */ factory?: ProbeableAdapterFactory; /** Set when registry.getFactory threw (doctor reports it; the server fails open). */ factoryLoadError?: unknown; - /** Metadata-preferred attach mechanism (entry attach when metadata is unavailable). */ - attach: AttachMechanism; + /** Whether a validate() probe could actually run (factory loaded and callable). */ + probeable: boolean; + /** The validate() outcome, when the probe ran and settled successfully. */ + validation?: FactoryValidationResult; + /** The validate() rejection, when the probe ran and threw (modes fail open). */ + validationError?: unknown; modes: LanguageModes; } +const KNOWN_ATTACH: readonly AttachMechanism[] = ['none', 'direct-connect', 'spawn']; + +/** + * Third-party factories are plain JS — an unknown attach string would fall + * through computeModeAvailability's switch and leave modes.attach undefined. + */ +function normalizeAttach(value: unknown, fallback: AttachMechanism): AttachMechanism { + return KNOWN_ATTACH.includes(value as AttachMechanism) ? (value as AttachMechanism) : fallback; +} + /** * The one per-entry availability probe shared by list_supported_languages and * `mcp-debugger doctor` (issue #435): disabled gate → installed gate → @@ -158,7 +171,7 @@ export async function probeLanguageEntry( options: AvailabilityProbeOptions ): Promise { const disabled = options.disabledSet.has(entry.language); - const entryAttach = entry.attach ?? 'none'; + const entryAttach = normalizeAttach(entry.attach, 'none'); let factory: ProbeableAdapterFactory | undefined; let factoryLoadError: unknown; @@ -171,20 +184,31 @@ export async function probeLanguageEntry( } // A malformed factory's getMetadata must not take the probe down — fall - // back to the registry entry's attach declaration. + // back to the registry entry's attach declaration, but leave a breadcrumb. let attach = entryAttach; if (factory) { try { - attach = factory.getMetadata().modes?.attach ?? entryAttach; - } catch { + attach = normalizeAttach(factory.getMetadata().modes?.attach, entryAttach); + } catch (error) { attach = entryAttach; + options.logger?.warn?.( + `[language-availability] getMetadata() threw for '${entry.language}'; using the registry attach declaration. ` + + `${error instanceof Error ? error.message : String(error)}` + ); } } const runValidate = options.runValidate ?? ((_language, validate) => validate()); const probeable = factory !== undefined && typeof factory.validate === 'function'; + if (factory && !probeable) { + options.logger?.warn?.( + `[language-availability] factory for '${entry.language}' has no validate() function; assuming available.` + ); + } + let validation: FactoryValidationResult | undefined; + let validationError: unknown; const modes = await computeModeAvailability({ language: entry.language, packageName: entry.packageName, @@ -192,17 +216,26 @@ export async function probeLanguageEntry( disabled, attach, validate: probeable - ? () => runValidate(entry.language, () => factory!.validate()) + ? async () => { + try { + validation = await runValidate(entry.language, () => factory!.validate()); + return validation; + } catch (error) { + validationError = error; + throw error; // computeModeAvailability fails open and logs + } + } : undefined, logger: options.logger }); return { disabled, - installed: entry.installed, factory, factoryLoadError, - attach, + probeable, + validation, + validationError, modes }; } diff --git a/tests/core/unit/server/server-doctor-parity.test.ts b/tests/core/unit/server/server-doctor-parity.test.ts index b203fbe8..bcd144c6 100644 --- a/tests/core/unit/server/server-doctor-parity.test.ts +++ b/tests/core/unit/server/server-doctor-parity.test.ts @@ -11,7 +11,10 @@ import { DebugMcpServer } from '../../../../src/server.js'; import { SessionManager } from '../../../../src/session/session-manager.js'; import { createProductionDependencies } from '../../../../src/container/dependencies.js'; import { diagnose, type DiagnoseDeps } from '../../../../src/cli/commands/doctor/diagnose.js'; -import type { IEnvironment, IFileSystem } from '@debugmcp/shared'; +import { + createMockEnvironment, + createMockFileSystem +} from '../../../test-utils/helpers/test-dependencies.js'; import { createMockDependencies, createMockServer, @@ -117,18 +120,14 @@ describe('doctor / list_supported_languages availability parity (issue #435)', ( ); // Doctor path, against a fresh identical registry (no shared validation cache) + const fileSystem = createMockFileSystem(); + vi.mocked(fileSystem.readFile).mockRejectedValue(new Error('ENOENT')); + vi.mocked(fileSystem.stat).mockRejectedValue(new Error('ENOENT')); + vi.mocked(fileSystem.readdir).mockRejectedValue(new Error('ENOENT')); const doctorDeps: DiagnoseDeps = { registry: buildSharedRegistry() as unknown as DiagnoseDeps['registry'], - environment: { - get: () => undefined, - getAll: () => ({}), - getCurrentWorkingDirectory: () => process.cwd() - } as IEnvironment, - fileSystem: { - readFile: vi.fn().mockRejectedValue(new Error('ENOENT')), - stat: vi.fn().mockRejectedValue(new Error('ENOENT')), - readdir: vi.fn().mockRejectedValue(new Error('ENOENT')) - } as unknown as IFileSystem, + environment: createMockEnvironment(), + fileSystem, env: { DEBUG_MCP_DISABLE_LANGUAGES: 'mock' }, platform: 'linux', timeoutMs: 5000, diff --git a/tests/unit/cli/doctor/diagnose.test.ts b/tests/unit/cli/doctor/diagnose.test.ts index 39a069d4..4c32408f 100644 --- a/tests/unit/cli/doctor/diagnose.test.ts +++ b/tests/unit/cli/doctor/diagnose.test.ts @@ -26,6 +26,10 @@ interface FakeAdapterSpec { installed?: boolean; attach?: 'none' | 'direct-connect' | 'spawn'; validate?: () => Promise<{ valid: boolean; errors: string[]; warnings: string[]; details?: Record }>; + /** Return a loaded factory that lacks a validate function (version skew). */ + factoryWithoutValidate?: boolean; + /** Delay (ms) before getFactory resolves — models a slow dynamic import. */ + factoryLoadDelayMs?: number; } function makeDeps(adapters: FakeAdapterSpec[], overrides: Partial = {}): DiagnoseDeps { @@ -40,7 +44,16 @@ function makeDeps(adapters: FakeAdapterSpec[], overrides: Partial ), getFactory: vi.fn(async (language: string) => { const spec = adapters.find((a) => a.name === language); - if (!spec || !(spec.installed ?? true) || !spec.validate) { + if (!spec || !(spec.installed ?? true)) { + return undefined; + } + if (spec.factoryLoadDelayMs) { + await new Promise((resolve) => setTimeout(resolve, spec.factoryLoadDelayMs)); + } + if (spec.factoryWithoutValidate) { + return { getMetadata: () => ({ modes: { launch: true, attach: spec.attach ?? 'none' } }) }; + } + if (!spec.validate) { return undefined; } return { @@ -235,6 +248,77 @@ 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 () => { + vi.useFakeTimers(); + const deps = makeDeps( + [ + { + name: 'dotnet', + factoryLoadDelayMs: 900, // slow cold import eats most of a naive shared budget + validate: okValidate({ debuggerPath: '/x' }) + } + ], + { + timeoutMs: 1000, + collectExtras: () => + new Promise((resolve) => setTimeout(() => resolve({ dotnetSdkVersion: '8.0.301' }), 500)) + } + ); + + const reportPromise = diagnose([], deps); + await vi.advanceTimersByTimeAsync(2000); + const report = await reportPromise; + + 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 + }); + + it('reports broken with probe.timedOut when getFactory itself hangs (wedged dynamic import)', async () => { + vi.useFakeTimers(); + const deps = makeDeps([{ name: 'java', validate: okValidate() }], { timeoutMs: 1000 }); + (deps.registry as unknown as { getFactory: ReturnType }).getFactory = vi.fn( + () => new Promise(() => undefined) + ); + + const reportPromise = diagnose([], deps); + await vi.advanceTimersByTimeAsync(3000); + const report = await reportPromise; + + const java = report.languages[0]; + expect(java.verdict).toBe('broken'); + expect(java.probe.timedOut).toBe(true); + }); + + it('reports the elapsed load time on a broken factory instead of durationMs 0', async () => { + vi.useFakeTimers(); + const deps = makeDeps([{ name: 'python', installed: true }], { timeoutMs: 5000 }); + (deps.registry as unknown as { getFactory: ReturnType }).getFactory = vi.fn( + () => + new Promise((_resolve, reject) => setTimeout(() => reject(new Error('import exploded')), 400)) + ); + + const reportPromise = diagnose([], deps); + await vi.advanceTimersByTimeAsync(500); + const report = await reportPromise; + + expect(report.languages[0].verdict).toBe('broken'); + expect(report.languages[0].probe.durationMs).toBeGreaterThanOrEqual(400); + }); + + it('diagnoses a loaded factory without validate() as version skew, not a load failure', async () => { + const deps = makeDeps([{ name: 'python', factoryWithoutValidate: true }]); + + const report = await diagnose(['python'], deps); + + const python = report.languages[0]; + expect(python.verdict).toBe('broken'); + expect(python.errors[0]).toContain('validate'); + expect(python.errors[0]).not.toContain('could not be loaded'); + expect(report.exitCode).toBe(1); + }); + it('sets probe.timedOut when the extras collector hangs, so the handler can force-exit', async () => { vi.useFakeTimers(); const deps = makeDeps( diff --git a/tests/unit/utils/language-availability.test.ts b/tests/unit/utils/language-availability.test.ts index 8775c765..c0ab8632 100644 --- a/tests/unit/utils/language-availability.test.ts +++ b/tests/unit/utils/language-availability.test.ts @@ -189,11 +189,12 @@ describe('probeLanguageEntry (issue #435)', () => { disabledSet: new Set() }); - expect(probe.attach).toBe('spawn'); + // entry said 'none' (unsupported); metadata's 'spawn' must win expect(probe.modes.attach.supported).toBe(true); }); - it('falls back to the entry attach when getMetadata throws, without failing the probe', async () => { + it('falls back to the entry attach when getMetadata throws, and logs a warning', async () => { + const warn = vi.fn(); const factory = makeFactory({ getMetadata: vi.fn(() => { throw new Error('metadata exploded'); @@ -202,11 +203,48 @@ describe('probeLanguageEntry (issue #435)', () => { const probe = await probeLanguageEntry(entry({ attach: 'direct-connect' }), { registry: { getFactory: vi.fn().mockResolvedValue(factory) }, - disabledSet: new Set() + disabledSet: new Set(), + logger: { warn } }); - expect(probe.attach).toBe('direct-connect'); + expect(probe.modes.attach.supported).toBe(true); // direct-connect from the entry expect(probe.modes.launch.available).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('metadata exploded')); + }); + + it('normalizes an unknown metadata attach string instead of producing undefined attach modes', async () => { + // A version-skewed third-party factory (plain JS) can return any string; + // computeModeAvailability's switch is not exhaustive, so an unknown value + // must be normalized here or modes.attach comes back undefined and + // crashes consumers. + const factory = makeFactory({ + getMetadata: vi.fn().mockReturnValue({ modes: { launch: true, attach: 'tcp' } }) + }); + + const probe = await probeLanguageEntry(entry({ attach: 'direct-connect' }), { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set() + }); + + expect(probe.modes.attach).toBeDefined(); + expect(probe.modes.attach.supported).toBe(true); // fell back to the entry's direct-connect + }); + + it("normalizes to 'none' when both metadata and entry attach are unknown strings", async () => { + const factory = makeFactory({ + getMetadata: vi.fn().mockReturnValue({ modes: { launch: true, attach: 'tcp' } }) + }); + + const probe = await probeLanguageEntry( + entry({ attach: 'udp' as never }), + { + registry: { getFactory: vi.fn().mockResolvedValue(factory) }, + disabledSet: new Set() + } + ); + + expect(probe.modes.attach).toBeDefined(); + expect(probe.modes.attach.supported).toBe(false); // treated as 'none' }); it('records a factory load failure and fails open like the server', async () => { @@ -230,7 +268,7 @@ describe('probeLanguageEntry (issue #435)', () => { expect(probe.modes.launch.available).toBe(true); }); - it('routes validate through the runValidate wrapper (cache/timeout injection point)', async () => { + it('routes validate through the runValidate wrapper and carries the result on the probe', async () => { const factory = makeFactory(); const runValidate = vi.fn(async (_language: string, validate: () => Promise) => { return validate(); @@ -244,6 +282,9 @@ describe('probeLanguageEntry (issue #435)', () => { expect(runValidate).toHaveBeenCalledWith('python', expect.any(Function)); expect(factory.validate).toHaveBeenCalledTimes(1); + expect(probe.probeable).toBe(true); + expect(probe.validation).toEqual(ok); + expect(probe.validationError).toBeUndefined(); expect(probe.modes.launch.available).toBe(true); }); @@ -257,6 +298,7 @@ describe('probeLanguageEntry (issue #435)', () => { runValidate }); + expect(probe.validation).toEqual(bad('toolchain gone')); expect(probe.modes.launch).toEqual({ supported: true, available: false, @@ -265,7 +307,7 @@ describe('probeLanguageEntry (issue #435)', () => { expect(factory.validate).not.toHaveBeenCalled(); }); - it('fails open when the wrapped validate throws, mirroring computeModeAvailability', async () => { + it('fails open and records validationError when the wrapped validate throws', async () => { const warn = vi.fn(); const factory = makeFactory({ validate: vi.fn().mockRejectedValue(new Error('probe exploded')) }); @@ -275,19 +317,26 @@ describe('probeLanguageEntry (issue #435)', () => { logger: { warn } }); + expect(probe.validation).toBeUndefined(); + expect(probe.validationError).toBeInstanceOf(Error); expect(probe.modes.launch.available).toBe(true); expect(warn).toHaveBeenCalledWith(expect.stringContaining('probe exploded')); }); - it('treats a factory without a validate function as unprobeable (assume valid)', async () => { + it('marks a factory without a validate function unprobeable (assume valid) and logs a warning', async () => { + const warn = vi.fn(); const factory = { getMetadata: vi.fn().mockReturnValue({}) }; const probe = await probeLanguageEntry(entry(), { registry: { getFactory: vi.fn().mockResolvedValue(factory) }, - disabledSet: new Set() + disabledSet: new Set(), + logger: { warn } }); + expect(probe.probeable).toBe(false); + expect(probe.factory).toBeDefined(); expect(probe.modes.launch.available).toBe(true); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('validate')); }); });