fix(runtime): register decorators at class-evaluation time, not construction#21
fix(runtime): register decorators at class-evaluation time, not construction#21eren-karakus0 wants to merge 2 commits into
Conversation
…ruction Member decorators (@tool/@interceptor/@command/@install/@upgrade/@run) recorded their registrations from context.addInitializer, which per the TC39 proposal only runs when an instance is first constructed. But the runtime bridge reads the registry BEFORE constructing anything, so every lifecycle/tool/run registration looked empty: @install hooks silently no-op'd, @run loops reported "exited before signaling ready", and tool dispatch never reached the capsule. Defer each member registration onto a module-scoped queue instead; the @capsule class decorator (which runs last and knows the constructor) flushes the queue. Registration now lands synchronously at class-evaluation time -- before any construction and before the bridge reads it -- and runs exactly once per class rather than once per instance. Fixes astrid-runtime#20. Also resolves the per-instance re-registration flagged in astrid-runtime#18 (a second construction previously threw "Only one @install method allowed").
There was a problem hiding this comment.
Code Review
This pull request refactors the decorator registration timing in the Astrid SDK. Instead of using context.addInitializer (which runs at construction time), member decorators now defer their registration onto a module-scoped queue, which is then flushed by the @capsule class decorator during class evaluation. The reviewer pointed out that using a single global deferred queue can cause registrations to leak between classes if multiple classes are evaluated in the same runtime context. To resolve this, the reviewer suggested isolating deferred registrations per class by leveraging context.metadata and storing the deferred list in a WeakMap keyed by the metadata object, providing specific code suggestions across all decorators and the registry.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type DeferredRecord = (ctor: CapsuleConstructor) => void; | ||
| let deferred: DeferredRecord[] = []; | ||
|
|
||
| function ensureRegistration(ctor: CapsuleConstructor): CapsuleRegistration { | ||
| if (registration !== undefined && registration.ctor === ctor) { | ||
| return registration; | ||
| } | ||
| let pending = pendingByCtor.get(ctor); | ||
| if (pending === undefined) { | ||
| pending = newRegistration(ctor, undefined); | ||
| pendingByCtor.set(ctor, pending); | ||
| } | ||
| return pending; | ||
| } | ||
|
|
||
| /** Adopt any decorator entries buffered before @capsule fired. */ | ||
| export function adoptPending(ctor: CapsuleConstructor): void { | ||
| if (registration === undefined || registration.ctor !== ctor) return; | ||
| const pending = pendingByCtor.get(ctor); | ||
| if (pending === undefined) return; | ||
| for (const [name, entry] of pending.tools) registration.tools.set(name, entry); | ||
| for (const [topic, entry] of pending.interceptors) registration.interceptors.set(topic, entry); | ||
| for (const [name, entry] of pending.commands) registration.commands.set(name, entry); | ||
| if (pending.installMethod !== undefined && registration.installMethod === undefined) { | ||
| registration.installMethod = pending.installMethod; | ||
| } | ||
| if (pending.upgradeMethod !== undefined && registration.upgradeMethod === undefined) { | ||
| registration.upgradeMethod = pending.upgradeMethod; | ||
| } | ||
| if (pending.runMethod !== undefined && registration.runMethod === undefined) { | ||
| registration.runMethod = pending.runMethod; | ||
| } | ||
| if (pending.description !== undefined && registration.description === undefined) { | ||
| registration.description = pending.description; | ||
| export function defer(record: DeferredRecord): void { | ||
| deferred.push(record); | ||
| } | ||
|
|
||
| /** Apply every deferred member registration against the now-known ctor. */ | ||
| export function flushDeferred(ctor: CapsuleConstructor): void { | ||
| const pending = deferred; | ||
| deferred = []; | ||
| for (const record of pending) record(ctor); | ||
| } |
There was a problem hiding this comment.
Using a single global deferred queue can cause registrations to leak between classes if multiple classes are evaluated in the same module/runtime context (e.g., base classes, helper classes, or multiple test classes in unit tests). If a class has member decorators but is not decorated with @capsule, its deferred registrations will remain in the global queue and get flushed against the next @capsule class that is evaluated.
To isolate deferred registrations per class, we can leverage context.metadata (which is shared across all decorators of the same class in TC39 Stage 3 decorators) and store the deferred list in a WeakMap keyed by context.metadata, falling back to the global queue if metadata is not available.
type DeferredRecord = (ctor: CapsuleConstructor) => void;
let deferred: DeferredRecord[] = [];
let pendingByMetadata = new WeakMap<object, DeferredRecord[]>();
export function defer(metadata: object | undefined, record: DeferredRecord): void {
if (!metadata) {
deferred.push(record);
return;
}
let list = pendingByMetadata.get(metadata);
if (!list) {
list = [];
pendingByMetadata.set(metadata, list);
}
list.push(record);
}
/** Apply every deferred member registration against the now-known ctor. */
export function flushDeferred(ctor: CapsuleConstructor, metadata?: object): void {
let pending: DeferredRecord[] = [];
if (metadata) {
pending = pendingByMetadata.get(metadata) ?? [];
pendingByMetadata.delete(metadata);
}
if (deferred.length > 0) {
pending = pending.concat(deferred);
deferred = [];
}
for (const record of pending) record(ctor);
}| export function capsule<T extends CapsuleConstructor>( | ||
| target: T, | ||
| _context: ClassDecoratorContext<T>, | ||
| ): T { | ||
| registerCapsule(target); | ||
| adoptPending(target); | ||
| flushDeferred(target); | ||
| return target; | ||
| } |
There was a problem hiding this comment.
Pass context.metadata to flushDeferred to isolate and flush only the deferred registrations belonging to this class.
| export function capsule<T extends CapsuleConstructor>( | |
| target: T, | |
| _context: ClassDecoratorContext<T>, | |
| ): T { | |
| registerCapsule(target); | |
| adoptPending(target); | |
| flushDeferred(target); | |
| return target; | |
| } | |
| export function capsule<T extends CapsuleConstructor>( | |
| target: T, | |
| context: ClassDecoratorContext<T>, | |
| ): T { | |
| registerCapsule(target); | |
| flushDeferred(target, context.metadata); | |
| return target; | |
| } |
| const methodName = String(context.name); | ||
| defer((ctor) => recordInstall(ctor, methodName)); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => recordInstall(ctor, methodName)); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => recordInstall(ctor, methodName)); |
| const methodName = String(context.name); | ||
| defer((ctor) => recordUpgrade(ctor, methodName)); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => recordUpgrade(ctor, methodName)); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => recordUpgrade(ctor, methodName)); |
| const methodName = String(context.name); | ||
| defer((ctor) => recordRun(ctor, methodName)); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => recordRun(ctor, methodName)); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => recordRun(ctor, methodName)); |
| const methodName = String(context.name); | ||
| defer((ctor) => | ||
| recordTool(ctor, { | ||
| name, | ||
| methodName: String(context.name), | ||
| methodName, | ||
| mutable: options.mutable === true, | ||
| description: options.description, | ||
| inputSchema: options.inputSchema, | ||
| }); | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => | |
| recordTool(ctor, { | |
| name, | |
| methodName: String(context.name), | |
| methodName, | |
| mutable: options.mutable === true, | |
| description: options.description, | |
| inputSchema: options.inputSchema, | |
| }); | |
| }); | |
| }), | |
| ); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => | |
| recordTool(ctor, { | |
| name, | |
| methodName, | |
| mutable: options.mutable === true, | |
| description: options.description, | |
| inputSchema: options.inputSchema, | |
| }), | |
| ); |
| const methodName = String(context.name); | ||
| defer((ctor) => | ||
| recordInterceptor(ctor, { | ||
| topic, | ||
| methodName: String(context.name), | ||
| methodName, | ||
| mutable: options.mutable === true, | ||
| }); | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => | |
| recordInterceptor(ctor, { | |
| topic, | |
| methodName: String(context.name), | |
| methodName, | |
| mutable: options.mutable === true, | |
| }); | |
| }); | |
| }), | |
| ); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => | |
| recordInterceptor(ctor, { | |
| topic, | |
| methodName, | |
| mutable: options.mutable === true, | |
| }), | |
| ); |
| const methodName = String(context.name); | ||
| defer((ctor) => | ||
| recordCommand(ctor, { | ||
| name, | ||
| methodName: String(context.name), | ||
| methodName, | ||
| mutable: options.mutable === true, | ||
| }); | ||
| }); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Pass context.metadata to defer to associate this registration with the current class context.
| const methodName = String(context.name); | |
| defer((ctor) => | |
| recordCommand(ctor, { | |
| name, | |
| methodName: String(context.name), | |
| methodName, | |
| mutable: options.mutable === true, | |
| }); | |
| }); | |
| }), | |
| ); | |
| const methodName = String(context.name); | |
| defer(context.metadata, (ctor) => | |
| recordCommand(ctor, { | |
| name, | |
| methodName, | |
| mutable: options.mutable === true, | |
| }), | |
| ); |
| export function __resetRegistry(): void { | ||
| registration = undefined; | ||
| deferred = []; | ||
| } |
There was a problem hiding this comment.
Reset the pendingByMetadata WeakMap as well to ensure a clean state between tests.
| export function __resetRegistry(): void { | |
| registration = undefined; | |
| deferred = []; | |
| } | |
| export function __resetRegistry(): void { | |
| registration = undefined; | |
| deferred = []; | |
| pendingByMetadata = new WeakMap(); | |
| } |
Address review: a single module-scoped queue could leak a non-@capsule class's member registrations into the next @capsule class evaluated in the same module. Key the deferred closures by the class's shared `context.metadata` object (stored in a WeakMap) so each class only flushes its own. Where the runtime does not expose decorator metadata (no `Symbol.metadata`, e.g. the current ComponentizeJS engine) `context.metadata` is undefined and we fall back to the single module queue — correct for the enforced one-@capsule-per-module case, i.e. no worse than before. __resetRegistry clears the WeakMap too.
|
Good catch — addressed in Deferred closures are now keyed by the class's shared Verified by compiling with the package's own Happy to add these as real unit tests if you'd like to seed the |
Fixes #20.
Problem
The member decorators (
@tool/@interceptor/@command/@install/@upgrade/@run) recorded their registrations from insidecontext.addInitializer(...). Per the TC39 decorators proposal, those initializers only run during instance construction — butcreateBridge()reads the registry before constructing anything. So on a stock capsule built with the published packages, every lifecycle/tool/run registration looked empty:astridInstall()sawinstallMethod === undefined→@installhooks silently no-op'd;run()sawrunMethod === undefined→ returned immediately, so the kernel reported "run loop exited before signaling ready" and health-restarted the capsule;tool_describe/ hook dispatch saw emptytools/interceptors/commandsmaps.Only
@capsuleworked, because the class decorator registers the constructor at class-evaluation time — which is exactly why the failure looked like "the capsule is fine but empty".Fix
Register at class-evaluation time instead of construction time. Member decorators can't name the constructor yet (they run before the class exists), so each defers a small closure onto a module-scoped queue; the
@capsuleclass decorator runs last, already knows the constructor, and flushes the queue against it:Registration now lands synchronously at class-evaluation time — before any construction and before the bridge reads it — and runs exactly once per class rather than once per instance. This lets the previous
pendingByCtor/ensureRegistration/adoptPendingbuffering machinery go away.As a bonus this also resolves the per-instance re-registration flagged by the audit in #18: previously a second
new Capsule()re-ran the initializers and threwOnly one @install method allowed.Validation
Compiled with the package's own
tsc(the same emit ComponentizeJS consumes) and run in Node — reading the registry the way the bridge does, without constructing the capsule:Before (published code):
After (this PR):
tsc --noEmitpasses (all strict flags:exactOptionalPropertyTypes,verbatimModuleSyntax,noUncheckedIndexedAccess, …).@installhook run a full HTTP session inside the kernel sandbox and kept@run+runtime.signalReady()healthy on astrid 0.9.0.The diff is limited to
capsule.ts,tool.ts, andruntime/registry.ts;bridge.tsis unchanged (its "the registry is populated before I read it" assumption is simply true again).Happy to fold the before/after check into a proper unit test if you'd like to seed the test surface the CI's
npm teststep is reserved for — just say the word and I'll add it (vitest or your harness of choice).