From 2357b443b5edc95f0ab458c28e3d0400ef42030f Mon Sep 17 00:00:00 2001 From: JF Date: Sat, 22 Aug 2026 22:22:38 -0400 Subject: [PATCH 1/2] refactor(availability): launch gate on the shared probe; typed IAdapterRegistry surface (#435, parts 3-4) Part 3 - checkLaunchToolchain is now a thin wrapper over probeLanguageEntry with a synthetic entry (installed: true, empty disabledSet, which provably neutralize the probe's notInstalled/disabled short-circuits - disabled languages are refused upstream at create_debug_session). The gate's answer and reason text are now literally computeModeAvailability's launch mode, so the gate can no longer drift from list_supported_languages the way the server/doctor loops used to. Direct unit tests pin the full fail-open contract, and a drift-fence matrix asserts gate === probe.modes.launch across every registry shape (the gate-side analogue of the doctor parity test). Part 4 - IAdapterRegistry gains the typed discovery surface the server was reaching through `as unknown as` casts: required listLanguages / listAvailableAdapters / getFactory / getFactoryMetadata (plus register aligned to Promise), and optional getFactoryResult. All four cast sites (server.ts x3, session-manager-operations.ts) are removed; a rename on the concrete AdapterRegistry now breaks the build instead of silently degrading every language to fail-open. Doctor's duplicate DoctorRegistry structural type becomes a Pick of the interface. getFactoryResult fixes the swallowed-loader-error hole: AdapterRegistry's `catch { return undefined }` collapsed a missing package, a broken export, and dynamic-loading-disabled into one undefined. getFactory now delegates to getFactoryResult (contract unchanged, one code path), the availability probe prefers getFactoryResult when present, and doctor finally reports the loader's real error message instead of "(the registry returned no factory)". The two remaining discovery-fallback swallows now log warnings. New shared types: AdapterManifestEntry (the loader manifest shape, disambiguated from the factory-declared AdapterMetadata), FactoryLoadResult. Test fixtures: the central IAdapterRegistry mock now provides fail-open- neutral defaults for the required members (mirroring the pre-typed fallback paths); the WithErrors/WithLanguages builders override the discovery members consistently. Co-Authored-By: Claude Fable 5 --- packages/shared/src/index.ts | 2 + .../shared/src/interfaces/adapter-registry.ts | 75 +++++- src/adapters/adapter-loader.ts | 14 +- src/adapters/adapter-registry.ts | 70 ++++-- src/cli/commands/doctor/diagnose.ts | 29 ++- src/server.ts | 26 +- src/session/session-manager-operations.ts | 9 +- src/utils/language-availability.ts | 72 ++++-- .../test-utils/mocks/mock-adapter-registry.ts | 41 +++- tests/unit/adapters/adapter-registry.test.ts | 45 ++++ tests/unit/cli/doctor/diagnose.test.ts | 18 ++ .../unit/utils/language-availability.test.ts | 231 ++++++++++++++++++ 12 files changed, 538 insertions(+), 94 deletions(-) diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index d13011a2..b738158f 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -66,6 +66,8 @@ export type { AdapterModes, AttachMechanism, AdapterInfo, + AdapterManifestEntry, + FactoryLoadResult, AdapterRegistryConfig, // Validation diff --git a/packages/shared/src/interfaces/adapter-registry.ts b/packages/shared/src/interfaces/adapter-registry.ts index 753579f1..e68fa064 100644 --- a/packages/shared/src/interfaces/adapter-registry.ts +++ b/packages/shared/src/interfaces/adapter-registry.ts @@ -20,7 +20,7 @@ export interface IAdapterRegistry { * @param factory Factory to create adapter instances * @throws Error if language is already registered */ - register(language: string, factory: IAdapterFactory): void; + register(language: string, factory: IAdapterFactory): Promise; /** * Unregister an adapter factory @@ -55,6 +55,42 @@ export interface IAdapterRegistry { */ isLanguageSupported(language: string): boolean; + /** + * List every known language, registered or dynamically loadable. + */ + listLanguages(): Promise; + + /** + * List every known adapter with its install state and attach mechanism. + */ + listAvailableAdapters(): Promise; + + /** + * Get the factory for a language, dynamically loading it when enabled. + * Returns undefined when no factory can be produced (never throws) — use + * getFactoryResult when the failure reason matters. + * + * These four members are the typed surface behind issue #435 part 4: the + * availability probe and launch gate used to reach them through + * `as unknown as` duck-typing, so a rename on the concrete registry + * compiled clean and silently degraded every language to fail-open. + */ + getFactory(language: string): Promise; + + /** + * Get a language's metadata without registering or instantiating anything. + */ + getFactoryMetadata(language: string): Promise; + + /** + * Diagnostics variant of getFactory: never throws, and carries the load + * failure so doctor can report the real import error instead of "the + * registry returned no factory". Optional so minimal registry doubles + * that stub only getFactory keep working; consumers must treat absence + * as "use getFactory". + */ + getFactoryResult?(language: string): Promise; + /** * Get metadata about a registered adapter * @param language Language identifier @@ -226,6 +262,43 @@ export interface FactoryValidationResult { details?: Record; } +/** + * A known-adapter manifest entry, as reported by + * IAdapterRegistry.listAvailableAdapters(): the static facts about an + * adapter package independent of whether its factory has been loaded. + */ +export interface AdapterManifestEntry { + /** Language identifier, e.g. 'python' */ + name: string; + + /** npm package name, e.g. '@debugmcp/adapter-python' */ + packageName: string; + + /** Human-readable description */ + description?: string; + + /** Whether the adapter package is installed in this runtime */ + installed: boolean; + + /** How the adapter implements attach; absent means unknown (treat as 'none') */ + attach?: AttachMechanism; +} + +/** + * Outcome of a non-throwing factory-load attempt + * (IAdapterRegistry.getFactoryResult). Exactly one of the fields is set. + */ +export interface FactoryLoadResult { + /** The loaded factory, when one could be produced */ + factory?: IAdapterFactory; + + /** Set when a dynamic load was attempted and failed */ + loadError?: Error; + + /** Set when nothing is registered/cached and dynamic loading is disabled */ + dynamicLoadingDisabled?: boolean; +} + /** * One resolved toolchain component (a doctor table cell): the runtime a * debuggee needs (Python, Node.js, a C++ compiler) or the debug backend that diff --git a/src/adapters/adapter-loader.ts b/src/adapters/adapter-loader.ts index 5b8f681a..3d74aa64 100644 --- a/src/adapters/adapter-loader.ts +++ b/src/adapters/adapter-loader.ts @@ -1,4 +1,4 @@ -import { IAdapterFactory, AttachMechanism } from '@debugmcp/shared'; +import { IAdapterFactory, AttachMechanism, AdapterManifestEntry } from '@debugmcp/shared'; import type { Logger as WinstonLogger } from 'winston'; import { createLogger } from '../utils/logger.js'; import { createRequire } from 'module'; @@ -53,11 +53,13 @@ export function createDefaultPackageResolver(io: { }; } -export interface AdapterMetadata { - name: string; - packageName: string; - description?: string; - installed: boolean; +/** + * The loader's manifest entry — the shared AdapterManifestEntry with attach + * required (the known-adapter list always declares it). Kept as a distinct + * name because shared's AdapterMetadata is the factory-declared metadata, a + * different shape entirely. + */ +export interface AdapterMetadata extends AdapterManifestEntry { /** How the adapter implements attach mode (static knowledge; kept in sync with each factory's declaration) */ attach: AttachMechanism; } diff --git a/src/adapters/adapter-registry.ts b/src/adapters/adapter-registry.ts index ffeebdfb..fa812310 100644 --- a/src/adapters/adapter-registry.ts +++ b/src/adapters/adapter-registry.ts @@ -4,9 +4,9 @@ * @since 2.0.0 */ import { EventEmitter } from 'events'; -import { - IAdapterRegistry, - IAdapterFactory, +import { + IAdapterRegistry, + IAdapterFactory, AdapterDependencies, AdapterInfo, AdapterNotFoundError, @@ -17,9 +17,10 @@ import { ActiveAdapterMap } from '@debugmcp/shared'; import { IDebugAdapter, AdapterConfig } from '@debugmcp/shared'; -import type { AdapterMetadata as SharedAdapterMetadata } from '@debugmcp/shared'; +import type { AdapterMetadata as SharedAdapterMetadata, AdapterManifestEntry, FactoryLoadResult } from '@debugmcp/shared'; import { AdapterLoader } from './adapter-loader.js'; import type { AdapterMetadata } from './adapter-loader.js'; +import { createLogger } from '../utils/logger.js'; /** * Default registry configuration @@ -43,6 +44,7 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { private readonly disposeTimers = new Map(); private readonly registrationTimestamps = new Map(); private readonly loader = new AdapterLoader(); + private readonly logger = createLogger('AdapterRegistry'); // Dynamic loading is opt-in via constructor config or MCP_CONTAINER=true env var private readonly dynamicEnabled: boolean; @@ -259,8 +261,14 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { installed.add(adapter.name); } } - } catch { - // Ignore loader errors in bundled environments where adapters are embedded. + } catch (error) { + // Fall back to registered adapters (bundled environments embed them), + // but leave a breadcrumb — a broken loader should not be silent. + this.logger.warn( + `[AdapterRegistry] listLanguages: loader discovery failed, falling back to registered adapters: ${ + error instanceof Error ? error.message : String(error) + }` + ); } // Always include statically registered adapters so bundled builds expose them. @@ -274,7 +282,7 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { /** * List detailed adapter metadata (known + install status) */ - async listAvailableAdapters(): Promise { + async listAvailableAdapters(): Promise { const registered = new Set(this.getSupportedLanguages()); const buildEntry = (language: string): AdapterMetadata => ({ @@ -298,8 +306,13 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { results.set(adapter.name, { ...adapter, installed }); registered.delete(adapter.name); } - } catch { - // Ignore loader failures and fall back to registered adapters. + } catch (error) { + // Fall back to registered adapters, but leave a breadcrumb. + this.logger.warn( + `[AdapterRegistry] listAvailableAdapters: loader discovery failed, falling back to registered adapters: ${ + error instanceof Error ? error.message : String(error) + }` + ); } for (const language of registered) { @@ -310,27 +323,40 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { } /** - * Get the factory for a language without creating an adapter instance. - * Checks registered factories first, then the loader cache, then attempts - * a dynamic load (when enabled). Returns undefined if unavailable. + * Get the factory for a language without creating an adapter instance, + * with the load failure preserved (issue #435 part 4): checks registered + * factories first, then the loader cache, then attempts a dynamic load + * (when enabled). Never throws — a failed load comes back as loadError so + * the availability probe can surface the real import error instead of + * "the registry returned no factory". */ - async getFactory(language: string): Promise { + async getFactoryResult(language: string): Promise { const registered = this.factories.get(language); if (registered) { - return registered; + return { factory: registered }; } const cached = this.loader.getCachedFactory(language); if (cached) { - return cached; + return { factory: cached }; } - if (this.dynamicEnabled) { - try { - return await this.loader.loadAdapter(language); - } catch { - return undefined; - } + if (!this.dynamicEnabled) { + return { dynamicLoadingDisabled: true }; } - return undefined; + try { + return { factory: await this.loader.loadAdapter(language) }; + } catch (error) { + return { loadError: error instanceof Error ? error : new Error(String(error)) }; + } + } + + /** + * Get the factory for a language without creating an adapter instance. + * Fail-open contract: returns undefined whenever no factory can be + * produced, whatever the reason — use getFactoryResult when the reason + * matters. + */ + async getFactory(language: string): Promise { + return (await this.getFactoryResult(language)).factory; } /** diff --git a/src/cli/commands/doctor/diagnose.ts b/src/cli/commands/doctor/diagnose.ts index 071719ee..f385a233 100644 --- a/src/cli/commands/doctor/diagnose.ts +++ b/src/cli/commands/doctor/diagnose.ts @@ -11,8 +11,8 @@ * probe.failed / probe.timedOut so the divergence is visible). */ import type { - AttachMechanism, - IAdapterFactory, + AdapterManifestEntry, + IAdapterRegistry, IEnvironment, IFileSystem, ILogger, @@ -59,18 +59,17 @@ export interface DoctorReport { exitCode: 0 | 1; } -interface RegistryAdapterEntry { - name: string; - packageName: string; - installed: boolean; - attach?: AttachMechanism; - description?: string; -} - -export interface DoctorRegistry { - listAvailableAdapters(): Promise; - getFactory(language: string): Promise; -} +/** + * The slice of IAdapterRegistry doctor needs (issue #435 part 4): the same + * typed surface the server uses, so a registry rename breaks doctor at + * compile time too. getFactoryResult stays optional (it is optional on the + * interface); when present, the shared probe prefers it and surfaces real + * load errors. + */ +export type DoctorRegistry = Pick< + IAdapterRegistry, + 'listAvailableAdapters' | 'getFactory' | 'getFactoryResult' +>; export interface DiagnoseDeps { registry: DoctorRegistry; @@ -157,7 +156,7 @@ export async function diagnose(requested: string[], deps: DiagnoseDeps): Promise } async function diagnoseLanguage( - entry: RegistryAdapterEntry, + entry: AdapterManifestEntry, disabledSet: Set, deps: DiagnoseDeps ): Promise { diff --git a/src/server.ts b/src/server.ts index daec6fd7..6fa3675f 100644 --- a/src/server.ts +++ b/src/server.ts @@ -50,8 +50,7 @@ import { probeLanguageEntry, checkLaunchToolchain, ValidationResultCache, - LanguageModes, - ProbeableAdapterFactory + LanguageModes } from './utils/language-availability.js'; import { isContainerMode, getWorkspaceRoot } from './utils/container-path-utils.js'; import { @@ -275,9 +274,9 @@ export class DebugMcpServer { if (!adapterRegistry) { return filter(getDefaultLanguages()); } - // Prefer dynamic discovery if available on the concrete registry - const dynRegistry = adapterRegistry as unknown as { listLanguages?: () => Promise }; - const maybeList = dynRegistry.listLanguages; + // Prefer dynamic discovery. listLanguages is on IAdapterRegistry (issue + // #435 part 4); the runtime guard stays for partial registry doubles. + const maybeList = adapterRegistry.listLanguages; if (typeof maybeList === 'function') { try { const langs = await maybeList.call(adapterRegistry); @@ -1245,9 +1244,7 @@ export class DebugMcpServer { this.logger ); if (!launchGate.available) { - const registry = this.getAdapterRegistry() as unknown as { - getFactory?: (language: string) => Promise<{ getMetadata?: () => { modes?: { attach?: string } } } | undefined>; - } | undefined; + const registry = this.getAdapterRegistry(); const attachMechanism = await (async () => { try { const factory = typeof registry?.getFactory === 'function' @@ -2810,14 +2807,11 @@ export class DebugMcpServer { attach: 'none' as const })); - const dyn = adapterRegistry as unknown as { - listAvailableAdapters?: () => Promise>; - getFactory?: (language: string) => Promise; - } | undefined; - - if (adapterRegistry && typeof dyn?.listAvailableAdapters === 'function') { + // listAvailableAdapters/getFactory are on IAdapterRegistry (issue #435 + // part 4); the runtime guards stay for partial registry doubles. + if (adapterRegistry && typeof adapterRegistry.listAvailableAdapters === 'function') { try { - const meta = await dyn.listAvailableAdapters!(); + const meta = await adapterRegistry.listAvailableAdapters(); baseEntries = meta.map(m => ({ language: m.name, package: m.packageName, @@ -2846,7 +2840,7 @@ export class DebugMcpServer { attach: entry.attach }, { - registry: dyn, + registry: adapterRegistry, disabledSet, runValidate: (language, validate) => this.validationCache.get(language, validate), logger: this.logger diff --git a/src/session/session-manager-operations.ts b/src/session/session-manager-operations.ts index 885c2f15..a994838f 100644 --- a/src/session/session-manager-operations.ts +++ b/src/session/session-manager-operations.ts @@ -2513,11 +2513,10 @@ export abstract class SessionManagerOperations extends SessionManagerData { // before any state mutation (issue #331). Only an explicit 'none' // declaration is enforced — absent metadata falls through to the // adapter's natural behavior. - const registryWithMeta = this.adapterRegistry as unknown as { - getFactoryMetadata?: (language: string) => Promise<{ modes?: { attach?: string } } | undefined>; - }; - if (typeof registryWithMeta.getFactoryMetadata === 'function') { - const factoryMeta = await registryWithMeta.getFactoryMetadata(session.language).catch(() => undefined); + // getFactoryMetadata is on IAdapterRegistry (issue #435 part 4); the + // runtime guard stays for partial registry doubles. + if (typeof this.adapterRegistry.getFactoryMetadata === 'function') { + const factoryMeta = await this.adapterRegistry.getFactoryMetadata(session.language).catch(() => undefined); if (factoryMeta?.modes?.attach === 'none') { return { success: false, diff --git a/src/utils/language-availability.ts b/src/utils/language-availability.ts index 880a0684..28f55fff 100644 --- a/src/utils/language-availability.ts +++ b/src/utils/language-availability.ts @@ -66,37 +66,43 @@ export class ValidationResultCache { /** * Launch-mode gate shared by create_debug_session and start_debugging - * (issue #360): consult the same toolchain probe list_supported_languages - * uses, so a known-unavailable adapter fails fast with the same reason text - * instead of reporting success and silently running nothing. + * (issue #360): a thin wrapper over probeLanguageEntry (issue #435 part 3), + * so the gate's answer — availability and reason text alike — is literally + * the launch mode list_supported_languages reports and cannot drift from it. + * + * The synthetic entry (installed: true, empty disabledSet) provably + * neutralizes the probe's notInstalled/disabled short-circuits: the gate + * gates on the toolchain only — disabled languages are refused upstream + * (server.ts create_debug_session), before this runs. * * Fail-open contract: any probe failure (registry without getFactory, missing - * factory, thrown validate) reports available — enforcement must never block - * a launch the advisory probe can't assess (mirrors computeModeAvailability). + * factory, load error, thrown validate) reports available — enforcement must + * never block a launch the advisory probe can't assess. */ export async function checkLaunchToolchain( language: string, - registry: unknown, + registry: AvailabilityProbeOptions['registry'], cache: ValidationResultCache, logger?: { warn?: (message: string) => void } ): Promise<{ available: true } | { available: false; reason: string }> { try { - const dyn = registry as - | { getFactory?: (language: string) => Promise<{ validate?: () => Promise } | undefined> } - | undefined; - if (typeof dyn?.getFactory !== 'function') { - return { available: true }; - } - const factory = await dyn.getFactory(language); - if (!factory || typeof factory.validate !== 'function') { - return { available: true }; - } - const validate = factory.validate.bind(factory) as () => Promise; - const validation = await cache.get(language, validate); - if (validation.valid) { + const probe = await probeLanguageEntry( + { language, packageName: `@debugmcp/adapter-${language}`, installed: true }, + { + registry, + disabledSet: new Set(), + runValidate: (lang, validate) => cache.get(lang, validate), + logger + } + ); + if (probe.modes.launch.available) { return { available: true }; } - const reason = validation.errors.join('; ') || `The '${language}' debug adapter is not available in this runtime.`; + // computeModeAvailability yields '' when validation.errors is empty; the + // gate's user-facing error needs a sentence either way. + const reason = + probe.modes.launch.reason || + `The '${language}' debug adapter is not available in this runtime.`; return { available: false, reason }; } catch (error) { logger?.warn?.( @@ -123,8 +129,20 @@ export interface LanguageAdapterEntry { export type ProbeableAdapterFactory = Pick; export interface AvailabilityProbeOptions { - /** Source of factories; absent getFactory means "cannot probe" (assume valid). */ - registry?: { getFactory?: (language: string) => Promise }; + /** + * Source of factories; absent getFactory means "cannot probe" (assume + * valid). getFactoryResult is preferred when present — it carries the + * dynamic-load failure, which getFactory's fail-open contract swallows + * (issue #435 part 4). + */ + registry?: { + getFactory?: (language: string) => Promise; + getFactoryResult?: (language: string) => Promise<{ + factory?: ProbeableAdapterFactory; + loadError?: Error; + dynamicLoadingDisabled?: boolean; + }>; + }; disabledSet: Set; /** * Wraps each factory.validate call — the injection point for the server's @@ -179,9 +197,15 @@ export async function probeLanguageEntry( let factory: ProbeableAdapterFactory | undefined; let factoryLoadError: unknown; - if (!disabled && entry.installed && typeof options.registry?.getFactory === 'function') { + if (!disabled && entry.installed) { try { - factory = await options.registry.getFactory(entry.language); + if (typeof options.registry?.getFactoryResult === 'function') { + const result = await options.registry.getFactoryResult(entry.language); + factory = result.factory; + factoryLoadError = result.loadError; + } else if (typeof options.registry?.getFactory === 'function') { + factory = await options.registry.getFactory(entry.language); + } } catch (error) { factoryLoadError = error; } diff --git a/tests/test-utils/mocks/mock-adapter-registry.ts b/tests/test-utils/mocks/mock-adapter-registry.ts index abed671b..9a577163 100644 --- a/tests/test-utils/mocks/mock-adapter-registry.ts +++ b/tests/test-utils/mocks/mock-adapter-registry.ts @@ -124,10 +124,30 @@ export function createMockAdapterRegistry(): IAdapterRegistry { })), register: vi.fn().mockResolvedValue(undefined), - + unregister: vi.fn().mockReturnValue(true), - - getAdapterInfo: vi.fn().mockImplementation((lang: string) => + + // Typed discovery surface (issue #435 part 4). Defaults are fail-open + // neutral and mirror the pre-typed fallback paths: languages from the + // supported list, entries with attach 'none', and no loadable factory + // (consumers assume availability when they cannot probe). Tests that + // need richer behavior override per-test, as before. + listLanguages: vi.fn().mockResolvedValue(supportedLanguages), + + listAvailableAdapters: vi.fn().mockResolvedValue( + supportedLanguages.map((language) => ({ + name: language, + packageName: `@debugmcp/adapter-${language}`, + installed: true, + attach: 'none' as const + })) + ), + + getFactory: vi.fn().mockResolvedValue(undefined), + + getFactoryMetadata: vi.fn().mockResolvedValue(undefined), + + getAdapterInfo: vi.fn().mockImplementation((lang: string) => adapterInfoMap.get(lang) ), @@ -149,6 +169,8 @@ export function createMockAdapterRegistryWithErrors(): IAdapterRegistry { // Override to simulate no languages supported mock.getSupportedLanguages = vi.fn().mockReturnValue([]); mock.isLanguageSupported = vi.fn().mockReturnValue(false); + mock.listLanguages = vi.fn().mockResolvedValue([]); + mock.listAvailableAdapters = vi.fn().mockResolvedValue([]); mock.create = vi.fn().mockRejectedValue(new Error('Adapter not found')); mock.getAdapterInfo = vi.fn().mockReturnValue(undefined); mock.getAllAdapterInfo = vi.fn().mockReturnValue(new Map()); @@ -164,10 +186,19 @@ export function createMockAdapterRegistryWithLanguages(languages: string[]): IAd const mock = createMockAdapterRegistry(); mock.getSupportedLanguages = vi.fn().mockReturnValue(languages); - mock.isLanguageSupported = vi.fn().mockImplementation((lang: string) => + mock.isLanguageSupported = vi.fn().mockImplementation((lang: string) => languages.includes(lang) ); - + mock.listLanguages = vi.fn().mockResolvedValue(languages); + mock.listAvailableAdapters = vi.fn().mockResolvedValue( + languages.map((language) => ({ + name: language, + packageName: `@debugmcp/adapter-${language}`, + installed: true, + attach: 'none' as const + })) + ); + // Update adapter info to match languages const adapterInfoMap = new Map(); languages.forEach(lang => { diff --git a/tests/unit/adapters/adapter-registry.test.ts b/tests/unit/adapters/adapter-registry.test.ts index b9751644..f23e47e3 100644 --- a/tests/unit/adapters/adapter-registry.test.ts +++ b/tests/unit/adapters/adapter-registry.test.ts @@ -187,6 +187,51 @@ describe('AdapterRegistry', () => { ).rejects.toBeInstanceOf(AdapterNotFoundError); }); + describe('getFactory / getFactoryResult', () => { + it('returns a registered factory without touching the loader', async () => { + const registry = new AdapterRegistry(); + const factory = createFactory(); + await registry.register('mock', factory as any); + + await expect(registry.getFactory('mock')).resolves.toBe(factory); + await expect(registry.getFactoryResult('mock')).resolves.toEqual({ factory }); + }); + + it('returns a loader-cached factory', async () => { + const cached = createFactory(); + const registry = new AdapterRegistry({ enableDynamicLoading: true }); + vi.spyOn(registry as any, 'loader', 'get').mockReturnValue({ + getCachedFactory: vi.fn().mockReturnValue(cached), + loadAdapter: vi.fn() + }); + + await expect(registry.getFactory('python')).resolves.toBe(cached); + await expect(registry.getFactoryResult('python')).resolves.toEqual({ factory: cached }); + }); + + it('surfaces the loader error through getFactoryResult while getFactory stays fail-open', async () => { + const registry = new AdapterRegistry({ enableDynamicLoading: true }); + vi.spyOn(registry as any, 'loader', 'get').mockReturnValue({ + getCachedFactory: vi.fn().mockReturnValue(undefined), + loadAdapter: vi.fn().mockRejectedValue(new Error('Failed to load adapter: corrupted dist')) + }); + + await expect(registry.getFactory('python')).resolves.toBeUndefined(); + + const result = await registry.getFactoryResult('python'); + expect(result.factory).toBeUndefined(); + expect(result.loadError).toBeInstanceOf(Error); + expect(result.loadError?.message).toContain('corrupted dist'); + }); + + it('reports dynamicLoadingDisabled for an unregistered language when loading is off', async () => { + const registry = new AdapterRegistry(); + + await expect(registry.getFactory('python')).resolves.toBeUndefined(); + await expect(registry.getFactoryResult('python')).resolves.toEqual({ dynamicLoadingDisabled: true }); + }); + }); + it('auto-disposes adapters on state change and clears timers', async () => { vi.useFakeTimers(); diff --git a/tests/unit/cli/doctor/diagnose.test.ts b/tests/unit/cli/doctor/diagnose.test.ts index 13b5d227..aec3de8a 100644 --- a/tests/unit/cli/doctor/diagnose.test.ts +++ b/tests/unit/cli/doctor/diagnose.test.ts @@ -315,6 +315,24 @@ describe('diagnose', () => { expect(report.languages[0].probe.durationMs).toBeGreaterThanOrEqual(400); }); + it('surfaces the real loader error when the registry provides getFactoryResult (issue #435 part 4)', async () => { + // Same corrupt-package scenario as above, but through a registry that can + // say WHY the load failed — doctor must report that instead of the vague + // "(the registry returned no factory)". + const deps = makeDeps([{ name: 'python', installed: true }]); + (deps.registry as unknown as Record).getFactoryResult = vi + .fn() + .mockResolvedValue({ loadError: new Error('corrupted dist') }); + + const report = await diagnose(['python'], deps); + + const python = report.languages[0]; + expect(python.verdict).toBe('broken'); + expect(python.errors[0]).toContain('corrupted dist'); + expect(python.errors[0]).not.toContain('the registry returned no factory'); + expect(report.exitCode).toBe(1); + }); + it('diagnoses a loaded factory without validate() as version skew, not a load failure', async () => { const deps = makeDeps([{ name: 'python', factoryWithoutValidate: true }]); diff --git a/tests/unit/utils/language-availability.test.ts b/tests/unit/utils/language-availability.test.ts index c0ab8632..89d0d099 100644 --- a/tests/unit/utils/language-availability.test.ts +++ b/tests/unit/utils/language-availability.test.ts @@ -4,6 +4,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { + checkLaunchToolchain, computeModeAvailability, probeLanguageEntry, ValidationResultCache @@ -338,6 +339,68 @@ describe('probeLanguageEntry (issue #435)', () => { expect(probe.modes.launch.available).toBe(true); expect(warn).toHaveBeenCalledWith(expect.stringContaining('validate')); }); + + describe('getFactoryResult support (issue #435 part 4)', () => { + it('prefers getFactoryResult over getFactory when the registry offers it', async () => { + const factory = makeFactory(); + const getFactory = vi.fn(); + const getFactoryResult = vi.fn().mockResolvedValue({ factory }); + + const probe = await probeLanguageEntry(entry(), { + registry: { getFactory, getFactoryResult }, + disabledSet: new Set() + }); + + expect(getFactoryResult).toHaveBeenCalledWith('python'); + expect(getFactory).not.toHaveBeenCalled(); + expect(probe.factory).toBe(factory); + expect(probe.modes.launch.available).toBe(true); + }); + + it('carries the loadError into factoryLoadError while modes fail open', async () => { + const loadError = new Error('Failed to load adapter: corrupted dist'); + const probe = await probeLanguageEntry(entry(), { + registry: { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue({ loadError }) + }, + disabledSet: new Set() + }); + + expect(probe.factory).toBeUndefined(); + expect(probe.factoryLoadError).toBe(loadError); + expect(probe.modes.launch.available).toBe(true); // fail-open contract + }); + + it('records a throwing getFactoryResult as factoryLoadError and fails open', async () => { + const probe = await probeLanguageEntry(entry(), { + registry: { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockRejectedValue(new Error('result exploded')) + }, + disabledSet: new Set() + }); + + expect(probe.factory).toBeUndefined(); + expect(probe.factoryLoadError).toBeInstanceOf(Error); + expect(probe.modes.launch.available).toBe(true); + }); + + it('treats dynamicLoadingDisabled as no factory, with no load error', async () => { + const probe = await probeLanguageEntry(entry(), { + registry: { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue({ dynamicLoadingDisabled: true }) + }, + disabledSet: new Set() + }); + + expect(probe.factory).toBeUndefined(); + expect(probe.factoryLoadError).toBeUndefined(); + expect(probe.probeable).toBe(false); + expect(probe.modes.launch.available).toBe(true); + }); + }); }); describe('ValidationResultCache', () => { @@ -384,3 +447,171 @@ describe('ValidationResultCache', () => { expect(validate).toHaveBeenCalledTimes(2); }); }); + +describe('checkLaunchToolchain (issues #360, #435)', () => { + const factoryWith = (validate: () => Promise) => ({ + validate, + getMetadata: () => ({ modes: { launch: true, attach: 'none' as const } }) + }); + const registryWith = (validate: () => Promise) => ({ + getFactory: vi.fn().mockResolvedValue(factoryWith(validate)) + }); + const cache = () => new ValidationResultCache(30_000); + + it('reports available for a valid toolchain', async () => { + await expect( + checkLaunchToolchain('python', registryWith(async () => ok), cache()) + ).resolves.toEqual({ available: true }); + }); + + it('reports unavailable with the joined validation errors', async () => { + const registry = registryWith(async () => ({ + valid: false, + errors: ['Delve not found.', 'Go too old.'], + warnings: [] + })); + + await expect(checkLaunchToolchain('go', registry, cache())).resolves.toEqual({ + available: false, + reason: 'Delve not found.; Go too old.' + }); + }); + + it('falls back to the generic sentence when validation fails with empty errors', async () => { + const registry = registryWith(async () => ({ valid: false, errors: [], warnings: [] })); + + await expect(checkLaunchToolchain('go', registry, cache())).resolves.toEqual({ + available: false, + reason: "The 'go' debug adapter is not available in this runtime." + }); + }); + + it('fails open when the registry is undefined or has no getFactory', async () => { + await expect(checkLaunchToolchain('python', undefined, cache())).resolves.toEqual({ + available: true + }); + await expect(checkLaunchToolchain('python', {}, cache())).resolves.toEqual({ + available: true + }); + }); + + it('fails open when getFactory rejects, resolves no factory, or the factory has no validate', async () => { + await expect( + checkLaunchToolchain( + 'python', + { getFactory: vi.fn().mockRejectedValue(new Error('import exploded')) }, + cache() + ) + ).resolves.toEqual({ available: true }); + await expect( + checkLaunchToolchain('python', { getFactory: vi.fn().mockResolvedValue(undefined) }, cache()) + ).resolves.toEqual({ available: true }); + await expect( + checkLaunchToolchain( + 'python', + { getFactory: vi.fn().mockResolvedValue({ getMetadata: () => ({}) }) }, + cache() + ) + ).resolves.toEqual({ available: true }); + }); + + it('fails open when validate itself rejects', async () => { + const registry = registryWith(async () => { + throw new Error('probe exploded'); + }); + + await expect(checkLaunchToolchain('ruby', registry, cache())).resolves.toEqual({ + available: true + }); + }); + + it('fails open on a getFactoryResult loadError (load failures never block a launch)', async () => { + const registry = { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue({ loadError: new Error('corrupted dist') }) + }; + + await expect(checkLaunchToolchain('python', registry, cache())).resolves.toEqual({ + available: true + }); + }); + + it('runs validate once across two gate checks through the shared cache', async () => { + const validate = vi.fn().mockResolvedValue(ok); + const registry = registryWith(validate); + const shared = cache(); + + await checkLaunchToolchain('python', registry, shared); + await checkLaunchToolchain('python', registry, shared); + + expect(validate).toHaveBeenCalledTimes(1); + }); + + it('gates purely on the toolchain — DEBUG_MCP_DISABLE_LANGUAGES is enforced upstream, not here', async () => { + vi.stubEnv('DEBUG_MCP_DISABLE_LANGUAGES', 'python'); + try { + await expect( + checkLaunchToolchain('python', registryWith(async () => ok), cache()) + ).resolves.toEqual({ available: true }); + } finally { + vi.unstubAllEnvs(); + } + }); + + describe('drift fence: gate ≡ probeLanguageEntry launch mode', () => { + // The gate is a wrapper over the shared probe (issue #435 part 3). This + // matrix is the gate-side analogue of server-doctor-parity: for every + // registry shape, the gate's answer must equal the probe's launch mode + // (modulo the documented empty-reason fallback sentence). + const shapes: Array<{ title: string; registry: unknown }> = [ + { title: 'valid toolchain', registry: registryWith(async () => ok) }, + { + title: 'invalid with reason', + registry: registryWith(async () => bad('toolchain gone')) + }, + { + title: 'invalid with empty errors', + registry: registryWith(async () => ({ valid: false, errors: [], warnings: [] })) + }, + { + title: 'throwing validate', + registry: registryWith(async () => { + throw new Error('boom'); + }) + }, + { title: 'missing factory', registry: { getFactory: vi.fn().mockResolvedValue(undefined) } }, + { + title: 'loadError via getFactoryResult', + registry: { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue({ loadError: new Error('nope') }) + } + } + ]; + + for (const { title, registry } of shapes) { + it(`agrees with the probe for: ${title}`, async () => { + const gate = await checkLaunchToolchain( + 'python', + registry as Parameters[1], + cache() + ); + const probe = await probeLanguageEntry( + { language: 'python', packageName: '@debugmcp/adapter-python', installed: true }, + { + registry: registry as Parameters[1]['registry'], + disabledSet: new Set() + } + ); + + expect(gate.available).toBe(probe.modes.launch.available); + if (!gate.available) { + const expectedReason = + probe.modes.launch.reason || + "The 'python' debug adapter is not available in this runtime."; + expect((gate as { reason: string }).reason).toBe(expectedReason); + } + }); + } + }); +}); From 60dbf6d29fc5aa567b554c8152c277de08e47d22 Mon Sep 17 00:00:00 2001 From: JF Date: Sun, 23 Aug 2026 00:00:21 -0400 Subject: [PATCH 2/2] fix(availability): harden the typed-registry seams per review (#443) Review findings on parts 3-4, fixed: - AdapterRegistry no longer creates a per-instance winston logger (which piped every HTTP-session registry into the process-lifetime shared transport with no detach path - the issue-#404 leak class, and it bypassed --log-level/--log-file). AdapterRegistryConfig gains an injected `logger` sink, wired from the container logger in createProductionDependencies; absent -> warnings dropped. - the drift-fence matrix was vacuous: its describe-scope vi.fn fixtures were wiped by the global afterEach vi.resetAllMocks before any fence test ran, so both sides always failed open and true===true passed. Registries are now built per-test via thunks and each shape carries an expectAvailable pin, so silent vacuity cannot recur. - checkLaunchToolchain warns with the real load failure carried in probe.factoryLoadError instead of discarding the breadcrumb the getFactoryResult plumbing exists to deliver; its fallback sentence moves to ErrorMessages.modeUnavailableReason.launchFallback (rule 5), and the docstring's "cannot drift" claim is rescoped to the toolchain axis. - the probe's getFactoryResult option type derives from the shared FactoryLoadResult (Omit + widened factory) so a field rename there is a build break, not silent drift; the resolution is optional-chained so an untyped registry resolving undefined lands in the honest no-factory branch instead of a bogus corrupt-adapter diagnosis. - listAvailableAdapters survives a registered factory whose getMetadata throws (attach falls back to 'none' with a warn) - one bad plain-JS factory no longer kills every doctor verdict. - the attach-'none' gate warn-logs when a registry double lacks getFactoryMetadata instead of silently self-disabling; the coverage suite's registry double gains the member so its ~20 attachToProcess tests actually exercise the gate. - isAdapterRegistry now checks the four new required members (a legacy three-method registry is no longer certified), with first guard tests. - the parity fence's fake registry offers getFactoryResult, so the fence covers the branch production actually runs; the central mock keeps it deliberately absent (a default would shadow per-test getFactory overrides) and its discovery defaults are implementations that survive mock resets. - DoctorDependencies.adapterRegistry is typed DoctorRegistry (cast gone; runtime guard kept as defense against override casts). Not addressed (deliberate): per-launch getMetadata warns for broken factories are undeduped - they only fire for already-broken adapters and aid diagnosis. Co-Authored-By: Claude Fable 5 --- .../shared/src/interfaces/adapter-registry.ts | 18 ++++- .../unit/adapter-registry-guards.test.ts | 49 ++++++++++++ src/adapters/adapter-registry.ts | 44 +++++++--- src/cli/commands/doctor/index.ts | 6 +- src/container/dependencies.ts | 6 +- src/session/session-manager-operations.ts | 9 ++- src/utils/error-messages.ts | 3 + src/utils/language-availability.ts | 44 +++++++--- .../unit/server/server-doctor-parity.test.ts | 7 ++ .../test-utils/mocks/mock-adapter-registry.ts | 26 +++--- tests/unit/adapters/adapter-registry.test.ts | 47 +++++++++++ ...ession-manager-operations-coverage.test.ts | 24 ++++++ .../unit/utils/language-availability.test.ts | 80 +++++++++++++++---- 13 files changed, 310 insertions(+), 53 deletions(-) create mode 100644 packages/shared/tests/unit/adapter-registry-guards.test.ts diff --git a/packages/shared/src/interfaces/adapter-registry.ts b/packages/shared/src/interfaces/adapter-registry.ts index e68fa064..e6431e33 100644 --- a/packages/shared/src/interfaces/adapter-registry.ts +++ b/packages/shared/src/interfaces/adapter-registry.ts @@ -389,6 +389,15 @@ export interface AdapterRegistryConfig { * enables it. */ enableDynamicLoading?: boolean; + + /** + * Sink for discovery-fallback warnings (loader failures, malformed factory + * metadata). Injected by the DI container so registry breadcrumbs follow + * the configured log level/file instead of a per-instance logger piping + * into the process-lifetime transport (issue #404 leak class). Absent → + * warnings are dropped. + */ + logger?: { warn?: (message: string) => void }; } // ===== Error Types ===== @@ -453,7 +462,14 @@ export function isAdapterRegistry(obj: unknown): obj is IAdapterRegistry { obj !== null && 'register' in obj && 'create' in obj && - 'getSupportedLanguages' in obj + 'getSupportedLanguages' in obj && + // The typed discovery surface (issue #435 part 4): certifying a legacy + // three-method registry would hand callers of these required members a + // runtime TypeError with no compile-time warning. + 'listLanguages' in obj && + 'listAvailableAdapters' in obj && + 'getFactory' in obj && + 'getFactoryMetadata' in obj ); } diff --git a/packages/shared/tests/unit/adapter-registry-guards.test.ts b/packages/shared/tests/unit/adapter-registry-guards.test.ts new file mode 100644 index 00000000..be289772 --- /dev/null +++ b/packages/shared/tests/unit/adapter-registry-guards.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from 'vitest'; +import { isAdapterRegistry, isAdapterFactory } from '../../src/index.js'; + +const noop = () => undefined; + +describe('isAdapterRegistry', () => { + const fullRegistry = { + register: noop, + unregister: noop, + create: noop, + getSupportedLanguages: noop, + isLanguageSupported: noop, + listLanguages: noop, + listAvailableAdapters: noop, + getFactory: noop, + getFactoryMetadata: noop, + getAdapterInfo: noop, + getAllAdapterInfo: noop, + disposeAll: noop, + getActiveAdapterCount: noop + }; + + it('accepts a registry with the full typed surface', () => { + expect(isAdapterRegistry(fullRegistry)).toBe(true); + }); + + it('rejects a legacy registry missing the typed discovery surface (issue #435 part 4)', () => { + // Pre-part-4 registries had only register/create/getSupportedLanguages; + // certifying one would reintroduce guard-shaped duck-typing: callers of + // the new required members would TypeError at runtime. + expect( + isAdapterRegistry({ register: noop, create: noop, getSupportedLanguages: noop }) + ).toBe(false); + }); + + it('rejects non-objects', () => { + expect(isAdapterRegistry(null)).toBe(false); + expect(isAdapterRegistry(undefined)).toBe(false); + expect(isAdapterRegistry('registry')).toBe(false); + }); +}); + +describe('isAdapterFactory', () => { + it('accepts a factory without the optional describeToolchain member', () => { + expect( + isAdapterFactory({ createAdapter: noop, getMetadata: noop, validate: noop }) + ).toBe(true); + }); +}); diff --git a/src/adapters/adapter-registry.ts b/src/adapters/adapter-registry.ts index fa812310..f036ea36 100644 --- a/src/adapters/adapter-registry.ts +++ b/src/adapters/adapter-registry.ts @@ -20,7 +20,6 @@ import { IDebugAdapter, AdapterConfig } from '@debugmcp/shared'; import type { AdapterMetadata as SharedAdapterMetadata, AdapterManifestEntry, FactoryLoadResult } from '@debugmcp/shared'; import { AdapterLoader } from './adapter-loader.js'; import type { AdapterMetadata } from './adapter-loader.js'; -import { createLogger } from '../utils/logger.js'; /** * Default registry configuration @@ -32,6 +31,11 @@ const DEFAULT_CONFIG: Required = { autoDispose: true, autoDisposeTimeout: 300000, // 5 minutes enableDynamicLoading: false, + // No injected sink → discovery warnings are dropped. Deliberately NOT a + // per-instance createLogger(): HTTP mode builds a registry per session and + // a per-instance winston logger pipes each into the process-lifetime + // shared transport with no detach path (the issue-#404 leak class). + logger: {}, }; /** @@ -44,10 +48,13 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { private readonly disposeTimers = new Map(); private readonly registrationTimestamps = new Map(); private readonly loader = new AdapterLoader(); - private readonly logger = createLogger('AdapterRegistry'); // Dynamic loading is opt-in via constructor config or MCP_CONTAINER=true env var private readonly dynamicEnabled: boolean; + private warn(message: string): void { + this.config.logger.warn?.(message); + } + constructor(config: AdapterRegistryConfig = {}) { super(); this.config = { ...DEFAULT_CONFIG, ...config }; @@ -264,7 +271,7 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { } catch (error) { // Fall back to registered adapters (bundled environments embed them), // but leave a breadcrumb — a broken loader should not be silent. - this.logger.warn( + this.warn( `[AdapterRegistry] listLanguages: loader discovery failed, falling back to registered adapters: ${ error instanceof Error ? error.message : String(error) }` @@ -285,13 +292,28 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { async listAvailableAdapters(): Promise { const registered = new Set(this.getSupportedLanguages()); - const buildEntry = (language: string): AdapterMetadata => ({ - name: language, - packageName: `@debugmcp/adapter-${language}`, - description: undefined, - installed: true, - attach: this.factories.get(language)?.getMetadata().modes?.attach ?? 'none' - }); + const buildEntry = (language: string): AdapterMetadata => { + // A registered plain-JS factory can throw from getMetadata(); one bad + // factory must not reject the whole listing (doctor would lose every + // verdict). Same defense probeLanguageEntry applies per entry. + let attach: AdapterMetadata['attach'] = 'none'; + try { + attach = this.factories.get(language)?.getMetadata().modes?.attach ?? 'none'; + } catch (error) { + this.warn( + `[AdapterRegistry] getMetadata() threw for registered '${language}'; listing it with attach 'none'. ${ + error instanceof Error ? error.message : String(error) + }` + ); + } + return { + name: language, + packageName: `@debugmcp/adapter-${language}`, + description: undefined, + installed: true, + attach + }; + }; if (!this.dynamicEnabled) { // Provide minimal metadata from registered factories @@ -308,7 +330,7 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { } } catch (error) { // Fall back to registered adapters, but leave a breadcrumb. - this.logger.warn( + this.warn( `[AdapterRegistry] listAvailableAdapters: loader discovery failed, falling back to registered adapters: ${ error instanceof Error ? error.message : String(error) }` diff --git a/src/cli/commands/doctor/index.ts b/src/cli/commands/doctor/index.ts index c235f985..9f31de55 100644 --- a/src/cli/commands/doctor/index.ts +++ b/src/cli/commands/doctor/index.ts @@ -16,7 +16,7 @@ import { formatHumanReport, formatJsonReport } from './format.js'; import type { DoctorOptions } from '../../setup.js'; export interface DoctorDependencies { - adapterRegistry: unknown; + adapterRegistry: DoctorRegistry; environment: IEnvironment; fileSystem: IFileSystem; logger: ILogger; @@ -75,7 +75,9 @@ export async function handleDoctorCommand( overrides.createDependencies ?? (() => createProductionDependencies({ logLevel: 'error' })); deps = createDependencies(); - const registry = deps.adapterRegistry as DoctorRegistry; + // Typed field (issue #435 part 4); the runtime guard stays as defense + // against overrides handing in a wrong-shaped object through a cast. + const registry = deps.adapterRegistry; if ( typeof registry?.listAvailableAdapters !== 'function' || typeof registry?.getFactory !== 'function' diff --git a/src/container/dependencies.ts b/src/container/dependencies.ts index 9b21cb7f..99768f6b 100644 --- a/src/container/dependencies.ts +++ b/src/container/dependencies.ts @@ -102,7 +102,11 @@ export function createProductionDependencies(config: ContainerConfig = {}): Depe const dynConfig: AdapterRegistryConfig = { validateOnRegister: false, allowOverride: false, - enableDynamicLoading: true + enableDynamicLoading: true, + // Route registry discovery warnings through the container logger so they + // follow the configured level/file and detach with it on stop() — + // a registry-owned logger would leak per HTTP session (issue #404 class). + logger: { warn: (message: string) => logger.warn?.(message) } }; const adapterRegistry = new AdapterRegistry(dynConfig); diff --git a/src/session/session-manager-operations.ts b/src/session/session-manager-operations.ts index a994838f..57583c44 100644 --- a/src/session/session-manager-operations.ts +++ b/src/session/session-manager-operations.ts @@ -2514,7 +2514,9 @@ export abstract class SessionManagerOperations extends SessionManagerData { // declaration is enforced — absent metadata falls through to the // adapter's natural behavior. // getFactoryMetadata is on IAdapterRegistry (issue #435 part 4); the - // runtime guard stays for partial registry doubles. + // runtime guard stays for partial registry doubles, but skipping the + // gate must never be silent — that is the fail-open degradation the + // typed surface exists to expose. if (typeof this.adapterRegistry.getFactoryMetadata === 'function') { const factoryMeta = await this.adapterRegistry.getFactoryMetadata(session.language).catch(() => undefined); if (factoryMeta?.modes?.attach === 'none') { @@ -2524,6 +2526,11 @@ export abstract class SessionManagerOperations extends SessionManagerData { error: ErrorMessages.attachModeNotSupported(session.language) }; } + } else { + this.logger.warn( + `[SessionManager] adapterRegistry has no getFactoryMetadata; skipping the attach-'none' ` + + `enforcement gate for '${session.language}'.` + ); } if (session.proxyManager) { diff --git a/src/utils/error-messages.ts b/src/utils/error-messages.ts index a1f2b1ea..1cad368d 100644 --- a/src/utils/error-messages.ts +++ b/src/utils/error-messages.ts @@ -154,5 +154,8 @@ export const ErrorMessages = { `Adapter package ${packageName} is not installed.`, attachNotImplemented: (language: string) => `The '${language}' adapter does not implement attach mode.`, + /** Launch-gate fallback when validation failed with an empty errors list */ + launchFallback: (language: string) => + `The '${language}' debug adapter is not available in this runtime.`, }, }; diff --git a/src/utils/language-availability.ts b/src/utils/language-availability.ts index 28f55fff..dbd9b6f9 100644 --- a/src/utils/language-availability.ts +++ b/src/utils/language-availability.ts @@ -6,7 +6,12 @@ * can't assess the toolchain — issue #360) and authoritative for attach * 'none' (enforced in SessionManagerOperations.attachToProcess). */ -import type { AttachMechanism, FactoryValidationResult, IAdapterFactory } from '@debugmcp/shared'; +import type { + AttachMechanism, + FactoryLoadResult, + FactoryValidationResult, + IAdapterFactory +} from '@debugmcp/shared'; import { ErrorMessages } from './error-messages.js'; export interface ModeAvailability { @@ -67,8 +72,11 @@ export class ValidationResultCache { /** * Launch-mode gate shared by create_debug_session and start_debugging * (issue #360): a thin wrapper over probeLanguageEntry (issue #435 part 3), - * so the gate's answer — availability and reason text alike — is literally - * the launch mode list_supported_languages reports and cannot drift from it. + * so on the toolchain axis the gate's answer — availability and reason text + * alike — is literally the launch mode list_supported_languages reports and + * cannot drift from it. (The disabled/not-installed axes intentionally + * diverge: there list_supported_languages reports unavailable while the + * gate fails open — see the synthetic entry below.) * * The synthetic entry (installed: true, empty disabledSet) provably * neutralizes the probe's notInstalled/disabled short-circuits: the gate @@ -95,14 +103,22 @@ export async function checkLaunchToolchain( logger } ); + if (probe.factoryLoadError !== undefined) { + // The real import failure is in hand (getFactoryResult plumbed it) — + // leave the breadcrumb even though the gate fails open, or the later + // launch death shows only an unrelated proxy/spawn error. + logger?.warn?.( + `[language-availability] adapter factory for '${language}' failed to load; allowing launch. ` + + `${probe.factoryLoadError instanceof Error ? probe.factoryLoadError.message : String(probe.factoryLoadError)}` + ); + } if (probe.modes.launch.available) { return { available: true }; } // computeModeAvailability yields '' when validation.errors is empty; the // gate's user-facing error needs a sentence either way. const reason = - probe.modes.launch.reason || - `The '${language}' debug adapter is not available in this runtime.`; + probe.modes.launch.reason || ErrorMessages.modeUnavailableReason.launchFallback(language); return { available: false, reason }; } catch (error) { logger?.warn?.( @@ -137,11 +153,12 @@ export interface AvailabilityProbeOptions { */ registry?: { getFactory?: (language: string) => Promise; - getFactoryResult?: (language: string) => Promise<{ - factory?: ProbeableAdapterFactory; - loadError?: Error; - dynamicLoadingDisabled?: boolean; - }>; + // Derived from the shared FactoryLoadResult (factory widened to the + // probe's structural minimum) so a field rename there is a build break + // here instead of silent drift back to the vague no-factory message. + getFactoryResult?: ( + language: string + ) => Promise & { factory?: ProbeableAdapterFactory }>; }; disabledSet: Set; /** @@ -200,9 +217,12 @@ export async function probeLanguageEntry( if (!disabled && entry.installed) { try { if (typeof options.registry?.getFactoryResult === 'function') { + // Optional chaining: an untyped plain-JS registry can resolve + // undefined — that is "no factory", not a load error a TypeError + // here would misattribute to a corrupt adapter package. const result = await options.registry.getFactoryResult(entry.language); - factory = result.factory; - factoryLoadError = result.loadError; + factory = result?.factory; + factoryLoadError = result?.loadError; } else if (typeof options.registry?.getFactory === 'function') { factory = await options.registry.getFactory(entry.language); } diff --git a/tests/core/unit/server/server-doctor-parity.test.ts b/tests/core/unit/server/server-doctor-parity.test.ts index d697a43d..59f777ff 100644 --- a/tests/core/unit/server/server-doctor-parity.test.ts +++ b/tests/core/unit/server/server-doctor-parity.test.ts @@ -73,6 +73,13 @@ function buildSharedRegistry() { 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]), + // The production AdapterRegistry always offers getFactoryResult, and the + // probe prefers it — the parity fence must exercise the branch production + // actually runs, not only the legacy getFactory fallback. + getFactoryResult: vi.fn(async (language: string) => { + const factory = factories[language]; + return factory ? { factory } : {}; + }), isLanguageSupported: vi.fn().mockReturnValue(true), create: vi.fn(), register: vi.fn() diff --git a/tests/test-utils/mocks/mock-adapter-registry.ts b/tests/test-utils/mocks/mock-adapter-registry.ts index 9a577163..5b03f375 100644 --- a/tests/test-utils/mocks/mock-adapter-registry.ts +++ b/tests/test-utils/mocks/mock-adapter-registry.ts @@ -131,10 +131,13 @@ export function createMockAdapterRegistry(): IAdapterRegistry { // neutral and mirror the pre-typed fallback paths: languages from the // supported list, entries with attach 'none', and no loadable factory // (consumers assume availability when they cannot probe). Tests that - // need richer behavior override per-test, as before. - listLanguages: vi.fn().mockResolvedValue(supportedLanguages), + // need richer behavior override per-test, as before. Implementations + // (not mockResolvedValue) so the defaults survive mockReset/ + // vi.resetAllMocks — a reset mockResolvedValue returns bare undefined, + // which TypeErrors consumers that chain .catch() on the result. + listLanguages: vi.fn(async () => supportedLanguages), - listAvailableAdapters: vi.fn().mockResolvedValue( + listAvailableAdapters: vi.fn(async () => supportedLanguages.map((language) => ({ name: language, packageName: `@debugmcp/adapter-${language}`, @@ -143,9 +146,14 @@ export function createMockAdapterRegistry(): IAdapterRegistry { })) ), - getFactory: vi.fn().mockResolvedValue(undefined), + getFactory: vi.fn(async () => undefined), - getFactoryMetadata: vi.fn().mockResolvedValue(undefined), + getFactoryMetadata: vi.fn(async () => undefined), + + // getFactoryResult is deliberately ABSENT: the availability probe prefers + // it over getFactory, so a default here would shadow the per-test + // getFactory overrides most suites use. Production-branch coverage lives + // in the parity fence and the direct probe/gate/registry tests. getAdapterInfo: vi.fn().mockImplementation((lang: string) => adapterInfoMap.get(lang) @@ -169,8 +177,8 @@ export function createMockAdapterRegistryWithErrors(): IAdapterRegistry { // Override to simulate no languages supported mock.getSupportedLanguages = vi.fn().mockReturnValue([]); mock.isLanguageSupported = vi.fn().mockReturnValue(false); - mock.listLanguages = vi.fn().mockResolvedValue([]); - mock.listAvailableAdapters = vi.fn().mockResolvedValue([]); + mock.listLanguages = vi.fn(async () => []); + mock.listAvailableAdapters = vi.fn(async () => []); mock.create = vi.fn().mockRejectedValue(new Error('Adapter not found')); mock.getAdapterInfo = vi.fn().mockReturnValue(undefined); mock.getAllAdapterInfo = vi.fn().mockReturnValue(new Map()); @@ -189,8 +197,8 @@ export function createMockAdapterRegistryWithLanguages(languages: string[]): IAd mock.isLanguageSupported = vi.fn().mockImplementation((lang: string) => languages.includes(lang) ); - mock.listLanguages = vi.fn().mockResolvedValue(languages); - mock.listAvailableAdapters = vi.fn().mockResolvedValue( + mock.listLanguages = vi.fn(async () => languages); + mock.listAvailableAdapters = vi.fn(async () => languages.map((language) => ({ name: language, packageName: `@debugmcp/adapter-${language}`, diff --git a/tests/unit/adapters/adapter-registry.test.ts b/tests/unit/adapters/adapter-registry.test.ts index f23e47e3..28164d0d 100644 --- a/tests/unit/adapters/adapter-registry.test.ts +++ b/tests/unit/adapters/adapter-registry.test.ts @@ -232,6 +232,53 @@ describe('AdapterRegistry', () => { }); }); + describe('discovery hardening (issue #435 part 4 review)', () => { + it('routes the listAvailableAdapters fallback warning through the injected config logger', async () => { + const warn = vi.fn(); + const registry = new AdapterRegistry({ enableDynamicLoading: true, logger: { warn } }); + vi.spyOn(registry as any, 'loader', 'get').mockReturnValue({ + listAvailableAdapters: vi.fn().mockRejectedValue(new Error('loader exploded')) + }); + + await registry.listAvailableAdapters(); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('loader exploded')); + }); + + it('routes the listLanguages fallback warning through the injected config logger', async () => { + const warn = vi.fn(); + const registry = new AdapterRegistry({ enableDynamicLoading: true, logger: { warn } }); + vi.spyOn(registry as any, 'loader', 'get').mockReturnValue({ + listAvailableAdapters: vi.fn().mockRejectedValue(new Error('loader exploded')) + }); + + await registry.listLanguages(); + + expect(warn).toHaveBeenCalledWith(expect.stringContaining('loader exploded')); + }); + + it('keeps listing when a registered factory getMetadata throws (attach falls back to none)', async () => { + // A plain-JS third-party factory can stay registered with a throwing + // getMetadata: register() sets the factories map BEFORE the emit that + // calls getMetadata(), and production registration sites swallow the + // rejection. One bad factory must not then reject the whole listing + // (which would kill every doctor verdict). + const registry = new AdapterRegistry(); + const factory = createFactory({ + getMetadata: vi.fn(() => { + throw new Error('metadata exploded'); + }) + }); + await registry.register('mock', factory as any).catch(() => undefined); + + const adapters = await registry.listAvailableAdapters(); + + expect(adapters).toEqual([ + expect.objectContaining({ name: 'mock', installed: true, attach: 'none' }) + ]); + }); + }); + it('auto-disposes adapters on state change and clears timers', async () => { vi.useFakeTimers(); diff --git a/tests/unit/session-manager-operations-coverage.test.ts b/tests/unit/session-manager-operations-coverage.test.ts index ec4fff19..5cbfc8f2 100644 --- a/tests/unit/session-manager-operations-coverage.test.ts +++ b/tests/unit/session-manager-operations-coverage.test.ts @@ -115,6 +115,10 @@ describe('Session Manager Operations Coverage - Error Paths and Edge Cases', () buildAdapterCommand: vi.fn().mockReturnValue('python -m debugpy'), resolveExecutablePath: vi.fn().mockResolvedValue('python') }), + // Implementation (not mockResolvedValue) so it survives mock resets; + // undefined metadata lets the attach-'none' gate fall through while + // still being exercised (issue #435 part 4 review). + getFactoryMetadata: vi.fn(async () => undefined), getAdapterPolicy: vi.fn().mockReturnValue({ name: 'python', getInitializationBehavior: () => ({}) @@ -1532,6 +1536,26 @@ describe('Session Manager Operations Coverage - Error Paths and Edge Cases', () (operations as unknown as { attachPauseStopTimeoutMs: number }).attachPauseStopTimeoutMs = 50; }); + it('warns and proceeds when the registry lacks getFactoryMetadata (attach gate self-disabled)', async () => { + delete (mockDependencies.adapterRegistry as Record).getFactoryMetadata; + mockProxyManager.sendDapRequest.mockImplementation(async (command: string) => + command === 'threads' ? { body: { threads: [{ id: 1, name: 'main' }] } } : {} + ); + vi.spyOn(operations as any, 'startProxyManager').mockImplementation(async () => { + mockSession.proxyManager = mockProxyManager; + }); + + const result = await operations.attachToProcess('test-session', { + port: 5005, + host: 'localhost' + }); + + expect(result.success).toBe(true); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('getFactoryMetadata') + ); + }); + it('should discover main thread when available', async () => { // Setup: threads request returns threads including "main" mockProxyManager.sendDapRequest.mockImplementation(async (command: string) => { diff --git a/tests/unit/utils/language-availability.test.ts b/tests/unit/utils/language-availability.test.ts index 89d0d099..1f0ef8e3 100644 --- a/tests/unit/utils/language-availability.test.ts +++ b/tests/unit/utils/language-availability.test.ts @@ -9,6 +9,7 @@ import { probeLanguageEntry, ValidationResultCache } from '../../../src/utils/language-availability.js'; +import { ErrorMessages } from '../../../src/utils/error-messages.js'; const ok = { valid: true, errors: [], warnings: [] }; const bad = (msg: string) => ({ valid: false, errors: [msg], warnings: [] }); @@ -386,6 +387,24 @@ describe('probeLanguageEntry (issue #435)', () => { expect(probe.modes.launch.available).toBe(true); }); + it('treats a contract-violating undefined resolution as no factory, not a load error', async () => { + // An untyped plain-JS registry can resolve undefined; that must land in + // the honest no-factory branch, not surface as a bogus corrupt-adapter + // diagnosis via a TypeError recorded in factoryLoadError. + const probe = await probeLanguageEntry(entry(), { + registry: { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue(undefined as never) + }, + disabledSet: new Set() + }); + + expect(probe.factory).toBeUndefined(); + expect(probe.factoryLoadError).toBeUndefined(); + expect(probe.probeable).toBe(false); + expect(probe.modes.launch.available).toBe(true); + }); + it('treats dynamicLoadingDisabled as no factory, with no load error', async () => { const probe = await probeLanguageEntry(entry(), { registry: { @@ -536,6 +555,19 @@ describe('checkLaunchToolchain (issues #360, #435)', () => { }); }); + it('warns with the real load failure while failing open — the breadcrumb must not be discarded', async () => { + const warn = vi.fn(); + const registry = { + getFactory: vi.fn(), + getFactoryResult: vi.fn().mockResolvedValue({ loadError: new Error('corrupted dist') }) + }; + + await expect(checkLaunchToolchain('python', registry, cache(), { warn })).resolves.toEqual({ + available: true + }); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('corrupted dist')); + }); + it('runs validate once across two gate checks through the shared cache', async () => { const validate = vi.fn().mockResolvedValue(ok); const registry = registryWith(validate); @@ -558,57 +590,73 @@ describe('checkLaunchToolchain (issues #360, #435)', () => { } }); - describe('drift fence: gate ≡ probeLanguageEntry launch mode', () => { + describe('drift fence: gate ≡ probeLanguageEntry launch mode (toolchain axis)', () => { // The gate is a wrapper over the shared probe (issue #435 part 3). This // matrix is the gate-side analogue of server-doctor-parity: for every // registry shape, the gate's answer must equal the probe's launch mode // (modulo the documented empty-reason fallback sentence). - const shapes: Array<{ title: string; registry: unknown }> = [ - { title: 'valid toolchain', registry: registryWith(async () => ok) }, + // + // Registries are built INSIDE each test via thunks: the global afterEach + // vi.resetAllMocks() wipes describe-scope vi.fn implementations before + // any test executes, which once made this whole fence vacuously pass as + // true===true fail-open on both sides. expectAvailable pins each shape's + // intended branch so silent vacuity cannot recur. + const shapes: Array<{ title: string; makeRegistry: () => unknown; expectAvailable: boolean }> = [ + { title: 'valid toolchain', makeRegistry: () => registryWith(async () => ok), expectAvailable: true }, { title: 'invalid with reason', - registry: registryWith(async () => bad('toolchain gone')) + makeRegistry: () => registryWith(async () => bad('toolchain gone')), + expectAvailable: false }, { title: 'invalid with empty errors', - registry: registryWith(async () => ({ valid: false, errors: [], warnings: [] })) + makeRegistry: () => registryWith(async () => ({ valid: false, errors: [], warnings: [] })), + expectAvailable: false }, { title: 'throwing validate', - registry: registryWith(async () => { - throw new Error('boom'); - }) + makeRegistry: () => + registryWith(async () => { + throw new Error('boom'); + }), + expectAvailable: true + }, + { + title: 'missing factory', + makeRegistry: () => ({ getFactory: vi.fn(async () => undefined) }), + expectAvailable: true }, - { title: 'missing factory', registry: { getFactory: vi.fn().mockResolvedValue(undefined) } }, { title: 'loadError via getFactoryResult', - registry: { + makeRegistry: () => ({ getFactory: vi.fn(), - getFactoryResult: vi.fn().mockResolvedValue({ loadError: new Error('nope') }) - } + getFactoryResult: vi.fn(async () => ({ loadError: new Error('nope') })) + }), + expectAvailable: true } ]; - for (const { title, registry } of shapes) { + for (const { title, makeRegistry, expectAvailable } of shapes) { it(`agrees with the probe for: ${title}`, async () => { const gate = await checkLaunchToolchain( 'python', - registry as Parameters[1], + makeRegistry() as Parameters[1], cache() ); const probe = await probeLanguageEntry( { language: 'python', packageName: '@debugmcp/adapter-python', installed: true }, { - registry: registry as Parameters[1]['registry'], + registry: makeRegistry() as Parameters[1]['registry'], disabledSet: new Set() } ); + expect(gate.available).toBe(expectAvailable); // non-vacuity pin expect(gate.available).toBe(probe.modes.launch.available); if (!gate.available) { const expectedReason = probe.modes.launch.reason || - "The 'python' debug adapter is not available in this runtime."; + ErrorMessages.modeUnavailableReason.launchFallback('python'); expect((gate as { reason: string }).reason).toBe(expectedReason); } });