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..e6431e33 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 @@ -316,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 ===== @@ -380,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-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..f036ea36 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,7 +17,7 @@ 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'; @@ -31,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: {}, }; /** @@ -46,6 +51,10 @@ export class AdapterRegistry extends EventEmitter implements IAdapterRegistry { // 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 }; @@ -259,8 +268,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.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,16 +289,31 @@ 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 => ({ - 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 @@ -298,8 +328,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.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 +345,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/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/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..57583c44 100644 --- a/src/session/session-manager-operations.ts +++ b/src/session/session-manager-operations.ts @@ -2513,11 +2513,12 @@ 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, 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') { return { success: false, @@ -2525,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 880a0684..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 { @@ -66,37 +71,54 @@ 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 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 + * 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 probe = await probeLanguageEntry( + { language, packageName: `@debugmcp/adapter-${language}`, installed: true }, + { + registry, + disabledSet: new Set(), + runValidate: (lang, validate) => cache.get(lang, validate), + 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)}` + ); } - const validate = factory.validate.bind(factory) as () => Promise; - const validation = await cache.get(language, validate); - if (validation.valid) { + 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 || ErrorMessages.modeUnavailableReason.launchFallback(language); return { available: false, reason }; } catch (error) { logger?.warn?.( @@ -123,8 +145,21 @@ 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; + // 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; /** * Wraps each factory.validate call — the injection point for the server's @@ -179,9 +214,18 @@ 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') { + // 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; + } else if (typeof options.registry?.getFactory === 'function') { + factory = await options.registry.getFactory(entry.language); + } } catch (error) { factoryLoadError = error; } 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 abed671b..5b03f375 100644 --- a/tests/test-utils/mocks/mock-adapter-registry.ts +++ b/tests/test-utils/mocks/mock-adapter-registry.ts @@ -124,10 +124,38 @@ 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. 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(async () => + supportedLanguages.map((language) => ({ + name: language, + packageName: `@debugmcp/adapter-${language}`, + installed: true, + attach: 'none' as const + })) + ), + + getFactory: vi.fn(async () => 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) ), @@ -149,6 +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(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()); @@ -164,10 +194,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(async () => languages); + mock.listAvailableAdapters = vi.fn(async () => + 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..28164d0d 100644 --- a/tests/unit/adapters/adapter-registry.test.ts +++ b/tests/unit/adapters/adapter-registry.test.ts @@ -187,6 +187,98 @@ 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 }); + }); + }); + + 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/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/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 c0ab8632..1f0ef8e3 100644 --- a/tests/unit/utils/language-availability.test.ts +++ b/tests/unit/utils/language-availability.test.ts @@ -4,10 +4,12 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { + checkLaunchToolchain, computeModeAvailability, 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: [] }); @@ -338,6 +340,86 @@ 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 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: { + 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 +466,200 @@ 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('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); + 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 (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). + // + // 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', + makeRegistry: () => registryWith(async () => bad('toolchain gone')), + expectAvailable: false + }, + { + title: 'invalid with empty errors', + makeRegistry: () => registryWith(async () => ({ valid: false, errors: [], warnings: [] })), + expectAvailable: false + }, + { + title: 'throwing validate', + makeRegistry: () => + registryWith(async () => { + throw new Error('boom'); + }), + expectAvailable: true + }, + { + title: 'missing factory', + makeRegistry: () => ({ getFactory: vi.fn(async () => undefined) }), + expectAvailable: true + }, + { + title: 'loadError via getFactoryResult', + makeRegistry: () => ({ + getFactory: vi.fn(), + getFactoryResult: vi.fn(async () => ({ loadError: new Error('nope') })) + }), + expectAvailable: true + } + ]; + + for (const { title, makeRegistry, expectAvailable } of shapes) { + it(`agrees with the probe for: ${title}`, async () => { + const gate = await checkLaunchToolchain( + 'python', + makeRegistry() as Parameters[1], + cache() + ); + const probe = await probeLanguageEntry( + { language: 'python', packageName: '@debugmcp/adapter-python', installed: true }, + { + 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 || + ErrorMessages.modeUnavailableReason.launchFallback('python'); + expect((gate as { reason: string }).reason).toBe(expectedReason); + } + }); + } + }); +});