From 06cebf1bf4cbd840b00a6689b78cf44ca99fb562 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:21:58 +0800 Subject: [PATCH] fix(scope): make the dispatch carrier method-transparent for native-private subjects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ds-review-bot delta-round finding, verified: cordis hands the carrier to listeners as `this`, and the event declarations type it Scoped — so driving the subject through it (this.send(...) in an agent/* listener) is a SUPPORTED shape. The withProps-based carrier delegated gets with the PROXY as receiver, so ReactLoopAgent's send/steer/cancel — which read the native-private #carrier through a getter — threw TypeError when called that way (private members do not exist on proxy receivers). scopeTarget now builds its own proxy: overlay props (the composed filter and the carrier mark) answer from a null-shadowed literal via hasOwn (`in` would let Object.prototype's toString/constructor shadow the subject's), every other get delegates with the BASE as receiver (getters see the real object) and returns functions bound to the base (method calls execute on the real receiver), sets land on the base. A proxy-invariant guard reports frozen own function props unchanged (binding them would violate the get invariant). This kills the class at the seam — any subject with native privates works, today's agents and whatever carries them next — instead of patching the one #carrier field. Pinned both ways: a scope.spec matrix (native-#private method/getter through the carrier mutates the real object; set delegation; frozen-own-prop invariant; overlay non-shadowing) and the bot's exact end-to-end scenario (an agent/session-start listener calling this.send drives a real turn) — both fail with TypeError against the withProps carrier. --- .../agent-loop/tests/scope-lifecycle.spec.ts | 23 +++++++ packages/core/scope/src/index.ts | 60 ++++++++++++++----- packages/core/scope/tests/scope.spec.ts | 43 +++++++++++++ 3 files changed, 111 insertions(+), 15 deletions(-) diff --git a/packages/core/agent-loop/tests/scope-lifecycle.spec.ts b/packages/core/agent-loop/tests/scope-lifecycle.spec.ts index 7a45898b8e..b89f06727d 100644 --- a/packages/core/agent-loop/tests/scope-lifecycle.spec.ts +++ b/packages/core/agent-loop/tests/scope-lifecycle.spec.ts @@ -170,6 +170,29 @@ describe('agent scope lifecycle', () => { expect(heard).toEqual(['a1:2']) }) + it('a listener may drive the agent through its declared `this` (the carrier is method-transparent)', async () => { + // ds-review-bot regression: agent/* listeners are typed + // `this: Scoped`, and ReactLoopAgent's send/steer/cancel read the + // native-private #carrier — a proxy-receiver carrier made + // `this.send(...)` throw TypeError. The carrier binds methods to the real + // agent, so driving through the event `this` is a working supported shape. + const adapter = new MockAdapter([textResponse('first'), textResponse('second')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + let followUpSent = false + ctx.on('agent/session-start', function (this: Agent) { + // Deliberately through `this`, not the args subject. + this.send(text('driven through this')) + followUpSent = true + }) + const second = ctx.agentLoop.create(AgentId('a2'), { model: 'mock' }) + expect(followUpSent).toBe(true) + await second.whenIdle() + // The send actually reached the loop: the prompt ran a turn. + expect(second.session.events.some(e => e.type === 'turn/start')).toBe(true) + await agent.whenIdle() + }) + it('owner unload honors the documented teardown order: unregistration AFTER the drain, before detach', async () => { const ctx = await harness() let handle!: ReturnType diff --git a/packages/core/scope/src/index.ts b/packages/core/scope/src/index.ts index 9c4fd0019e..1243b7d715 100644 --- a/packages/core/scope/src/index.ts +++ b/packages/core/scope/src/index.ts @@ -23,7 +23,7 @@ */ import type { Context } from 'cordis' -import { Context as CordisContext, withProps } from 'cordis' +import { Context as CordisContext } from 'cordis' /** * The identity a scope is keyed by. Opaque and compared by object identity — @@ -178,10 +178,14 @@ export function scopeOf(ctx: Context): ScopeKey | undefined { * * Use it as the `thisArg` of the dispatch: * `ctx.waterfall(scopeTarget(this, exec.agent), 'tools/pre-execute', …)`. The - * carrier is a proxy over `base` — listener `this` stays `base`-shaped, but - * identity-comparing `this` against the subject is not supported; the subject - * always travels in the event's arguments. The returned carrier is branded - * {@link Scoped} and runtime-marked ({@link isScopeCarrier} / + * carrier is a TRANSPARENT proxy over `base`: reads delegate with `base` as + * the receiver and retrieved methods are bound to `base`, so a listener may + * call subject methods through its `this` (`this.send(…)` on a + * `Scoped`) even when the subject uses native `#private` fields — a + * bare proxy receiver would throw on those. Identity is still not + * transparent: `this !== subject` and method identity varies per read; the + * subject always travels in the event's arguments. The returned carrier is + * branded {@link Scoped} and runtime-marked ({@link isScopeCarrier} / * {@link carrierKeyOf}) so both the type system and the dev invariants can * tell a carrier from a bare subject. * @param base - the object the event is dispatched on behalf of (the owning @@ -198,15 +202,41 @@ export function scopeTarget(base: T, key: ScopeKey | undefined const tag = scopeOf(ctx) return tag === undefined || tag === key } - // withProps overlays own-property reads; the symbol-keyed props have no - // structural overlap with T. withProps is typed `any` upstream (a generic - // proxy helper); the carrier is structurally the same T it overlays plus - // the compile-time brand, so pin the type via the return annotation. - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return withProps(base, { + const overlay: Record = { [CordisContext.filter]: filter, [kCarrier]: { key }, - }) + } + // A hand-rolled proxy, NOT cordis withProps: withProps delegates gets with + // the PROXY as receiver, so a getter on `base` runs with proxy `this` and a + // method call through the carrier gets a proxy receiver — either one throws + // on a native `#private` field of the subject (TypeError: private member + // not declared). Cordis hands the carrier to listeners as `this`, and the + // event declarations type it `Scoped` — so subject method calls + // through it are a SUPPORTED shape and must reach the real object: gets + // delegate with `base` as receiver, functions come back bound to `base`, + // and sets land on `base` directly. + return new Proxy(base, { + get(target, prop) { + // hasOwn, not `in`: the overlay literal inherits Object.prototype, so + // `in` would claim `toString`/`constructor` and shadow the subject's. + if (Object.hasOwn(overlay, prop)) return overlay[prop] + const value: unknown = Reflect.get(target, prop, target) + if (typeof value !== 'function') return value + // Proxy invariant guard: a non-configurable, non-writable OWN data + // property must be reported unchanged, so it cannot be bound. Class + // methods live on the prototype (no own descriptor) and bind freely; + // only a frozen own-function prop keeps the raw (unbound) function. + const own = Reflect.getOwnPropertyDescriptor(target, prop) + if (own !== undefined && own.configurable === false && own.writable === false) return value + // `Function.prototype.bind` types as `any`; the value is structurally + // T[prop] and the trap's contract is untyped (`any`), so unknown is the + // honest safe return. + return value.bind(target) as unknown + }, + set(target, prop, value) { + return Reflect.set(target, prop, value, target) + }, + }) as Scoped } /** @@ -219,9 +249,9 @@ export function scopeTarget(base: T, key: ScopeKey | undefined */ export function isScopeCarrier(value: unknown): value is Scoped { if (typeof value !== 'object' || value === null) return false - // A property READ, not an `in` check: withProps overlays props via get/set - // traps only (no `has` trap), so `kCarrier in carrier` would fall through to - // the wrapped base and always answer false. + // A property READ, not an `in` check: the carrier overlays its marks in the + // get trap only (no `has` trap), so `kCarrier in carrier` would fall + // through to the wrapped base and always answer false. return (value as { [kCarrier]?: { key: ScopeKey | undefined } })[kCarrier] !== undefined } diff --git a/packages/core/scope/tests/scope.spec.ts b/packages/core/scope/tests/scope.spec.ts index e932ed80db..53e5bc931b 100644 --- a/packages/core/scope/tests/scope.spec.ts +++ b/packages/core/scope/tests/scope.spec.ts @@ -179,6 +179,49 @@ describe('scopeTarget dispatch filtering', () => { expect(result).toBe('seed+v') expect(seenLabel).toBe('the-base') }) + + it('is transparent for subjects with native #private fields: methods and getters through the carrier reach the real object', () => { + // The ds-review-bot regression: cordis hands the carrier to listeners as + // `this` (typed Scoped), so subject method calls through it are a + // supported shape. A proxy that delegates with the PROXY as receiver + // (cordis withProps) throws TypeError on any native #private the method + // or getter touches; the carrier must delegate with the BASE as receiver + // and bind retrieved methods to it. + class Subject { + #count = 0 + bump(): number { return ++this.#count } + get count(): number { return this.#count } + } + const subject = new Subject() + const carrier = scopeTarget(subject, subject) + expect(carrier.bump()).toBe(1) // method call: bound to the base + expect(subject.count).toBe(1) // ...and it mutated the REAL object + expect(carrier.count).toBe(1) // getter: runs with the base as receiver + // The get trap returns the method already bound to the base; + // detachability IS the assertion. + // eslint-disable-next-line @typescript-eslint/unbound-method + const detached = carrier.bump + expect(detached()).toBe(2) + }) + + it('delegates sets to the base and leaves frozen own function props unbound (proxy invariant)', () => { + const frozenFn = (): string => 'frozen' + const base: { mutable: number; pinned: () => string; toString: () => string } = { + mutable: 0, + pinned: frozenFn, + toString: () => 'base-str', + } + Object.defineProperty(base, 'pinned', { value: frozenFn, writable: false, configurable: false }) + const carrier = scopeTarget(base, undefined) + carrier.mutable = 7 + expect(base.mutable).toBe(7) // sets land on the base, not a detached overlay + // A non-configurable, non-writable own data prop must be reported + // unchanged (binding it would violate the proxy get invariant). + expect(carrier.pinned).toBe(frozenFn) + // The overlay literal inherits Object.prototype; hasOwn (not `in`) keeps + // it from shadowing the subject's own prototype-surface members. + expect(String(carrier)).toBe('base-str') + }) }) describe('carrier marks', () => {