From f65615b85d4575ca56dbdd4e9484b5e668c47220 Mon Sep 17 00:00:00 2001 From: "haozhe.yang" Date: Tue, 9 Jun 2026 18:12:10 +0800 Subject: [PATCH] refactor(agent-core): align DI container with VS Code instantiation service - move InstantiationType to extensions module; change SyncDescriptor0 to interface\n- add _strict mode and spread args to invokeFunction\n- construct parent-owned descriptors in parent scope\n- materialise delayed services in child scope with global graph tracking\n- update TestInstantiationService ctor and tests for new semantics --- packages/agent-core/src/di/descriptors.ts | 20 +- packages/agent-core/src/di/errors.ts | 19 +- packages/agent-core/src/di/extensions.ts | 56 ++-- packages/agent-core/src/di/index.ts | 7 +- packages/agent-core/src/di/instantiation.ts | 48 +-- .../agent-core/src/di/instantiationService.ts | 311 ++++++++---------- .../src/di/testInstantiationService.ts | 15 +- packages/agent-core/test/di/child.test.ts | 88 ++++- packages/agent-core/test/di/delayed.test.ts | 76 ++++- .../agent-core/test/di/descriptor.test.ts | 18 +- .../agent-core/test/di/extensions.test.ts | 20 +- .../agent-core/test/di/instantiation.test.ts | 63 +++- .../agent-core/test/di/self-register.test.ts | 4 + packages/agent-core/test/di/trace.test.ts | 12 +- packages/daemon/src/start.ts | 6 +- packages/daemon/test/oauth.e2e.test.ts | 6 +- packages/daemon/test/prompt.e2e.test.ts | 2 - packages/daemon/test/tasks.e2e.test.ts | 9 +- packages/services/test/interfaces.test.ts | 8 +- 19 files changed, 469 insertions(+), 319 deletions(-) diff --git a/packages/agent-core/src/di/descriptors.ts b/packages/agent-core/src/di/descriptors.ts index 7016a1831..8156b9ca5 100644 --- a/packages/agent-core/src/di/descriptors.ts +++ b/packages/agent-core/src/di/descriptors.ts @@ -4,14 +4,6 @@ * `SyncDescriptor`. */ -/** How a service is instantiated. Delayed support lands in a later phase. */ -export enum InstantiationType { - /** Construct immediately on first `get`. */ - Eager = 0, - /** Construct lazily via a Proxy when a method is actually called. */ - Delayed = 1, -} - /** * Wraps a constructor plus optional static arguments. The container picks up * a `SyncDescriptor` from the `ServiceCollection` (rather than an already- @@ -34,14 +26,6 @@ export class SyncDescriptor { } } -/** - * `SyncDescriptor0` is the no-static-args specialisation used by the - * `createInstance(descriptor)` overload at the type level so a zero-arg ctor - * descriptor type-checks with no extra rest args. Mirrors krow - * `descriptors.ts:13-17`. - */ -export class SyncDescriptor0 extends SyncDescriptor { - constructor(ctor: new () => T) { - super(ctor, []); - } +export interface SyncDescriptor0 { + readonly ctor: new () => T; } diff --git a/packages/agent-core/src/di/errors.ts b/packages/agent-core/src/di/errors.ts index 16e568e04..3a2c96dce 100644 --- a/packages/agent-core/src/di/errors.ts +++ b/packages/agent-core/src/di/errors.ts @@ -9,20 +9,17 @@ import type { Graph } from './graph'; * * Two construction forms are supported: * - * 1. **Legacy `path: string[]` form** — used by the linear `_inProgress` + * 1. **`path: string[]` form** — used by the linear `_inProgress` * tree-stack check inside `_getOrCreateInstance`. This was the only form - * in P0; it is retained because that defensive layer is preserved per - * PLAN D3 (it catches same-id reentrancy from a ctor body even before - * the Graph traversal would discover it). The path is the construction - * stack at the moment the cycle was detected, in construction order - * (root → ... → repeated-id). The repeated id appears at both ends so - * the cycle is visually obvious. + * stack check. The path is the construction stack at the moment the + * cycle was detected, in construction order (root → ... → repeated-id). + * The repeated id appears at both ends so the cycle is visually obvious. * * 2. **`Graph` form** — used by the Graph-based - * `_createAndCacheServiceInstance` introduced in P1.1. The path is - * computed lazily via `graph.findCycleSlow()` when the message is built. - * If the cycle finder returns `undefined` we fall back to dumping the - * entire graph so the failure is still diagnosable. + * `_createAndCacheServiceInstance`. The path is computed lazily via + * `graph.findCycleSlow()` when the message is built. If the cycle finder + * returns `undefined` we fall back to dumping the entire graph so the + * failure is still diagnosable. * * Both forms expose `path: ReadonlyArray` so existing call sites * (and tests) keep working. For the Graph form the `path` array is diff --git a/packages/agent-core/src/di/extensions.ts b/packages/agent-core/src/di/extensions.ts index 9b70342cc..9dc88e851 100644 --- a/packages/agent-core/src/di/extensions.ts +++ b/packages/agent-core/src/di/extensions.ts @@ -9,17 +9,22 @@ * Registry shape: `Array<[ServiceIdentifier, SyncDescriptor]>`. Each * entry pairs an id with the `SyncDescriptor` that captures both the * constructor + static args AND the `supportsDelayedInstantiation` flag. - * Later registrations of the same id overwrite earlier ones — override - * semantics live in the `ServiceCollection` stage; this layer is intentionally - * permissive so module load order can be reshuffled without surprise. + * Registrations are appended as-is. Override semantics live in the + * `ServiceCollection` stage that consumes the registry, matching VS Code's + * permissive module-load registry. */ -import { InstantiationType, SyncDescriptor } from './descriptors'; -import type { ServiceIdentifier } from './instantiation'; +import { SyncDescriptor } from './descriptors'; +import type { BrandedService, ServiceIdentifier } from './instantiation'; // eslint-disable-next-line @typescript-eslint/no-explicit-any const _registry: Array<[ServiceIdentifier, SyncDescriptor]> = []; +export enum InstantiationType { + Eager = 0, + Delayed = 1, +} + /** * Register a service implementation under its identifier. Typically called * at module top-level. @@ -29,51 +34,47 @@ const _registry: Array<[ServiceIdentifier, SyncDescriptor]> = []; * - `registerSingleton(id, ctor, instantiationType?)` — the back-compat ctor * overload. Internally wraps `ctor` in `new SyncDescriptor(ctor, [], * supportsDelayedInstantiation)` where - * `supportsDelayedInstantiation = (instantiationType === InstantiationType.Delayed)`. + * `supportsDelayedInstantiation = Boolean(instantiationType)`. * - `registerSingleton(id, descriptor)` — the descriptor overload. Stores the * descriptor as-is; the caller owns `staticArguments` and * `supportsDelayedInstantiation`. * - * If `id` was previously registered, the new entry replaces the old one - * (matching VS Code semantics — no throw). + * If `id` was previously registered, the new entry is appended. Consumers + * that seed a `ServiceCollection` decide the effective binding by insertion + * order. */ -export function registerSingleton( +export function registerSingleton( id: ServiceIdentifier, // eslint-disable-next-line @typescript-eslint/no-explicit-any - ctor: new (...args: any[]) => T, + ctor: new (...services: Services) => T, instantiationType?: InstantiationType, ): void; -export function registerSingleton( +export function registerSingleton( id: ServiceIdentifier, - descriptor: SyncDescriptor, + descriptor: SyncDescriptor, ): void; -export function registerSingleton( +export function registerSingleton( id: ServiceIdentifier, ctorOrDescriptor: - | SyncDescriptor + | SyncDescriptor // eslint-disable-next-line @typescript-eslint/no-explicit-any - | (new (...args: any[]) => T), - instantiationType: InstantiationType = InstantiationType.Eager, + | (new (...services: Services) => T), + instantiationType?: boolean | InstantiationType, ): void { const descriptor = ctorOrDescriptor instanceof SyncDescriptor ? ctorOrDescriptor : new SyncDescriptor( - ctorOrDescriptor, + ctorOrDescriptor as new (...args: unknown[]) => T, [], - instantiationType === InstantiationType.Delayed, + Boolean(instantiationType), ); - const existing = _registry.findIndex(([existingId]) => existingId === id); - if (existing >= 0) { - _registry[existing] = [id, descriptor]; - } else { - _registry.push([id, descriptor]); - } + _registry.push([id, descriptor]); } /** - * Snapshot the registry as a list suitable for `ServiceCollection` + * Return the registry as a list suitable for `ServiceCollection` * construction. * * Shape: `ReadonlyArray, SyncDescriptor]>` @@ -81,14 +82,13 @@ export function registerSingleton( * `supportsDelayedInstantiation` flag travels on the descriptor itself, not * as a separate registry slot. * - * The returned array is a fresh shallow copy on each call: subsequent - * registrations do not retroactively mutate prior snapshots. + * The returned array is the live registry, matching VS Code. */ export function getSingletonServiceDescriptors(): ReadonlyArray< // eslint-disable-next-line @typescript-eslint/no-explicit-any readonly [ServiceIdentifier, SyncDescriptor] > { - return _registry.map(([id, descriptor]) => [id, descriptor] as const); + return _registry; } /** diff --git a/packages/agent-core/src/di/index.ts b/packages/agent-core/src/di/index.ts index 27712bb66..020493234 100644 --- a/packages/agent-core/src/di/index.ts +++ b/packages/agent-core/src/di/index.ts @@ -2,8 +2,7 @@ * Barrel for `@moonshot-ai/agent-core` DI subsystem. This file is the only * surface that should be imported from outside the `di/` directory. * - * Modelled after VSCode's `vs/platform/instantiation`. See `./README.md` for - * usage (lands in W2.5). + * Modelled after VSCode's `vs/platform/instantiation`. */ export type { @@ -23,7 +22,8 @@ export { // or `accessor.get(IInstantiationService)`. IInstantiationService, } from './instantiation'; -export { InstantiationType, SyncDescriptor, SyncDescriptor0 } from './descriptors'; +export { SyncDescriptor } from './descriptors'; +export type { SyncDescriptor0 } from './descriptors'; export { ServiceCollection } from './serviceCollection'; export { InstantiationService } from './instantiationService'; export { @@ -36,6 +36,7 @@ export { export type { IDisposable } from './lifecycle'; export { CyclicDependencyError } from './errors'; export { + InstantiationType, registerSingleton, getSingletonServiceDescriptors, _clearRegistryForTests, diff --git a/packages/agent-core/src/di/instantiation.ts b/packages/agent-core/src/di/instantiation.ts index f34ca1157..be53f019c 100644 --- a/packages/agent-core/src/di/instantiation.ts +++ b/packages/agent-core/src/di/instantiation.ts @@ -7,28 +7,14 @@ * defines the brands and contracts so `serviceCollection.ts` can stay free of * container code. * - * P0.3 alignment with krow / VSCode: - * - `createDecorator(name)` is now singleton-per-name: calling it twice with - * the same `name` returns the same identifier. (Previously every call - * minted a fresh callable.) - * - Decorator body actually stashes `{ id, index }` on the ctor as - * `$di$dependencies` own-property metadata (instead of being a no-op). - * `InstantiationService._createInstance` does not yet consume this — that - * wiring lands in P1.1 — so the daemon's existing - * `ix.createInstance(Impl, a.get(IDepA), ...)` call sites remain - * bytewise unchanged. - * - `ServiceIdentifier` exposes `_serviceBrand` (krow naming) instead of - * the prior internal `$serviceMarker`. - * - * P0.4 alignment: - * - `BrandedService` + `GetLeadingNonServiceArgs` type tools added so the - * `createInstance(ctor, ...rest)` signature can trim trailing service - * parameters once `@IFoo` auto-injection lands in P1.1. - * - `IInstantiationService.createInstance` gains a `SyncDescriptor0` - * overload mirroring krow. + * `createDecorator(name)` is singleton-per-name, decorator application stores + * constructor dependency metadata, and `InstantiationService.createInstance` + * consumes that metadata for service auto-injection. */ import type { SyncDescriptor0 } from './descriptors'; +import type { DisposableStore } from './lifecycle'; +import type { ServiceCollection } from './serviceCollection'; /** * Internal metadata utilities shared with `instantiationService.ts`. Not @@ -65,8 +51,8 @@ export type BrandedService = { _serviceBrand: undefined }; * Type-level slicer that retains only the leading non-`BrandedService` args * of a constructor parameter list. Used by `createInstance(ctor, ...args)` * so callers can omit any trailing `@IFoo`-decorated service parameters - * (those are auto-injected by the container in a later phase). Mirrors krow - * `instantiation.ts:32-35`. + * (those are auto-injected by the container). Mirrors VS Code + * `instantiation.ts`. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export type GetLeadingNonServiceArgs = @@ -89,8 +75,8 @@ export interface ServiceIdentifier { // eslint-disable-next-line @typescript-eslint/no-explicit-any (target: any, key: string | symbol | undefined, index: number): void; - /** Phantom marker so two decorators with different `T` are not assignable. */ - readonly _serviceBrand: { readonly _: T }; + /** Phantom marker matching VS Code's `ServiceIdentifier` surface. */ + readonly type: T; toString(): string; } @@ -183,7 +169,10 @@ export interface ServicesAccessor { export interface IInstantiationService { readonly _serviceBrand: undefined; - invokeFunction(fn: (accessor: ServicesAccessor) => R): R; + invokeFunction( + fn: (accessor: ServicesAccessor, ...args: TS) => R, + ...args: TS + ): R; /** * Construct a class via a `SyncDescriptor` packaging its ctor + static args. * Mirrors the krow / VSCode `createInstance(descriptor)` overload — useful @@ -195,8 +184,7 @@ export interface IInstantiationService { * Construct a class with positional arguments. `GetLeadingNonServiceArgs` * trims any trailing `@IFoo`-decorated service parameters off the inferred * signature so callers only have to supply the non-service prefix; the - * container auto-injects the service tail (auto-injection itself lands in - * P1.1 — this commit only widens the type). + * container auto-injects the service tail. */ createInstance< Ctor extends new ( @@ -208,7 +196,7 @@ export interface IInstantiationService { ctor: Ctor, ...args: GetLeadingNonServiceArgs> ): R; - createChild(services: ServiceCollectionLike): IInstantiationService; + createChild(services: ServiceCollection, store?: DisposableStore): IInstantiationService; dispose(): void; } @@ -222,11 +210,11 @@ export interface IInstantiationService { * so child containers see their own slot, not the parent's. */ export const IInstantiationService: ServiceIdentifier = - createDecorator('IInstantiationService'); + createDecorator('instantiationService'); /** - * Structural alias to avoid a circular import with `./serviceCollection.ts`. - * Anything `ServiceCollection`-shaped (set/get/has/forEach) satisfies this. + * Structural alias kept for callers that type against the collection shape. + * The runtime `createChild` API requires a real `ServiceCollection`. */ export interface ServiceCollectionLike { // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/packages/agent-core/src/di/instantiationService.ts b/packages/agent-core/src/di/instantiationService.ts index 5168db499..51aa892b7 100644 --- a/packages/agent-core/src/di/instantiationService.ts +++ b/packages/agent-core/src/di/instantiationService.ts @@ -2,28 +2,10 @@ * Runtime container for the DI subsystem. See `./README.md` for usage. * Modelled after VSCode's `InstantiationService`. * - * History: - * - W2.2: basic single-level container. - * - W2.3: `createChild` scopes + `dispose` lifecycle. - * - W2.4: cyclic dependency detection across the parent chain - * (linear `_inProgress` tree-stack). - * - P0.2: `Trace` class + `_enableTracing` flag installed (not yet wired). - * - P0.5: `IInstantiationService` self-registers in every container. - * - P1.1: `_util.getServiceDependencies` is now consumed — `@IFoo`-decorated - * constructor parameters auto-inject from the container; Graph-based - * dependency-subtree resolution catches cycles that the linear - * `_inProgress` stack would miss (e.g. detected statically before - * any ctor body runs). Both defensive layers are preserved per - * PLAN D3: `_inProgress` still catches ctor-body re-entry where a - * ctor synchronously calls `accessor.get(self)`. LIFO dispose order - * via `_constructionOrder` is preserved per PLAN D8. - * - P1.2: `SyncDescriptor.supportsDelayedInstantiation === true` now returns - * a `Proxy` that defers real construction until the first non-event - * property access. `onDid*`/`onWill*` subscriptions made BEFORE - * materialisation are parked in a `LinkedList` and rebound to the - * real event when the proxy resolves. Proxy-materialised instances - * join `_servicesToMaybeDispose` so dispose() tears them down in - * addition to the eager `_constructionOrder` set. + * It follows VS Code's service-collection-as-source-of-truth model: + * descriptors are replaced with constructed instances in the owning + * collection, parent-owned services are constructed in the parent scope, and + * delayed services materialise through a child scope. */ import { SyncDescriptor } from './descriptors'; @@ -33,11 +15,10 @@ import { IInstantiationService as IInstantiationServiceDecorator, _util, type IInstantiationService, - type ServiceCollectionLike, type ServiceIdentifier, type ServicesAccessor, } from './instantiation'; -import type { IDisposable } from './lifecycle'; +import { toDisposable, type DisposableStore, type IDisposable } from './lifecycle'; import { ServiceCollection } from './serviceCollection'; import { GlobalIdleValue } from './util/idleValue'; import { LinkedList } from './util/linkedList'; @@ -46,7 +27,7 @@ import { LinkedList } from './util/linkedList'; // // `Trace` is vendored verbatim from krow // `packages/core/src/platform/instantiation/instantiationService.ts:7-83` -// (which in turn is the VSCode original). P1.1 wires the call sites: +// (which in turn is the VSCode original). The call sites are wired so: // `invokeFunction` opens an Invocation trace; `createInstance` opens a // Creation trace; `_safeCreateAndCacheServiceInstance` opens a Creation // trace per-service; and `_getOrCreateServiceInstance` calls @@ -141,30 +122,14 @@ export class InstantiationService implements IInstantiationService { /** Phantom brand so the class satisfies the `IInstantiationService` interface. */ declare readonly _serviceBrand: undefined; + readonly _globalGraph?: Graph; + private _globalGraphImplicitDependency?: string; + /** Parent container in the scope chain (root container has no parent). */ - protected readonly _parent: InstantiationService | null; + protected readonly _parent?: InstantiationService; - /** - * Cached instances per identifier. First `get(id)` constructs and caches; - * subsequent calls return the same reference (singleton-per-container). - * - * Note: as of P1.1 the "constructed instance" for a registration lives - * inside `services` itself (the SyncDescriptor entry is replaced with the - * built instance once construction completes) — this is the krow shape. - * `_instances` is kept as a lookup fast path AND remains the source of - * truth for the LIFO `_constructionOrder` (PLAN D8); `_setCreatedServiceInstance` - * writes BOTH so dispose() can walk the construction order without - * re-querying `services`. - */ // eslint-disable-next-line @typescript-eslint/no-explicit-any - protected readonly _instances = new Map, any>(); - - /** - * Order in which identifiers were first constructed in this container. - * Used to teardown in reverse order on `dispose`. Preserved per PLAN D8. - */ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - protected readonly _constructionOrder: ServiceIdentifier[] = []; + protected readonly _constructionOrder: any[] = []; /** Live children created via `createChild`. Disposed transitively. */ protected readonly _children = new Set(); @@ -203,8 +168,7 @@ export class InstantiationService implements IInstantiationService { * container by `_setCreatedServiceInstance`), but the underlying real * instance lives behind `idle.value` and is not part of * `_constructionOrder`. We add it here so `dispose()` can still tear it - * down — see `dispose()` for the LIFO-first / set-second order (PLAN D8 - * preserves the kimi LIFO order; krow ONLY has this set). + * down — see `dispose()` for the LIFO-first / set-second order. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any private readonly _servicesToMaybeDispose = new Set(); @@ -213,31 +177,48 @@ export class InstantiationService implements IInstantiationService { constructor( public readonly services: ServiceCollection = new ServiceCollection(), - parent: InstantiationService | null = null, + private readonly _strict: boolean = false, + parent?: InstantiationService, protected readonly _enableTracing: boolean = false, ) { this._parent = parent; + this._globalGraph = _enableTracing ? parent?._globalGraph ?? new Graph(e => e) : undefined; // Self-register so `@IInstantiationService`-decorated ctor params resolve - // to the live container that constructed them (krow / VSCode parity: - // `instantiationService.ts:110`). Each container — root and every child - // — stamps its OWN slot, so `child.invokeFunction(a => a.get(I)) === child` - // even when the parent already registered itself. The Graph rewrite - // preserves this invariant per Phase-0 reviewer note #4: the - // self-registration lives in the local `services` map, so + // to the live container that constructed them. Each container — root and + // every child — stamps its OWN slot, so + // `child.invokeFunction(a => a.get(I)) === child` + // even when the parent already registered itself. The self-registration + // lives in the local `services` map, so // `_getServiceInstanceOrDescriptor(IInstantiationService)` in the child // finds the child's own slot before walking to the parent. this.services.set(IInstantiationServiceDecorator, this); } - invokeFunction(fn: (accessor: ServicesAccessor) => R): R { + invokeFunction( + fn: (accessor: ServicesAccessor, ...args: TS) => R, + ...args: TS + ): R { this._assertNotDisposed(); const _trace = Trace.traceInvocation(this._enableTracing, fn); + let done = false; try { const accessor: ServicesAccessor = { - get: (id: ServiceIdentifier): T => this._getOrCreateServiceInstance(id, _trace), + get: (id: ServiceIdentifier): T => { + if (done) { + throw new Error( + 'service accessor is only valid during the invocation of its target method', + ); + } + const result = this._getOrCreateServiceInstance(id, _trace); + if (!result) { + this._throwIfStrict(`[invokeFunction] unknown service '${id}'`, false); + } + return result; + }, }; - return fn(accessor); + return fn(accessor, ...args); } finally { + done = true; _trace.stop(); } } @@ -285,24 +266,24 @@ export class InstantiationService implements IInstantiationService { * Tracing flag is inherited from the parent so a deep child can't * accidentally suppress tracing the parent enabled. */ - createChild(services: ServiceCollectionLike): IInstantiationService { + createChild(services: ServiceCollection, store?: DisposableStore): IInstantiationService { this._assertNotDisposed(); // Defensive: only accept real ServiceCollection instances. The - // `ServiceCollectionLike` alias exists for the interface surface to avoid - // a circular type import, but at runtime the child needs a real Map. + // child creation needs a real map-backed collection. if (!(services instanceof ServiceCollection)) { throw new TypeError( 'createChild requires a ServiceCollection instance (got something else)', ); } - const child = new InstantiationService(services, this, this._enableTracing); + const child = new InstantiationService(services, this._strict, this, this._enableTracing); this._children.add(child); + store?.add(child); return child; } /** * Tear down this container and all children. Disposes any cached instance - * with a `dispose()` method, in REVERSE construction order (PLAN D8). + * with a `dispose()` method, in reverse construction order. * Idempotent: a second call is a no-op. Also notifies parent if any (so * parent can drop its back-reference) and disposes children transitively. */ @@ -327,8 +308,7 @@ export class InstantiationService implements IInstantiationService { // 2) Dispose own instances in reverse construction order, duck-typed // against the `IDisposable` shape. for (let i = this._constructionOrder.length - 1; i >= 0; i--) { - const id = this._constructionOrder[i]!; - const instance = this._instances.get(id); + const instance = this._constructionOrder[i]!; if (instance && typeof (instance as Partial).dispose === 'function') { try { (instance as IDisposable).dispose(); @@ -338,19 +318,10 @@ export class InstantiationService implements IInstantiationService { this._servicesToMaybeDispose.delete(instance); } } - this._instances.clear(); this._constructionOrder.length = 0; - // 3) Dispose any Proxy-materialised instances that were NOT seen by the - // LIFO `_constructionOrder` loop (P1.2). Eager services are written - // to `_constructionOrder` via `_setCreatedServiceInstance` and are - // therefore covered above; the lazy Proxy path adds the real instance - // to `_servicesToMaybeDispose` from inside the `GlobalIdleValue` - // executor, but the Proxy itself doesn't carry a `dispose` method — - // so this set is the only handle to the underlying instance. Order - // among Proxy-materialised instances is insertion order; the LIFO - // invariant is intentionally not extended here (PLAN D8 ties LIFO to - // `_constructionOrder`, which only tracks eager construction). + // 3) Dispose any materialised instances that were not seen by the LIFO + // `_constructionOrder` loop. for (const candidate of this._servicesToMaybeDispose) { if (candidate && typeof (candidate as Partial).dispose === 'function') { try { @@ -362,7 +333,7 @@ export class InstantiationService implements IInstantiationService { } this._servicesToMaybeDispose.clear(); - // 3) Drop our back-reference from parent so parent doesn't double-dispose + // 4) Drop our back-reference from parent so parent doesn't double-dispose // us later. if (this._parent) { this._parent._children.delete(this); @@ -386,6 +357,12 @@ export class InstantiationService implements IInstantiationService { const serviceArgs: unknown[] = []; for (const dependency of serviceDependencies) { const service = this._getOrCreateServiceInstance(dependency.id, _trace); + if (!service) { + this._throwIfStrict( + `[createInstance] ${ctor.name} depends on UNKNOWN service ${dependency.id}.`, + false, + ); + } serviceArgs.push(service); } @@ -413,32 +390,26 @@ export class InstantiationService implements IInstantiationService { * chain if not registered locally. Construction happens in the OWNING * container so its cache holds the singleton. * - * P1.1 makes this a thin router that delegates to the Graph-based + * This is a thin router that delegates to the Graph-based * `_safeCreateAndCacheServiceInstance` whenever the resolved entry is a - * `SyncDescriptor`. The Graph walk is the PRIMARY cycle-detection path — + * `SyncDescriptor`. The Graph walk is the primary cycle-detection path — * it builds the entire dependency subtree before constructing anything, * so cycles expressed via `@IFoo` decorator metadata are caught * statically (no ctor body need run). * - * The legacy linear `_inProgress` stack (mutated below) is preserved as - * the SECONDARY defensive layer per PLAN D3: it catches the case where a + * The linear `_inProgress` stack (mutated below) is preserved as + * a secondary defensive layer: it catches the case where a * ctor body synchronously calls `accessor.get(peer)` — a ctor-time * dynamic edge that the Graph walk cannot predict. */ protected _getOrCreateServiceInstance(id: ServiceIdentifier, _trace: Trace): T { - const cached = this._instances.get(id); - if (cached !== undefined) { - _trace.branch(id, false); - return cached as T; + if (this._globalGraph && this._globalGraphImplicitDependency) { + this._globalGraph.insertEdge(this._globalGraphImplicitDependency, String(id)); } - const entry = this._getServiceInstanceOrDescriptor(id); - if (entry === undefined) { - throw new Error(`No service registered for identifier '${String(id)}'`); - } if (entry instanceof SyncDescriptor) { - // Linear tree-wide cycle check (PLAN D3, second defensive layer): + // Linear tree-wide cycle check as a second defensive layer: // a ctor body calling `accessor.get(peer)` synchronously will reach // here while `peer` is mid-construction. The Graph walk inside // `_createAndCacheServiceInstance` cannot predict ctor-body edges @@ -462,9 +433,8 @@ export class InstantiationService implements IInstantiationService { // begins. return this._safeCreateAndCacheServiceInstance(id, entry, _trace.branch(id, true)); } - // Pre-built instance shorthand — cache locally and return. + _trace.branch(id, false); - this._setCreatedServiceInstance(id, entry as T); return entry as T; } @@ -496,9 +466,8 @@ export class InstantiationService implements IInstantiationService { * (leaves first) so each node is constructed AFTER all of its dependencies * are cached. If `graph.roots()` becomes empty while the graph is * non-empty, a cycle exists — throw `CyclicDependencyError(graph)`. The - * legacy `_inProgress` stack also catches ctor-body-induced cycles - * directly inside `_getOrCreateServiceInstance` below; both layers are - * preserved per PLAN D3. + * `_inProgress` stack also catches ctor-body-induced cycles directly + * inside `_getOrCreateServiceInstance` below. * * Mirrors krow `instantiationService.ts:266-323`. */ @@ -530,16 +499,15 @@ export class InstantiationService implements IInstantiationService { for (const dependency of _util.getServiceDependencies(item.desc.ctor)) { const instanceOrDesc = this._getServiceInstanceOrDescriptor(dependency.id); - if (instanceOrDesc === undefined) { - // Mirror krow: warn but don't throw — the constructor will get - // `undefined` for that arg and either crash with a more useful - // message or work if the dependency is optional. - // eslint-disable-next-line no-console - globalThis.console.warn( + if (!instanceOrDesc) { + this._throwIfStrict( `[createInstance] ${String(item.id)} depends on ${String(dependency.id)} which is NOT registered.`, + true, ); } + this._globalGraph?.insertEdge(String(item.id), String(dependency.id)); + if (instanceOrDesc instanceof SyncDescriptor) { const d: Triple = { id: dependency.id, @@ -568,19 +536,14 @@ export class InstantiationService implements IInstantiationService { // identifier across nested subgraphs). const instanceOrDesc = this._getServiceInstanceOrDescriptor(data.id); if (instanceOrDesc instanceof SyncDescriptor) { - const lazy = data.desc.supportsDelayedInstantiation; - const instance = this._createServiceInstance( + const instance = this._createServiceInstanceWithOwner( data.id, - data.desc, - lazy, + data.desc.ctor, + data.desc.staticArguments, + data.desc.supportsDelayedInstantiation, data._trace, ); - // For lazy services, the returned value is a Proxy; the real - // instance lands in `_servicesToMaybeDispose` only after - // materialisation. We deposit the Proxy without touching - // `_constructionOrder` so dispose() does not accidentally trigger - // materialisation just to call a non-existent `.dispose()`. - this._setCreatedServiceInstance(data.id, instance, lazy); + this._setCreatedServiceInstance(data.id, instance); } graph.removeNode(data); } @@ -591,14 +554,12 @@ export class InstantiationService implements IInstantiationService { /** * Construct a service instance — either eagerly (the default) or wrapped * in a `Proxy` that defers real construction until the first non-event - * property access (P1.2). + * property access. * * Eager path: pushes `id` onto the root-tree `_inProgress` stack so a ctor * body calling `accessor.get(self)` synchronously is caught by - * `_getOrCreateServiceInstance` as a cycle (PLAN D3 second defensive - * layer). Returns the real instance immediately; the caller writes it - * into the owning container via `_setCreatedServiceInstance` which also - * stamps `_constructionOrder` for LIFO dispose (PLAN D8). + * `_getOrCreateServiceInstance` as a cycle. Returns the real instance immediately; the caller writes it + * into the owning container via `_setCreatedServiceInstance`. * * Lazy path (`supportsDelayedInstantiation: true`): returns a `Proxy` * over `Object.create(null)` whose `get` trap: @@ -619,17 +580,54 @@ export class InstantiationService implements IInstantiationService { * * Mirrors krow `instantiationService.ts:335-421`. */ - private _createServiceInstance( + private _createServiceInstanceWithOwner( id: ServiceIdentifier, - desc: SyncDescriptor, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ctor: any, + args: ReadonlyArray = [], supportsDelayedInstantiation: boolean, _trace: Trace, + ): T { + if (this.services.get(id) instanceof SyncDescriptor) { + return this._createServiceInstance( + id, + ctor, + args, + supportsDelayedInstantiation, + _trace, + this._servicesToMaybeDispose, + ); + } else if (this._parent) { + return this._parent._createServiceInstanceWithOwner( + id, + ctor, + args, + supportsDelayedInstantiation, + _trace, + ); + } else { + throw new Error(`illegalState - creating UNKNOWN service instance ${ctor.name}`); + } + } + + private _createServiceInstance( + id: ServiceIdentifier, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ctor: any, + args: ReadonlyArray = [], + supportsDelayedInstantiation: boolean, + _trace: Trace, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + disposeBucket: Set, ): T { if (!supportsDelayedInstantiation) { const root = this._root(); root._inProgress.push(id); try { - return this._createInstance(desc.ctor, desc.staticArguments.slice(), _trace); + const result = this._createInstance(ctor, args.slice(), _trace); + disposeBucket.add(result); + this._constructionOrder.push(result); + return result; } finally { const popIdx = root._inProgress.lastIndexOf(id); if (popIdx >= 0) { @@ -647,23 +645,12 @@ export class InstantiationService implements IInstantiationService { disposable?: IDisposable; }; const earlyListeners = new Map>(); - const _ctor = desc.ctor; - const _args = desc.staticArguments.slice(); - // Capture references the executor needs. - // eslint-disable-next-line @typescript-eslint/no-this-alias - const self = this; + const child = new InstantiationService(undefined, this._strict, this, this._enableTracing); + child._globalGraphImplicitDependency = String(id); + const _ctor = ctor; + const _args = args.slice(); const idle = new GlobalIdleValue(() => { - const root = self._root(); - root._inProgress.push(id); - let result: T; - try { - result = self._createInstance(_ctor, _args.slice(), _trace); - } finally { - const popIdx = root._inProgress.lastIndexOf(id); - if (popIdx >= 0) { - root._inProgress.splice(popIdx, 1); - } - } + const result = child._createInstance(_ctor, _args.slice(), _trace); // Replay parked event subscriptions against the real instance. for (const [key, values] of earlyListeners) { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -675,7 +662,8 @@ export class InstantiationService implements IInstantiationService { } } earlyListeners.clear(); - self._servicesToMaybeDispose.add(result); + disposeBucket.add(result); + this._constructionOrder.push(result); return result; }); @@ -704,12 +692,10 @@ export class InstantiationService implements IInstantiationService { disposable: undefined, }; const rm = list!.push(entry); - return { - dispose() { - rm(); - entry.disposable?.dispose(); - }, - }; + return toDisposable(() => { + rm(); + entry.disposable?.dispose(); + }); }; return event; } @@ -750,36 +736,15 @@ export class InstantiationService implements IInstantiationService { * parent chain so a child can deposit a parent-owned service back into the * parent's cache. Mirrors krow `instantiationService.ts:220-228`. * - * Also stamps the local `_instances` + (eager path only) `_constructionOrder` - * so dispose() can walk teardown in reverse construction order (PLAN D8). - * Lazy (Proxy-wrapped) services are intentionally NOT added to - * `_constructionOrder` — disposing them would require reading a property - * of the Proxy, which would force materialisation. Lazy disposal is - * handled by `_servicesToMaybeDispose` once the Proxy materialises. + * Construction order is tracked when services are created, not when they + * are deposited into the collection. Pre-built instances are therefore not + * container-owned for disposal. */ - private _setCreatedServiceInstance( - id: ServiceIdentifier, - instance: T, - lazy: boolean = false, - ): void { + private _setCreatedServiceInstance(id: ServiceIdentifier, instance: T): void { if (this.services.get(id) instanceof SyncDescriptor) { - // Replace the descriptor in-place with the constructed instance so a - // second lookup short-circuits via the `_instances` cache OR the - // services map directly. this.services.set(id, instance); - this._instances.set(id, instance); - if (!lazy) { - this._constructionOrder.push(id); - } - } else if (this.services.has(id)) { - // Pre-built instance shorthand — already cached locally. - this._instances.set(id, instance); - // Don't add to `_constructionOrder` again if it was already pushed. - if (!lazy && !this._constructionOrder.includes(id)) { - this._constructionOrder.push(id); - } } else if (this._parent) { - this._parent._setCreatedServiceInstance(id, instance, lazy); + this._parent._setCreatedServiceInstance(id, instance); } else { throw new Error( `illegal state - setting UNKNOWN service instance '${String(id)}'`, @@ -803,6 +768,16 @@ export class InstantiationService implements IInstantiationService { return instanceOrDesc as T | SyncDescriptor | undefined; } + private _throwIfStrict(msg: string, printWarning: boolean): void { + if (printWarning) { + // eslint-disable-next-line no-console + globalThis.console.warn(msg); + } + if (this._strict) { + throw new Error(msg); + } + } + /** Walk up to the tree root. Used for the shared in-progress stack. */ private _root(): InstantiationService { // eslint-disable-next-line @typescript-eslint/no-this-alias diff --git a/packages/agent-core/src/di/testInstantiationService.ts b/packages/agent-core/src/di/testInstantiationService.ts index 128da6281..e23a43752 100644 --- a/packages/agent-core/src/di/testInstantiationService.ts +++ b/packages/agent-core/src/di/testInstantiationService.ts @@ -7,12 +7,8 @@ * Adapted from krow `testInstantiationService.ts` (in turn the VSCode * original). Two divergences from krow: * - * 1. **Ctor signature**: kimi's `InstantiationService` constructor is - * `(services, parent, _enableTracing)` (parent second, no `_strict` - * mode); krow's is `(services, strict, parent, _enableTracing)`. - * The `strict` boolean does not exist in kimi yet — `_throwIfStrict` - * was not ported because the daemon never enables it. If a future - * phase adds strict mode, surface a 2nd ctor param here. + * 1. **Ctor signature**: follows the runtime container's VS Code order + * `(services, strict, parent, _enableTracing)`. * 2. **`createServices` factory**: krow uses `DisposableStore` / * `toDisposable` from `base/`. kimi's `Disposable` class is a * different shape (LIFO subdisposable owner, not a Set). The factory @@ -60,10 +56,11 @@ export class TestInstantiationService extends InstantiationService implements Se constructor( serviceCollection: ServiceCollection = new ServiceCollection(), - parent: InstantiationService | null = null, + strict: boolean = false, + parent?: InstantiationService, enableTracing: boolean = false, ) { - super(serviceCollection, parent, enableTracing); + super(serviceCollection, strict, parent, enableTracing); this._serviceCollection = serviceCollection; } @@ -120,7 +117,7 @@ export class TestInstantiationService extends InstantiationService implements Se 'createChild requires a ServiceCollection instance (got something else)', ); } - const child = new TestInstantiationService(services, this); + const child = new TestInstantiationService(services, false, this); // The base class tracks children for cascade-dispose via its private // `_children` set; we mirror by relying on the parent's `_children` // being populated through the base ctor's parent reference. But the diff --git a/packages/agent-core/test/di/child.test.ts b/packages/agent-core/test/di/child.test.ts index 69e995f31..f421f201b 100644 --- a/packages/agent-core/test/di/child.test.ts +++ b/packages/agent-core/test/di/child.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'; import { SyncDescriptor } from '#/di/descriptors'; import { InstantiationService } from '#/di/instantiationService'; -import { createDecorator } from '#/di/instantiation'; +import { IInstantiationService, createDecorator, type IInstantiationService as IInstantiationServiceType } from '#/di/instantiation'; import { Disposable, type IDisposable } from '#/di/lifecycle'; import { ServiceCollection } from '#/di/serviceCollection'; @@ -52,6 +52,66 @@ describe('InstantiationService.createChild', () => { expect(fromChild).not.toBe(fromParent); }); + it('constructs parent-owned descriptors in the parent scope when resolved from a child', () => { + interface IDep { + tag: string; + } + class ParentDep implements IDep { + tag = 'parent'; + } + class ChildDep implements IDep { + tag = 'child'; + } + class ParentOwned { + constructor(public readonly dep: IDep) {} + } + + const IDep = createDecorator('owner-scope-dep'); + const IParentOwned = createDecorator('owner-scope-parent-owned'); + (IDep as unknown as (t: unknown, k: string, i: number) => void)( + ParentOwned, + '', + 0, + ); + + const parent = new InstantiationService( + new ServiceCollection( + [IDep, new SyncDescriptor(ParentDep)], + [IParentOwned, new SyncDescriptor(ParentOwned)], + ), + ); + const child = parent.createChild( + new ServiceCollection([IDep, new SyncDescriptor(ChildDep)]), + ); + + const fromChild = child.invokeFunction((a) => a.get(IParentOwned)); + const fromParent = parent.invokeFunction((a) => a.get(IParentOwned)); + expect(fromChild).toBe(fromParent); + expect(fromChild.dep).toBeInstanceOf(ParentDep); + expect(fromChild.dep.tag).toBe('parent'); + }); + + it('injects the parent instantiation service into parent-owned services resolved from a child', () => { + class ParentOwned { + constructor(public readonly ix: IInstantiationServiceType) {} + } + const IParentOwned = createDecorator('owner-scope-parent-ix'); + (IInstantiationService as unknown as (t: unknown, k: string, i: number) => void)( + ParentOwned, + '', + 0, + ); + + const parent = new InstantiationService( + new ServiceCollection([IParentOwned, new SyncDescriptor(ParentOwned)]), + ); + const child = parent.createChild(new ServiceCollection()); + + const instance = child.invokeFunction((a) => a.get(IParentOwned)); + expect(instance.ix).toBe(parent); + expect(instance.ix).not.toBe(child); + }); + it('sibling isolation: two children of the same parent do not share scoped services', () => { interface IScoped { tag: string; @@ -74,10 +134,9 @@ describe('InstantiationService.createChild', () => { expect(childA.invokeFunction((a) => a.get(IScoped).tag)).toBe('A'); expect(childB.invokeFunction((a) => a.get(IScoped).tag)).toBe('B'); - // Parent has no registration; resolution from parent must throw. - expect(() => parent.invokeFunction((a) => a.get(IScoped))).toThrowError( - /No service registered/, - ); + // Parent has no registration; non-strict resolution follows VS Code and + // returns undefined. + expect(parent.invokeFunction((a) => a.get(IScoped))).toBeUndefined(); }); it('dispose order: A→B→C construction yields C→B→A teardown', () => { @@ -122,6 +181,25 @@ describe('InstantiationService.createChild', () => { expect(events).toEqual(['disposed C', 'disposed B', 'disposed A']); }); + it('does not dispose pre-built service instances from the ServiceCollection', () => { + const events: string[] = []; + interface IFoo { + tag: string; + } + const IFoo = createDecorator('prebuilt-not-disposed'); + class Foo implements IFoo, IDisposable { + tag = 'foo'; + dispose(): void { + events.push('disposed'); + } + } + const instance = new Foo(); + const ix = new InstantiationService(new ServiceCollection([IFoo, instance])); + expect(ix.invokeFunction((a) => a.get(IFoo))).toBe(instance); + ix.dispose(); + expect(events).toEqual([]); + }); + it('idempotent dispose: second call is a no-op', () => { const events: string[] = []; interface IFoo { diff --git a/packages/agent-core/test/di/delayed.test.ts b/packages/agent-core/test/di/delayed.test.ts index 71551feb9..ae794d387 100644 --- a/packages/agent-core/test/di/delayed.test.ts +++ b/packages/agent-core/test/di/delayed.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'; import { SyncDescriptor } from '#/di/descriptors'; import { InstantiationService } from '#/di/instantiationService'; -import { createDecorator } from '#/di/instantiation'; +import { IInstantiationService, createDecorator, type IInstantiationService as IInstantiationServiceType } from '#/di/instantiation'; import { ServiceCollection } from '#/di/serviceCollection'; /** @@ -130,4 +130,78 @@ describe('Delayed instantiation Proxy (P1.2)', () => { proxy.fire('hello-world'); expect(received).toEqual(['hello-world']); }); + + it('materialises delayed services in a child scope and records implicit dependency cycles', () => { + interface IA { + _serviceBrand: undefined; + doIt(): boolean; + } + interface IB { + _serviceBrand: undefined; + b(): boolean; + } + const IA = createDecorator('delayed-graph-A'); + const IB = createDecorator('delayed-graph-B'); + + class BConsumer { + constructor(private readonly b: IB) {} + doIt(): boolean { + return this.b.b(); + } + } + (IB as unknown as (t: unknown, k: string, i: number) => void)( + BConsumer, + '', + 0, + ); + + class AService implements IA { + _serviceBrand: undefined; + private readonly consumer: BConsumer; + constructor(ix: IInstantiationServiceType) { + this.consumer = ix.createInstance(BConsumer); + } + doIt(): boolean { + return this.consumer.doIt(); + } + } + (IInstantiationService as unknown as (t: unknown, k: string, i: number) => void)( + AService, + '', + 0, + ); + + class BService implements IB { + _serviceBrand: undefined; + constructor(public readonly a: IA) {} + b(): boolean { + return true; + } + } + (IA as unknown as (t: unknown, k: string, i: number) => void)( + BService, + '', + 0, + ); + + class ExposedInstantiationService extends InstantiationService { + cycle(): string | undefined { + return this._globalGraph?.findCycleSlow(); + } + } + + const ix = new ExposedInstantiationService( + new ServiceCollection( + [IA, new SyncDescriptor(AService, [], true)], + [IB, new SyncDescriptor(BService)], + ), + true, + undefined, + true, + ); + + const a = ix.invokeFunction((accessor) => accessor.get(IA)); + expect(a.doIt()).toBe(true); + expect(ix.cycle()).toBe('delayed-graph-A -> delayed-graph-B -> delayed-graph-A'); + }); }); diff --git a/packages/agent-core/test/di/descriptor.test.ts b/packages/agent-core/test/di/descriptor.test.ts index a57aa1b76..7004d158d 100644 --- a/packages/agent-core/test/di/descriptor.test.ts +++ b/packages/agent-core/test/di/descriptor.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest'; -import { InstantiationType, SyncDescriptor, SyncDescriptor0 } from '#/di/descriptors'; +import * as descriptorsModule from '#/di/descriptors'; +import { SyncDescriptor, type SyncDescriptor0 } from '#/di/descriptors'; +import { InstantiationType } from '#/di/extensions'; class MyClass { constructor( @@ -37,14 +39,16 @@ describe('SyncDescriptor', () => { }); describe('SyncDescriptor0 (P0.4)', () => { - it('is a SyncDescriptor with empty staticArguments', () => { + it('is a type-only zero-argument descriptor shape', () => { class Zero { constructor() {} } - const d = new SyncDescriptor0(Zero); - expect(d).toBeInstanceOf(SyncDescriptor); + const d: SyncDescriptor0 = { ctor: Zero }; expect(d.ctor).toBe(Zero); - expect(d.staticArguments).toEqual([]); + }); + + it('is not exported as a runtime value from descriptors', () => { + expect('SyncDescriptor0' in descriptorsModule).toBe(false); }); }); @@ -53,4 +57,8 @@ describe('InstantiationType', () => { expect(InstantiationType.Eager).toBe(0); expect(InstantiationType.Delayed).toBe(1); }); + + it('is not exported as a runtime value from descriptors', () => { + expect('InstantiationType' in descriptorsModule).toBe(false); + }); }); diff --git a/packages/agent-core/test/di/extensions.test.ts b/packages/agent-core/test/di/extensions.test.ts index f45e195e1..4dfbf7ea1 100644 --- a/packages/agent-core/test/di/extensions.test.ts +++ b/packages/agent-core/test/di/extensions.test.ts @@ -1,7 +1,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { SyncDescriptor, InstantiationType } from '#/di/descriptors'; +import { SyncDescriptor } from '#/di/descriptors'; import { + InstantiationType, _clearRegistryForTests, getSingletonServiceDescriptors, registerSingleton, @@ -70,7 +71,7 @@ describe('registerSingleton / getSingletonServiceDescriptors', () => { expect(map.get('bar')).toBe(true); }); - it('re-registering the same id overwrites the previous entry (VS Code semantics)', () => { + it('re-registering the same id appends another registry entry', () => { interface ILogger { log(m: string): void; } @@ -88,12 +89,10 @@ describe('registerSingleton / getSingletonServiceDescriptors', () => { registerSingleton(ILogger, A); registerSingleton(ILogger, B); - // Snapshot length stays at 1; the entry wraps B's ctor. const snapshot = getSingletonServiceDescriptors(); - expect(snapshot).toHaveLength(1); - const [id, descriptor] = snapshot[0]!; - expect(id).toBe(ILogger); - expect(descriptor.ctor).toBe(B); + expect(snapshot).toHaveLength(2); + expect(snapshot.map(([id]) => id)).toEqual([ILogger, ILogger]); + expect(snapshot.map(([, descriptor]) => descriptor.ctor)).toEqual([A, B]); }); it('accepts a SyncDescriptor overload directly', () => { @@ -192,7 +191,7 @@ describe('registerSingleton / getSingletonServiceDescriptors', () => { logSpy.mockRestore(); }); - it('snapshot is independent of subsequent registrations (returns a fresh array)', () => { + it('getSingletonServiceDescriptors returns the live registry array', () => { interface IFoo { a: number; } @@ -214,10 +213,11 @@ describe('registerSingleton / getSingletonServiceDescriptors', () => { } registerSingleton(IBar, Bar); - // Prior snapshot must not have been mutated retroactively. - expect(snap1).toHaveLength(1); + expect(snap1).toHaveLength(2); + expect(snap1).toBe(getSingletonServiceDescriptors()); const snap2 = getSingletonServiceDescriptors(); expect(snap2).toHaveLength(2); + expect(snap2).toBe(snap1); }); }); diff --git a/packages/agent-core/test/di/instantiation.test.ts b/packages/agent-core/test/di/instantiation.test.ts index 472033889..f9df75f31 100644 --- a/packages/agent-core/test/di/instantiation.test.ts +++ b/packages/agent-core/test/di/instantiation.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from 'vitest'; import { SyncDescriptor } from '#/di/descriptors'; import { InstantiationService } from '#/di/instantiationService'; -import { createDecorator } from '#/di/instantiation'; +import { createDecorator, type ServicesAccessor } from '#/di/instantiation'; import { ServiceCollection } from '#/di/serviceCollection'; interface ILogger { @@ -146,9 +146,66 @@ describe('InstantiationService (basic)', () => { expect(ix.invokeFunction((a) => a.get(ILogger))).toBe(inst); }); - it('throws when getting an unregistered id', () => { + it('non-strict mode returns undefined for an unregistered id', () => { const ix = new InstantiationService(); - expect(() => ix.invokeFunction((a) => a.get(ILogger))).toThrowError(/No service registered/); + expect(ix.invokeFunction((a) => a.get(ILogger))).toBeUndefined(); + }); + + it('strict mode throws when getting an unregistered id', () => { + const ix = new InstantiationService(new ServiceCollection(), true); + expect(() => ix.invokeFunction((a) => a.get(ILogger))).toThrowError( + /unknown service 'logger'/, + ); + }); + + it('invokeFunction forwards additional arguments to the callback', () => { + const ix = new InstantiationService(); + expect( + ix.invokeFunction( + (_a, prefix: string, count: number) => `${prefix}:${count}`, + 'req', + 7, + ), + ).toBe('req:7'); + }); + + it('invokeFunction accessor is invalid after the callback returns', () => { + class AccessorLogger implements ILogger { + log(_m: string): void { + /* noop */ + } + } + const ix = new InstantiationService( + new ServiceCollection([ILogger, new SyncDescriptor(AccessorLogger)]), + ); + let captured: ServicesAccessor | undefined; + ix.invokeFunction((a) => { + captured = a; + expect(a.get(ILogger)).toBeInstanceOf(AccessorLogger); + }); + expect(() => captured!.get(ILogger)).toThrowError( + /service accessor is only valid/, + ); + }); + + it('uses the live ServiceCollection entry instead of a stale instance cache', () => { + class InitialLogger implements ILogger { + log(_m: string): void { + /* noop */ + } + } + class ReplacementLogger implements ILogger { + log(_m: string): void { + /* noop */ + } + } + const first = new InitialLogger(); + const second = new ReplacementLogger(); + const services = new ServiceCollection([ILogger, first]); + const ix = new InstantiationService(services); + expect(ix.invokeFunction((a) => a.get(ILogger))).toBe(first); + services.set(ILogger, second); + expect(ix.invokeFunction((a) => a.get(ILogger))).toBe(second); }); it('createChild returns a child container, dispose tears down', () => { diff --git a/packages/agent-core/test/di/self-register.test.ts b/packages/agent-core/test/di/self-register.test.ts index bb5208dd0..d32c9739f 100644 --- a/packages/agent-core/test/di/self-register.test.ts +++ b/packages/agent-core/test/di/self-register.test.ts @@ -12,6 +12,10 @@ import { ServiceCollection } from '#/di/serviceCollection'; * having to thread the container through manually. */ describe('IInstantiationService self-registration (P0.5)', () => { + it('uses the VS Code diagnostic service id', () => { + expect(String(IInstantiationService)).toBe('instantiationService'); + }); + it('root container exposes itself via accessor.get(IInstantiationService)', () => { const ix = new InstantiationService(); const resolved = ix.invokeFunction((a) => a.get(IInstantiationService)); diff --git a/packages/agent-core/test/di/trace.test.ts b/packages/agent-core/test/di/trace.test.ts index 8804616e1..e3f486104 100644 --- a/packages/agent-core/test/di/trace.test.ts +++ b/packages/agent-core/test/di/trace.test.ts @@ -4,10 +4,9 @@ import { InstantiationService, Trace } from '#/di/instantiationService'; import { ServiceCollection } from '#/di/serviceCollection'; /** - * P0.2: `Trace` class + `_enableTracing` ctor param installed but not yet - * consumed by any code path inside `InstantiationService`. These assertions - * only verify the class is reachable and the ctor signature is backward - * compatible (third param defaults to `false`). + * P0.2: `Trace` class + `_enableTracing` ctor param installed. These + * assertions verify the class is reachable and the constructor follows the + * VS Code argument order `(services, strict, parent, enableTracing)`. */ class ExposedInstantiationService extends InstantiationService { @@ -23,10 +22,9 @@ describe('InstantiationService Trace installation (P0.2)', () => { expect(ix).toBeInstanceOf(InstantiationService); }); - it('constructs with explicit null parent and accepts the 3rd tracing arg = true', () => { + it('constructs with strict=false, undefined parent, and tracing=true', () => { const coll = new ServiceCollection(); - // `parent: null` mirrors the no-parent case; `_enableTracing: true` opts in. - const ix = new ExposedInstantiationService(coll, null, true); + const ix = new ExposedInstantiationService(coll, false, undefined, true); expect(ix).toBeInstanceOf(InstantiationService); expect(ix.tracingEnabled).toBe(true); }); diff --git a/packages/daemon/src/start.ts b/packages/daemon/src/start.ts index 42ff74529..21913140e 100644 --- a/packages/daemon/src/start.ts +++ b/packages/daemon/src/start.ts @@ -509,7 +509,7 @@ export async function startDaemon(opts: DaemonStartOptions): Promise { coreProcessOptions: { homeDir: bridgeHome }, }); // Override the IOAuthService in the container post-boot. The container's - // `ServiceCollection` is public; we re-set the slot and also clear the - // `_instances` cache so per-request `accessor.get(IOAuthService)` returns - // the stub instead of the cached real impl. + // `ServiceCollection` is live, so subsequent requests resolve the stub. const ix = daemon.services as unknown as { services: { set: (id: unknown, v: unknown) => void }; - _instances: Map; }; ix.services.set(IOAuthService, stub); - ix._instances.set(IOAuthService, stub); return daemon; } diff --git a/packages/daemon/test/prompt.e2e.test.ts b/packages/daemon/test/prompt.e2e.test.ts index bedab6ad5..732b4831a 100644 --- a/packages/daemon/test/prompt.e2e.test.ts +++ b/packages/daemon/test/prompt.e2e.test.ts @@ -115,10 +115,8 @@ function overridePromptService( const replacement = { ...defaultImpl, ...stub }; const ix = r.services as unknown as { services: { set: (id: unknown, impl: unknown) => void }; - _instances: Map; }; ix.services.set(IPromptService, replacement); - ix._instances.set(IPromptService, replacement); } function buildMultipart(parts: { diff --git a/packages/daemon/test/tasks.e2e.test.ts b/packages/daemon/test/tasks.e2e.test.ts index 81e46a812..30305fbe0 100644 --- a/packages/daemon/test/tasks.e2e.test.ts +++ b/packages/daemon/test/tasks.e2e.test.ts @@ -119,11 +119,8 @@ async function createSession(r: RunningDaemon): Promise { * 40904 / 40406 envelope mapping paths without seeding real background * tasks. * - * The InstantiationService caches resolved instances in `_instances` after - * the first `a.get(...)`. The daemon's `start.ts` warms the cache for every - * registered identifier, so a `services.set(...)` would not be observed by - * subsequent route requests. We mutate both the registration map and the - * instance cache. + * The InstantiationService reads from the live `ServiceCollection`, so a + * post-boot `services.set(...)` is observed by subsequent route requests. */ function overrideTaskService( r: RunningDaemon, @@ -140,10 +137,8 @@ function overrideTaskService( const replacement = { ...defaultImpl, ...stub }; const ix = r.services as unknown as { services: { set: (id: unknown, impl: unknown) => void }; - _instances: Map; }; ix.services.set(ITaskService, replacement); - ix._instances.set(ITaskService, replacement); } describe('GET /api/v1/sessions/{sid}/tasks', () => { diff --git a/packages/services/test/interfaces.test.ts b/packages/services/test/interfaces.test.ts index f666a70b6..cbd48aedb 100644 --- a/packages/services/test/interfaces.test.ts +++ b/packages/services/test/interfaces.test.ts @@ -193,12 +193,12 @@ describe('@moonshot-ai/services · interfaces', () => { } }); - it('looking up an unregistered service throws with the decorator diagnostic name', () => { + it('looking up an unregistered service returns undefined in non-strict mode', () => { const ix = new InstantiationService(new ServiceCollection()); try { - expect(() => ix.invokeFunction((a) => a.get(IEventService))).toThrow(/eventService/); - expect(() => ix.invokeFunction((a) => a.get(IApprovalService))).toThrow(/approvalService/); - expect(() => ix.invokeFunction((a) => a.get(IQuestionService))).toThrow(/questionService/); + expect(ix.invokeFunction((a) => a.get(IEventService))).toBeUndefined(); + expect(ix.invokeFunction((a) => a.get(IApprovalService))).toBeUndefined(); + expect(ix.invokeFunction((a) => a.get(IQuestionService))).toBeUndefined(); } finally { ix.dispose(); }