From 5fba953e0617ca5ea1b36bf753fea0f55cd0da79 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 01:36:18 -0400 Subject: [PATCH 01/26] fix(mcpl): reconcile channel lifecycle operations in order Overlapping registration reconciliation and channel tools could leave the server in an older state than durable intent. A removed/re-added entry made this worse: a late close mutated the deleted entry while the current entry incorrectly stayed open. Serialize transport operations per logical server/channel, keeping registration responses outside the queue. Re-read live registration, server, descriptor target, and desired state at dispatch and after each completion. Apply receipts only to the same live entry/connection, and reconcile again when the target or transport intent changed. Label-only updates do not repeat transport operations. Rejected operations release the queue for later decisions. Record admission intent before waiting on other channels. Explicit operations preserve immediate desired-state persistence and report when newer intent supersedes their requested state. Open/close/send fast paths now respect pending lifecycle work, so provisional live state cannot bypass the queue. Added deterministic deferred-RPC tests for the reported out-of-order reproduction, explicit open/close races, removal, descriptor updates, failure recovery, unrelated-channel progress, response-before-reconcile, and tune-out intent. The initial nine-case original-source run was 2 pass / 7 fail. The fixed expanded focused run is 66 pass / 0 fail without force-exit; full compiled-suite and final ten-case original-source comparison are running. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 4 + src/mcpl/channel-registry.ts | 150 +++++++++++++-------- test/channel-lifecycle-order.test.ts | 194 +++++++++++++++++++++++++++ 3 files changed, 294 insertions(+), 54 deletions(-) create mode 100644 test/channel-lifecycle-order.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b9a30b4c..ba3a8f7dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ Releases up to and including 0.7.3 predate this file; for their contents see ## Unreleased +### Fixed + +- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. + ## 0.19.0 — 2026-09-28 ### Added diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 01c81ec1a..098b036d8 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -16,6 +16,7 @@ import type { ContentBlock } from '@animalabs/membrane'; import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; import type { JsStore } from '@animalabs/chronicle'; +import { isDeepStrictEqual } from 'node:util'; import type { ChannelDescriptor, @@ -656,6 +657,15 @@ export class ChannelRegistry { /** One-time migration inputs from the retired recipe auto-open policy. */ private legacyPolicies = new Map(); private migratedLegacyPolicies = new Set(); + + // Keep one transport operation in flight per logical channel, including + // across descriptor replacement. Settled tails release themselves; a + // failed operation must not poison later decisions. + private channelLifecycleTails = new Map>(); + + private hasPendingLifecycle(entry: ChannelEntry): boolean { + return this.channelLifecycleTails.has(this.lifecycleKey(entry.serverId, entry.descriptor.id)); + } private handleCoalescedIncoming?: ChannelRegistryOptions['handleCoalescedIncoming']; constructor( @@ -2065,6 +2075,68 @@ export class ChannelRegistry { return channel.initiallyOpen !== true && byPolicy; } + /** + * Apply current durable intent, serializing the transport rather than the + * server's registration ACK. Re-read after every await: registration, + * descriptor updates, or a newer decision may have replaced our snapshot. + */ + private applyDesiredChannelState( + serverId: string, + channelId: string, + history?: ChannelHistoryRequest, + ): Promise<{ open: boolean; result?: ChannelsOpenResult }> { + const lifecycleKey = this.lifecycleKey(serverId, channelId); + const channelKey = `${serverId}:${channelId}`; + const previous = this.channelLifecycleTails.get(lifecycleKey) ?? Promise.resolve(); + const operation = previous.then(async () => { + for (;;) { + const entry = this.channels.get(channelKey); + if (!entry) throw new Error(`Channel is no longer registered: ${channelId}`); + const server = this.serverRegistry.getServer(serverId); + if (!server) throw new Error(`Server not found: ${serverId}`); + if (!CapabilityGrant.of(server).has('channels.lifecycle')) { + throw new Error(`channels.lifecycle not in "${serverId}"'s effective grant (§14.1)`); + } + const descriptor = entry.descriptor; + const desired = this.getDesiredState(serverId, channelId); + const open = desired === 'open' || desired === 'tuned-out'; + let result: ChannelsOpenResult | undefined; + try { + if (open) { + result = await server.sendChannelsOpen({ + channelId, type: descriptor.type, address: descriptor.address, + ...(history ? { history } : {}), + }); + } else { + await server.sendChannelsClose({ channelId }); + } + } catch (error) { + if (open && this.channels.get(channelKey) === entry && + this.serverRegistry.getServer(serverId) === server) entry.open = false; + throw error; + } + // Never apply a receipt to a removed entry or a replacement connection. + // Reconcile their current state before releasing this channel's queue. + if (this.channels.get(channelKey) !== entry || + this.serverRegistry.getServer(serverId) !== server) continue; + entry.open = open; + const now = this.getDesiredState(serverId, channelId); + const targetChanged = entry.descriptor.type !== descriptor.type || + !isDeepStrictEqual(entry.descriptor.address, descriptor.address); + if (targetChanged || open !== (now === 'open' || now === 'tuned-out')) continue; + return { open, result }; + } + }); + const tail = operation.then(() => {}, () => {}); + this.channelLifecycleTails.set(lifecycleKey, tail); + void tail.then(() => { + if (this.channelLifecycleTails.get(lifecycleKey) === tail) { + this.channelLifecycleTails.delete(lifecycleKey); + } + }); + return operation; + } + private async reconcileChannels( serverId: string, channels: ChannelDescriptor[], @@ -2090,46 +2162,26 @@ export class ChannelRegistry { // first boot on a new policy doesn't produce one notice per channel. const policyOpened: Array<{ channelId: string; label?: string }> = []; + // Record all admissions before waiting on any channel. A slow first RPC + // must not delay intent for the rest of this registration batch. for (const channel of channels) { - if (this.ensureInitialDesiredState(serverId, channel)) { - policyOpened.push({ channelId: channel.id, label: channel.label }); + const entry = this.channels.get(`${serverId}:${channel.id}`); + if (entry && this.ensureInitialDesiredState(serverId, entry.descriptor)) { + policyOpened.push({ channelId: channel.id, label: entry.descriptor.label }); } - const key = `${serverId}:${channel.id}`; - const desired = this.getDesiredState(serverId, channel.id); - const entry = this.channels.get(key); - // Tuned-out sits on the OPEN side of reconcile: traffic must keep - // arriving (the subconscious reads it); only main's wake/visibility - // is diverted, downstream at ingestion. - if (desired !== 'open' && desired !== 'tuned-out') { - try { - await server.sendChannelsClose({ channelId: channel.id }); - if (entry) entry.open = false; - } catch (err) { - this.emitTraceFn({ - type: 'mcpl:channel-reconcile-failed', - serverId, - channelId: channel.id, - desired, - error: (err as Error).message, - }); - } - continue; - } - + } + for (const channel of channels) { + if (!this.channels.has(`${serverId}:${channel.id}`)) continue; try { - await server.sendChannelsOpen({ - channelId: channel.id, - type: channel.type, - address: channel.address, - }); - if (entry) entry.open = true; + await this.applyDesiredChannelState(serverId, channel.id); } catch (err) { - if (entry) entry.open = false; + // Removal while queued is cancellation, not a failed subscription. + if (!this.channels.has(`${serverId}:${channel.id}`)) continue; this.emitTraceFn({ type: 'mcpl:channel-reconcile-failed', serverId, channelId: channel.id, - desired, + desired: this.getDesiredState(serverId, channel.id), error: (err as Error).message, }); } @@ -2284,23 +2336,11 @@ export class ChannelRegistry { history?: ChannelHistoryRequest, ): Promise { this.setDesiredState(entry.serverId, entry.descriptor.id, 'open', source); - const server = this.serverRegistry.getServer(entry.serverId); - if (!server) { - throw new Error(`Server not found: ${entry.serverId}`); + const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, history); + if (!applied.open || this.getDesiredState(entry.serverId, entry.descriptor.id) !== 'open') { + throw new Error('Channel open was superseded by a newer lifecycle decision'); } - if (!CapabilityGrant.of(server).has('channels.lifecycle')) { - // Desired state is already recorded — intent sticks; reconciliation - // will retry if the grant later widens (§6.7 expansion-on-receipt). - throw new Error(`channels.lifecycle not in "${entry.serverId}"'s effective grant (§14.1)`); - } - const result: ChannelsOpenResult = await server.sendChannelsOpen({ - channelId: entry.descriptor.id, - type: entry.descriptor.type, - address: entry.descriptor.address, - ...(history ? { history } : {}), - }); - entry.open = true; - return result; + return applied.result!; } /** @@ -2475,7 +2515,7 @@ export class ChannelRegistry { if (matches.length > 1) return { status: 'ambiguous' }; const entry = matches[0]!; const resolved = { channelId: entry.descriptor.id, label: entry.descriptor.label }; - if (entry.open && this.getDesiredState(entry.serverId, entry.descriptor.id) === 'open') { + if (entry.open && this.getDesiredState(entry.serverId, entry.descriptor.id) === 'open' && !this.hasPendingLifecycle(entry)) { return { status: 'already-open', ...resolved }; } try { @@ -2530,7 +2570,7 @@ export class ChannelRegistry { } const alreadyDesiredOpen = this.getDesiredState(entry.serverId, input.channelId) === 'open'; - if (entry.open && alreadyDesiredOpen && !input.backscroll) { + if (entry.open && alreadyDesiredOpen && !input.backscroll && !this.hasPendingLifecycle(entry)) { return { success: true, data: { channelId: input.channelId, status: 'already open' }, @@ -2593,7 +2633,7 @@ export class ChannelRegistry { } const alreadyDesiredClosed = this.getDesiredState(entry.serverId, input.channelId) === 'closed'; - if (!entry.open && alreadyDesiredClosed) { + if (!entry.open && alreadyDesiredClosed && !this.hasPendingLifecycle(entry)) { return { success: true, data: { channelId: input.channelId, status: 'already closed' }, @@ -2662,8 +2702,10 @@ export class ChannelRegistry { isError: true, }; } - await server.sendChannelsClose({ channelId: input.channelId }); - entry.open = false; + const applied = await this.applyDesiredChannelState(entry.serverId, input.channelId); + if (applied.open || this.getDesiredState(entry.serverId, input.channelId) !== 'closed') { + throw new Error('Channel close was superseded by a newer lifecycle decision'); + } return { success: true, data: { channelId: input.channelId, status: 'closed' }, @@ -2955,7 +2997,7 @@ export class ChannelRegistry { // so typing indicators, reaction machinery, and inbound forwarding come // alive with the reply. If the open fails, the speech does NOT go out: // a half-alive delivery is worse than a loud marker. - if (!entry.open) { + if (!entry.open || this.hasPendingLifecycle(entry)) { try { await this.openChannelNow(entry, 'opened-by-delivery'); console.error( diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts new file mode 100644 index 000000000..ea084bd62 --- /dev/null +++ b/test/channel-lifecycle-order.test.ts @@ -0,0 +1,194 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; +import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; +import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; +import type { ChannelDescriptor, ChannelsOpenResult } from '../src/mcpl/types.js'; + +const tick = () => new Promise((resolve) => setImmediate(resolve)); +function descriptor(id = 'x', initiallyOpen = false, address = 'original'): ChannelDescriptor { + return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: address } }; +} + +function fixture() { + const actual = new Map(); + const calls: Array<{ kind: 'open' | 'close'; id: string; address?: unknown }> = []; + const pending: Array<{ finish: () => void; fail: () => void }> = []; + const held = new Set(); + const serve = (kind: 'open' | 'close', params: { channelId?: string; address?: unknown }) => { + const id = params.channelId!; + calls.push({ kind, id, address: params.address }); + return new Promise((resolve, reject) => { + const finish = () => { + actual.set(id, kind === 'open'); + resolve(kind === 'open' ? { channel: descriptor(id) } : { closed: true }); + }; + if (held.has(id)) pending.push({ finish, fail: () => reject(new Error('fixture lifecycle failure')) }); + else finish(); + }); + }; + const server = { + grant: new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), []), + sendChannelsOpen: (params: { channelId?: string; address?: unknown }) => serve('open', params) as Promise, + sendChannelsClose: (params: { channelId: string }) => serve('close', params), + }; + const traces: Array<{ type: string; [key: string]: unknown }> = []; + const registry = new ChannelRegistry( + { getServer: () => server } as unknown as McplServerRegistry, + {} as FeatureSetManager, () => {}, (event) => traces.push(event), + ); + const isOpen = (id = 'x') => registry.getOpenChannels().some((e) => e.descriptor.id === id); + const tool = (kind: 'open' | 'close') => registry.handleChannelToolCall('channel_' + kind, { serverId: 'test', channelId: 'x' }); + // An out-of-order server serves the most recently received operation first. + // The fixed host should never have two in flight for this channel. + const drain = async (...work: Promise[]) => { + let settled = false; + const result = Promise.all(work).finally(() => { settled = true; }); + for (let i = 0; i < 100 && !settled; i++) { + await tick(); + pending.pop()?.finish(); + } + assert.equal(settled, true, 'lifecycle operations must settle'); + await result; + }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain }; +} + +test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { + const f = fixture(); + f.held.add('x'); + const first = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + assert.equal(f.pending.length, 1); + let acknowledged = false; + const replacement = f.registry.handleChanged('test', { + removed: ['x'], added: [descriptor('x', true, 'replacement')], + }, { respond: () => { acknowledged = true; } }); + assert.equal(acknowledged, true, 'registration ACK must not wait for the lifecycle queue'); + await f.drain(first, replacement); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + assert.equal(f.isOpen(), true); + assert.equal(f.actual.get('x'), true, 'older close must not be the last server-side operation'); + assert.deepEqual(f.calls.filter((c) => c.kind === 'open').at(-1)?.address, { value: 'replacement' }); +}); + +test('explicit open racing a default-close reconcile uses the latest intent', async () => { + const f = fixture(); + f.held.add('x'); + const reconcile = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + const opened = f.tool('open'); + await f.drain(reconcile, opened); + assert.equal((await opened).success, true); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + assert.equal(f.isOpen(), true); + assert.equal(f.actual.get('x'), true); +}); + +for (const firstKind of ['open', 'close'] as const) { + test('overlapping explicit ' + firstKind + ' then opposite intent leaves the newest decision in force', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', firstKind === 'close')] }); + f.held.add('x'); + const first = f.tool(firstKind); + await tick(); + const lastKind = firstKind === 'open' ? 'close' : 'open'; + const last = f.tool(lastKind); + await f.drain(first, last); + assert.equal((await last).success, true); + const desired = lastKind === 'open'; + assert.equal(f.registry.getDesiredState('test', 'x'), lastKind === 'open' ? 'open' : 'closed'); + assert.equal(f.isOpen(), desired); + assert.equal(f.actual.get('x'), desired); + }); +} + +test('an already-closed shortcut waits for pending transport work', async () => { + const f = fixture(); + f.actual.set('x', true); + f.held.add('x'); + const reconcile = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + let closeSettled = false; + const close = f.tool('close').then((result) => { closeSettled = true; return result; }); + await tick(); + assert.equal(closeSettled, false, 'entry.open is provisional while a lifecycle RPC is pending'); + await f.drain(reconcile, close); + assert.equal((await close).success, true); + assert.equal(f.actual.get('x'), false); +}); + +test('removal cancels queued work rather than recreating or mutating an unregistered channel', async () => { + const f = fixture(); + f.held.add('x'); + const reconcile = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + const open = f.tool('open'); + await f.registry.handleChanged('test', { removed: ['x'] }); + await f.drain(reconcile, open); + assert.equal((await open).success, false); + assert.equal(f.registry.listChannelsRaw().length, 0); + assert.equal(f.calls.filter((c) => c.kind === 'open').length, 0); +}); + +test('a failed operation releases the queue for a later decision', async () => { + const f = fixture(); + f.held.add('x'); + const reconcile = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + const open = f.tool('open'); + f.pending.shift()!.fail(); + await f.drain(reconcile, open); + assert.equal((await open).success, true); + assert.equal(f.actual.get('x'), true); + assert.equal(f.isOpen(), true); + assert.ok(f.traces.some((e) => e.type === 'mcpl:channel-reconcile-failed')); +}); + +test('a blocked channel does not delay another channel lifecycle or its ACK', async () => { + const f = fixture(); + f.held.add('x'); + const blocked = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + let acknowledged = false; + await f.registry.handleChanged('test', { added: [descriptor('y', true)] }, { + respond: () => { acknowledged = true; }, + }); + assert.equal(acknowledged, true); + assert.equal(f.actual.get('y'), true); + assert.equal(f.pending.length, 1); + await f.drain(blocked); +}); + +test('descriptor updates during an open refresh its target, but labels alone do not reopen', async () => { + for (const changesAddress of [false, true]) { + const f = fixture(); + f.held.add('x'); + const opening = f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + await tick(); + await f.registry.handleChanged('test', { + updated: [{ ...descriptor('x', true, changesAddress ? 'new-target' : 'original'), label: 'new label' }], + }); + await f.drain(opening); + assert.equal(f.calls.length, changesAddress ? 2 : 1); + assert.deepEqual(f.calls.at(-1)?.address, { value: changesAddress ? 'new-target' : 'original' }); + assert.equal(f.isOpen(), true); + } +}); + +test('a newer tune-out decision keeps transport open when an older close completes', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const close = f.tool('close'); + await tick(); + f.registry.enterTuneOut('test', 'x', { + epochId: 'new-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + await f.drain(close); + assert.equal(f.registry.getDesiredState('test', 'x'), 'tuned-out'); + assert.equal(f.actual.get('x'), true); + assert.equal(f.isOpen(), true); + assert.equal((await close).success, false, 'superseded close must not report current state as closed'); +}); From cd2638d882c85c50fac2e683e874ebfe4a9c0772 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 01:42:07 -0400 Subject: [PATCH 02/26] fix(mcpl): preserve tune-out while settling delivery transport Felix independently reproduced a regression in the new routeSpeech pending-operation path: waiting for transport called openChannelNow, whose desired-open write erased a newer tune-out epoch. The original focused and full suites did not cover that interaction. Automatic reply and delivery opens now preserve an existing tuned-out decision, and accept a tune-out established while their RPC is pending. They need open transport, not normal resident attention. Explicit channel_open retains the requirement to cancel tune-out first. Narrow the source type to the three real callers so this distinction stays visible. Added three regressions from the review: pending speech, pending reply preparation, and a new tune-out during automatic open. All three fail at 5fba953. The final 13-case file gives 3 pass / 10 fail on original main and 13 pass / 0 fail here. Expanded compiled focused tests pass 69/69 without force-exit. The full suite on 5fba953 was 1111/0; the revised full suite is running and will be reported independently. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 2 +- src/mcpl/channel-registry.ts | 12 +++++-- test/channel-lifecycle-order.test.ts | 47 +++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba3a8f7dd..b02aec4b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ Releases up to and including 0.7.3 predate this file; for their contents see ### Fixed -- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. +- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. ## 0.19.0 — 2026-09-28 diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 098b036d8..d7b8be6a5 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -2329,15 +2329,21 @@ export class ChannelRegistry { * events, desired state, and live state can never diverge by path. * Desired state is recorded BEFORE the server round-trip: intent sticks * even if the subscribe fails, and reconciliation retries later. + * Automatic delivery needs open transport, not a change of attention: + * preserve tune-out epochs both before and during the round-trip. */ private async openChannelNow( entry: ChannelEntry, - source: string, + source: 'agent-tool' | 'opened-by-reply' | 'opened-by-delivery', history?: ChannelHistoryRequest, ): Promise { - this.setDesiredState(entry.serverId, entry.descriptor.id, 'open', source); + const transportOnly = source !== 'agent-tool'; + if (!transportOnly || this.getDesiredState(entry.serverId, entry.descriptor.id) !== 'tuned-out') { + this.setDesiredState(entry.serverId, entry.descriptor.id, 'open', source); + } const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, history); - if (!applied.open || this.getDesiredState(entry.serverId, entry.descriptor.id) !== 'open') { + const desired = this.getDesiredState(entry.serverId, entry.descriptor.id); + if (!applied.open || (desired !== 'open' && !(transportOnly && desired === 'tuned-out'))) { throw new Error('Channel open was superseded by a newer lifecycle decision'); } return applied.result!; diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index ea084bd62..3d63f6116 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -16,6 +16,7 @@ function fixture() { const calls: Array<{ kind: 'open' | 'close'; id: string; address?: unknown }> = []; const pending: Array<{ finish: () => void; fail: () => void }> = []; const held = new Set(); + const publishedWhileOpen: boolean[] = []; const serve = (kind: 'open' | 'close', params: { channelId?: string; address?: unknown }) => { const id = params.channelId!; calls.push({ kind, id, address: params.address }); @@ -32,6 +33,10 @@ function fixture() { grant: new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), []), sendChannelsOpen: (params: { channelId?: string; address?: unknown }) => serve('open', params) as Promise, sendChannelsClose: (params: { channelId: string }) => serve('close', params), + sendChannelsPublish: async (params: { channelId: string }) => { + publishedWhileOpen.push(actual.get(params.channelId) === true); + return { delivered: true }; + }, }; const traces: Array<{ type: string; [key: string]: unknown }> = []; const registry = new ChannelRegistry( @@ -52,7 +57,7 @@ function fixture() { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -177,6 +182,46 @@ test('descriptor updates during an open refresh its target, but labels alone do } }); +for (const delivery of ['speech', 'reply'] as const) { + test(delivery + ' during pending transport work preserves the newer tune-out epoch', async () => { + // Felix's review reproduction: merely waiting for transport must not + // turn a delivery into a decision to cancel attention diversion. + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const close = f.tool('close'); + await tick(); + f.registry.enterTuneOut('test', 'x', { + epochId: 'new-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + const sent = delivery === 'speech' + ? f.registry.routeSpeech('resident', 'hello', 'x').then((result) => result?.delivered === true) + : f.registry.openIfClosedForSend('x', 'test').then((result) => result.status === 'opened'); + await f.drain(close, sent); + assert.equal(await sent, true); + assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'new-epoch'); + assert.equal(f.actual.get('x'), true); + if (delivery === 'speech') assert.deepEqual(f.publishedWhileOpen, [true]); + assert.equal((await f.tool('open')).success, false, 'explicit channel_open still requires tune-out cancellation'); + assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'new-epoch'); + }); +} + +test('automatic opening accepts a newer tune-out established during its RPC without clearing it', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.held.add('x'); + const speech = f.registry.routeSpeech('resident', 'hello', 'x'); + await tick(); + f.registry.enterTuneOut('test', 'x', { + epochId: 'mid-open-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + await f.drain(speech); + assert.equal((await speech)?.delivered, true); + assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'mid-open-epoch'); + assert.deepEqual(f.publishedWhileOpen, [true]); +}); + test('a newer tune-out decision keeps transport open when an older close completes', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From d64c8034c35e27b09b59e28a8107ea61a15eca58 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 02:42:59 -0400 Subject: [PATCH 03/26] fix(mcpl): distinguish delivery joins from opening intent Address the five source-confirmed review findings on PR #210. The prior pending-work check turned a delivery wait into a new open decision, overriding an explicit close and repeating opens/notices for backscroll. Its open-error catch also erased known-open state, and callers could describe an obsolete registration. Delivery on an already-open channel now joins its per-channel lifecycle tail without changing desired intent. If the join settles closed or loses registration, delivery fails visibly. Fresh delivery into a settled closed channel retains the existing engagement behavior. Concurrent delivery-forced opens skip redundant RPCs and report a transition only once. Re-read the entry after waiting so notices and reply preparation report the current label. Failed redundant open requests preserve known-live state rather than inventing a close. Policy notices now report durable admission intent before transport work rather than delayed Now open confirmations. That preserves once-per-admission/reboot semantics and informs the resident even when the first RPC fails; later reconciliation needs no second ledger or notice. Explicitly document the callback timing/meaning change for direct ChannelRegistry consumers. Built-in text distinguishes intent from confirmed delivery opens and accurately names the fresh-send engagement contract. Wren independently corroborated pending-close and stale-admission behaviors and helped clarify the intent/transport boundary. The final 23-case regression file yields 15 pass / 8 fail on published cd2638d and 23/23 here. It includes current replacement labels, failed backscroll, close during a join, duplicate delivery joins, fresh settled-close engagement, and real resident notice text through first-open failure and registry reconstruction. Expanded compiled focused tests pass 79/79 without force-exit. The final full compiled suite is running before push; independent review will cover this committed head. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 6 + src/framework.ts | 17 ++- src/mcpl/channel-registry.ts | 194 +++++++++++++++++---------- test/channel-lifecycle-order.test.ts | 190 +++++++++++++++++++++++++- 4 files changed, 322 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b02aec4b1..9dbbe6281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,14 @@ Releases up to and including 0.7.3 predate this file; for their contents see ## Unreleased +### Breaking (direct ChannelRegistry consumers) + +- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` still reports a confirmed transition. The built-in host notice now distinguishes selected-for-opening from currently open. + ### Fixed +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait, and reports only actual delivery-forced opens using the current registration label. + - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. ## 0.19.0 — 2026-09-28 diff --git a/src/framework.ts b/src/framework.ts index 510b9183a..92b652791 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -8324,15 +8324,14 @@ export class AgentFramework { .map((c) => (c.label && c.label !== c.channelId ? `${c.label} (${c.channelId})` : c.channelId)) .join(', '); const plural = channels.length > 1; - const causeText = - cause === 'subscription-policy' - ? 'your subscription policy just admitted ' + (plural ? 'them' : 'it') - : 'your own message into the closed channel engaged it'; - const text = - `[channels] Now open: ${shown} — ${causeText}. Ongoing traffic from ` + - (plural ? 'these channels' : 'this channel') + - ` will reach you. If you don't want ${plural ? 'one of them' : 'it'}, use channel_close — ` + - 'your choice sticks; neither policy nor a later send will silently reopen it.'; + const text = (cause === 'subscription-policy' + ? `[channels] Selected for opening by subscription policy: ${shown}. ` + + 'This records the admission decision; channel_list shows current transport state. ' + + 'When open, ' + (plural ? 'their' : 'its') + ' ongoing traffic will reach you. ' + : `[channels] Now open: ${shown} — your own message into the closed channel engaged it. ` + + 'Ongoing traffic from ' + (plural ? 'these channels' : 'this channel') + ' will reach you. ') + + `If you don't want ${plural ? 'one of them' : 'it'}, use channel_close; your choice outranks policy. ` + + 'A fresh message into a closed channel engages it again.'; try { // Through framework.addMessage for mid-turn safety (deferral + // hear-while-acting injection) — a delivery-forced open happens inside diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index d7b8be6a5..7ef70c9cb 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -485,13 +485,12 @@ interface ChannelRegistryOptions { textLen: number; }) => void; /** - * Called when channels were opened WITHOUT the agent asking (subscription - * policy admitting a newly discovered channel, or delivery into a closed - * locus). The host wires this to drop a durable notice into the agent's - * window — the agent must always learn that new traffic will start - * flowing, and that `channel_close` opts out (their decision outranks - * policy). Fires ONCE per channel ever: the desired-state decision is - * durable, so reboots do not re-announce. Must not trigger inference. + * Announces either a durable subscription-policy admission (before its + * transport RPC; not an open confirmation), or a confirmed delivery-forced + * closed-to-open transition. The host records a durable, non-waking notice. + * Policy admissions fire once per channel: the persisted desired-state + * decision prevents re-announcement on retry or reboot. channel_close + * supersedes policy; a fresh delivery into a closed locus engages it again. */ onChannelAutoOpened?: (info: { /** Agent whose action caused the open (delivery); absent for policy opens. */ @@ -2083,12 +2082,14 @@ export class ChannelRegistry { private applyDesiredChannelState( serverId: string, channelId: string, - history?: ChannelHistoryRequest, - ): Promise<{ open: boolean; result?: ChannelsOpenResult }> { + options?: { history?: ChannelHistoryRequest; onlyIfClosed?: boolean }, + ): Promise<{ open: boolean; opened: boolean; result?: ChannelsOpenResult }> { const lifecycleKey = this.lifecycleKey(serverId, channelId); const channelKey = `${serverId}:${channelId}`; const previous = this.channelLifecycleTails.get(lifecycleKey) ?? Promise.resolve(); const operation = previous.then(async () => { + let issued = false; + let opened = false; for (;;) { const entry = this.channels.get(channelKey); if (!entry) throw new Error(`Channel is no longer registered: ${channelId}`); @@ -2100,31 +2101,33 @@ export class ChannelRegistry { const descriptor = entry.descriptor; const desired = this.getDesiredState(serverId, channelId); const open = desired === 'open' || desired === 'tuned-out'; + if (!issued && options?.onlyIfClosed && open && entry.open) { + return { open: true, opened: false }; + } + issued = true; let result: ChannelsOpenResult | undefined; - try { - if (open) { - result = await server.sendChannelsOpen({ - channelId, type: descriptor.type, address: descriptor.address, - ...(history ? { history } : {}), - }); - } else { - await server.sendChannelsClose({ channelId }); - } - } catch (error) { - if (open && this.channels.get(channelKey) === entry && - this.serverRegistry.getServer(serverId) === server) entry.open = false; - throw error; + if (open) { + result = await server.sendChannelsOpen({ + channelId, type: descriptor.type, address: descriptor.address, + ...(options?.history ? { history: options.history } : {}), + }); + } else { + await server.sendChannelsClose({ channelId }); } + // Failure is not a confirmed transition. In particular, a failed + // backscroll request must not erase an already-live subscription. // Never apply a receipt to a removed entry or a replacement connection. // Reconcile their current state before releasing this channel's queue. if (this.channels.get(channelKey) !== entry || this.serverRegistry.getServer(serverId) !== server) continue; + if (open && !entry.open) opened = true; + if (!open) opened = false; entry.open = open; const now = this.getDesiredState(serverId, channelId); const targetChanged = entry.descriptor.type !== descriptor.type || !isDeepStrictEqual(entry.descriptor.address, descriptor.address); if (targetChanged || open !== (now === 'open' || now === 'tuned-out')) continue; - return { open, result }; + return { open, opened, result }; } }); const tail = operation.then(() => {}, () => {}); @@ -2137,6 +2140,26 @@ export class ChannelRegistry { return operation; } + /** + * Join work on an already-open channel without recording new intent. + * A close that settles during the wait remains authoritative; waiting for + * it is not an instruction to open the channel again. + */ + private async waitForOpenChannel(entry: ChannelEntry): Promise { + const lifecycleKey = this.lifecycleKey(entry.serverId, entry.descriptor.id); + for (;;) { + const pending = this.channelLifecycleTails.get(lifecycleKey); + if (!pending) break; + await pending; + } + const current = this.channels.get(`${entry.serverId}:${entry.descriptor.id}`); + if (!current) throw new Error('Channel registration was removed while delivery waited'); + if (!current.open || this.getDesiredState(current.serverId, current.descriptor.id) === 'closed') { + throw new Error('Channel was closed or selected for closure while delivery waited'); + } + return current; + } + private async reconcileChannels( serverId: string, channels: ChannelDescriptor[], @@ -2170,6 +2193,28 @@ export class ChannelRegistry { policyOpened.push({ channelId: channel.id, label: entry.descriptor.label }); } } + + // Announce the durable admission decision before awaiting transport. + // This is intent, not confirmation that an open RPC succeeded. It stays + // truthful if the first attempt fails, and a retry needs no new admission. + // channel_close supersedes policy; use current entries at this boundary. + const currentAdmissions = policyOpened.flatMap(({ channelId }) => { + const entry = this.channels.get(`${serverId}:${channelId}`); + if (!entry || this.getDesiredState(serverId, channelId) !== 'open') return []; + return [{ channelId, label: entry.descriptor.label }]; + }); + if (currentAdmissions.length > 0) { + try { + this.onChannelAutoOpened?.({ + serverId, + source: 'subscription-policy', + channels: currentAdmissions, + }); + } catch (err) { + console.error('onChannelAutoOpened (policy) failed:', err); + } + } + for (const channel of channels) { if (!this.channels.has(`${serverId}:${channel.id}`)) continue; try { @@ -2186,21 +2231,6 @@ export class ChannelRegistry { }); } } - - // Announce policy admissions to the agent — nothing may start flowing - // traffic into their window without them being told, and told how to - // opt out (channel_close; their decision outranks policy). - if (policyOpened.length > 0) { - try { - this.onChannelAutoOpened?.({ - serverId, - source: 'subscription-policy', - channels: policyOpened, - }); - } catch (err) { - console.error('onChannelAutoOpened (policy) failed:', err); - } - } } // ========================================================================== @@ -2336,17 +2366,17 @@ export class ChannelRegistry { entry: ChannelEntry, source: 'agent-tool' | 'opened-by-reply' | 'opened-by-delivery', history?: ChannelHistoryRequest, - ): Promise { + ): Promise<{ result: ChannelsOpenResult; opened: boolean }> { const transportOnly = source !== 'agent-tool'; if (!transportOnly || this.getDesiredState(entry.serverId, entry.descriptor.id) !== 'tuned-out') { this.setDesiredState(entry.serverId, entry.descriptor.id, 'open', source); } - const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, history); + const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, { history, onlyIfClosed: transportOnly }); const desired = this.getDesiredState(entry.serverId, entry.descriptor.id); if (!applied.open || (desired !== 'open' && !(transportOnly && desired === 'tuned-out'))) { throw new Error('Channel open was superseded by a newer lifecycle decision'); } - return applied.result!; + return { result: applied.result!, opened: applied.opened }; } /** @@ -2519,19 +2549,25 @@ export class ChannelRegistry { } if (matches.length === 0) return { status: 'unknown-channel' }; if (matches.length > 1) return { status: 'ambiguous' }; - const entry = matches[0]!; - const resolved = { channelId: entry.descriptor.id, label: entry.descriptor.label }; - if (entry.open && this.getDesiredState(entry.serverId, entry.descriptor.id) === 'open' && !this.hasPendingLifecycle(entry)) { - return { status: 'already-open', ...resolved }; - } + let entry = matches[0]!; try { - await this.openChannelNow(entry, 'opened-by-reply'); - this.emitTraceFn({ - type: 'mcpl:channel-opened-by-send', - serverId: entry.serverId, - channelId: entry.descriptor.id, - }); - return { status: 'opened', ...resolved }; + if (entry.open && this.hasPendingLifecycle(entry)) { + entry = await this.waitForOpenChannel(entry); + } + const desired = this.getDesiredState(entry.serverId, entry.descriptor.id); + if (entry.open && (desired === 'open' || desired === 'tuned-out')) { + return { status: 'already-open', channelId: entry.descriptor.id, label: entry.descriptor.label }; + } + const { opened } = await this.openChannelNow(entry, 'opened-by-reply'); + entry = await this.waitForOpenChannel(entry); + if (opened) { + this.emitTraceFn({ + type: 'mcpl:channel-opened-by-send', + serverId: entry.serverId, + channelId: entry.descriptor.id, + }); + } + return { status: opened ? 'opened' : 'already-open', channelId: entry.descriptor.id, label: entry.descriptor.label }; } catch (err) { this.emitTraceFn({ type: 'mcpl:channel-open-failed', @@ -2539,7 +2575,9 @@ export class ChannelRegistry { channelId: entry.descriptor.id, error: (err as Error).message, }); - return { status: 'open-failed', ...resolved }; + const current = this.channels.get(`${entry.serverId}:${entry.descriptor.id}`); + return { status: 'open-failed', channelId: entry.descriptor.id, + ...(current ? { label: current.descriptor.label } : {}) }; } } @@ -2590,7 +2628,7 @@ export class ChannelRegistry { ...(input.beforeMessageId ? { beforeMessageId: input.beforeMessageId } : {}), } : undefined; - const result = await this.openChannelNow(entry, 'agent-tool', history); + const { result } = await this.openChannelNow(entry, 'agent-tool', history); return { success: true, data: { @@ -2992,10 +3030,17 @@ export class ChannelRegistry { return fail(null, 'turn has no locus (no home/trigger channel; nothing to deliver into)'); } - const entry = this.findChannelEntry(channelId); + let entry = this.findChannelEntry(channelId); if (!entry) { return fail(channelId, `no registered channel for locus "${channelId}"`); } + if (entry.open && this.hasPendingLifecycle(entry)) { + try { + entry = await this.waitForOpenChannel(entry); + } catch (err) { + return fail(channelId, `lifecycle work prevented delivery: ${(err as Error).message}`); + } + } // INVARIANT: it is not possible to send into a closed channel. Speech // routed into a closed-but-registered locus (the canonical case: a DM @@ -3003,26 +3048,29 @@ export class ChannelRegistry { // so typing indicators, reaction machinery, and inbound forwarding come // alive with the reply. If the open fails, the speech does NOT go out: // a half-alive delivery is worse than a loud marker. - if (!entry.open || this.hasPendingLifecycle(entry)) { + if (!entry.open) { try { - await this.openChannelNow(entry, 'opened-by-delivery'); - console.error( - `[routeSpeech] ${conversationId}: locus ${channelId} was closed — opened by delivery (speech implies engagement)`, - ); - this.emitTraceFn({ - type: 'mcpl:channel-opened-by-send', - serverId: entry.serverId, - channelId, - }); - try { - this.onChannelAutoOpened?.({ - conversationId, + const { opened } = await this.openChannelNow(entry, 'opened-by-delivery'); + entry = await this.waitForOpenChannel(entry); + if (opened) { + console.error( + `[routeSpeech] ${conversationId}: locus ${channelId} was closed — opened by delivery (speech implies engagement)`, + ); + this.emitTraceFn({ + type: 'mcpl:channel-opened-by-send', serverId: entry.serverId, - source: 'opened-by-delivery', - channels: [{ channelId, label: entry.descriptor.label }], + channelId, }); - } catch (err) { - console.error('onChannelAutoOpened (delivery) failed:', err); + try { + this.onChannelAutoOpened?.({ + conversationId, + serverId: entry.serverId, + source: 'opened-by-delivery', + channels: [{ channelId, label: entry.descriptor.label }], + }); + } catch (err) { + console.error('onChannelAutoOpened (delivery) failed:', err); + } } } catch (err) { return fail(channelId, `locus channel is closed and open failed: ${(err as Error).message}`); diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 3d63f6116..e7a3ff984 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -1,4 +1,10 @@ import { test } from 'node:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { JsStore } from '@animalabs/chronicle'; +import { AgentFramework } from '../src/index.js'; +import { MockMembrane } from './helpers/mock-membrane.js'; import assert from 'node:assert/strict'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; @@ -11,12 +17,15 @@ function descriptor(id = 'x', initiallyOpen = false, address = 'original'): Chan return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: address } }; } -function fixture() { +type NoticeHandler = NonNullable[4]>['onChannelAutoOpened']>; + +function fixture(onNotice?: NoticeHandler, store?: JsStore) { const actual = new Map(); const calls: Array<{ kind: 'open' | 'close'; id: string; address?: unknown }> = []; const pending: Array<{ finish: () => void; fail: () => void }> = []; const held = new Set(); const publishedWhileOpen: boolean[] = []; + const notices: Array<{ source: string; channels: Array<{ channelId: string; label?: string }> }> = []; const serve = (kind: 'open' | 'close', params: { channelId?: string; address?: unknown }) => { const id = params.channelId!; calls.push({ kind, id, address: params.address }); @@ -42,6 +51,7 @@ function fixture() { const registry = new ChannelRegistry( { getServer: () => server } as unknown as McplServerRegistry, {} as FeatureSetManager, () => {}, (event) => traces.push(event), + { store, onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); } }, ); const isOpen = (id = 'x') => registry.getOpenChannels().some((e) => e.descriptor.id === id); const tool = (kind: 'open' | 'close') => registry.handleChannelToolCall('channel_' + kind, { serverId: 'test', channelId: 'x' }); @@ -57,7 +67,7 @@ function fixture() { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -196,7 +206,7 @@ for (const delivery of ['speech', 'reply'] as const) { }, 'agent-tool'); const sent = delivery === 'speech' ? f.registry.routeSpeech('resident', 'hello', 'x').then((result) => result?.delivered === true) - : f.registry.openIfClosedForSend('x', 'test').then((result) => result.status === 'opened'); + : f.registry.openIfClosedForSend('x', 'test').then((result) => result.status === 'opened' || result.status === 'already-open'); await f.drain(close, sent); assert.equal(await sent, true); assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'new-epoch'); @@ -222,6 +232,180 @@ test('automatic opening accepts a newer tune-out established during its RPC with assert.deepEqual(f.publishedWhileOpen, [true]); }); +test('a failed backscroll open preserves an already-live subscription', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + await tick(); + f.pending.shift()!.fail(); + assert.equal((await backscroll).success, false); + assert.equal(f.isOpen(), true, 'a failed redundant open is not a confirmed close'); + const speech = f.registry.routeSpeech('resident', 'hello', 'x'); + await f.drain(speech); + assert.equal((await speech)?.delivered, true); + assert.equal(f.calls.length, 1, 'delivery must not need another open after a failed backscroll'); + assert.deepEqual(f.publishedWhileOpen, [true]); +}); + +test('speech does not replace a pending explicit close with a new open decision', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const close = f.tool('close'); + await tick(); + const speech = f.registry.routeSpeech('resident', 'tail of the turn', 'x'); + await f.drain(close, speech); + assert.equal((await close).success, true); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal(f.actual.get('x'), false); + assert.equal(f.isOpen(), false); + assert.equal(f.notices.length, 0); +}); + +test('waiting for a backscroll does not issue new opens or auto-open notices for each speech', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + await tick(); + const speech1 = f.registry.routeSpeech('resident', 'one', 'x'); + const speech2 = f.registry.routeSpeech('resident', 'two', 'x'); + await f.drain(backscroll, speech1, speech2); + assert.equal((await speech1)?.delivered, true); + assert.equal((await speech2)?.delivered, true); + assert.equal(f.calls.length, 1, 'only the explicit backscroll needs an open RPC'); + assert.equal(f.notices.length, 0, 'waiting is not a closed-to-open transition'); +}); + +for (const delivery of ['speech', 'reply'] as const) { + test(delivery + ' opening reports the replacement registration label', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.held.add('x'); + const opening = delivery === 'speech' + ? f.registry.routeSpeech('resident', 'hello', 'x') + : f.registry.openIfClosedForSend('x', 'test'); + await tick(); + const replacement = f.registry.handleChanged('test', { + removed: ['x'], added: [{ ...descriptor('x', true, 'replacement'), label: 'Replacement label' }], + }); + await f.drain(opening, replacement); + if (delivery === 'speech') { + assert.equal(f.notices.length, 1); + assert.equal(f.notices[0].channels[0].label, 'Replacement label'); + } else { + const result = await opening; + assert.ok(result && 'label' in result); + assert.equal(result.label, 'Replacement label'); + } + }); +} + +test('policy announces intent before transport rather than a stale open after supersession', async () => { + const f = fixture(); + f.registry.setSubscriptionPolicy('test', 'auto'); + f.held.add('x'); + const batch = f.registry.handleChanged('test', { added: [descriptor(), descriptor('y')] }); + assert.equal(f.notices.length, 1, 'the decision is announced before transport can stall'); + assert.deepEqual(f.notices[0].channels, [{ channelId: 'x', label: 'x' }, { channelId: 'y', label: 'y' }]); + await tick(); + const closeY = await f.registry.handleChannelToolCall('channel_close', { serverId: 'test', channelId: 'y' }); + assert.equal(closeY.success, true); + await f.drain(batch); + assert.equal(f.registry.getDesiredState('test', 'y'), 'closed'); + assert.equal(f.notices.length, 1, 'no stale confirmation is emitted after the later close'); +}); + +test('a fresh delivery into a settled closed channel remains a new engagement', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + assert.equal((await f.tool('close')).success, true); + assert.equal(f.actual.get('x'), false); + const delivered = await f.registry.routeSpeech('resident', 'a new engagement', 'x'); + assert.equal(delivered?.delivered, true); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + assert.equal(f.notices.length, 1); +}); + +test('a close that completes while delivery joins a backscroll remains authoritative', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + await tick(); + const speech = f.registry.routeSpeech('resident', 'tail', 'x'); + const close = f.tool('close'); + await f.drain(backscroll, speech, close); + assert.equal((await close).success, true); + assert.equal(await speech, null); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.deepEqual(f.publishedWhileOpen, []); + assert.equal(f.notices.length, 0); +}); + +test('concurrent delivery into a closed channel shares one opening and one notice', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.calls.length = 0; + f.held.add('x'); + const first = f.registry.routeSpeech('resident', 'one', 'x'); + const second = f.registry.routeSpeech('resident', 'two', 'x'); + await f.drain(first, second); + assert.equal((await first)?.delivered, true); + assert.equal((await second)?.delivered, true); + assert.equal(f.calls.length, 1); + assert.equal(f.notices.length, 1); + assert.deepEqual(f.publishedWhileOpen, [true, true]); +}); + +test('failed policy opening records intent once, with truthful resident text through a retry', async () => { + const dir = mkdtempSync(join(tmpdir(), 'policy-intent-notice-')); + const framework = await AgentFramework.create({ + storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'resident', model: 'test-model', systemPrompt: 'Test.' }], + modules: [], syncIntervalMs: 0, + }); + try { + const recordNotice = (framework as unknown as { + recordChannelAutoOpenNotice(agent: undefined, channels: Array<{ channelId: string; label?: string }>, cause: 'subscription-policy' | 'opened-by-delivery'): void; + }).recordChannelAutoOpenNotice.bind(framework); + const onNotice: NoticeHandler = (info) => recordNotice(undefined, info.channels, info.source); + const f = fixture(onNotice, framework.getStore()); + f.registry.setSubscriptionPolicy('test', 'auto'); + f.held.add('x'); + const first = f.registry.handleChanged('test', { added: [descriptor()] }); + assert.equal(f.notices.length, 1); + await tick(); + f.pending.shift()!.fail(); + await first; + assert.equal(f.isOpen(), false); + + const noticeTexts = () => framework.getAgent('resident')!.getContextManager().getAllMessages() + .flatMap((m) => m.content) + .filter((b) => b.type === 'text' && b.text.startsWith('[channels]')) + .map((b) => (b as { text: string }).text); + assert.equal(noticeTexts().length, 1); + assert.match(noticeTexts()[0], /Selected for opening by subscription policy/); + assert.match(noticeTexts()[0], /admission decision; channel_list shows current transport state/); + assert.doesNotMatch(noticeTexts()[0], /Now open/); + + // Reconstruct the registry from the same Chronicle state before retrying. + // The admission persists even though its first transport attempt failed. + const retry = fixture(onNotice, framework.getStore()); + retry.registry.setSubscriptionPolicy('test', 'auto'); + await retry.registry.handleChanged('test', { added: [descriptor()] }); + assert.equal(retry.isOpen(), true); + assert.equal(retry.notices.length, 0); + assert.equal(noticeTexts().length, 1); + } finally { + await framework.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + test('a newer tune-out decision keeps transport open when an older close completes', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From 9c94d0a33bc84e6a7fcbf22645584487967d0845 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 14:53:49 -0400 Subject: [PATCH 04/26] fix(mcpl): bound channel lifecycle convergence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Weft’s blocking review of PR #210: a server could change its descriptor after each successful open and keep the convergence loop plus joined delivery pending indefinitely. Per-RPC timeouts did not bound this sequence of completed requests. Limit one operation to five transport attempts. Exhaustion emits a diagnostic, invalidates the unconfirmed current-target open flag, and rejects the operation. Keep that rejection visible to existing delivery joiners while observing it for unhandled-rejection safety and allowing queued successors to proceed. A failed ordinary backscroll still preserves known-open state; a later stable operation gets a fresh convergence budget. New public-API regressions hold each RPC and change descriptors before release. They prove initiating-tool failure, route-failure notification, reply-preparation failure, no publication while unconverged, queue release and stable subsequent retry, unrelated-channel progress, and success on the fifth attempt. On published d64c803, the first two new cases fail to settle within the bounded probe while the finite control passes; the original 23 tests also remain green after the fix. Move only this PR’s two changelog categories to unique fixed/breaking fragments, explicitly authorized by Felix in Commons message20050, avoiding shared Unreleased conflicts. Preserve the compatibility notice verbatim and add the convergence bound. Current-main integration and independent/full validation follow before push. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 10 -- .../210-channel-lifecycle-order.fixed.md | 5 + .../210-policy-admission-notice.breaking.md | 1 + src/mcpl/channel-registry.ts | 50 ++++++++-- test/channel-lifecycle-order.test.ts | 95 ++++++++++++++++++- 5 files changed, 140 insertions(+), 21 deletions(-) create mode 100644 changelog.d/210-channel-lifecycle-order.fixed.md create mode 100644 changelog.d/210-policy-admission-notice.breaking.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dbbe6281..6b9a30b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,16 +12,6 @@ Releases up to and including 0.7.3 predate this file; for their contents see ## Unreleased -### Breaking (direct ChannelRegistry consumers) - -- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` still reports a confirmed transition. The built-in host notice now distinguishes selected-for-opening from currently open. - -### Fixed - -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait, and reports only actual delivery-forced opens using the current registration label. - -- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. - ## 0.19.0 — 2026-09-28 ### Added diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md new file mode 100644 index 000000000..707a1bda3 --- /dev/null +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -0,0 +1,5 @@ +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait, and reports only actual delivery-forced opens using the current registration label. + +- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. + +- Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. diff --git a/changelog.d/210-policy-admission-notice.breaking.md b/changelog.d/210-policy-admission-notice.breaking.md new file mode 100644 index 000000000..f47b54371 --- /dev/null +++ b/changelog.d/210-policy-admission-notice.breaking.md @@ -0,0 +1 @@ +- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` still reports a confirmed transition. The built-in host notice now distinguishes selected-for-opening from currently open. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 7ef70c9cb..c51552400 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -46,6 +46,10 @@ import { CapabilityGrant } from './capability-grant.js'; // ============================================================================ const TYPING_INTERVAL_MS = 7_000; +const MAX_CHANNEL_LIFECYCLE_ATTEMPTS = 5; + +/** A successful RPC can still fail to establish the latest channel target. */ +class ChannelLifecycleConvergenceError extends Error {} const CHANNEL_LIFECYCLE_LOG_ID = 'mcpl/channel-lifecycle'; /** @@ -2087,8 +2091,8 @@ export class ChannelRegistry { const lifecycleKey = this.lifecycleKey(serverId, channelId); const channelKey = `${serverId}:${channelId}`; const previous = this.channelLifecycleTails.get(lifecycleKey) ?? Promise.resolve(); - const operation = previous.then(async () => { - let issued = false; + const operation = previous.catch(() => {}).then(async () => { + let attempts = 0; let opened = false; for (;;) { const entry = this.channels.get(channelKey); @@ -2101,10 +2105,26 @@ export class ChannelRegistry { const descriptor = entry.descriptor; const desired = this.getDesiredState(serverId, channelId); const open = desired === 'open' || desired === 'tuned-out'; - if (!issued && options?.onlyIfClosed && open && entry.open) { + if (attempts === 0 && options?.onlyIfClosed && open && entry.open) { return { open: true, opened: false }; } - issued = true; + // Per-RPC timeouts do not bound a stream of promptly successful but + // continually superseded receipts. Bound this whole operation, then + // release its queue slot and surface the failure to delivery joiners. + if (attempts >= MAX_CHANNEL_LIFECYCLE_ATTEMPTS) { + // No receipt confirmed the current target/intent. A later delivery + // must reconcile it rather than reuse an earlier target's open flag. + entry.open = false; + const error = new ChannelLifecycleConvergenceError( + `Channel ${channelId} did not converge after ${attempts} lifecycle attempts; its target or desired state kept changing`, + ); + this.emitTraceFn({ + type: 'mcpl:channel-reconcile-failed', + serverId, channelId, desired, attempts, error: error.message, + }); + throw error; + } + attempts++; let result: ChannelsOpenResult | undefined; if (open) { result = await server.sendChannelsOpen({ @@ -2130,13 +2150,16 @@ export class ChannelRegistry { return { open, opened, result }; } }); - const tail = operation.then(() => {}, () => {}); + // Keep the rejection visible to existing joiners, while observing it + // immediately and letting each queued successor start after either result. + const tail = operation.then(() => {}); this.channelLifecycleTails.set(lifecycleKey, tail); - void tail.then(() => { + const cleanup = () => { if (this.channelLifecycleTails.get(lifecycleKey) === tail) { this.channelLifecycleTails.delete(lifecycleKey); } - }); + }; + void tail.then(cleanup, cleanup); return operation; } @@ -2150,7 +2173,14 @@ export class ChannelRegistry { for (;;) { const pending = this.channelLifecycleTails.get(lifecycleKey); if (!pending) break; - await pending; + try { + await pending; + } catch (error) { + // A redundant backscroll failure can leave a known-open channel + // usable. Exhausted convergence is different: it never established + // the current target, so joined delivery must fail visibly. + if (error instanceof ChannelLifecycleConvergenceError) throw error; + } } const current = this.channels.get(`${entry.serverId}:${entry.descriptor.id}`); if (!current) throw new Error('Channel registration was removed while delivery waited'); @@ -2221,7 +2251,9 @@ export class ChannelRegistry { await this.applyDesiredChannelState(serverId, channel.id); } catch (err) { // Removal while queued is cancellation, not a failed subscription. - if (!this.channels.has(`${serverId}:${channel.id}`)) continue; + // Exhausted convergence already emitted its diagnostic at the bound. + if (!this.channels.has(`${serverId}:${channel.id}`) || + err instanceof ChannelLifecycleConvergenceError) continue; this.emitTraceFn({ type: 'mcpl:channel-reconcile-failed', serverId, diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index e7a3ff984..33960de20 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -26,6 +26,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { const held = new Set(); const publishedWhileOpen: boolean[] = []; const notices: Array<{ source: string; channels: Array<{ channelId: string; label?: string }> }> = []; + const routeFailures: Array<{ reason: string }> = []; const serve = (kind: 'open' | 'close', params: { channelId?: string; address?: unknown }) => { const id = params.channelId!; calls.push({ kind, id, address: params.address }); @@ -51,7 +52,8 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { const registry = new ChannelRegistry( { getServer: () => server } as unknown as McplServerRegistry, {} as FeatureSetManager, () => {}, (event) => traces.push(event), - { store, onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); } }, + { store, onRouteFailure: (info) => routeFailures.push(info), + onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); } }, ); const isOpen = (id = 'x') => registry.getOpenChannels().some((e) => e.descriptor.id === id); const tool = (kind: 'open' | 'close') => registry.handleChannelToolCall('channel_' + kind, { serverId: 'test', channelId: 'x' }); @@ -67,7 +69,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -406,6 +408,95 @@ test('failed policy opening records intent once, with truthful resident text thr } }); +test('descriptor churn exhausts convergence and fails the tool plus joined deliveries instead of hanging', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + let toolSettled = false, speechSettled = false, replySettled = false; + const opening = f.registry.handleChannelToolCall('channel_open', { + serverId: 'test', channelId: 'x', backscroll: 5, + }).then((result) => { toolSettled = true; return result; }); + await tick(); + const speech = f.registry.routeSpeech('resident', 'waiting reply', 'x') + .then((result) => { speechSettled = true; return result; }); + const reply = f.registry.openIfClosedForSend('x', 'test') + .then((result) => { replySettled = true; return result; }); + try { + // Every RPC succeeds promptly, but its target is superseded before the + // receipt arrives. Bound the test itself without a hanging provider. + for (let i = 0; i < 12 && !toolSettled; i++) { + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'churn-' + i)] }); + f.pending.shift()?.finish(); + await tick(); + } + assert.equal(toolSettled, true, 'the whole convergence operation must be bounded'); + assert.equal(speechSettled, true, 'joined speech must receive a terminal result'); + assert.equal(replySettled, true, 'reply preparation must receive a terminal result'); + assert.equal(f.calls.length, 5, 'at most five transport attempts per operation'); + assert.equal((await opening).success, false); + assert.match((await opening).error!, /did not converge/); + assert.equal(await speech, null); + assert.equal((await reply).status, 'open-failed'); + assert.equal(f.isOpen(), false, 'an unconfirmed current target is not reported open'); + assert.deepEqual(f.publishedWhileOpen, []); + assert.equal(f.routeFailures.length, 1); + assert.match(f.routeFailures[0].reason, /did not converge/); + assert.ok(f.traces.some((e) => e.type === 'mcpl:channel-reconcile-failed' && + String(e.error).includes('did not converge'))); + } finally { + // Original code needs one stable receipt to release its unbounded loop. + await f.drain(opening, speech, reply); + } + f.held.delete('x'); + const retry = await f.registry.routeSpeech('resident', 'a later stable retry', 'x'); + assert.equal(retry?.delivered, true); + assert.equal(f.calls.length, 6, 'later delivery reconfirms the latest target before publishing'); + assert.equal((await f.tool('close')).success, true, 'exhaustion releases the queue'); + assert.equal((await f.tool('open')).success, true, 'a later stable decision gets a fresh retry budget'); +}); + +test('background reconciliation reports supersession exhaustion and still permits other channels', async () => { + const f = fixture(); + f.held.add('x'); + let settled = false; + const reconcile = f.registry.handleChanged('test', { added: [descriptor('x', true)] }) + .then(() => { settled = true; }); + await tick(); + try { + await f.registry.handleChanged('test', { added: [descriptor('y', true)] }); + assert.equal(f.actual.get('y'), true); + for (let i = 0; i < 12 && !settled; i++) { + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'churn-' + i)] }); + f.pending.shift()?.finish(); + await tick(); + } + assert.equal(settled, true); + assert.equal(f.calls.filter((call) => call.id === 'x').length, 5); + const failures = f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed'); + assert.equal(failures.length, 1); + assert.match(String(failures[0].error), /did not converge/); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open', 'failure does not erase desired intent'); + } finally { await f.drain(reconcile); } +}); + +test('finite descriptor supersession can converge on its fifth transport attempt', async () => { + const f = fixture(); + f.held.add('x'); + const reconcile = f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + await tick(); + for (let i = 0; i < 4; i++) { + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'target-' + i)] }); + f.pending.shift()!.finish(); + await tick(); + } + await f.drain(reconcile); + assert.equal(f.calls.length, 5); + assert.equal(f.isOpen(), true); + assert.deepEqual(f.calls.at(-1)?.address, { value: 'target-3' }); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0); +}); + test('a newer tune-out decision keeps transport open when an older close completes', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From 8af55988e441262ffc7f083e624dbd5110b2df70 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 14:57:56 -0400 Subject: [PATCH 05/26] docs(changelog): qualify lifecycle convergence at success The new retry bound deliberately returns a visible failure when the latest target cannot be confirmed. Qualify the earlier blanket state-convergence wording so it does not claim exhausted operations synchronize remote state. Source and tests remain identical to 541a339. Co-Authored-By: GPT-6 Astra --- changelog.d/210-channel-lifecycle-order.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 707a1bda3..7c4db23a5 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,5 +1,5 @@ - Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait, and reports only actual delivery-forced opens using the current registration label. -- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Concurrent registration, open, and close requests can no longer leave the server in an older state; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. +- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. From de1a9f36579ccef2e30339cd9677128dc8b5c087 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 15:02:58 -0400 Subject: [PATCH 06/26] fix(mcpl): validate open receipts against the current target Wren independently reproduced a receipt-validity hole after the convergence cap: successful A receipt marked the entry open even after its descriptor became B, then a failed corrective B open preserved that stale bit and allowed joined speech to publish. Invalidate known-open on type/address retargeting while preserving label-only updates. Check entry, connection, and transport target before applying a successful receipt; a stale receipt triggers reconciliation without transiently confirming the new target. On ordinary RPC failure, retain open state only when the captured connection and target still match. Joined delivery reports an unconfirmed target honestly rather than claiming a close happened. Three added regressions fail on 8af5598 (old-address, old-type, replaced-connection confirmation) and pass after this change. The same-target backscroll and finite/exhausted convergence controls remain green. Runtime source is now ready for independent re-review before serialized full validation and push. Co-Authored-By: GPT-6 Astra --- .../210-channel-lifecycle-order.fixed.md | 2 + src/mcpl/channel-registry.ts | 50 +++++++++++------ test/channel-lifecycle-order.test.ts | 53 ++++++++++++++++++- 3 files changed, 86 insertions(+), 19 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 7c4db23a5..94e63d742 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -2,4 +2,6 @@ - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. +- Open receipts confirm only the current registration, connection, type, and address. Retargeting invalidates an older open flag; failed corrective opens leave the new target unconfirmed and fail joined delivery, while same-target backscroll failures retain known-open state. + - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index c51552400..9abbd0851 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -212,9 +212,14 @@ function shallowEqualRecord( interface ChannelEntry { serverId: string; descriptor: ChannelDescriptor; + /** Last confirmed state for this entry's current transport target. */ open: boolean; } +function sameChannelTarget(a: ChannelDescriptor, b: ChannelDescriptor): boolean { + return a.type === b.type && isDeepStrictEqual(a.address, b.address); +} + /** Minimal responder interface for sending JSON-RPC results back. */ interface Responder { respond(result: unknown): void; @@ -813,6 +818,9 @@ export class ChannelRegistry { this.emitTraceFn({ type: 'mcpl:channel-descriptor-rejected', serverId, channelId: channel.id, reason: 'update for unregistered channel' }); continue; } + // A live subscription to the old target does not confirm the new + // type/address. Label-only updates preserve that confirmation. + if (!sameChannelTarget(existing.descriptor, channel)) existing.open = false; existing.descriptor = channel; this.appendLabelSighting(channel.id, channel.label, extractDmMeta(channel.metadata)); addedResults.push({ id: channel.id, accepted: true }); @@ -2125,28 +2133,36 @@ export class ChannelRegistry { throw error; } attempts++; + const sameTarget = () => this.serverRegistry.getServer(serverId) === server && + sameChannelTarget(entry.descriptor, descriptor); let result: ChannelsOpenResult | undefined; - if (open) { - result = await server.sendChannelsOpen({ - channelId, type: descriptor.type, address: descriptor.address, - ...(options?.history ? { history: options.history } : {}), - }); - } else { - await server.sendChannelsClose({ channelId }); + try { + if (open) { + result = await server.sendChannelsOpen({ + channelId, type: descriptor.type, address: descriptor.address, + ...(options?.history ? { history: options.history } : {}), + }); + } else { + await server.sendChannelsClose({ channelId }); + } + } catch (error) { + // A failed same-target backscroll can retain its known-open state. + // A receipt/failure from an older connection or target cannot. + if (this.channels.get(channelKey) === entry && !sameTarget()) entry.open = false; + throw error; + } + // Never apply an old receipt to the current target, even transiently: + // a corrective open might fail before that stale bit is replaced. + if (this.channels.get(channelKey) !== entry) continue; + if (!sameTarget()) { + entry.open = false; + continue; } - // Failure is not a confirmed transition. In particular, a failed - // backscroll request must not erase an already-live subscription. - // Never apply a receipt to a removed entry or a replacement connection. - // Reconcile their current state before releasing this channel's queue. - if (this.channels.get(channelKey) !== entry || - this.serverRegistry.getServer(serverId) !== server) continue; if (open && !entry.open) opened = true; if (!open) opened = false; entry.open = open; const now = this.getDesiredState(serverId, channelId); - const targetChanged = entry.descriptor.type !== descriptor.type || - !isDeepStrictEqual(entry.descriptor.address, descriptor.address); - if (targetChanged || open !== (now === 'open' || now === 'tuned-out')) continue; + if (open !== (now === 'open' || now === 'tuned-out')) continue; return { open, opened, result }; } }); @@ -2185,7 +2201,7 @@ export class ChannelRegistry { const current = this.channels.get(`${entry.serverId}:${entry.descriptor.id}`); if (!current) throw new Error('Channel registration was removed while delivery waited'); if (!current.open || this.getDesiredState(current.serverId, current.descriptor.id) === 'closed') { - throw new Error('Channel was closed or selected for closure while delivery waited'); + throw new Error('Channel is not confirmed open for delivery, or was selected for closure while delivery waited'); } return current; } diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 33960de20..a26f7be60 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -48,9 +48,11 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { return { delivered: true }; }, }; + let currentServer = server; + const replaceServer = () => { currentServer = { ...server }; }; const traces: Array<{ type: string; [key: string]: unknown }> = []; const registry = new ChannelRegistry( - { getServer: () => server } as unknown as McplServerRegistry, + { getServer: () => currentServer } as unknown as McplServerRegistry, {} as FeatureSetManager, () => {}, (event) => traces.push(event), { store, onRouteFailure: (info) => routeFailures.push(info), onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); } }, @@ -69,7 +71,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures, replaceServer }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -408,6 +410,53 @@ test('failed policy opening records intent once, with truthful resident text thr } }); +for (const change of ['address', 'type'] as const) { + test('failed corrective open does not reuse the old ' + change + ' confirmation', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.calls.length = 0; + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { + serverId: 'test', channelId: 'x', backscroll: 5, + }); + await tick(); + const speech = f.registry.routeSpeech('resident', 'waiting reply', 'x'); + const updated = change === 'address' + ? descriptor('x', true, 'B') + : { ...descriptor('x', true, 'A'), type: 'new-type' }; + await f.registry.handleChanged('test', { updated: [updated] }); + assert.equal(f.isOpen(), false, 'old target confirmation is invalid after retargeting'); + f.pending.shift()!.finish(); // successful receipt for A + await tick(); + f.pending.shift()!.fail(); // corrective open for the new target fails + await f.drain(backscroll, speech); + assert.equal((await backscroll).success, false); + assert.equal(await speech, null); + assert.equal(f.isOpen(), false); + assert.deepEqual(f.publishedWhileOpen, []); + assert.equal(f.routeFailures.length, 1); + }); +} + +test('a failed RPC from a replaced connection cannot preserve its old open confirmation', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { + serverId: 'test', channelId: 'x', backscroll: 5, + }); + await tick(); + const speech = f.registry.routeSpeech('resident', 'waiting reply', 'x'); + f.replaceServer(); + f.pending.shift()!.fail(); + await f.drain(backscroll, speech); + assert.equal((await backscroll).success, false); + assert.equal(await speech, null); + assert.equal(f.isOpen(), false); + assert.deepEqual(f.publishedWhileOpen, []); + assert.equal(f.routeFailures.length, 1); +}); + test('descriptor churn exhausts convergence and fails the tool plus joined deliveries instead of hanging', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From cd726dd915bb510f8ec2f7a26d83f4c8c918122f Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 15:07:59 -0400 Subject: [PATCH 07/26] fix(mcpl): preserve pending decisions after target invalidation Felix reproduced a delivery regression in the receipt correction: an address update cleared the open flag during a pending explicit close, causing speech/reply preparation to treat the wait as a fresh opening decision and supersede the close. Both automatic delivery paths now join any outstanding lifecycle operation, independent of transport-confirmation state. Fresh engagement is possible when no decision is pending; waiting itself records no new intent. This separates three facts that the open bit alone cannot represent: desired intent, outstanding decision, and confirmed transport. Two public-API retargeted-close regressions fail on de1a9f3 for speech and reply preparation. They now preserve desired closed, publish nothing, and return a visible failure, while the expanded lifecycle/provenance/routing/tune-out/release set remains green. The convergence budget and target-validity helper are unchanged; re-review precedes full validation and push. Co-Authored-By: GPT-6 Astra --- src/mcpl/channel-registry.ts | 7 ++++--- test/channel-lifecycle-order.test.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 9abbd0851..44b368537 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -2180,7 +2180,8 @@ export class ChannelRegistry { } /** - * Join work on an already-open channel without recording new intent. + * Join outstanding lifecycle work without recording new intent. Retargeting + * may have invalidated open confirmation while the decision remains pending. * A close that settles during the wait remains authoritative; waiting for * it is not an instruction to open the channel again. */ @@ -2599,7 +2600,7 @@ export class ChannelRegistry { if (matches.length > 1) return { status: 'ambiguous' }; let entry = matches[0]!; try { - if (entry.open && this.hasPendingLifecycle(entry)) { + if (this.hasPendingLifecycle(entry)) { entry = await this.waitForOpenChannel(entry); } const desired = this.getDesiredState(entry.serverId, entry.descriptor.id); @@ -3082,7 +3083,7 @@ export class ChannelRegistry { if (!entry) { return fail(channelId, `no registered channel for locus "${channelId}"`); } - if (entry.open && this.hasPendingLifecycle(entry)) { + if (this.hasPendingLifecycle(entry)) { try { entry = await this.waitForOpenChannel(entry); } catch (err) { diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index a26f7be60..29599a5cc 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -410,6 +410,34 @@ test('failed policy opening records intent once, with truthful resident text thr } }); +for (const kind of ['speech', 'reply'] as const) { + test(kind + ' joins a pending close even after retargeting clears open confirmation', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.calls.length = 0; + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + assert.equal(f.isOpen(), false); + const delivery = kind === 'speech' + ? f.registry.routeSpeech('resident', 'waiting reply', 'x') + : f.registry.openIfClosedForSend('x', 'test'); + await f.drain(closing, delivery); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal((await closing).success, true); + assert.equal(f.isOpen(), false); + assert.deepEqual(f.publishedWhileOpen, []); + assert.equal(f.calls.every((call) => call.kind === 'close'), true); + if (kind === 'speech') { + assert.equal(await delivery, null); + assert.equal(f.routeFailures.length, 1); + } else { + assert.equal((await delivery as { status: string }).status, 'open-failed'); + } + }); +} + for (const change of ['address', 'type'] as const) { test('failed corrective open does not reuse the old ' + change + ' confirmation', async () => { const f = fixture(); From 24a84f59d4acb7d3867138de2fff8063f05ab979 Mon Sep 17 00:00:00 2001 From: Jasper Date: Fri, 2 Oct 2026 15:11:29 -0400 Subject: [PATCH 08/26] fix(mcpl): distinguish unconfirmed targets from confirmed closure A bounded public-API probe reproduced another reader of the invalidated open flag: open A, start close, retarget B, fail the close, then explicitly close again. The second tool call claimed success through the already-closed shortcut while the simulated server remained open. Track invalidated target confirmation privately with a WeakSet keyed by the current registration entry. Invalidation still projects open:false, but the explicit-close shortcut requires confirmed closure. A successful current-target lifecycle receipt or accepted open-channel incoming message restores confirmation. Removed registrations need no separate cleanup and the public entry shape stays stable. The new regression fails on cd726dd and passes with the fix; its final repeated close also proves genuinely confirmed closure still avoids another RPC. Expanded focused lifecycle/provenance/routing/tune-out/release tests pass112/112. Final independent review and one bounded serialized full run precede publication. Co-Authored-By: GPT-6 Astra --- .../210-channel-lifecycle-order.fixed.md | 2 +- src/mcpl/channel-registry.ts | 26 ++++++++++++++----- test/channel-lifecycle-order.test.ts | 20 ++++++++++++++ 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 94e63d742..50b39afd1 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -2,6 +2,6 @@ - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. -- Open receipts confirm only the current registration, connection, type, and address. Retargeting invalidates an older open flag; failed corrective opens leave the new target unconfirmed and fail joined delivery, while same-target backscroll failures retain known-open state. +- Open receipts confirm only the current registration, connection, type, and address. Retargeting invalidates an older open flag; failed corrective opens leave the new target unconfirmed and fail joined delivery, while same-target backscroll failures retain known-open state. Explicit close retries an unconfirmed target instead of reporting it already closed. - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 44b368537..0873fd438 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -212,7 +212,7 @@ function shallowEqualRecord( interface ChannelEntry { serverId: string; descriptor: ChannelDescriptor; - /** Last confirmed state for this entry's current transport target. */ + /** Whether the current target is confirmed open; false can be unconfirmed. */ open: boolean; } @@ -670,6 +670,14 @@ export class ChannelRegistry { // across descriptor replacement. Settled tails release themselves; a // failed operation must not poison later decisions. private channelLifecycleTails = new Map>(); + // Losing an open receipt does not confirm closure. Keep that distinction + // private while preserving the public open:boolean availability projection. + private unconfirmedChannelTargets = new WeakSet(); + + private invalidateTransportConfirmation(entry: ChannelEntry): void { + entry.open = false; + this.unconfirmedChannelTargets.add(entry); + } private hasPendingLifecycle(entry: ChannelEntry): boolean { return this.channelLifecycleTails.has(this.lifecycleKey(entry.serverId, entry.descriptor.id)); @@ -820,7 +828,7 @@ export class ChannelRegistry { } // A live subscription to the old target does not confirm the new // type/address. Label-only updates preserve that confirmation. - if (!sameChannelTarget(existing.descriptor, channel)) existing.open = false; + if (!sameChannelTarget(existing.descriptor, channel)) this.invalidateTransportConfirmation(existing); existing.descriptor = channel; this.appendLabelSighting(channel.id, channel.label, extractDmMeta(channel.metadata)); addedResults.push({ id: channel.id, accepted: true }); @@ -963,7 +971,9 @@ export class ChannelRegistry { // the transport is actually open. This repairs transient status only; // durable desired state still changes exclusively through lifecycle // operations. - this.channels.get(incomingKey)!.open = true; + const incomingEntry = this.channels.get(incomingKey)!; + incomingEntry.open = true; + this.unconfirmedChannelTargets.delete(incomingEntry); }; if (!coalesced) markAccepted(); @@ -2122,7 +2132,7 @@ export class ChannelRegistry { if (attempts >= MAX_CHANNEL_LIFECYCLE_ATTEMPTS) { // No receipt confirmed the current target/intent. A later delivery // must reconcile it rather than reuse an earlier target's open flag. - entry.open = false; + this.invalidateTransportConfirmation(entry); const error = new ChannelLifecycleConvergenceError( `Channel ${channelId} did not converge after ${attempts} lifecycle attempts; its target or desired state kept changing`, ); @@ -2148,19 +2158,20 @@ export class ChannelRegistry { } catch (error) { // A failed same-target backscroll can retain its known-open state. // A receipt/failure from an older connection or target cannot. - if (this.channels.get(channelKey) === entry && !sameTarget()) entry.open = false; + if (this.channels.get(channelKey) === entry && !sameTarget()) this.invalidateTransportConfirmation(entry); throw error; } // Never apply an old receipt to the current target, even transiently: // a corrective open might fail before that stale bit is replaced. if (this.channels.get(channelKey) !== entry) continue; if (!sameTarget()) { - entry.open = false; + this.invalidateTransportConfirmation(entry); continue; } if (open && !entry.open) opened = true; if (!open) opened = false; entry.open = open; + this.unconfirmedChannelTargets.delete(entry); const now = this.getDesiredState(serverId, channelId); if (open !== (now === 'open' || now === 'tuned-out')) continue; return { open, opened, result }; @@ -2726,7 +2737,8 @@ export class ChannelRegistry { } const alreadyDesiredClosed = this.getDesiredState(entry.serverId, input.channelId) === 'closed'; - if (!entry.open && alreadyDesiredClosed && !this.hasPendingLifecycle(entry)) { + if (!entry.open && alreadyDesiredClosed && !this.hasPendingLifecycle(entry) && + !this.unconfirmedChannelTargets.has(entry)) { return { success: true, data: { channelId: input.channelId, status: 'already closed' }, diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 29599a5cc..cb7c25b9b 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -410,6 +410,26 @@ test('failed policy opening records intent once, with truthful resident text thr } }); +test('an unconfirmed target retries explicit close instead of claiming already closed', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.calls.length = 0; + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + f.pending.shift()!.fail(); + assert.equal((await closing).success, false); + assert.equal(f.isOpen(), false); + assert.equal(f.actual.get('x'), true, 'failed close did not change the server state'); + f.held.delete('x'); + assert.equal((await f.tool('close')).success, true); + assert.equal(f.actual.get('x'), false, 'explicit retry must actually close the channel'); + assert.equal(f.calls.length, 2, 'unconfirmed is different from confirmed closed'); + assert.equal((await f.tool('close')).success, true); + assert.equal(f.calls.length, 2, 'the confirmed-closed fast path remains available'); +}); + for (const kind of ['speech', 'reply'] as const) { test(kind + ' joins a pending close even after retargeting clears open confirmation', async () => { const f = fixture(); From 8fb4c7848c7136de7e0ebfc71feb0de17cb5b12a Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 17:49:30 -0400 Subject: [PATCH 09/26] fix(mcpl): a channels/open answered for another channel fails the open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit slimepriestess suggested this host-side check in her review of discord-mcpl #62 (room-410 #64335, relayed by Agnes-984 in room-203 #64343): #210 checks host state around an open but never compared the answer's channel with the one requested. Her probe on discord-mcpl's base showed the substitution is real: a channels/open of a stale `discord:g1:c9` fell back to the first registered channel of the type, opened `c1`, and returned c1's history. slack-mcpl's handleChannelOpen has the same first-of-type fallback today. discord-mcpl #62 closes it on that server; this closes it for every server, on the host. In applyDesiredChannelState, right after the open RPC returns (before the stale-receipt checks, since a substituted answer is wrong whatever the target did meanwhile): when the result names a channel id (`result.channel.id` present) that isn't the requested channelId, - the current entry's transport confirmation is invalidated (open=false, unconfirmed): an answer for another channel confirms nothing about this one, and a server that no longer maps our id probably hasn't got it open. Incoming traffic from the channel still re-confirms it, and a later open retries it; - a ChannelOpenSubstitutionError is thrown, so nothing in the answer is used: channel_open returns a failure with no history, delivery-forced opens fail the route visibly, reply opens report open-failed; - the host logs `[channel-open-substituted] server=… requested=… answered=…` (console.error, so headless hosts without a trace client see it), with the server-chosen id printed only when it's a short visible-ASCII token and otherwise described (plainIdForLog), so it can't add a line or a field; - an `mcpl:channel-open-substituted` trace carries serverId, channelId, answeredFor and the message. reconcileChannels skips its generic reconcile-failed trace for this error, as it does for exhausted convergence: one diagnostic per incident. waitForOpenChannel is unchanged on purpose: joiners fail through the invalidated open flag, and a later operation that does establish the channel still lets them through. A result that names no channel (several test doubles return `{}` or nothing) can't be checked and is accepted as before. Checked servers: discord-mcpl and slack-mcpl return their registered descriptor on the normal path, so a matching id is what production sends. Tests (test/channel-open-substitution.test.ts, 5): the tool open fails with no history and neither channel marked open, one trace, one log line; a policy-open channel stays closed, reports once (no reconcile-failed), and speech into it fails with the reason and sends nothing; a known-open channel loses confirmation and reopens once the server answers for it; an odd id (newline plus a forged log line) is described, not printed; controls: a same-id answer and a no-channel answer open as before. With src reverted, the first four fail and the control passes. Related files (substitution, lifecycle-order, registry-routing, machine-close-provenance, mcpl-gate-roundtrip, channels-changed-respond-first): 79/79. #210's author, Jasper-314, is archived; Ada-1017 adds this as agent-framework's owner (room-203 #64353). --- .../210-channel-lifecycle-order.fixed.md | 2 + src/mcpl/channel-registry.ts | 44 +++++- test/channel-open-substitution.test.ts | 146 ++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 test/channel-open-substitution.test.ts diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 50b39afd1..f7060528f 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -5,3 +5,5 @@ - Open receipts confirm only the current registration, connection, type, and address. Retargeting invalidates an older open flag; failed corrective opens leave the new target unconfirmed and fail joined delivery, while same-target backscroll failures retain known-open state. Explicit close retries an unconfirmed target instead of reporting it already closed. - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. + +- A `channels/open` answered for a different channel than the one requested fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace. An answer that names no channel is accepted as before. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 0873fd438..c47a82015 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -50,8 +50,22 @@ const MAX_CHANNEL_LIFECYCLE_ATTEMPTS = 5; /** A successful RPC can still fail to establish the latest channel target. */ class ChannelLifecycleConvergenceError extends Error {} +/** A channels/open answered for a channel other than the one it named. */ +class ChannelOpenSubstitutionError extends Error {} const CHANNEL_LIFECYCLE_LOG_ID = 'mcpl/channel-lifecycle'; +/** + * A server-chosen channel id for a host log line: as it is when it's a short + * token of visible ASCII, as composite ids such as `discord:g1:c1` are, and + * otherwise only described, so an odd id can't add a line or a field. + */ +function plainIdForLog(id: unknown): string { + if (typeof id === 'string' && /^[\x21-\x7e]{1,200}$/.test(id)) return id; + return typeof id === 'string' + ? `(not a plain id: a ${id.length}-character string)` + : `(not a plain id: ${id === null ? 'null' : typeof id})`; +} + /** * Durable "which label did this channelId have, each time we saw it" log. * `resolveProseTarget()` resolves a label to an id only for channels @@ -2161,6 +2175,30 @@ export class ChannelRegistry { if (this.channels.get(channelKey) === entry && !sameTarget()) this.invalidateTransportConfirmation(entry); throw error; } + // A server can answer for a channel other than the one named: one + // that falls back to its first channel of the type for an id it no + // longer knows opens that channel and returns its history. That + // receipt confirms nothing about this channel, and the history is + // another channel's. So the open fails, nothing is marked open, the + // history goes nowhere, and this channel's open state is unconfirmed. + // A result that names no channel can't be checked and is taken as + // before. + const answeredFor = result?.channel?.id; + if (answeredFor !== undefined && answeredFor !== channelId) { + if (this.channels.get(channelKey) === entry) this.invalidateTransportConfirmation(entry); + const error = new ChannelOpenSubstitutionError( + `Server "${serverId}" answered channels/open for ${channelId} with a different channel; ` + + `${channelId} is not marked open, and nothing in the answer is used`, + ); + console.error( + `[channel-open-substituted] server=${serverId} requested=${channelId} answered=${plainIdForLog(answeredFor)}`, + ); + this.emitTraceFn({ + type: 'mcpl:channel-open-substituted', + serverId, channelId, answeredFor, error: error.message, + }); + throw error; + } // Never apply an old receipt to the current target, even transiently: // a corrective open might fail before that stale bit is replaced. if (this.channels.get(channelKey) !== entry) continue; @@ -2279,9 +2317,11 @@ export class ChannelRegistry { await this.applyDesiredChannelState(serverId, channel.id); } catch (err) { // Removal while queued is cancellation, not a failed subscription. - // Exhausted convergence already emitted its diagnostic at the bound. + // Exhausted convergence and a substituted answer already emitted + // their own diagnostics. if (!this.channels.has(`${serverId}:${channel.id}`) || - err instanceof ChannelLifecycleConvergenceError) continue; + err instanceof ChannelLifecycleConvergenceError || + err instanceof ChannelOpenSubstitutionError) continue; this.emitTraceFn({ type: 'mcpl:channel-reconcile-failed', serverId, diff --git a/test/channel-open-substitution.test.ts b/test/channel-open-substitution.test.ts new file mode 100644 index 000000000..d4a4b8260 --- /dev/null +++ b/test/channel-open-substitution.test.ts @@ -0,0 +1,146 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; +import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; +import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; +import type { ChannelDescriptor, ChannelIncomingMessage, ChannelsOpenResult } from '../src/mcpl/types.js'; + +// A server that answers channels/open for another channel than the one named, +// as discord-mcpl's base did for a stale id: it fell back to its first channel +// of the type, opened that, and returned that channel's history. + +function descriptor(id: string, initiallyOpen = false): ChannelDescriptor { + return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: id } }; +} + +function historyOf(channelId: string): ChannelIncomingMessage[] { + return [{ + channelId, messageId: `${channelId}-1`, author: { id: 'u1', name: 'someone' }, + timestamp: '2026-10-09T00:00:00Z', content: [{ type: 'text', text: `from ${channelId}` }], + } as ChannelIncomingMessage]; +} + +function fixture(answer: (requested: string) => ChannelsOpenResult) { + const calls: Array<{ kind: 'open' | 'close'; id: string }> = []; + const published: string[] = []; + const traces: Array<{ type: string; [key: string]: unknown }> = []; + const routeFailures: Array<{ reason: string }> = []; + const server = { + grant: new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), []), + sendChannelsOpen: async (params: { channelId?: string }) => { + calls.push({ kind: 'open', id: params.channelId! }); + return answer(params.channelId!); + }, + sendChannelsClose: async (params: { channelId: string }) => { + calls.push({ kind: 'close', id: params.channelId }); + return { closed: true }; + }, + sendChannelsPublish: async (params: { channelId: string }) => { + published.push(params.channelId); + return { delivered: true }; + }, + }; + const registry = new ChannelRegistry( + { getServer: () => server } as unknown as McplServerRegistry, + {} as FeatureSetManager, () => {}, (event) => traces.push(event), + { onRouteFailure: (info) => routeFailures.push(info) }, + ); + const isOpen = (id: string) => registry.getOpenChannels().some((e) => e.descriptor.id === id); + const substituted = () => traces.filter((t) => t.type === 'mcpl:channel-open-substituted'); + return { registry, calls, published, traces, routeFailures, isOpen, substituted }; +} + +/** Run `work` with console.error captured, returning what it printed. */ +async function capturingErrors(work: () => Promise): Promise<{ value: T; lines: string[] }> { + const lines: string[] = []; + const original = console.error; + console.error = (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }; + try { + return { value: await work(), lines }; + } finally { + console.error = original; + } +} + +test('an open answered for another channel fails, and that channel\'s history goes nowhere', async () => { + const f = fixture(() => ({ channel: descriptor('y'), history: historyOf('y') })); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + const { value: result, lines } = await capturingErrors(() => + f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 })); + + assert.equal(result.success, false); + assert.match(String(result.error), /answered channels\/open for x with a different channel/); + assert.equal(result.data, undefined, 'no history, or anything else from the answer, reaches the resident'); + assert.equal(f.isOpen('x'), false, 'the requested channel is not marked open'); + assert.equal(f.isOpen('y'), false, 'nor is the channel the server answered for'); + + const [trace, ...more] = f.substituted(); + assert.equal(more.length, 0); + assert.equal(trace?.serverId, 'test'); + assert.equal(trace?.channelId, 'x'); + assert.equal(trace?.answeredFor, 'y'); + assert.deepEqual(lines.filter((l) => l.startsWith('[channel-open-substituted]')), + ['[channel-open-substituted] server=test requested=x answered=y']); +}); + +test('a channel admitted open stays closed on a substituted answer, reports it once, and speech into it fails loudly', async () => { + const f = fixture(() => ({ channel: descriptor('y') })); + await capturingErrors(() => f.registry.handleChanged('test', { added: [descriptor('x', true), descriptor('y')] })); + assert.equal(f.isOpen('x'), false); + assert.equal(f.substituted().length, 1); + assert.equal(f.traces.filter((t) => t.type === 'mcpl:channel-reconcile-failed').length, 0, + 'the substitution is its own diagnostic, not also a generic reconcile failure'); + + const { value: delivery } = await capturingErrors(() => f.registry.routeSpeech('resident', 'hello', 'x')); + assert.equal(delivery, null); + assert.deepEqual(f.published, [], 'nothing is sent into a channel the server would not confirm'); + assert.equal(f.routeFailures.length, 1); + assert.match(f.routeFailures[0]!.reason, /answered channels\/open for x with a different channel/); +}); + +test('a known-open channel loses its confirmation on a substituted answer, and reopens once the server answers for it', async () => { + let answerFor: string | undefined; + const f = fixture((requested) => ({ channel: descriptor(answerFor ?? requested) })); + await f.registry.handleChanged('test', { added: [descriptor('x', true), descriptor('y')] }); + assert.equal(f.isOpen('x'), true); + + answerFor = 'y'; + const { value: backscroll } = await capturingErrors(() => + f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 })); + assert.equal(backscroll.success, false); + assert.equal(f.isOpen('x'), false, 'an answer for another channel confirms nothing about this one'); + + answerFor = undefined; + f.calls.length = 0; + const delivery = await f.registry.routeSpeech('resident', 'hello', 'x'); + assert.equal(delivery?.delivered, true); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }], 'delivery reopens the channel before sending'); + assert.deepEqual(f.published, ['x']); +}); + +test('an odd answered id is described in the log line, never printed raw', async () => { + const forged = 'y\n[channel-open-substituted] server=other requested=x answered=x'; + const f = fixture(() => ({ channel: { ...descriptor('y'), id: forged } })); + await f.registry.handleChanged('test', { added: [descriptor('x')] }); + const { lines } = await capturingErrors(() => + f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x' })); + const logged = lines.filter((l) => l.includes('channel-open-substituted')); + assert.deepEqual(logged, [ + `[channel-open-substituted] server=test requested=x answered=(not a plain id: a ${forged.length}-character string)`, + ]); +}); + +test('an answer for the requested channel, or one naming no channel, opens it as before', async () => { + for (const answer of [ + (requested: string) => ({ channel: descriptor(requested), history: historyOf(requested) }), + () => ({}) as ChannelsOpenResult, + ]) { + const f = fixture(answer); + await f.registry.handleChanged('test', { added: [descriptor('x')] }); + const result = await f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + assert.equal(result.success, true); + assert.equal(f.isOpen('x'), true); + assert.equal(f.substituted().length, 0); + } +}); From 1997b120339bfc9dacbd90e39cf8f14f12c9e83e Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 18:00:18 -0400 Subject: [PATCH 10/26] fix(mcpl): undo the open a substituting server made; check history ids; retry stale receipts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Basil-1007's review of 8fb4c78 (room-376 #64491), all five points taken, one adjusted: 1. The server really opened the channel it answered for, and nothing closed it: Basil's probe showed y (policy-closed) then forwarding a message, being marked open by handleIncoming, reaching the resident and becoming the default publish channel. Now closeSubstitutedChannel closes it through y's own lifecycle queue when y is registered on this server and its desired state is 'closed'. Adjusted from Basil's `onlyIfClosed: true` suggestion: a channel the host wants open is left alone, so the correction only ever sends a close, and a close can't be answered for another channel. That rules out ping-pong between two channels a pathological server keeps swapping. An id the host doesn't know needs nothing: §14.5 refuses its incoming (channel-registry ~950). Its failure is traced as mcpl:channel-reconcile-failed (convergence exhaustion already traced itself). 2. Invalidation kept, as Basil agreed. 3. substitutedChannel() also reads history items: a history item whose channelId names another channel is substitution, with or without `channel` in the answer. discord-mcpl's history items carry desc.id, the same id the answer's channel carries. Absent ids still can't be checked and pass, the same rule for both fields. The ChannelsOpenParams.channelId doc now says the id is exact and a server that doesn't know it should refuse, instead of "preferred over type/address matching", which read as permitting the fallback. 4. Placement: the log line, trace and corrective close happen as soon as the answer names another channel; the failure itself comes after the two stale-receipt checks. A stale receipt is retried whatever it says, so a substitution on it now retries instead of failing; order only matters there, and it turns a failure into a retry, never a success into a failure. The current-entry guard on the invalidation is gone (always true after the checks). 5. plainIdForLog now guards both ids on the log line: requested ids come from server registration too, which only checks for a non-empty string. The trace no longer carries `error` (it's emitted before the error exists); serverId, channelId and answeredFor remain. Tests: 9 in test/channel-open-substitution.test.ts (4 new: history-only substitution, the corrective close, no correction for a wanted-open channel, a stale substituted receipt retried and still corrected). Red check: with src at 24a84f5, 7 fail (the wanted-open guard and the controls pass); with src at 8fb4c78, the three new behaviours fail. Related channel files 83/83. Full suite at 8fb4c78: 1158 tests, 19 fail and 5 cancelled, all in mcpl-awareness-barrier, mcpl-tool-lifecycle-e2e and tune-out-e2e, whose child MCPL servers don't start in this worktree; those files fail the same at #210's own head 24a84f5 (19 fail, 4 cancelled; tune-out-e2e cancelled). --- src/mcpl/channel-registry.ts | 77 +++++++++++++++------ src/mcpl/types.ts | 7 +- test/channel-open-substitution.test.ts | 93 +++++++++++++++++++++----- 3 files changed, 142 insertions(+), 35 deletions(-) diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index c47a82015..b9edc829b 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -66,6 +66,23 @@ function plainIdForLog(id: unknown): string { : `(not a plain id: ${id === null ? 'null' : typeof id})`; } +/** + * The other channel a channels/open answer names, if it names one: its + * `channel.id`, or else the `channelId` of a history item. Each is evidence of + * which channel the server opened. An answer that names no channel at all + * can't be checked, and gives undefined. + */ +function substitutedChannel(result: ChannelsOpenResult | undefined, requested: string): unknown { + const answered = result?.channel?.id; + if (answered !== undefined && answered !== requested) return answered; + const history: unknown[] = Array.isArray(result?.history) ? result.history : []; + for (const item of history) { + const from = (item as { channelId?: unknown } | null)?.channelId; + if (from !== undefined && from !== requested) return from; + } + return undefined; +} + /** * Durable "which label did this channelId have, each time we saw it" log. * `resolveProseTarget()` resolves a label to an id only for channels @@ -2177,27 +2194,16 @@ export class ChannelRegistry { } // A server can answer for a channel other than the one named: one // that falls back to its first channel of the type for an id it no - // longer knows opens that channel and returns its history. That - // receipt confirms nothing about this channel, and the history is - // another channel's. So the open fails, nothing is marked open, the - // history goes nowhere, and this channel's open state is unconfirmed. - // A result that names no channel can't be checked and is taken as - // before. - const answeredFor = result?.channel?.id; - if (answeredFor !== undefined && answeredFor !== channelId) { - if (this.channels.get(channelKey) === entry) this.invalidateTransportConfirmation(entry); - const error = new ChannelOpenSubstitutionError( - `Server "${serverId}" answered channels/open for ${channelId} with a different channel; ` + - `${channelId} is not marked open, and nothing in the answer is used`, - ); + // longer knows opens that channel and returns its history. Whatever + // becomes of this receipt, the server did that: say so, and undo the + // open it made if the host wants that channel closed. + const substitute = open ? substitutedChannel(result, channelId) : undefined; + if (substitute !== undefined) { console.error( - `[channel-open-substituted] server=${serverId} requested=${channelId} answered=${plainIdForLog(answeredFor)}`, + `[channel-open-substituted] server=${serverId} requested=${plainIdForLog(channelId)} answered=${plainIdForLog(substitute)}`, ); - this.emitTraceFn({ - type: 'mcpl:channel-open-substituted', - serverId, channelId, answeredFor, error: error.message, - }); - throw error; + this.emitTraceFn({ type: 'mcpl:channel-open-substituted', serverId, channelId, answeredFor: substitute }); + if (typeof substitute === 'string') this.closeSubstitutedChannel(serverId, substitute); } // Never apply an old receipt to the current target, even transiently: // a corrective open might fail before that stale bit is replaced. @@ -2206,6 +2212,18 @@ export class ChannelRegistry { this.invalidateTransportConfirmation(entry); continue; } + // A current receipt for another channel confirms nothing about this + // one, and its history is another channel's. So the open fails, + // nothing in the answer is used, and this channel's open state is + // unconfirmed: a server that doesn't resolve this id may not resolve + // its sends either. + if (substitute !== undefined) { + this.invalidateTransportConfirmation(entry); + throw new ChannelOpenSubstitutionError( + `Server "${serverId}" answered channels/open for ${channelId} with a different channel; ` + + `${channelId} is not marked open, and nothing in the answer is used`, + ); + } if (open && !entry.open) opened = true; if (!open) opened = false; entry.open = open; @@ -2228,6 +2246,27 @@ export class ChannelRegistry { return operation; } + /** + * A server that answered an open with another channel really opened that + * one. When it's registered here and the host wants it closed, close it + * through its own lifecycle queue: otherwise its traffic would reach the + * resident, marked open by its first incoming message. A channel the host + * wants open is left alone, and a close can't be answered for another + * channel, so this never starts another substitution. An id the host + * doesn't know needs nothing: §14.5 refuses its traffic. + */ + private closeSubstitutedChannel(serverId: string, channelId: string): void { + if (!this.channels.has(`${serverId}:${channelId}`)) return; + if (this.getDesiredState(serverId, channelId) !== 'closed') return; + this.applyDesiredChannelState(serverId, channelId).catch((err) => { + if (err instanceof ChannelLifecycleConvergenceError) return; + this.emitTraceFn({ + type: 'mcpl:channel-reconcile-failed', + serverId, channelId, desired: 'closed', error: (err as Error).message, + }); + }); + } + /** * Join outstanding lifecycle work without recording new intent. Retargeting * may have invalidated open confirmation while the decision remains pending. diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index c080af76d..d6c9e21cd 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -1048,7 +1048,12 @@ export interface ChannelsListResult { * Spec Section 14.3. */ export interface ChannelsOpenParams { - /** Exact registered id. Preferred over type/address matching. */ + /** + * The exact registered id to open. A server that doesn't know it should + * refuse rather than open another channel: the host fails an open whose + * answer names any other channel, by its `channel.id` or a history item's + * `channelId`. + */ channelId?: string; type: string; address?: Record; diff --git a/test/channel-open-substitution.test.ts b/test/channel-open-substitution.test.ts index d4a4b8260..f6d2a8b2c 100644 --- a/test/channel-open-substitution.test.ts +++ b/test/channel-open-substitution.test.ts @@ -10,18 +10,20 @@ import type { ChannelDescriptor, ChannelIncomingMessage, ChannelsOpenResult } fr // as discord-mcpl's base did for a stale id: it fell back to its first channel // of the type, opened that, and returned that channel's history. -function descriptor(id: string, initiallyOpen = false): ChannelDescriptor { - return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: id } }; +const tick = () => new Promise((resolve) => setImmediate(resolve)); + +function descriptor(id: string, initiallyOpen = false, address = id): ChannelDescriptor { + return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: address } }; } function historyOf(channelId: string): ChannelIncomingMessage[] { return [{ channelId, messageId: `${channelId}-1`, author: { id: 'u1', name: 'someone' }, timestamp: '2026-10-09T00:00:00Z', content: [{ type: 'text', text: `from ${channelId}` }], - } as ChannelIncomingMessage]; + }]; } -function fixture(answer: (requested: string) => ChannelsOpenResult) { +function fixture(answer: (requested: string) => ChannelsOpenResult | Promise) { const calls: Array<{ kind: 'open' | 'close'; id: string }> = []; const published: string[] = []; const traces: Array<{ type: string; [key: string]: unknown }> = []; @@ -48,7 +50,9 @@ function fixture(answer: (requested: string) => ChannelsOpenResult) { ); const isOpen = (id: string) => registry.getOpenChannels().some((e) => e.descriptor.id === id); const substituted = () => traces.filter((t) => t.type === 'mcpl:channel-open-substituted'); - return { registry, calls, published, traces, routeFailures, isOpen, substituted }; + const openX = (backscroll?: number) => + registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', ...(backscroll ? { backscroll } : {}) }); + return { registry, calls, published, traces, routeFailures, isOpen, substituted, openX }; } /** Run `work` with console.error captured, returning what it printed. */ @@ -66,8 +70,7 @@ async function capturingErrors(work: () => Promise): Promise<{ value: T; l test('an open answered for another channel fails, and that channel\'s history goes nowhere', async () => { const f = fixture(() => ({ channel: descriptor('y'), history: historyOf('y') })); await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); - const { value: result, lines } = await capturingErrors(() => - f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 })); + const { value: result, lines } = await capturingErrors(() => f.openX(5)); assert.equal(result.success, false); assert.match(String(result.error), /answered channels\/open for x with a different channel/); @@ -84,6 +87,39 @@ test('an open answered for another channel fails, and that channel\'s history go ['[channel-open-substituted] server=test requested=x answered=y']); }); +test('an answer naming no channel but carrying another channel\'s history fails the same way', async () => { + const f = fixture(() => ({ history: historyOf('y') }) as unknown as ChannelsOpenResult); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + const { value: result } = await capturingErrors(() => f.openX(5)); + assert.equal(result.success, false); + assert.equal(result.data, undefined); + assert.equal(f.isOpen('x'), false); + assert.equal(f.substituted()[0]?.answeredFor, 'y'); +}); + +test('the channel opened instead is closed again when the host wants it closed', async () => { + const f = fixture(() => ({ channel: descriptor('y') })); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + f.calls.length = 0; + await capturingErrors(() => f.openX()); + await tick(); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }, { kind: 'close', id: 'y' }], + 'the server opened y, so the host closes it through y\'s own lifecycle'); + assert.equal(f.registry.getDesiredState('test', 'y'), 'closed'); + assert.equal(f.isOpen('y'), false); +}); + +test('the channel opened instead is left alone when the host wants it open', async () => { + const f = fixture((requested) => ({ channel: descriptor(requested === 'x' ? 'y' : requested) })); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y', true)] }); + assert.equal(f.isOpen('y'), true); + f.calls.length = 0; + await capturingErrors(() => f.openX()); + await tick(); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }], 'no corrective close, and no reopening of y'); + assert.equal(f.isOpen('y'), true); +}); + test('a channel admitted open stays closed on a substituted answer, reports it once, and speech into it fails loudly', async () => { const f = fixture(() => ({ channel: descriptor('y') })); await capturingErrors(() => f.registry.handleChanged('test', { added: [descriptor('x', true), descriptor('y')] })); @@ -106,12 +142,12 @@ test('a known-open channel loses its confirmation on a substituted answer, and r assert.equal(f.isOpen('x'), true); answerFor = 'y'; - const { value: backscroll } = await capturingErrors(() => - f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 })); + const { value: backscroll } = await capturingErrors(() => f.openX(5)); assert.equal(backscroll.success, false); assert.equal(f.isOpen('x'), false, 'an answer for another channel confirms nothing about this one'); answerFor = undefined; + await tick(); f.calls.length = 0; const delivery = await f.registry.routeSpeech('resident', 'hello', 'x'); assert.equal(delivery?.delivered, true); @@ -119,28 +155,55 @@ test('a known-open channel loses its confirmation on a substituted answer, and r assert.deepEqual(f.published, ['x']); }); -test('an odd answered id is described in the log line, never printed raw', async () => { +test('a substituted answer on a receipt made stale by retargeting is retried, not failed', async () => { + let release!: (answer: ChannelsOpenResult) => void; + let first = true; + const f = fixture((requested) => { + if (!first) return { channel: descriptor(requested) }; + first = false; + return new Promise((resolve) => { release = resolve; }); + }); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + f.calls.length = 0; + const { value: opened } = await capturingErrors(async () => { + const opening = f.openX(); + await tick(); + await f.registry.handleChanged('test', { updated: [descriptor('x', false, 'new-target')] }); + release({ channel: descriptor('y') }); + return opening; + }); + assert.equal(opened.success, true, 'the stale receipt is not trusted either way; the retry answers for x'); + assert.equal(f.isOpen('x'), true); + assert.equal(f.substituted().length, 1, 'the substitution the server made is still reported'); + await tick(); + assert.deepEqual(f.calls.filter((c) => c.id === 'y'), [{ kind: 'close', id: 'y' }], + 'and the channel it opened instead is still closed again'); +}); + +test('odd ids are described in the log line, never printed raw', async () => { const forged = 'y\n[channel-open-substituted] server=other requested=x answered=x'; const f = fixture(() => ({ channel: { ...descriptor('y'), id: forged } })); await f.registry.handleChanged('test', { added: [descriptor('x')] }); - const { lines } = await capturingErrors(() => - f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x' })); + const { lines } = await capturingErrors(() => f.openX()); const logged = lines.filter((l) => l.includes('channel-open-substituted')); assert.deepEqual(logged, [ `[channel-open-substituted] server=test requested=x answered=(not a plain id: a ${forged.length}-character string)`, ]); }); -test('an answer for the requested channel, or one naming no channel, opens it as before', async () => { +test('an answer for the requested channel, with or without its history, or one naming no channel, opens it as before', async () => { for (const answer of [ (requested: string) => ({ channel: descriptor(requested), history: historyOf(requested) }), + (requested: string) => ({ channel: descriptor(requested) }), () => ({}) as ChannelsOpenResult, ]) { const f = fixture(answer); - await f.registry.handleChanged('test', { added: [descriptor('x')] }); - const result = await f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + f.calls.length = 0; + const result = await f.openX(5); assert.equal(result.success, true); assert.equal(f.isOpen('x'), true); assert.equal(f.substituted().length, 0); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }]); } }); From c46e853691472e53cda369d201c7f1925073bd3d Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 18:00:32 -0400 Subject: [PATCH 11/26] docs(changelog): the substitution check reads history ids, closes the channel opened instead, retries stale receipts Brings #210's fragment in line with 1997b12 (Basil-1007's review round). --- changelog.d/210-channel-lifecycle-order.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index f7060528f..7b5e4a269 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -6,4 +6,4 @@ - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. -- A `channels/open` answered for a different channel than the one requested fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace. An answer that names no channel is accepted as before. +- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel is accepted as before. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. From e5cb38ad279db4a071717ad3b7e798fbbc858d1f Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:21:58 -0400 Subject: [PATCH 12/26] fix(mcpl): announce a delivery's open only when it turned a closed channel open; drop pieces later fixes made redundant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upstream-PR skill pass removed each piece of #210 on its own (42 removals over the 23 channel test files). Several survivors weren't missing tests: they were added for problems that later commits on this branch solved more generally, and each now changes nothing, or changes something for the worse. - `onlyIfClosed` and its shortcut in applyDesiredChannelState, with the receipt-time `opened` tracking (`open && !entry.open`), came with d64c803 so concurrent delivery-forced opens skipped redundant RPCs and reported one transition. cd726dd then made both delivery paths join any pending lifecycle work, so a second delivery never reaches openChannelNow while the first is in flight. What was left: the shortcut fired only when the transport was known open but the decision was closed (a failed channel_close), where a reply flipped the decision back to open and reported 'already-open', with no trace and no notice. main said 'opened' there and told the resident. And routeSpeech's `opened` was false only when incoming traffic marked the entry open during the open RPC. - `opened` now means what the notices tell the resident: this call turned a channel decided closed open. openChannelNow reads the desired state before recording its own; a channel already decided 'open' or 'tuned-out' isn't one. Compared with c46e853: - a reply after a failed close: 'opened', the trace, and one open RPC that reconfirms the transport (was 'already-open', silent, no RPC); - a delivery or reply into a channel decided open whose transport lost confirmation (retarget, substitution, failed open): no "Now open … your own message into the closed channel engaged it" notice and no trace, 'already-open' (c46e853 and main both announced it as closed); - the same into a tuned-out channel: the tune-out is kept, as before, and nothing claims "ongoing traffic will reach you" (c46e853 did); - a fresh delivery into a settled closed channel: unchanged, one notice. - The convergence cap's invalidateTransportConfirmation came with 9c94d0a for descriptor churn. de1a9f3 then made retargeting invalidate in handleChanged and after a stale receipt. At the cap, every retry has followed a replaced registration (a fresh entry), a retarget (already invalidated), or a receipt for this very target (a valid confirmation the line threw away). Its comment, "reuse an earlier target's open flag", can't happen there. - reconcileChannels' `currentAdmissions` re-filtered the admissions it had just recorded, with no await in between, so it always equalled `policyOpened`. The notice now uses `policyOpened`. Tests (channel-lifecycle-order.test.ts), five new: - a reply into a channel whose close failed opens it again, and says so; - speech/reply into a channel decided open whose transport lost confirmation is not announced as opening it; - speech/reply into a tuned-out channel whose transport lost confirmation keeps the tune-out, unannounced. All five fail against c46e853's src, each on the claim it names ('already-open' vs 'opened'; 1 notice vs 0; 'opened' vs 'already-open'), and the file's other 32 tests pass there. Locally (node 24.19) the two #210 files pass 46/46 with typecheck clean. Changelog: the fixed fragment's delivery sentence and the breaking fragment's `opened-by-delivery` sentence now say which deliveries are announced. --- .../210-channel-lifecycle-order.fixed.md | 2 +- .../210-policy-admission-notice.breaking.md | 2 +- src/mcpl/channel-registry.ts | 46 +++++++------- test/channel-lifecycle-order.test.ts | 61 +++++++++++++++++++ 4 files changed, 84 insertions(+), 27 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 7b5e4a269..fb6f6546f 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,4 +1,4 @@ -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait, and reports only actual delivery-forced opens using the current registration label. +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. diff --git a/changelog.d/210-policy-admission-notice.breaking.md b/changelog.d/210-policy-admission-notice.breaking.md index f47b54371..a324c147a 100644 --- a/changelog.d/210-policy-admission-notice.breaking.md +++ b/changelog.d/210-policy-admission-notice.breaking.md @@ -1 +1 @@ -- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` still reports a confirmed transition. The built-in host notice now distinguishes selected-for-opening from currently open. +- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` reports a delivery that turned a closed channel open, once its transport is confirmed; a delivery that only reconfirms the transport of a channel already decided open or tuned out isn't reported. The built-in host notice now distinguishes selected-for-opening from currently open. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index b9edc829b..e0911c70c 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -526,8 +526,10 @@ interface ChannelRegistryOptions { }) => void; /** * Announces either a durable subscription-policy admission (before its - * transport RPC; not an open confirmation), or a confirmed delivery-forced - * closed-to-open transition. The host records a durable, non-waking notice. + * transport RPC; not an open confirmation), or a delivery that turned a + * closed channel open, once its transport is confirmed. Reconfirming the + * transport of a channel already decided open or tuned out isn't one. + * The host records a durable, non-waking notice. * Policy admissions fire once per channel: the persisted desired-state * decision prevents re-announcement on retry or reboot. channel_close * supersedes policy; a fresh delivery into a closed locus engages it again. @@ -2135,14 +2137,13 @@ export class ChannelRegistry { private applyDesiredChannelState( serverId: string, channelId: string, - options?: { history?: ChannelHistoryRequest; onlyIfClosed?: boolean }, - ): Promise<{ open: boolean; opened: boolean; result?: ChannelsOpenResult }> { + options?: { history?: ChannelHistoryRequest }, + ): Promise<{ open: boolean; result?: ChannelsOpenResult }> { const lifecycleKey = this.lifecycleKey(serverId, channelId); const channelKey = `${serverId}:${channelId}`; const previous = this.channelLifecycleTails.get(lifecycleKey) ?? Promise.resolve(); const operation = previous.catch(() => {}).then(async () => { let attempts = 0; - let opened = false; for (;;) { const entry = this.channels.get(channelKey); if (!entry) throw new Error(`Channel is no longer registered: ${channelId}`); @@ -2154,16 +2155,13 @@ export class ChannelRegistry { const descriptor = entry.descriptor; const desired = this.getDesiredState(serverId, channelId); const open = desired === 'open' || desired === 'tuned-out'; - if (attempts === 0 && options?.onlyIfClosed && open && entry.open) { - return { open: true, opened: false }; - } // Per-RPC timeouts do not bound a stream of promptly successful but // continually superseded receipts. Bound this whole operation, then // release its queue slot and surface the failure to delivery joiners. + // Its open flag needs no change here: every retry follows a replaced + // registration, a retarget (already invalidated), or a receipt for + // this very target. if (attempts >= MAX_CHANNEL_LIFECYCLE_ATTEMPTS) { - // No receipt confirmed the current target/intent. A later delivery - // must reconcile it rather than reuse an earlier target's open flag. - this.invalidateTransportConfirmation(entry); const error = new ChannelLifecycleConvergenceError( `Channel ${channelId} did not converge after ${attempts} lifecycle attempts; its target or desired state kept changing`, ); @@ -2224,13 +2222,11 @@ export class ChannelRegistry { `${channelId} is not marked open, and nothing in the answer is used`, ); } - if (open && !entry.open) opened = true; - if (!open) opened = false; entry.open = open; this.unconfirmedChannelTargets.delete(entry); const now = this.getDesiredState(serverId, channelId); if (open !== (now === 'open' || now === 'tuned-out')) continue; - return { open, opened, result }; + return { open, result }; } }); // Keep the rejection visible to existing joiners, while observing it @@ -2332,18 +2328,12 @@ export class ChannelRegistry { // Announce the durable admission decision before awaiting transport. // This is intent, not confirmation that an open RPC succeeded. It stays // truthful if the first attempt fails, and a retry needs no new admission. - // channel_close supersedes policy; use current entries at this boundary. - const currentAdmissions = policyOpened.flatMap(({ channelId }) => { - const entry = this.channels.get(`${serverId}:${channelId}`); - if (!entry || this.getDesiredState(serverId, channelId) !== 'open') return []; - return [{ channelId, label: entry.descriptor.label }]; - }); - if (currentAdmissions.length > 0) { + if (policyOpened.length > 0) { try { this.onChannelAutoOpened?.({ serverId, source: 'subscription-policy', - channels: currentAdmissions, + channels: policyOpened, }); } catch (err) { console.error('onChannelAutoOpened (policy) failed:', err); @@ -2500,6 +2490,10 @@ export class ChannelRegistry { * even if the subscribe fails, and reconciliation retries later. * Automatic delivery needs open transport, not a change of attention: * preserve tune-out epochs both before and during the round-trip. + * + * `opened` says whether this call turned a closed channel open, which is + * what the delivery and reply notices tell the resident. Reconfirming the + * transport of a channel already decided open or tuned out isn't one. */ private async openChannelNow( entry: ChannelEntry, @@ -2507,15 +2501,17 @@ export class ChannelRegistry { history?: ChannelHistoryRequest, ): Promise<{ result: ChannelsOpenResult; opened: boolean }> { const transportOnly = source !== 'agent-tool'; - if (!transportOnly || this.getDesiredState(entry.serverId, entry.descriptor.id) !== 'tuned-out') { + const before = this.getDesiredState(entry.serverId, entry.descriptor.id); + const opened = before !== 'open' && before !== 'tuned-out'; + if (!transportOnly || before !== 'tuned-out') { this.setDesiredState(entry.serverId, entry.descriptor.id, 'open', source); } - const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, { history, onlyIfClosed: transportOnly }); + const applied = await this.applyDesiredChannelState(entry.serverId, entry.descriptor.id, { history }); const desired = this.getDesiredState(entry.serverId, entry.descriptor.id); if (!applied.open || (desired !== 'open' && !(transportOnly && desired === 'tuned-out'))) { throw new Error('Channel open was superseded by a newer lifecycle decision'); } - return { result: applied.result!, opened: applied.opened }; + return { result: applied.result!, opened }; } /** diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index cb7c25b9b..e4a581b76 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -609,3 +609,64 @@ test('a newer tune-out decision keeps transport open when an older close complet assert.equal(f.isOpen(), true); assert.equal((await close).success, false, 'superseded close must not report current state as closed'); }); + +test('a reply into a channel whose close failed opens it again, and says so', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + f.pending.shift()!.fail(); + assert.equal((await closing).success, false); + await tick(); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal(f.isOpen(), true, 'a failed same-target close leaves the transport known open'); + f.held.delete('x'); + const reply = await f.registry.openIfClosedForSend('x', 'test'); + assert.equal(reply.status, 'opened', 'the reply turned a channel decided closed open again'); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 1); + assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'open'], 'the transport is confirmed again after the failed close'); +}); + +for (const delivery of ['speech', 'reply'] as const) { + test(delivery + ' into a channel decided open whose transport lost confirmation is not announced as opening it', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + assert.equal(f.isOpen(), false, 'retargeting leaves the new target unconfirmed'); + f.calls.length = 0; + if (delivery === 'speech') { + assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); + assert.deepEqual(f.publishedWhileOpen, [true]); + } else { + assert.equal((await f.registry.openIfClosedForSend('x', 'test')).status, 'already-open'); + } + assert.deepEqual(f.calls.map((c) => [c.kind, c.address]), [['open', { value: 'B' }]], 'the new target is confirmed first'); + assert.equal(f.notices.length, 0, 'the channel was never closed'); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 0); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + }); + + test(delivery + ' into a tuned-out channel whose transport lost confirmation keeps the tune-out, unannounced', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.registry.enterTuneOut('test', 'x', { + epochId: 'kept-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + assert.equal(f.isOpen(), false); + if (delivery === 'speech') { + assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); + assert.deepEqual(f.publishedWhileOpen, [true]); + } else { + assert.equal((await f.registry.openIfClosedForSend('x', 'test')).status, 'already-open'); + } + assert.equal(f.registry.getDesiredState('test', 'x'), 'tuned-out'); + assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'kept-epoch'); + assert.equal(f.isOpen(), true); + assert.equal(f.notices.length, 0, 'its traffic is still diverted, so nothing new reaches the resident'); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 0); + }); +} From a6130d03c94c79465fe163f1f67ef1c88b650b74 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:22:40 -0400 Subject: [PATCH 13/26] test(mcpl): pin the lifecycle pieces no test reached Removing each piece of #210 on its own left these with every test passing, across all 23 channel test files. Each now has a test that fails without it: - The stale-entry retry (`channels.get(channelKey) !== entry`): "an explicit open answers only once the current registration is open". The server registers the channel again while channel_open is in flight; without the check, the old registration's receipt marks the replaced entry open and the tool answers while the current one is still closed. - channel_open's pending-work condition on its already-open shortcut: "an explicit open waits for a close in flight rather than answering already open", the open-side twin of the existing already-closed test. - The removal clause in reconcile's catch: the removal test now also asserts no mcpl:channel-reconcile-failed trace. - The superseded-open throw in openChannelNow: the overlapping-intent tests now assert that the first tool result says it was superseded. Without the throw, the open variant answers with a TypeError about `history`. - closeSubstitutedChannel's registration check: "the channel opened instead needs nothing once it is no longer registered" (substitution file). Without it, a removed channel whose durable desired state is closed gets a lifecycle operation that fails at once and emits a reconcile-failed trace. - closeSubstitutedChannel's quiet catch for exhausted convergence: "a corrective close that cannot converge reports its exhaustion once". The fixture gains `answeredFor`, a server answering an open for another channel; y's corrective close is retargeted before each receipt, and the cap's own trace is the only one. The join's removed-registration check is caught by typecheck. The tail cleanup's removal hangs (an awaited settled tail spins in microtasks, where no test timeout can fire), so it can't have a failing test. Locally (node 24.19), the two #210 files pass 50/50: 41 at c46e853, 5 in the commit before this, and 4 here, plus new assertions in two existing tests (the removal test and the overlapping-intent pair). --- test/channel-lifecycle-order.test.ts | 74 ++++++++++++++++++++++++-- test/channel-open-substitution.test.ts | 13 +++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index e4a581b76..fbc9b180e 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -27,13 +27,16 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { const publishedWhileOpen: boolean[] = []; const notices: Array<{ source: string; channels: Array<{ channelId: string; label?: string }> }> = []; const routeFailures: Array<{ reason: string }> = []; + // A server that answers an open for another channel, by requested id. + const answeredFor = new Map(); const serve = (kind: 'open' | 'close', params: { channelId?: string; address?: unknown }) => { const id = params.channelId!; calls.push({ kind, id, address: params.address }); return new Promise((resolve, reject) => { const finish = () => { - actual.set(id, kind === 'open'); - resolve(kind === 'open' ? { channel: descriptor(id) } : { closed: true }); + const target = kind === 'open' ? answeredFor.get(id) ?? id : id; + actual.set(target, kind === 'open'); + resolve(kind === 'open' ? { channel: descriptor(target) } : { closed: true }); }; if (held.has(id)) pending.push({ finish, fail: () => reject(new Error('fixture lifecycle failure')) }); else finish(); @@ -71,7 +74,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures, replaceServer }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures, replaceServer, answeredFor }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -115,6 +118,8 @@ for (const firstKind of ['open', 'close'] as const) { const lastKind = firstKind === 'open' ? 'close' : 'open'; const last = f.tool(lastKind); await f.drain(first, last); + assert.equal((await first).success, false); + assert.match(String((await first).error), /superseded by a newer lifecycle decision/); assert.equal((await last).success, true); const desired = lastKind === 'open'; assert.equal(f.registry.getDesiredState('test', 'x'), lastKind === 'open' ? 'open' : 'closed'); @@ -149,6 +154,8 @@ test('removal cancels queued work rather than recreating or mutating an unregist assert.equal((await open).success, false); assert.equal(f.registry.listChannelsRaw().length, 0); assert.equal(f.calls.filter((c) => c.kind === 'open').length, 0); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0, + 'removal while queued is cancellation, not a failed subscription'); }); test('a failed operation releases the queue for a later decision', async () => { @@ -610,6 +617,39 @@ test('a newer tune-out decision keeps transport open when an older close complet assert.equal((await close).success, false, 'superseded close must not report current state as closed'); }); +test('an explicit open answers only once the current registration is open', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.held.add('x'); + let openWhenAnswered: boolean | undefined; + const opening = f.tool('open').then((result) => { openWhenAnswered = f.isOpen(); return result; }); + await tick(); + assert.equal(f.pending.length, 1); + // The server registers the channel again while the open is in flight. + const reRegistered = f.registry.handleChanged('test', { added: [descriptor()] }); + await f.drain(opening, reRegistered); + assert.equal((await opening).success, true); + assert.equal(openWhenAnswered, true, 'a receipt for the replaced registration confirms nothing about the new one'); + assert.equal(f.isOpen(), true); +}); + +test('an explicit open waits for a close in flight rather than answering already open', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + const reopening = f.tool('open'); + let settled = false; + const again = f.tool('open').then((result) => { settled = true; return result; }); + await tick(); + assert.equal(settled, false, 'entry.open is provisional while the close is in flight'); + await f.drain(closing, reopening, again); + assert.equal((await again).success, true); + assert.equal(f.actual.get('x'), true); + assert.equal(f.isOpen(), true); +}); + test('a reply into a channel whose close failed opens it again, and says so', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); @@ -670,3 +710,31 @@ for (const delivery of ['speech', 'reply'] as const) { assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 0); }); } + +test('a corrective close that cannot converge reports its exhaustion once', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + f.calls.length = 0; + // Asked to open x, the server opens y instead, and y's corrective close is + // retargeted before each receipt. + f.answeredFor.set('x', 'y'); + f.held.add('y'); + assert.equal((await f.tool('open')).success, false); + await tick(); + assert.equal(f.pending.length, 1, 'the corrective close for y is in flight'); + try { + for (let i = 0; i < 12 && f.pending.length > 0; i++) { + await f.registry.handleChanged('test', { updated: [descriptor('y', false, 'churn-' + i)] }); + f.pending.shift()!.finish(); + await tick(); + } + assert.equal(f.pending.length, 0, 'the corrective close is bounded'); + assert.equal(f.calls.filter((c) => c.id === 'y' && c.kind === 'close').length, 5); + const failures = f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed' && e.channelId === 'y'); + assert.equal(failures.length, 1); + assert.match(String(failures[0].error), /did not converge/); + } finally { + f.held.delete('y'); + while (f.pending.length > 0) f.pending.shift()!.finish(); + } +}); diff --git a/test/channel-open-substitution.test.ts b/test/channel-open-substitution.test.ts index f6d2a8b2c..bfecb4578 100644 --- a/test/channel-open-substitution.test.ts +++ b/test/channel-open-substitution.test.ts @@ -120,6 +120,19 @@ test('the channel opened instead is left alone when the host wants it open', asy assert.equal(f.isOpen('y'), true); }); +test('the channel opened instead needs nothing once it is no longer registered', async () => { + const f = fixture(() => ({ channel: descriptor('y') })); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + await f.registry.handleChanged('test', { removed: ['y'] }); + f.calls.length = 0; + await capturingErrors(() => f.openX()); + await tick(); + assert.equal(f.substituted().length, 1, 'the substitution is still reported'); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }], 'nothing is sent for y'); + assert.equal(f.traces.filter((t) => t.type === 'mcpl:channel-reconcile-failed').length, 0, + 'an unregistered channel has no lifecycle to fail'); +}); + test('a channel admitted open stays closed on a substituted answer, reports it once, and speech into it fails loudly', async () => { const f = fixture(() => ({ channel: descriptor('y') })); await capturingErrors(() => f.registry.handleChanged('test', { added: [descriptor('x', true), descriptor('y')] })); From 7d859cb91aac4e06fd206a7e1e714186cc5d08e8 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:32:04 -0400 Subject: [PATCH 14/26] fix(mcpl): speech engages a channel decided closed as a reply does; a failed channel_open says the channel stays selected Basil-1007's review of e5cb38a (room-203 #65256) found two states where the resident's picture is "closed" while the code treated it otherwise. - A failed channel_close, then speech. routeSpeech opened only when `!entry.open`. After a failed close the transport is still known open and the decision is `closed`, so speech published, left the decision closed, and announced nothing, while a reply in the same state opened and announced (e5cb38a's test). routeSpeech now also engages a channel decided `closed`, so the two paths agree and `opened` announces it. This is the PR's own contract: a fresh send after a close has settled is a new engagement. It doesn't take the reply path's whole condition (anything not decided open or tuned out): a server without the lifecycle grant records no decision, its channels can be open by the server's choice, and speech opens BEFORE it sends, so engaging an undecided channel there would block speech that main delivers. A reply opens after its send, so its broader condition costs nothing there. - A failed channel_open. openChannelNow records `open` before the round-trip, and the decision outlives a failure (intent sticks; reconciliation retries). Under e5cb38a's rule a later delivery then opens the channel without a notice, which would be the resident's first sign that it was still wanted open. The failure text now says so when the channel isn't open and its decision is still `open`: "(it stays selected for opening: a later delivery or reconnect tries again, and channel_close withdraws that)". A failed backscroll on a live channel, and an open superseded by a newer decision, don't carry it. Also pins queue serialization, which the decision rule had left with no failing test: e5cb38a's `opened` no longer depends on transport order, so "speech opening reports the replacement registration label" stopped catching the queue's removal. The #185 test now asserts one RPC in flight per channel once the replacement has registered, which is the claim its fixture comment makes. Tests: the failed-close test now runs for speech as well as reply; a new "a failed explicit open says the channel stays selected for opening, and a later delivery opens it quietly"; doesNotMatch checks in the failed-backscroll and overlapping-intent tests. Against a6130d0's src the speech twin and the failed-open test fail, each on its own claim, and the file's other 40 pass. Locally (node 24.19) the two #210 files pass 52/52, and the 23 channel files one per process pass apart from conversation-routing's "Queue is closed" after-test rejection, the race #263 addresses. --- .../210-channel-lifecycle-order.fixed.md | 2 +- src/mcpl/channel-registry.ts | 20 ++++++- test/channel-lifecycle-order.test.ts | 55 +++++++++++++++---- 3 files changed, 61 insertions(+), 16 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index fb6f6546f..24d85ad31 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,4 +1,4 @@ -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index e0911c70c..4f62d9d6d 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -2774,9 +2774,18 @@ export class ChannelRegistry { }, }; } catch (err) { + // The decision was recorded before the round-trip and outlives its + // failure. Say so: otherwise a later delivery that opens the channel + // would be the resident's first sign that it was still wanted open. + const current = this.channels.get(`${entry.serverId}:${input.channelId}`); + const stillSelected = current !== undefined && !current.open && + this.getDesiredState(entry.serverId, input.channelId) === 'open'; return { success: false, - error: `Failed to open channel: ${(err as Error).message}`, + error: `Failed to open channel: ${(err as Error).message}` + + (stillSelected + ? ' (it stays selected for opening: a later delivery or reconnect tries again, and channel_close withdraws that)' + : ''), isError: true, }; } @@ -3183,8 +3192,13 @@ export class ChannelRegistry { // that woke the agent — DMs register closed) OPENS the channel first, // so typing indicators, reaction machinery, and inbound forwarding come // alive with the reply. If the open fails, the speech does NOT go out: - // a half-alive delivery is worse than a loud marker. - if (!entry.open) { + // a half-alive delivery is worse than a loud marker. A channel decided + // closed is engaged even while its transport is still open (a close that + // failed), as a reply would engage it. A channel with no decision at all + // (a server without the lifecycle grant records none) keeps publishing + // while its transport is open: speech opens before it sends, and that + // open could never succeed there. + if (!entry.open || this.getDesiredState(entry.serverId, entry.descriptor.id) === 'closed') { try { const { opened } = await this.openChannelNow(entry, 'opened-by-delivery'); entry = await this.waitForOpenChannel(entry); diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index fbc9b180e..17f4f5e7e 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -88,6 +88,8 @@ test('remove/re-add during reconcile converges on the replacement desired state, removed: ['x'], added: [descriptor('x', true, 'replacement')], }, { respond: () => { acknowledged = true; } }); assert.equal(acknowledged, true, 'registration ACK must not wait for the lifecycle queue'); + await tick(); + assert.equal(f.pending.length, 1, 'the replacement waits for the operation in flight: one RPC per channel at a time'); await f.drain(first, replacement); assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); assert.equal(f.isOpen(), true); @@ -120,6 +122,7 @@ for (const firstKind of ['open', 'close'] as const) { await f.drain(first, last); assert.equal((await first).success, false); assert.match(String((await first).error), /superseded by a newer lifecycle decision/); + assert.doesNotMatch(String((await first).error), /stays selected/, 'a newer decision replaced it'); assert.equal((await last).success, true); const desired = lastKind === 'open'; assert.equal(f.registry.getDesiredState('test', 'x'), lastKind === 'open' ? 'open' : 'closed'); @@ -252,6 +255,7 @@ test('a failed backscroll open preserves an already-live subscription', async () await tick(); f.pending.shift()!.fail(); assert.equal((await backscroll).success, false); + assert.doesNotMatch(String((await backscroll).error), /stays selected/, 'the channel is still open'); assert.equal(f.isOpen(), true, 'a failed redundant open is not a confirmed close'); const speech = f.registry.routeSpeech('resident', 'hello', 'x'); await f.drain(speech); @@ -650,24 +654,51 @@ test('an explicit open waits for a close in flight rather than answering already assert.equal(f.isOpen(), true); }); -test('a reply into a channel whose close failed opens it again, and says so', async () => { +for (const delivery of ['reply', 'speech'] as const) { + test('a ' + delivery + ' into a channel whose close failed opens it again, and says so', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + f.pending.shift()!.fail(); + assert.equal((await closing).success, false); + await tick(); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal(f.isOpen(), true, 'a failed same-target close leaves the transport known open'); + f.held.delete('x'); + if (delivery === 'reply') { + const reply = await f.registry.openIfClosedForSend('x', 'test'); + assert.equal(reply.status, 'opened', 'the reply turned a channel decided closed open again'); + } else { + assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); + assert.equal(f.notices.length, 1, 'the speech turned a channel decided closed open again'); + assert.deepEqual(f.publishedWhileOpen, [true]); + } + assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 1); + assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'open'], 'the transport is confirmed again after the failed close'); + }); +} + +test('a failed explicit open says the channel stays selected for opening, and a later delivery opens it quietly', async () => { const f = fixture(); - await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + await f.registry.handleChanged('test', { added: [descriptor()] }); f.calls.length = 0; f.held.add('x'); - const closing = f.tool('close'); + const opening = f.tool('open'); await tick(); f.pending.shift()!.fail(); - assert.equal((await closing).success, false); - await tick(); - assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); - assert.equal(f.isOpen(), true, 'a failed same-target close leaves the transport known open'); + const result = await opening; + assert.equal(result.success, false); + assert.match(String(result.error), /stays selected for opening/); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open', 'the decision outlives a failed round-trip'); f.held.delete('x'); - const reply = await f.registry.openIfClosedForSend('x', 'test'); - assert.equal(reply.status, 'opened', 'the reply turned a channel decided closed open again'); - assert.equal(f.registry.getDesiredState('test', 'x'), 'open'); - assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-opened-by-send').length, 1); - assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'open'], 'the transport is confirmed again after the failed close'); + await tick(); + assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); + assert.equal(f.notices.length, 0, 'the resident was already told the channel stays selected for opening'); + assert.deepEqual(f.calls.map((c) => c.kind), ['open', 'open']); }); for (const delivery of ['speech', 'reply'] as const) { From fd5fc52964b06f512764241dee5ae114a1b74f20 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:35:16 -0400 Subject: [PATCH 15/26] fix(mcpl): speech engages a decided-closed channel only where the host can open it Basil-1007's review of 7d859cb (room-203 #65264): a server without the channels.lifecycle grant can still hold a `closed` decision. handleToolClose records `closed` before its grant check, so on such a server channel_close returns the grant error with the decision recorded (as on main), and a server whose grant was withdrawn keeps its old decisions. Its transport can still be open: accepted inbound traffic marks the entry open, lifecycle grant or not. 7d859cb then sent speech into that locus through openChannelNow, which fails at the grant check, and blocked the speech ("locus channel is closed and open failed"), where main and a6130d0 deliver it. routeSpeech now engages a channel decided closed, with its transport still open, only where the server's grant includes channels.lifecycle. Elsewhere such a channel keeps publishing while its transport is open, as on main. A channel whose transport is closed still opens first, as before; that open can fail visibly on a grant-less server, as on main. Test: "speech into a channel decided closed keeps publishing where no open can succeed, without the lifecycle grant". The fixture gains setGrant. The channel opens under the full grant; the grant then loses channels.lifecycle; channel_close is refused for it with the decision recorded; speech is delivered, with no open attempted and no notice. Against 7d859cb's src it fails on `delivered` (undefined: the speech was blocked), and the file's other 42 pass. Locally (node 24.19) the two #210 files pass 53/53. --- .../210-channel-lifecycle-order.fixed.md | 2 +- src/mcpl/channel-registry.ts | 11 ++++++----- test/channel-lifecycle-order.test.ts | 19 ++++++++++++++++++- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 24d85ad31..eee7abf91 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,4 +1,4 @@ -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open, wherever the server's grant lets the host open it. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 4f62d9d6d..9aecdf63d 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -3194,11 +3194,12 @@ export class ChannelRegistry { // alive with the reply. If the open fails, the speech does NOT go out: // a half-alive delivery is worse than a loud marker. A channel decided // closed is engaged even while its transport is still open (a close that - // failed), as a reply would engage it. A channel with no decision at all - // (a server without the lifecycle grant records none) keeps publishing - // while its transport is open: speech opens before it sends, and that - // open could never succeed there. - if (!entry.open || this.getDesiredState(entry.serverId, entry.descriptor.id) === 'closed') { + // failed), as a reply would engage it, wherever an open can succeed. + // Without the lifecycle grant it can't, and speech opens before it + // sends, so there a channel whose transport is open keeps publishing. + const engagesDecidedClosed = this.getDesiredState(entry.serverId, entry.descriptor.id) === 'closed' && + CapabilityGrant.of(this.serverRegistry.getServer(entry.serverId)).has('channels.lifecycle'); + if (!entry.open || engagesDecidedClosed) { try { const { opened } = await this.openChannelNow(entry, 'opened-by-delivery'); entry = await this.waitForOpenChannel(entry); diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 17f4f5e7e..2bf48d2c6 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -53,6 +53,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { }; let currentServer = server; const replaceServer = () => { currentServer = { ...server }; }; + const setGrant = (paths: readonly string[]) => { currentServer.grant = new CapabilityGrant(new Set(paths), []); }; const traces: Array<{ type: string; [key: string]: unknown }> = []; const registry = new ChannelRegistry( { getServer: () => currentServer } as unknown as McplServerRegistry, @@ -74,7 +75,7 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { assert.equal(settled, true, 'lifecycle operations must settle'); await result; }; - return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures, replaceServer, answeredFor }; + return { registry, actual, calls, pending, held, traces, isOpen, tool, drain, publishedWhileOpen, notices, routeFailures, replaceServer, answeredFor, setGrant }; } test('remove/re-add during reconcile converges on the replacement desired state, with ACK first (#185)', async () => { @@ -682,6 +683,22 @@ for (const delivery of ['reply', 'speech'] as const) { }); } +test('speech into a channel decided closed keeps publishing where no open can succeed, without the lifecycle grant', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + assert.equal(f.isOpen(), true); + f.setGrant(ALL_CAPABILITY_PATHS.filter((path) => path !== 'channels.lifecycle')); + const closing = await f.tool('close'); + assert.equal(closing.success, false); + assert.match(String(closing.error), /channels\.lifecycle/); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed', 'the refused close is still recorded as a decision'); + f.calls.length = 0; + assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); + assert.deepEqual(f.publishedWhileOpen, [true]); + assert.deepEqual(f.calls, [], 'no open is attempted where it could not succeed'); + assert.equal(f.notices.length, 0); +}); + test('a failed explicit open says the channel stays selected for opening, and a later delivery opens it quietly', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor()] }); From 5633ca2a27df71d9b8403ac8e6fc4b318752da79 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:37:54 -0400 Subject: [PATCH 16/26] test(mcpl): state the open decision the in-flight-close test depends on Basil-1007's read of a6130d0 (room-203 #65271): 'an explicit open waits for a close in flight rather than answering already open' reaches channel_open's already-open shortcut only if the decision is already 'open' when the second open runs; otherwise the shortcut is skipped whatever its pending-work condition says, and the test would be blind. It holds because openChannelNow records the decision before its first await and the first open runs first. The test now asserts it. --- test/channel-lifecycle-order.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 2bf48d2c6..86cb11694 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -645,6 +645,7 @@ test('an explicit open waits for a close in flight rather than answering already const closing = f.tool('close'); await tick(); const reopening = f.tool('open'); + assert.equal(f.registry.getDesiredState('test', 'x'), 'open', 'the next open meets an open decision and a pending close'); let settled = false; const again = f.tool('open').then((result) => { settled = true; return result; }); await tick(); From f5402db2b091c059f04328ccf85860570553fa51 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:47:51 -0400 Subject: [PATCH 17/26] test(mcpl): pin five more lifecycle pieces the removal run found untested A second removal pass over pieces the first one hadn't named, at 5633ca2, left six with every test passing. Five do real work, and each now has a test that fails without it: - waitForOpenChannel swallowing a joined operation's ordinary failure: "speech joined to a failed backscroll still reaches a channel that stayed open". Without it, a failed redundant backscroll failed every delivery that had joined it, although the channel stayed open. - The reply path's tuned-out clause in its already-open answer: "a reply into a tuned-out channel whose transport is open needs no lifecycle RPC". Without it the reply sends a redundant open. - A failed reply open reporting the current registration's label: "a failed reply open reports the label of the registration that replaced it". Without it the result names the replaced registration's label. - reconcileChannels skipping a channel removed earlier in its batch: "a channel removed earlier in its batch does not hold up the rest of the batch". Without it the batch queues behind the removed channel's operation still in flight, then fails it as removed. - The admission loop reading only registered entries: "a descriptor the registration rejected is neither admitted nor announced". channels/ register reconciles every submitted descriptor, the rejected ones too. Without the check, a descriptor rejected for a missing id was admitted by an 'auto' policy and listed in its notice. The test registers twice, because the first registration migrates the legacy policy instead. The sixth, markAccepted clearing the unconfirmed flag, can't be reached: the flag is read only while the entry isn't open, and the entry leaves the open state again only through a receipt, which clears the flag, or an invalidation, which sets it. Locally (node 24.19) the two #210 files pass 58/58. --- test/channel-lifecycle-order.test.ts | 71 ++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 86cb11694..e916e73d5 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -787,3 +787,74 @@ test('a corrective close that cannot converge reports its exhaustion once', asyn while (f.pending.length > 0) f.pending.shift()!.finish(); } }); + +test('speech joined to a failed backscroll still reaches a channel that stayed open', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const backscroll = f.registry.handleChannelToolCall('channel_open', { serverId: 'test', channelId: 'x', backscroll: 5 }); + await tick(); + const speech = f.registry.routeSpeech('resident', 'joined', 'x'); + f.pending.shift()!.fail(); + assert.equal((await backscroll).success, false); + assert.equal((await speech)?.delivered, true, 'a failed redundant open leaves the channel usable for a joined delivery'); + assert.deepEqual(f.publishedWhileOpen, [true]); +}); + +test('a reply into a tuned-out channel whose transport is open needs no lifecycle RPC', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.registry.enterTuneOut('test', 'x', { + epochId: 'quiet', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + f.calls.length = 0; + assert.equal((await f.registry.openIfClosedForSend('x', 'test')).status, 'already-open'); + assert.deepEqual(f.calls, []); + assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'quiet'); +}); + +test('a failed reply open reports the label of the registration that replaced it', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.held.add('x'); + const reply = f.registry.openIfClosedForSend('x', 'test'); + await tick(); + const replacement = f.registry.handleChanged('test', { + removed: ['x'], added: [{ ...descriptor('x', false, 'replacement'), label: 'Replacement label' }], + }); + f.pending.shift()!.fail(); + const result = await reply; + assert.equal(result.status, 'open-failed'); + assert.equal(result.label, 'Replacement label'); + await f.drain(replacement); +}); + +test('a channel removed earlier in its batch does not hold up the rest of the batch', async () => { + const f = fixture(); + f.held.add('x'); + const first = f.registry.handleChanged('test', { added: [descriptor('x')] }); + await tick(); + assert.equal(f.pending.length, 1, "x's first operation is in flight"); + f.held.add('y'); + let batchSettled = false; + const batch = f.registry.handleChanged('test', { added: [descriptor('y'), descriptor('x')] }) + .then(() => { batchSettled = true; }); + await tick(); + assert.equal(f.pending.length, 2, "y's operation is in flight too"); + await f.registry.handleChanged('test', { removed: ['x'] }); + f.pending.pop()!.finish(); // y's receipt; x's first operation stays in flight + for (let i = 0; i < 5; i++) await tick(); + assert.equal(batchSettled, true, "the batch skips x rather than queueing behind x's operation in flight"); + await f.drain(first, batch); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0); +}); + +test('a descriptor the registration rejected is neither admitted nor announced', async () => { + const f = fixture(); + f.registry.setSubscriptionPolicy('test', 'auto'); + // The first registration migrates the legacy policy; the second is admitted by it. + await f.registry.handleRegister('test', { channels: [descriptor('x')] }); + await f.registry.handleRegister('test', { channels: [descriptor('y'), { ...descriptor('rejected'), id: '' }] }); + assert.deepEqual(f.notices.map((n) => n.channels.map((c) => c.channelId)), [['y']]); + assert.equal(f.registry.getDesiredState('test', ''), undefined); +}); From c67d06c798ea3ac23319802fb2d0c383aedc3c83 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:52:35 -0400 Subject: [PATCH 18/26] docs(changelog): an announced open is one the channel wasn't already decided for The fixed fragment said a send is announced only when it 'turned a channel decided closed open'. openChannelNow's rule is that the channel wasn't already decided open or tuned out, which includes a channel with no decision at all, such as a lazily registered DM. Say that. --- changelog.d/210-channel-lifecycle-order.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index eee7abf91..e5de6a05e 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,4 +1,4 @@ -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel only when it turned a channel decided closed open, using the current registration label; reconfirming the transport of a channel already decided open or tuned out isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open, wherever the server's grant lets the host open it. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel, with the current registration label, only when the channel wasn't already decided open or tuned out; reconfirming the transport of a channel that was isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open, wherever the server's grant lets the host open it. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. - Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. From 27fdd5bc302e719a512b638530e156ec824fbd22 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 20:59:54 -0400 Subject: [PATCH 19/26] docs(changelog): state the delivery-notice rule the same way in both fragments Basil-1007 (room-203 #65286): since c67d06c the fixed fragment states the rule as 'wasn't already decided open or tuned out', while the breaking fragment still said 'turned a closed channel open'. Not wrong, since a channel with no decision is closed by default, but one wording leaves a reader nothing to reconcile. --- changelog.d/210-policy-admission-notice.breaking.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/210-policy-admission-notice.breaking.md b/changelog.d/210-policy-admission-notice.breaking.md index a324c147a..eb209f3ff 100644 --- a/changelog.d/210-policy-admission-notice.breaking.md +++ b/changelog.d/210-policy-admission-notice.breaking.md @@ -1 +1 @@ -- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` reports a delivery that turned a closed channel open, once its transport is confirmed; a delivery that only reconfirms the transport of a channel already decided open or tuned out isn't reported. The built-in host notice now distinguishes selected-for-opening from currently open. +- `onChannelAutoOpened` with `source: 'subscription-policy'` now reports a durable admission decision before transport reconciliation. Treat it as intent; await the registration call and inspect `getOpenChannels()` when confirmed live state is needed. `opened-by-delivery` reports a delivery that opened a channel not already decided open or tuned out, once its transport is confirmed; a delivery that only reconfirms the transport of a channel that was isn't reported. The built-in host notice now distinguishes selected-for-opening from currently open. From 9e45ae1d6b415266dba34cbf73cf3821df0cf137 Mon Sep 17 00:00:00 2001 From: Basil-1007 Date: Sun, 11 Oct 2026 17:21:16 -0400 Subject: [PATCH 20/26] fix(mcpl): a channel's transport is known only from a receipt for its current target; removal cancels queued lifecycle work A review of the lifecycle ordering found ways the registry still claimed a transport state nothing had confirmed, queued work that outlived its server's registrations, and text that stated more than the code does. One rule, where the host directs a server's lifecycle (its channels.lifecycle grant): a channel's transport state is known only from a receipt for its current target. - Every entry a registration creates (channels/register, channels/changed added) starts unconfirmed, so channel_close's "already closed" shortcut can't answer for a channel nothing confirmed closed. - A failed open or close for the current target leaves the outcome unknown, since an RPC can fail after the server acted: the entry is marked unconfirmed with its open flag unchanged, so a confirmed-open channel stays usable after a failed backscroll (the mark is read only while not open). A failure from an older connection or target also withdraws the open flag, as before. - Incoming traffic confirms nothing unconfirmed: after a retarget it may still come from the old subscription. It marks a channel open only while the entry the message was accepted for is confirmed and decided open or tuned out, so a close receipt stands, whether it landed before the message or during the coalescer's await; an entry a re-registration put in its place meanwhile waits for its own evidence. The mark no longer needs deleting there. - Where the host doesn't direct a server's lifecycle, the server opens and closes its own channels and sends no receipt, so its traffic marks a channel open, as it always has. Lifecycle work: - Removing a server cancels its queued lifecycle operations: each records the server's removal count when queued and, when its turn comes after a removal, rejects with a cancellation error instead of acting on a closing connection or a later re-registration's entries. Reconciliation and the corrective close of a substituted channel treat that, and a channel removed while queued, as cancellation, not a failed reconcile. - disconnectMcplServer removes the channel entries before closing the connection. - A retarget through channels/changed of a channel decided open or tuned out opens the new target. When an operation is already queued or running for the channel, it reads the new target itself before confirming anything, so nothing more is queued; a per-channel count of such operations, dropped synchronously at each one's last look at its target, tells the two apart. - Declining an invitation goes through the lifecycle queue like any closed decision, unless a receipt confirmed the transport closed with nothing pending. A failed close is reported in the result (closeError); the decision stands. Nothing is sent to a server the host doesn't direct. - waitForOpenChannel stops when the tail it awaited is still current after settling, instead of awaiting it again without yielding. - A registration batch reconciles its channels side by side (Promise.allSettled): each channel has its own queue, so one held RPC no longer delays its siblings' opens. The substitution check: - A null or absent id in the answer, or in a history item, names no channel and is accepted as before. Another string is a substitution, as before. Any other type fails the open the same way, unconfirmed and traced, with no corrective close, since it names nothing registered. - Ids compare exactly, as the server's traffic does: every server-facing path keys channels by exact id, and handleIncoming refuses any other, so canonicalising only the open check would make it looser than the same server's traffic. The ChannelsOpenParams.channelId doc and the changelog now say so, instead of "any other channel". The doc also says what a server does without channelId (match by type and address) and that the host closes a channel opened in substitution when it's decided closed. - The mcpl:channel-open-substituted trace records the answered id in the same bounded form as the log line, and that form counts code points, not UTF-16 units. Text: - The delivery and reply notice says what happened ("your own message into the closed channel opened it") and states no rule about later sends; whether speaking opens a closed channel is still open with the maintainers. - The notice builder's doc says a policy admission is announced as the decision, before transport, and names the opened-by-reply cause. - The framework's engagement comment says a channel decided open whose transport is unconfirmed is opened again first, and its engagement recorded when that open returns. The open-on-send failure log says a later registration or delivery retries it, not that it "will reconcile". - The changelog states the one rule and its scope, when channel_open's "stays selected" note appears, the cancellation and side-by-side reconciling, the null and non-string ids, exact comparison, the trace's bounded id, and that either rollout order is safe. Tests (channel-lifecycle-order, channel-open-substitution): - a failed initial close leaves the channel unconfirmed, so channel_close sends a real close, and a re-registration's entry is unconfirmed too, so without the lifecycle grant channel_close is refused for the grant rather than answered "already closed"; - traffic after a retarget confirms nothing, and a failed open for the new target leaves it unconfirmed; after a failed open of the same target, traffic marks nothing open either; - traffic for a channel decided closed leaves its confirmed close standing, and so does a close that lands while the coalescer holds a message; - a retarget opens a channel decided open at its new target and sends nothing for one decided closed, whose close is then real; - removing a server cancels its queued work across a re-registration, a substitute removed while its corrective close is queued is cancelled, not traced as failed, and disconnectMcplServer has removed the channel entries by the time it closes the connection; - declining closes an unconfirmed transport for real, needs no RPC once closed is confirmed, and queues behind pending work; - a settled operation releases its queue key; - a batch's channels reconcile side by side; - a server the host doesn't direct still has its channels marked open by traffic, and a decline there sends nothing; - non-string answered ids, null ids, the trace's bounded id and code-point counting; - the notices' exact wording, including the opt-out; - the tune-out delivery test asserts the one status a reply returns and the RPCs sent, the lost-confirmation delivery tests make their retarget's open fail (a retarget now opens), and the removed-channel batch test checks that its sibling opened. mcpl-gate-roundtrip's minimal registry gains a server registry, which traffic now consults for the lifecycle grant. --- .../210-channel-lifecycle-order.fixed.md | 8 +- src/framework.ts | 37 ++- src/mcpl/channel-registry.ts | 235 +++++++++---- src/mcpl/types.ts | 9 +- test/channel-lifecycle-order.test.ts | 310 +++++++++++++++++- test/channel-open-substitution.test.ts | 31 ++ test/mcpl-gate-roundtrip.test.ts | 4 +- 7 files changed, 541 insertions(+), 93 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index e5de6a05e..9f3449d9a 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -1,9 +1,9 @@ -- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel, with the current registration label, only when the channel wasn't already decided open or tuned out; reconfirming the transport of a channel that was isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open, wherever the server's grant lets the host open it. A failed `channel_open` says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. +- Failed backscroll requests preserve the registry's known-open state rather than claiming a close. Automatic delivery joins pending lifecycle work without reopening a channel closed during the wait. A delivery or reply is announced as opening a channel, with the current registration label, only when the channel wasn't already decided open or tuned out; reconfirming the transport of a channel that was isn't announced. Speech engages a channel decided closed as a reply does, even while a failed close has left its transport open, wherever the server's grant lets the host open it. When `channel_open` fails for a channel that isn't open and is still decided open, its error says the channel stays selected for opening, since that decision outlives the failed round-trip and a later delivery or reconnect tries again. -- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. +- Channel lifecycle operations now run in order per channel and reconcile the latest desired state and registration after an in-flight operation completes. Successful reconciliation confirms the current target and desired state at its receipt boundary; a superseded tool request reports that its requested state no longer applies. Automatic delivery preserves active tune-out attention state while opening or awaiting transport. A registration batch reconciles its channels side by side, so one held RPC doesn't delay its siblings. Removing a server cancels its queued lifecycle operations, and the framework removes its channel entries before closing its connection, so nothing queued runs against a closing connection or a later re-registration. -- Open receipts confirm only the current registration, connection, type, and address. Retargeting invalidates an older open flag; failed corrective opens leave the new target unconfirmed and fail joined delivery, while same-target backscroll failures retain known-open state. Explicit close retries an unconfirmed target instead of reporting it already closed. +- A channel's transport state is known only from a receipt for its current registration, connection, type, and address. A channel a registration creates or replaces, a retarget, a substitution, or a failed open or close leaves it unconfirmed: failed corrective opens fail joined delivery, while a same-target backscroll failure keeps a channel that was open usable. Incoming traffic confirms nothing unconfirmed (after a retarget it may still come from the old subscription), and it marks a channel open only while that channel is decided open or tuned out, so a close receipt stands. Explicit close, and declining an invitation, close an unconfirmed target for real instead of reporting it already closed. A retarget of a channel decided open or tuned out opens the new target. - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. -- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel is accepted as before. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. +- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, compared exactly as the server's traffic is, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace, whose `answeredFor` is the answered id in the bounded form the log line prints. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel, by a null id or none, is accepted as before; one whose id isn't a string fails the open the same way, with nothing to close. Either rollout order is safe: on a server that still falls back, the resident sees a failed open for an id that server no longer knows (a deleted channel), rather than another channel's traffic. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. diff --git a/src/framework.ts b/src/framework.ts index d0e792061..43be0bf5c 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -8359,10 +8359,12 @@ export class AgentFramework { } /** - * Durable window notice for a channel the agent did not ask to open — - * subscription-policy admission, or an open forced by the agent's own - * delivery into a closed channel. The agent must always learn that new - * traffic will start flowing, and that `channel_close` opts out (their + * Durable window notice for a channel the agent did not ask to open: a + * subscription-policy admission, announced as the decision before its + * transport RPC (a failed open keeps the decision, and channel_list shows + * transport state), or an open that the agent's own delivery or reply + * caused (`opened-by-delivery`, `opened-by-reply`), once its transport is + * confirmed. Either way it names `channel_close` as the opt-out (their * decision outranks policy). System message: never triggers inference, * never conversational, never locus-eligible. */ @@ -8380,10 +8382,9 @@ export class AgentFramework { ? `[channels] Selected for opening by subscription policy: ${shown}. ` + 'This records the admission decision; channel_list shows current transport state. ' + 'When open, ' + (plural ? 'their' : 'its') + ' ongoing traffic will reach you. ' - : `[channels] Now open: ${shown} — your own message into the closed channel engaged it. ` + + : `[channels] Now open: ${shown}: your own message into the closed channel opened it. ` + 'Ongoing traffic from ' + (plural ? 'these channels' : 'this channel') + ' will reach you. ') + - `If you don't want ${plural ? 'one of them' : 'it'}, use channel_close; your choice outranks policy. ` + - 'A fresh message into a closed channel engages it again.'; + `If you don't want ${plural ? 'one of them' : 'it'}, use channel_close; your choice outranks policy.`; try { // Through framework.addMessage for mid-turn safety (deferral + // hear-while-acting injection) — a delivery-forced open happens inside @@ -13541,10 +13542,11 @@ export class AgentFramework { } /** - * Disconnect an MCPL server at runtime: close the connection, destroy its - * feature-set and checkpoint state (permanent removal — unlike a transient - * transport close, which preserves checkpoints for the reconnect), remove - * its channels from the registry, drop routing entries, and refresh tools. + * Disconnect an MCPL server at runtime: remove its channels from the + * registry (first, cancelling their queued lifecycle work), close the + * connection, destroy its feature-set and checkpoint state (permanent + * removal — unlike a transient transport close, which preserves checkpoints + * for the reconnect), drop routing entries, and refresh tools. * No-op-ish if the server is not connected (still clears routing state). */ async disconnectMcplServer(id: string): Promise { @@ -13554,8 +13556,10 @@ export class AgentFramework { const config = this.mcplServerConfigs.get(id); const oldToolNames = new Set(this.mcplTools.map(t => t.name)); - await this.mcplServerRegistry.removeServer(id); + // Channel entries go first: removing them cancels the server's queued + // lifecycle work, which must not run against the closing connection. this.channelRegistry?.removeServer(id); + await this.mcplServerRegistry.removeServer(id); // Permanent removal: also destroy feature-set and checkpoint state // explicitly. The 'close' handler usually does this, but a connection that @@ -14243,9 +14247,11 @@ export class AgentFramework { // Recorded here — not eagerly — because injection metadata // carries the CANONICAL channel id (descriptor form), while // send args often carry the raw provider id; this callback - // has the resolved form. The already-open path resolves on + // has the resolved form. A channel confirmed open resolves on // the microtask queue, i.e. before the tool-result event - // that evaluates injections. + // that evaluates injections. One decided open whose transport + // is unconfirmed is opened again first, so its engagement is + // recorded when that open returns. if (status !== 'unknown-channel' && status !== 'ambiguous') { let engaged = this.turnEngagedChannels.get(agentName); if (!engaged) { @@ -14265,7 +14271,8 @@ export class AgentFramework { ); } else if (status === 'open-failed') { console.error( - `[channel] ${agentName}: ${toolName} delivered to ${target} but the open-on-send failed — channel remains half-alive, will reconcile`, + `[channel] ${agentName}: ${toolName} delivered to ${target} but the open-on-send failed; ` + + 'a later registration or delivery tries the open again', ); } }) diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 9aecdf63d..c3b69e6f0 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -52,6 +52,8 @@ const MAX_CHANNEL_LIFECYCLE_ATTEMPTS = 5; class ChannelLifecycleConvergenceError extends Error {} /** A channels/open answered for a channel other than the one it named. */ class ChannelOpenSubstitutionError extends Error {} +/** An operation queued for a server's registrations that were removed. */ +class ChannelLifecycleCancelledError extends Error {} const CHANNEL_LIFECYCLE_LOG_ID = 'mcpl/channel-lifecycle'; /** @@ -62,23 +64,32 @@ const CHANNEL_LIFECYCLE_LOG_ID = 'mcpl/channel-lifecycle'; function plainIdForLog(id: unknown): string { if (typeof id === 'string' && /^[\x21-\x7e]{1,200}$/.test(id)) return id; return typeof id === 'string' - ? `(not a plain id: a ${id.length}-character string)` + ? `(not a plain id: a ${[...id].length}-character string)` : `(not a plain id: ${id === null ? 'null' : typeof id})`; } /** - * The other channel a channels/open answer names, if it names one: its - * `channel.id`, or else the `channelId` of a history item. Each is evidence of - * which channel the server opened. An answer that names no channel at all - * can't be checked, and gives undefined. + * What a channels/open answer says about which channel the server opened: + * its `channel.id`, or else the `channelId` of a history item. Each is + * evidence. A string other than the requested id names another channel (ids + * compare exactly, as the server's traffic is matched). Null or absence names + * no channel, and gives undefined, as does an answer that names none at all. + * Any other value is malformed: it names nothing the host could register. */ -function substitutedChannel(result: ChannelsOpenResult | undefined, requested: string): unknown { - const answered = result?.channel?.id; - if (answered !== undefined && answered !== requested) return answered; +function substitutedChannel( + result: ChannelsOpenResult | undefined, + requested: string, +): { kind: 'other'; id: string } | { kind: 'malformed'; value: unknown } | undefined { + const named = (value: unknown) => { + if (value === undefined || value === null || value === requested) return undefined; + return typeof value === 'string' ? { kind: 'other' as const, id: value } : { kind: 'malformed' as const, value }; + }; + const answered = named(result?.channel?.id); + if (answered) return answered; const history: unknown[] = Array.isArray(result?.history) ? result.history : []; for (const item of history) { - const from = (item as { channelId?: unknown } | null)?.channelId; - if (from !== undefined && from !== requested) return from; + const from = named((item as { channelId?: unknown } | null)?.channelId); + if (from) return from; } return undefined; } @@ -703,6 +714,14 @@ export class ChannelRegistry { // across descriptor replacement. Settled tails release themselves; a // failed operation must not poison later decisions. private channelLifecycleTails = new Map>(); + // Bumped when a server's registrations are removed: an operation queued + // before that is cancelled when it would run, rather than acting on a + // closing connection or on a later re-registration's entries. + private serverRemovals = new Map(); + // Operations queued or running per channel that will still read its + // target before confirming anything. Unlike the tail, which settles a few + // microtasks after its operation's last check, this drops at that check. + private lifecycleOutstanding = new Map(); // Losing an open receipt does not confirm closure. Keep that distinction // private while preserving the public open:boolean availability projection. private unconfirmedChannelTargets = new WeakSet(); @@ -712,6 +731,21 @@ export class ChannelRegistry { this.unconfirmedChannelTargets.add(entry); } + /** Whether the host directs this server's channel lifecycle (§14.1: its + * `channels.lifecycle` grant). Where it doesn't, the server opens and + * closes its own channels, and sends no receipt the host could wait for. */ + private directsLifecycle(serverId: string): boolean { + return CapabilityGrant.of(this.serverRegistry.getServer(serverId)).has('channels.lifecycle'); + } + + /** A registration's entry: nothing about its transport is known until a + * receipt for its target confirms it open or closed. */ + private registerEntry(key: string, serverId: string, descriptor: ChannelDescriptor): void { + const entry: ChannelEntry = { serverId, descriptor, open: false }; + this.channels.set(key, entry); + this.unconfirmedChannelTargets.add(entry); + } + private hasPendingLifecycle(entry: ChannelEntry): boolean { return this.channelLifecycleTails.has(this.lifecycleKey(entry.serverId, entry.descriptor.id)); } @@ -785,11 +819,7 @@ export class ChannelRegistry { continue; } const key = `${serverId}:${channel.id}`; - this.channels.set(key, { - serverId, - descriptor: channel, - open: false, - }); + this.registerEntry(key, serverId, channel); this.appendLabelSighting(channel.id, channel.label, extractDmMeta(channel.metadata)); registeredIds.push(channel.id); results.push({ id: channel.id, accepted: true }); @@ -830,6 +860,7 @@ export class ChannelRegistry { // channels/register — a changed-set cannot smuggle in what register // would have rejected (PR #79 review blocker 5). const addedResults: Array<{ id: string; accepted: boolean; reason?: string }> = []; + const retargeted: ChannelDescriptor[] = []; // Process removed channels — itemized truthfully: removal of a channel // we never had is reported, not silently accepted. if (params.removed) { @@ -860,8 +891,19 @@ export class ChannelRegistry { continue; } // A live subscription to the old target does not confirm the new - // type/address. Label-only updates preserve that confirmation. - if (!sameChannelTarget(existing.descriptor, channel)) this.invalidateTransportConfirmation(existing); + // type/address. Label-only updates preserve that confirmation. A + // channel decided open (or tuned out) is opened at its new target. + if (!sameChannelTarget(existing.descriptor, channel)) { + this.invalidateTransportConfirmation(existing); + // An operation queued or running for this channel reads its target + // again before it confirms anything, so it opens the new one. With + // none, a channel decided open (or tuned out) is reconciled here. + const desired = this.getDesiredState(serverId, channel.id); + if ((desired === 'open' || desired === 'tuned-out') && + !this.lifecycleOutstanding.has(this.lifecycleKey(serverId, channel.id))) { + retargeted.push(channel); + } + } existing.descriptor = channel; this.appendLabelSighting(channel.id, channel.label, extractDmMeta(channel.metadata)); addedResults.push({ id: channel.id, accepted: true }); @@ -879,11 +921,7 @@ export class ChannelRegistry { continue; } const key = `${serverId}:${channel.id}`; - this.channels.set(key, { - serverId, - descriptor: channel, - open: false, - }); + this.registerEntry(key, serverId, channel); this.appendLabelSighting(channel.id, channel.label, extractDmMeta(channel.metadata)); addedResults.push({ id: channel.id, accepted: true }); accepted.push(channel); @@ -898,7 +936,8 @@ export class ChannelRegistry { // settled above, so nothing in the response depends on reconciling. responder?.respond({ results: addedResults }); - if (accepted.length > 0) await this.reconcileChannels(serverId, accepted); + const reconcile = [...accepted, ...retargeted]; + if (reconcile.length > 0) await this.reconcileChannels(serverId, reconcile); this.emitTraceFn({ type: 'mcpl:channels-changed', @@ -996,17 +1035,31 @@ export class ChannelRegistry { // exactly the authority a self-attested channel would be stealing). // A coalesced item is "accepted" only once the coalescer admits it, so // for those this runs after the hook (below). + const incomingEntry = this.channels.get(incomingKey)!; const markAccepted = () => { this.defaultPublishChannel = message.channelId; this.defaultPublishMessageId = message.messageId; this.defaultPublishThreadId = message.threadId; - // A server sending channels/incoming is authoritative evidence that - // the transport is actually open. This repairs transient status only; - // durable desired state still changes exclusively through lifecycle - // operations. - const incomingEntry = this.channels.get(incomingKey)!; + // Where the host directs this server's lifecycle, only a receipt for + // the current target confirms it: traffic shows the transport open + // only for a confirmed target, and only for a channel still decided + // open or tuned out. After a retarget, traffic for this id may still + // come from the old subscription, and a close receipt that landed + // first (or during the coalescer's await) stands. Where the host + // doesn't direct it, the server opens its own channels and its + // traffic is the only evidence the host gets, so it marks the + // channel open, as it always has. The entry is the one the message + // was accepted for: one a re-registration put in its place meanwhile + // waits for its own evidence. Durable desired state still changes + // exclusively through lifecycle operations. + if (this.directsLifecycle(serverId)) { + if (this.unconfirmedChannelTargets.has(incomingEntry)) return; + const desired = this.getDesiredState(serverId, message.channelId); + if (desired !== 'open' && desired !== 'tuned-out') return; + } else { + this.unconfirmedChannelTargets.delete(incomingEntry); + } incomingEntry.open = true; - this.unconfirmedChannelTargets.delete(incomingEntry); }; if (!coalesced) markAccepted(); @@ -1454,6 +1507,10 @@ export class ChannelRegistry { * server's channels don't linger and route speech into the void. */ removeServer(serverId: string): void { + // Every lifecycle operation queued for this server is cancelled when it + // would run (ChannelLifecycleCancelledError), so none acts on a closing + // connection or on a later re-registration's entries. + this.serverRemovals.set(serverId, (this.serverRemovals.get(serverId) ?? 0) + 1); for (const [key, entry] of this.channels) { if (entry.serverId !== serverId) continue; this.channels.delete(key); @@ -2140,11 +2197,42 @@ export class ChannelRegistry { options?: { history?: ChannelHistoryRequest }, ): Promise<{ open: boolean; result?: ChannelsOpenResult }> { const lifecycleKey = this.lifecycleKey(serverId, channelId); - const channelKey = `${serverId}:${channelId}`; + const removals = this.serverRemovals.get(serverId) ?? 0; const previous = this.channelLifecycleTails.get(lifecycleKey) ?? Promise.resolve(); - const operation = previous.catch(() => {}).then(async () => { + this.lifecycleOutstanding.set(lifecycleKey, (this.lifecycleOutstanding.get(lifecycleKey) ?? 0) + 1); + const operation = previous.catch(() => {}).then( + () => this.runLifecycleOperation(serverId, channelId, removals, options), + ); + // Keep the rejection visible to existing joiners, while observing it + // immediately and letting each queued successor start after either result. + const tail = operation.then(() => {}); + this.channelLifecycleTails.set(lifecycleKey, tail); + const cleanup = () => { + if (this.channelLifecycleTails.get(lifecycleKey) === tail) { + this.channelLifecycleTails.delete(lifecycleKey); + } + }; + void tail.then(cleanup, cleanup); + return operation; + } + + /** One queued lifecycle operation, run when its turn in the queue comes. */ + private async runLifecycleOperation( + serverId: string, + channelId: string, + removals: number, + options?: { history?: ChannelHistoryRequest }, + ): Promise<{ open: boolean; result?: ChannelsOpenResult }> { + const channelKey = `${serverId}:${channelId}`; + const lifecycleKey = this.lifecycleKey(serverId, channelId); + try { let attempts = 0; for (;;) { + if ((this.serverRemovals.get(serverId) ?? 0) !== removals) { + throw new ChannelLifecycleCancelledError( + `Channel ${channelId}: its server's registrations were removed while this operation was queued`, + ); + } const entry = this.channels.get(channelKey); if (!entry) throw new Error(`Channel is no longer registered: ${channelId}`); const server = this.serverRegistry.getServer(serverId); @@ -2185,9 +2273,16 @@ export class ChannelRegistry { await server.sendChannelsClose({ channelId }); } } catch (error) { - // A failed same-target backscroll can retain its known-open state. - // A receipt/failure from an older connection or target cannot. - if (this.channels.get(channelKey) === entry && !sameTarget()) this.invalidateTransportConfirmation(entry); + // An RPC can fail after the server acted, so a failure for the + // current target leaves its outcome unknown: it is marked + // unconfirmed with open unchanged, which keeps a confirmed-open + // channel usable after a failed backscroll (the mark is read only + // while not open). A failure from an older connection or target + // also withdraws the open flag. + if (this.channels.get(channelKey) === entry) { + if (sameTarget()) this.unconfirmedChannelTargets.add(entry); + else this.invalidateTransportConfirmation(entry); + } throw error; } // A server can answer for a channel other than the one named: one @@ -2197,11 +2292,14 @@ export class ChannelRegistry { // open it made if the host wants that channel closed. const substitute = open ? substitutedChannel(result, channelId) : undefined; if (substitute !== undefined) { + const answered = plainIdForLog(substitute.kind === 'other' ? substitute.id : substitute.value); console.error( - `[channel-open-substituted] server=${serverId} requested=${plainIdForLog(channelId)} answered=${plainIdForLog(substitute)}`, + `[channel-open-substituted] server=${serverId} requested=${plainIdForLog(channelId)} answered=${answered}`, ); - this.emitTraceFn({ type: 'mcpl:channel-open-substituted', serverId, channelId, answeredFor: substitute }); - if (typeof substitute === 'string') this.closeSubstitutedChannel(serverId, substitute); + this.emitTraceFn({ type: 'mcpl:channel-open-substituted', serverId, channelId, answeredFor: answered }); + // A malformed id names nothing registered here, so there is + // nothing to close. + if (substitute.kind === 'other') this.closeSubstitutedChannel(serverId, substitute.id); } // Never apply an old receipt to the current target, even transiently: // a corrective open might fail before that stale bit is replaced. @@ -2218,8 +2316,9 @@ export class ChannelRegistry { if (substitute !== undefined) { this.invalidateTransportConfirmation(entry); throw new ChannelOpenSubstitutionError( - `Server "${serverId}" answered channels/open for ${channelId} with a different channel; ` + - `${channelId} is not marked open, and nothing in the answer is used`, + `Server "${serverId}" answered channels/open for ${channelId} with ` + + (substitute.kind === 'other' ? 'a different channel' : 'a channel id that is not a string') + + `; ${channelId} is not marked open, and nothing in the answer is used`, ); } entry.open = open; @@ -2228,18 +2327,13 @@ export class ChannelRegistry { if (open !== (now === 'open' || now === 'tuned-out')) continue; return { open, result }; } - }); - // Keep the rejection visible to existing joiners, while observing it - // immediately and letting each queued successor start after either result. - const tail = operation.then(() => {}); - this.channelLifecycleTails.set(lifecycleKey, tail); - const cleanup = () => { - if (this.channelLifecycleTails.get(lifecycleKey) === tail) { - this.channelLifecycleTails.delete(lifecycleKey); - } - }; - void tail.then(cleanup, cleanup); - return operation; + } finally { + // Synchronous with the operation's last look at its target: from here + // on, nothing it does will open a new target. + const left = (this.lifecycleOutstanding.get(lifecycleKey) ?? 1) - 1; + if (left > 0) this.lifecycleOutstanding.set(lifecycleKey, left); + else this.lifecycleOutstanding.delete(lifecycleKey); + } } /** @@ -2255,7 +2349,11 @@ export class ChannelRegistry { if (!this.channels.has(`${serverId}:${channelId}`)) return; if (this.getDesiredState(serverId, channelId) !== 'closed') return; this.applyDesiredChannelState(serverId, channelId).catch((err) => { - if (err instanceof ChannelLifecycleConvergenceError) return; + // Removal while queued is cancellation, as in reconcileChannels, and + // exhausted convergence emitted its own diagnostic. + if (!this.channels.has(`${serverId}:${channelId}`) || + err instanceof ChannelLifecycleCancelledError || + err instanceof ChannelLifecycleConvergenceError) return; this.emitTraceFn({ type: 'mcpl:channel-reconcile-failed', serverId, channelId, desired: 'closed', error: (err as Error).message, @@ -2282,6 +2380,10 @@ export class ChannelRegistry { // the current target, so joined delivery must fail visibly. if (error instanceof ChannelLifecycleConvergenceError) throw error; } + // A settled tail releases its own key. One still current after it + // settled would be awaited again without ever yielding to the event + // loop, so stop waiting rather than spin. + if (this.channelLifecycleTails.get(lifecycleKey) === pending) break; } const current = this.channels.get(`${entry.serverId}:${entry.descriptor.id}`); if (!current) throw new Error('Channel registration was removed while delivery waited'); @@ -2340,8 +2442,10 @@ export class ChannelRegistry { } } - for (const channel of channels) { - if (!this.channels.has(`${serverId}:${channel.id}`)) continue; + // Each channel has its own lifecycle queue, so a held RPC for one (up to + // its timeout, times its attempts) doesn't delay its siblings. + await Promise.allSettled(channels.map(async (channel) => { + if (!this.channels.has(`${serverId}:${channel.id}`)) return; try { await this.applyDesiredChannelState(serverId, channel.id); } catch (err) { @@ -2349,8 +2453,9 @@ export class ChannelRegistry { // Exhausted convergence and a substituted answer already emitted // their own diagnostics. if (!this.channels.has(`${serverId}:${channel.id}`) || + err instanceof ChannelLifecycleCancelledError || err instanceof ChannelLifecycleConvergenceError || - err instanceof ChannelOpenSubstitutionError) continue; + err instanceof ChannelOpenSubstitutionError) return; this.emitTraceFn({ type: 'mcpl:channel-reconcile-failed', serverId, @@ -2359,7 +2464,7 @@ export class ChannelRegistry { error: (err as Error).message, }); } - } + })); } // ========================================================================== @@ -2960,6 +3065,23 @@ export class ChannelRegistry { } } + // Where the host directs the lifecycle, a decline goes through the + // lifecycle queue like any closed decision, unless a receipt confirmed + // the transport closed with nothing pending: an unconfirmed transport may + // be open, and pending work must settle on this decision. A failed close + // is reported; the decision stands. + let closeError: string | undefined; + const current = this.channels.get(`${entry.serverId}:${input.channelId}`); + if (current && this.directsLifecycle(entry.serverId) && + (this.unconfirmedChannelTargets.has(current) || this.hasPendingLifecycle(current))) { + try { + const applied = await this.applyDesiredChannelState(entry.serverId, input.channelId); + if (applied.open) closeError = 'the close was superseded by a newer lifecycle decision'; + } catch (error) { + closeError = (error as Error).message; + } + } + this.appendLifecycleEvent({ kind: 'invitation-declined', serverId: entry.serverId, @@ -2976,6 +3098,7 @@ export class ChannelRegistry { acknowledged, ...(representation ? { representation } : {}), ...(acknowledgmentError ? { acknowledgmentError } : {}), + ...(closeError ? { closeError: `Failed to close the channel's transport: ${closeError}` } : {}), }, }; } diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index d6c9e21cd..f0eecf44a 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -1049,10 +1049,13 @@ export interface ChannelsListResult { */ export interface ChannelsOpenParams { /** - * The exact registered id to open. A server that doesn't know it should + * The exact registered id to open. Without it, a server matches by `type` + * and `address`, as before. A server that doesn't know the id should * refuse rather than open another channel: the host fails an open whose - * answer names any other channel, by its `channel.id` or a history item's - * `channelId`. + * answer names a different channel id, by its `channel.id` or a history + * item's `channelId`, compared exactly, as the server's traffic is. When + * that other channel is registered and decided closed, the host closes it + * again. */ channelId?: string; type: string; diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index e916e73d5..a787b74b3 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -10,16 +10,27 @@ import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; -import type { ChannelDescriptor, ChannelsOpenResult } from '../src/mcpl/types.js'; +import type { ChannelDescriptor, ChannelIncomingMessage, ChannelsOpenResult } from '../src/mcpl/types.js'; const tick = () => new Promise((resolve) => setImmediate(resolve)); function descriptor(id = 'x', initiallyOpen = false, address = 'original'): ChannelDescriptor { return { id, type: 'test', label: id, direction: 'bidirectional', initiallyOpen, address: { value: address } }; } -type NoticeHandler = NonNullable[4]>['onChannelAutoOpened']>; +type RegistryOptions = NonNullable[4]>; +type NoticeHandler = NonNullable; -function fixture(onNotice?: NoticeHandler, store?: JsStore) { +/** A tool result's data, as the channel tools shape it. */ +const dataOf = (result: { data?: unknown }) => (result.data ?? {}) as { status?: string; closeError?: string }; + +function message(channelId = 'x', messageId = 'm1'): ChannelIncomingMessage { + return { + channelId, messageId, author: { id: 'u1', name: 'someone' }, + timestamp: '2026-10-11T00:00:00Z', content: [{ type: 'text', text: 'hello' }], + }; +} + +function fixture(onNotice?: NoticeHandler, store?: JsStore, coalesce?: RegistryOptions['handleCoalescedIncoming']) { const actual = new Map(); const calls: Array<{ kind: 'open' | 'close'; id: string; address?: unknown }> = []; const pending: Array<{ finish: () => void; fail: () => void }> = []; @@ -59,7 +70,8 @@ function fixture(onNotice?: NoticeHandler, store?: JsStore) { { getServer: () => currentServer } as unknown as McplServerRegistry, {} as FeatureSetManager, () => {}, (event) => traces.push(event), { store, onRouteFailure: (info) => routeFailures.push(info), - onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); } }, + onChannelAutoOpened: (info) => { notices.push(info); onNotice?.(info); }, + ...(coalesce ? { handleCoalescedIncoming: coalesce } : {}) }, ); const isOpen = (id = 'x') => registry.getOpenChannels().some((e) => e.descriptor.id === id); const tool = (kind: 'open' | 'close') => registry.handleChannelToolCall('channel_' + kind, { serverId: 'test', channelId: 'x' }); @@ -213,17 +225,22 @@ for (const delivery of ['speech', 'reply'] as const) { // turn a delivery into a decision to cancel attention diversion. const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; f.held.add('x'); const close = f.tool('close'); await tick(); f.registry.enterTuneOut('test', 'x', { epochId: 'new-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, }, 'agent-tool'); + // The close in flight converges on the tune-out by opening again, and the + // delivery joins it: the channel was already decided open, so a reply + // reports it already open, and nothing sends a third RPC. const sent = delivery === 'speech' ? f.registry.routeSpeech('resident', 'hello', 'x').then((result) => result?.delivered === true) - : f.registry.openIfClosedForSend('x', 'test').then((result) => result.status === 'opened' || result.status === 'already-open'); + : f.registry.openIfClosedForSend('x', 'test').then((result) => result.status === 'already-open'); await f.drain(close, sent); assert.equal(await sent, true); + assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'open']); assert.equal(f.registry.getTuneOutState('test', 'x')?.params.epochId, 'new-epoch'); assert.equal(f.actual.get('x'), true); if (delivery === 'speech') assert.deepEqual(f.publishedWhileOpen, [true]); @@ -723,8 +740,15 @@ for (const delivery of ['speech', 'reply'] as const) { test(delivery + ' into a channel decided open whose transport lost confirmation is not announced as opening it', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); - await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); - assert.equal(f.isOpen(), false, 'retargeting leaves the new target unconfirmed'); + // A retarget opens the new target; that open fails, so the channel stays + // decided open with its transport unconfirmed. + f.held.add('x'); + const retarget = f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + await tick(); + f.pending.shift()!.fail(); + await retarget; + f.held.delete('x'); + assert.equal(f.isOpen(), false, 'a failed open leaves the new target unconfirmed'); f.calls.length = 0; if (delivery === 'speech') { assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); @@ -744,7 +768,12 @@ for (const delivery of ['speech', 'reply'] as const) { f.registry.enterTuneOut('test', 'x', { epochId: 'kept-epoch', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, }, 'agent-tool'); - await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + f.held.add('x'); + const retarget = f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + await tick(); + f.pending.shift()!.fail(); + await retarget; + f.held.delete('x'); assert.equal(f.isOpen(), false); if (delivery === 'speech') { assert.equal((await f.registry.routeSpeech('resident', 'hello', 'x'))?.delivered, true); @@ -829,24 +858,34 @@ test('a failed reply open reports the label of the registration that replaced it await f.drain(replacement); }); -test('a channel removed earlier in its batch does not hold up the rest of the batch', async () => { +test('a channel removed from its batch holds up none of its siblings, and fails nothing', async () => { const f = fixture(); f.held.add('x'); const first = f.registry.handleChanged('test', { added: [descriptor('x')] }); await tick(); assert.equal(f.pending.length, 1, "x's first operation is in flight"); f.held.add('y'); - let batchSettled = false; - const batch = f.registry.handleChanged('test', { added: [descriptor('y'), descriptor('x')] }) - .then(() => { batchSettled = true; }); + const batch = f.registry.handleChanged('test', { added: [descriptor('y', true), descriptor('x')] }); await tick(); assert.equal(f.pending.length, 2, "y's operation is in flight too"); await f.registry.handleChanged('test', { removed: ['x'] }); f.pending.pop()!.finish(); // y's receipt; x's first operation stays in flight for (let i = 0; i < 5; i++) await tick(); - assert.equal(batchSettled, true, "the batch skips x rather than queueing behind x's operation in flight"); + assert.equal(f.isOpen('y'), true, "y opened while x's operation was still in flight"); await f.drain(first, batch); - assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0, + 'the removed channel is cancelled, not failed'); +}); + +test("a batch's channels reconcile side by side: one held RPC delays none of its siblings", async () => { + const f = fixture(); + f.held.add('x'); + const batch = f.registry.handleChanged('test', { added: [descriptor('x', true), descriptor('y', true)] }); + for (let i = 0; i < 5; i++) await tick(); + assert.equal(f.isOpen('y'), true, "y's open didn't wait for x's"); + assert.equal(f.isOpen('x'), false); + await f.drain(batch); + assert.equal(f.isOpen('x'), true); }); test('a descriptor the registration rejected is neither admitted nor announced', async () => { @@ -858,3 +897,246 @@ test('a descriptor the registration rejected is neither admitted nor announced', assert.deepEqual(f.notices.map((n) => n.channels.map((c) => c.channelId)), [['y']]); assert.equal(f.registry.getDesiredState('test', ''), undefined); }); + +test('a failed initial close leaves the channel unconfirmed, so channel_close closes it for real', async () => { + const f = fixture(); + f.held.add('x'); + const registration = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + f.pending.shift()!.fail(); + await registration; + f.held.delete('x'); + f.calls.length = 0; + assert.equal(dataOf(await f.tool('close')).status, 'closed'); + assert.deepEqual(f.calls.map((c) => c.kind), ['close'], 'a failed close may have closed nothing'); + assert.equal(dataOf(await f.tool('close')).status, 'already closed', 'its receipt confirms the close'); + assert.equal(f.calls.length, 1); +}); + +test("a re-registration's entry is unconfirmed until a receipt, so channel_close never answers already closed for it", async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + assert.equal(dataOf(await f.tool('close')).status, 'already closed', 'its initial close was confirmed'); + // The server registers again, without the lifecycle grant this time: the + // host sends nothing, and nothing confirms the new entry closed. + f.setGrant(ALL_CAPABILITY_PATHS.filter((path) => path !== 'channels.lifecycle')); + f.calls.length = 0; + await f.registry.handleRegister('test', { channels: [descriptor()] }); + assert.equal(f.calls.length, 0); + const result = await f.tool('close'); + assert.equal(result.success, false); + assert.match(String(result.error), /channels\.lifecycle not in/, 'refused for the grant, not answered already closed'); +}); + +test('traffic after a retarget confirms nothing: only a receipt for the new target does', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.held.add('x'); + const retarget = f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + await tick(); + await f.registry.handleIncoming('test', { messages: [message()] }); + assert.equal(f.isOpen(), false, "the old subscription's traffic doesn't confirm the new target"); + f.pending.shift()!.fail(); + await retarget; + assert.equal(f.isOpen(), false, 'and a failed open for the new target leaves it unconfirmed'); +}); + +test('traffic for a channel decided closed leaves its confirmed close standing', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + assert.equal((await f.tool('close')).success, true); + await f.registry.handleIncoming('test', { messages: [message()] }); + assert.equal(f.isOpen(), false); +}); + +test('a close that lands while the coalescer holds a message stands', async () => { + let admit!: () => void; + const f = fixture(undefined, undefined, async (_serverId, held) => { + await new Promise((resolve) => { admit = resolve; }); + return { messageId: held.messageId, accepted: true }; + }); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + const incoming = f.registry.handleIncoming('test', { messages: [{ ...message(), coalesce: { key: 'subject' } }] }); + await tick(); + assert.equal((await f.tool('close')).success, true); + admit(); + await incoming; + assert.equal(f.isOpen(), false, 'the close receipt stands'); +}); + +test('a retarget of a channel decided open opens the new target; one decided closed sends nothing', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A'), descriptor('y', false, 'A')] }); + f.calls.length = 0; + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B'), descriptor('y', false, 'B')] }); + assert.deepEqual(f.calls.map((c) => [c.kind, c.id, c.address]), [['open', 'x', { value: 'B' }]]); + assert.equal(f.isOpen('x'), true); + const closeY = await f.registry.handleChannelToolCall('channel_close', { serverId: 'test', channelId: 'y' }); + assert.equal(dataOf(closeY).status, 'closed', "y's new target is unconfirmed, so its close is sent"); + assert.deepEqual(f.calls.at(-1), { kind: 'close', id: 'y', address: undefined }); +}); + +test("removing a server cancels its queued lifecycle work, even across a re-registration", async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.calls.length = 0; + f.held.add('x'); + const closing = f.tool('close'); + await tick(); + const reopening = f.tool('open'); + f.registry.removeServer('test'); + const reregistration = f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + await f.drain(closing, reopening, reregistration); + assert.equal((await reopening).success, false, 'the open queued before the removal never runs'); + assert.match(String((await reopening).error), /removed while this operation was queued/); + assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'open'], "the close in flight, then the re-registration's own open"); + assert.equal(f.isOpen(), true); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0); +}); + +test('a substitute removed while its corrective close is queued is cancelled, not traced as failed', async () => { + const f = fixture(); + f.held.add('y'); + const registration = f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + await tick(); + assert.equal(f.pending.length, 1, "y's initial close is in flight"); + f.answeredFor.set('x', 'y'); + const opening = f.tool('open'); + for (let i = 0; i < 3; i++) await tick(); + await f.registry.handleChanged('test', { removed: ['y'] }); + await f.drain(registration, opening); + assert.equal((await opening).success, false); + assert.equal(f.traces.filter((e) => e.type === 'mcpl:channel-reconcile-failed').length, 0); +}); + +test('declining an invitation closes an unconfirmed transport for real, and waits for pending work', async () => { + const f = fixture(); + const decline = () => f.registry.handleChannelToolCall('channel_decline', { serverId: 'test', channelId: 'x', messageId: 'm1' }); + f.held.add('x'); + const registration = f.registry.handleChanged('test', { added: [descriptor()] }); + await tick(); + f.pending.shift()!.fail(); + await registration; + f.held.delete('x'); + f.calls.length = 0; + const unconfirmed = await decline(); + assert.equal(unconfirmed.success, true); + assert.equal(dataOf(unconfirmed).status, 'remained closed'); + assert.equal(dataOf(unconfirmed).closeError, undefined); + assert.deepEqual(f.calls.map((c) => c.kind), ['close'], 'the unconfirmed transport is closed for real'); + f.calls.length = 0; + assert.equal((await decline()).success, true); + assert.equal(f.calls.length, 0, 'a confirmed close with nothing pending needs no RPC'); + + // A re-registration's close in flight: the decline queues behind it. + f.held.add('x'); + const reregistration = f.registry.handleChanged('test', { removed: ['x'], added: [descriptor()] }); + await tick(); + assert.equal(f.pending.length, 1); + const pending = decline(); + await f.drain(reregistration, pending); + assert.equal((await pending).success, true); + assert.deepEqual(f.calls.map((c) => c.kind), ['close', 'close'], 'the decline went through the queue behind the close'); +}); + +test("a settled operation releases its channel's queue key", async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + const internals = f.registry as unknown as { channelLifecycleTails: Map; lifecycleOutstanding: Map }; + for (let i = 0; i < 3; i++) await tick(); + assert.equal(internals.channelLifecycleTails.size, 0); + assert.equal(internals.lifecycleOutstanding.size, 0); +}); + +test('the notices say what happened, name the opt-out, and state no rule about later sends', async () => { + const dir = mkdtempSync(join(tmpdir(), 'channel-notice-text-')); + const framework = await AgentFramework.create({ + storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'resident', model: 'test-model', systemPrompt: 'Test.' }], + modules: [], syncIntervalMs: 0, + }); + try { + const recordNotice = (framework as unknown as { + recordChannelAutoOpenNotice( + agent: undefined, + channels: Array<{ channelId: string; label?: string }>, + cause: 'subscription-policy' | 'opened-by-delivery' | 'opened-by-reply', + ): void; + }).recordChannelAutoOpenNotice.bind(framework); + recordNotice(undefined, [{ channelId: 'discord:g1:c1', label: '#general' }], 'opened-by-reply'); + recordNotice(undefined, [{ channelId: 'c2' }], 'opened-by-delivery'); + recordNotice(undefined, [{ channelId: 'c3' }, { channelId: 'c4', label: 'ops' }], 'subscription-policy'); + const texts = framework.getAgent('resident')!.getContextManager().getAllMessages() + .flatMap((m) => m.content) + .filter((b) => b.type === 'text' && b.text.startsWith('[channels]')) + .map((b) => (b as { text: string }).text); + assert.deepEqual(texts, [ + "[channels] Now open: #general (discord:g1:c1): your own message into the closed channel opened it. " + + "Ongoing traffic from this channel will reach you. If you don't want it, use channel_close; your choice outranks policy.", + "[channels] Now open: c2: your own message into the closed channel opened it. " + + "Ongoing traffic from this channel will reach you. If you don't want it, use channel_close; your choice outranks policy.", + '[channels] Selected for opening by subscription policy: c3, ops (c4). ' + + 'This records the admission decision; channel_list shows current transport state. ' + + "When open, their ongoing traffic will reach you. If you don't want one of them, use channel_close; your choice outranks policy.", + ]); + } finally { + await framework.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("a server the host doesn't direct opens its own channels: its traffic marks them open, and a decline sends nothing", async () => { + const f = fixture(); + f.setGrant(ALL_CAPABILITY_PATHS.filter((path) => path !== 'channels.lifecycle')); + await f.registry.handleChanged('test', { added: [descriptor()] }); + await f.registry.handleIncoming('test', { messages: [message()] }); + assert.equal(f.isOpen(), true, 'traffic is the only evidence such a server gives'); + await f.registry.handleChanged('test', { updated: [descriptor('x', false, 'B')] }); + assert.equal(f.isOpen(), false); + await f.registry.handleIncoming('test', { messages: [message('x', 'm2')] }); + assert.equal(f.isOpen(), true); + await f.registry.handleChanged('test', { added: [descriptor('y')] }); + const declined = await f.registry.handleChannelToolCall('channel_decline', { serverId: 'test', channelId: 'y', messageId: 'm3' }); + assert.equal(declined.success, true); + assert.equal(dataOf(declined).closeError, undefined); + assert.equal(f.calls.length, 0, 'the host sends no lifecycle RPC to a server it does not direct'); +}); + +test('a failed open leaves the channel unconfirmed: traffic afterwards marks nothing open', async () => { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor()] }); + f.held.add('x'); + const opening = f.tool('open'); + await tick(); + f.pending.shift()!.fail(); + assert.equal((await opening).success, false); + f.held.delete('x'); + await f.registry.handleIncoming('test', { messages: [message()] }); + assert.equal(f.isOpen(), false, 'an RPC can fail after the server acted, and traffic does not settle which'); +}); + +test('disconnecting a server removes its channel entries before its connection closes', async () => { + const dir = mkdtempSync(join(tmpdir(), 'disconnect-order-')); + const framework = await AgentFramework.create({ + storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'resident', model: 'test-model', systemPrompt: 'Test.' }], + modules: [], syncIntervalMs: 0, + }); + try { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + let channelsAtClose: number | undefined; + const internals = framework as unknown as { channelRegistry: ChannelRegistry | null; mcplServerRegistry: unknown }; + internals.channelRegistry = f.registry; + internals.mcplServerRegistry = { + removeServer: async () => { channelsAtClose = f.registry.listChannelsRaw().length; }, + getAllServers: () => [], + closeAll: async () => {}, + }; + await framework.disconnectMcplServer('test'); + assert.equal(channelsAtClose, 0, 'nothing queued for its channels can run against the closing connection'); + } finally { + await framework.stop(); + rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/test/channel-open-substitution.test.ts b/test/channel-open-substitution.test.ts index bfecb4578..34fa8b12e 100644 --- a/test/channel-open-substitution.test.ts +++ b/test/channel-open-substitution.test.ts @@ -202,6 +202,34 @@ test('odd ids are described in the log line, never printed raw', async () => { assert.deepEqual(logged, [ `[channel-open-substituted] server=test requested=x answered=(not a plain id: a ${forged.length}-character string)`, ]); + assert.equal(f.substituted()[0]?.answeredFor, `(not a plain id: a ${forged.length}-character string)`, + 'the trace records the same bounded form, not the raw value'); +}); + +test('an odd id is measured in characters, not UTF-16 units', async () => { + const f = fixture(() => ({ channel: { ...descriptor('y'), id: 'y\u{1F600}' } })); + await f.registry.handleChanged('test', { added: [descriptor('x')] }); + const { lines } = await capturingErrors(() => f.openX()); + assert.deepEqual(lines.filter((l) => l.includes('channel-open-substituted')), + ['[channel-open-substituted] server=test requested=x answered=(not a plain id: a 2-character string)']); +}); + +test('an answer whose channel id is not a string fails the open, and closes nothing', async () => { + for (const answer of [ + () => ({ channel: { ...descriptor('y'), id: 5 } }) as unknown as ChannelsOpenResult, + () => ({ history: [{ ...historyOf('y')[0]!, channelId: false }] }) as unknown as ChannelsOpenResult, + ]) { + const f = fixture(answer); + await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); + f.calls.length = 0; + const { value: result } = await capturingErrors(() => f.openX()); + await tick(); + assert.equal(result.success, false); + assert.match(String(result.error), /with a channel id that is not a string/); + assert.equal(f.isOpen('x'), false); + assert.match(String(f.substituted()[0]?.answeredFor), /^\(not a plain id: (number|boolean)\)$/); + assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }], 'nothing registered by that id to close'); + } }); test('an answer for the requested channel, with or without its history, or one naming no channel, opens it as before', async () => { @@ -209,6 +237,9 @@ test('an answer for the requested channel, with or without its history, or one n (requested: string) => ({ channel: descriptor(requested), history: historyOf(requested) }), (requested: string) => ({ channel: descriptor(requested) }), () => ({}) as ChannelsOpenResult, + // A null id names no channel, in the answer or in a history item. + (requested: string) => ({ channel: { ...descriptor(requested), id: null } }) as unknown as ChannelsOpenResult, + (requested: string) => ({ history: [{ ...historyOf(requested)[0]!, channelId: null }] }) as unknown as ChannelsOpenResult, ]) { const f = fixture(answer); await f.registry.handleChanged('test', { added: [descriptor('x'), descriptor('y')] }); diff --git a/test/mcpl-gate-roundtrip.test.ts b/test/mcpl-gate-roundtrip.test.ts index 3ede04df9..1d10485b4 100644 --- a/test/mcpl-gate-roundtrip.test.ts +++ b/test/mcpl-gate-roundtrip.test.ts @@ -35,7 +35,9 @@ function makeGate(config: GateConfig) { function makeRegistry(shouldTriggerInference?: (c: string, m: Record) => boolean) { const pushed: ProcessEvent[] = []; const registry = new ChannelRegistry( - {} as never, + // No server connection: the host directs no channel lifecycle here, so + // traffic is what marks a channel open. + { getServer: () => undefined } as never, {} as never, (ev) => pushed.push(ev), () => {}, From 23e28e02b433999e58fe02048bb5b806b2bfe53d Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sat, 17 Oct 2026 13:47:20 -0400 Subject: [PATCH 21/26] fix(mcpl): a failed RPC for a retargeted channel's old target tries the new one Greptile's review of #210 at 9e45ae1 found that a retarget arriving while an open for the old target is in flight queues nothing for the new target, since handleChanged leaves that to the operation already running, and that operation threw on a failed RPC without reading the target again. So a channel decided open stayed unopened at its new target until another send or registration retried it; a probe at 9e45ae1 showed one open, of the old address, and a reconcile-failed trace. runLifecycleOperation now treats a failed RPC like a stale receipt when the entry is still current and the channel's target moved: a channel still decided open or tuned out tries its new target, within the same attempt limit, re-reading its server as every attempt does. A failure for the current target, or for a channel no longer decided open or tuned out, is final, as before, so a retarget of a channel decided closed still sends nothing. channel-lifecycle-order.test.ts: the old target's failed open is followed by an open of the new one, for a channel decided open and for one tuned out, with no reconcile-failed trace; a failed close of the old target sends nothing more. --- src/mcpl/channel-registry.ts | 19 +++++++++--- test/channel-lifecycle-order.test.ts | 44 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index c3b69e6f0..5fe7ee27a 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -896,8 +896,9 @@ export class ChannelRegistry { if (!sameChannelTarget(existing.descriptor, channel)) { this.invalidateTransportConfirmation(existing); // An operation queued or running for this channel reads its target - // again before it confirms anything, so it opens the new one. With - // none, a channel decided open (or tuned out) is reconciled here. + // again before it confirms anything, and after a failed RPC, so it + // opens the new one. With none, a channel decided open (or tuned + // out) is reconciled here. const desired = this.getDesiredState(serverId, channel.id); if ((desired === 'open' || desired === 'tuned-out') && !this.lifecycleOutstanding.has(this.lifecycleKey(serverId, channel.id))) { @@ -2247,8 +2248,9 @@ export class ChannelRegistry { // continually superseded receipts. Bound this whole operation, then // release its queue slot and surface the failure to delivery joiners. // Its open flag needs no change here: every retry follows a replaced - // registration, a retarget (already invalidated), or a receipt for - // this very target. + // registration, a retarget (already invalidated, whether the old + // target's RPC was answered or failed), or a receipt for this very + // target. if (attempts >= MAX_CHANNEL_LIFECYCLE_ATTEMPTS) { const error = new ChannelLifecycleConvergenceError( `Channel ${channelId} did not converge after ${attempts} lifecycle attempts; its target or desired state kept changing`, @@ -2283,6 +2285,15 @@ export class ChannelRegistry { if (sameTarget()) this.unconfirmedChannelTargets.add(entry); else this.invalidateTransportConfirmation(entry); } + // A retarget while this RPC was out queued nothing for the new + // target, since handleChanged leaves that to this operation. So a + // failure for the old target says nothing about the new one, and a + // channel still decided open or tuned out tries its new target, + // within the attempt limit. + if (this.channels.get(channelKey) === entry && !sameChannelTarget(entry.descriptor, descriptor)) { + const now = this.getDesiredState(serverId, channelId); + if (now === 'open' || now === 'tuned-out') continue; + } throw error; } // A server can answer for a channel other than the one named: one diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index a787b74b3..5b5b08501 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -976,6 +976,50 @@ test('a retarget of a channel decided open opens the new target; one decided clo assert.deepEqual(f.calls.at(-1), { kind: 'close', id: 'y', address: undefined }); }); +for (const decision of ['open', 'tuned-out'] as const) { + test(`a retarget while the old target's open is in flight opens the new target when that open fails, for a channel decided ${decision}`, async () => { + // Greptile's P1 on #210: the retarget queued nothing, since an operation + // was running, and that operation threw on its failed RPC without + // reading the new target. + const f = fixture(); + f.held.add('x'); + const opening = f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + await tick(); + if (decision === 'tuned-out') { + f.registry.enterTuneOut('test', 'x', { + epochId: 'e1', cadenceSeconds: 60, backlogCap: 20, maxWakes: 3, startedAtSequence: 1, + }, 'agent-tool'); + } + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + f.held.delete('x'); + f.pending.shift()!.fail(); + await f.drain(opening); + assert.deepEqual(f.calls.map((c) => [c.kind, c.address]), [['open', { value: 'A' }], ['open', { value: 'B' }]]); + assert.equal(f.isOpen(), true); + assert.equal(f.registry.getDesiredState('test', 'x'), decision); + assert.deepEqual(f.traces.filter((t) => t.type === 'mcpl:channel-reconcile-failed'), []); + }); +} + +test("a retarget while the old target's close is in flight sends nothing more when that close fails", async () => { + // As for any retarget of a channel decided closed, the new target is left + // unconfirmed for channel_close to close for real. + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true, 'A')] }); + f.calls.length = 0; + f.held.add('x'); + const close = f.tool('close'); + await tick(); + await f.registry.handleChanged('test', { updated: [descriptor('x', true, 'B')] }); + f.held.delete('x'); + f.pending.shift()!.fail(); + await f.drain(close); + assert.equal((await close).success, false); + assert.deepEqual(f.calls.map((c) => c.kind), ['close']); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal(f.isOpen(), false); +}); + test("removing a server cancels its queued lifecycle work, even across a re-registration", async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From 3380fc965a8f82d6acda18792399b3f71de1ceec Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sat, 17 Oct 2026 16:01:22 -0400 Subject: [PATCH 22/26] refactor(mcpl): the substitution line prints its ids as the registry's other log lines do The merge of main brought #267's logValue, which every other log line in channel-registry.ts uses for a sender-chosen id: it prints the id escaped, quoted when it isn't a plain token, and cut at 256 units, and it has its own tests. This branch's plainIdForLog did the same job its own way, printing a short visible-ASCII id as it is and only describing any other. Ada-1017 chose one rule for the module (room-203 #133764), so plainIdForLog goes: the [channel-open-substituted] line and the trace's answeredFor print each id through logValue, so a forged id still adds no line or field, and now shows, escaped, rather than being described. channel-open-substitution.test.ts: the two odd-id tests become one, which checks that the line and the trace print a forged id as logValue does, with no raw newline, space or = from it; the malformed-id test reads logValue's JSON text for a non-string id. The changelog fragment says what answeredFor holds. --- .../210-channel-lifecycle-order.fixed.md | 2 +- src/mcpl/channel-registry.ts | 18 ++++----------- test/channel-open-substitution.test.ts | 22 ++++++------------- 3 files changed, 12 insertions(+), 30 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 9f3449d9a..077b1af7f 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -6,4 +6,4 @@ - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. -- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, compared exactly as the server's traffic is, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace, whose `answeredFor` is the answered id in the bounded form the log line prints. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel, by a null id or none, is accepted as before; one whose id isn't a string fails the open the same way, with nothing to close. Either rollout order is safe: on a server that still falls back, the resident sees a failed open for an id that server no longer knows (a deleted channel), rather than another channel's traffic. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. +- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, compared exactly as the server's traffic is, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace, whose `answeredFor` is the answered id as the log line prints it, escaped as every id the channel registry logs is. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel, by a null id or none, is accepted as before; one whose id isn't a string fails the open the same way, with nothing to close. Either rollout order is safe: on a server that still falls back, the resident sees a failed open for an id that server no longer knows (a deleted channel), rather than another channel's traffic. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index a0e0fc2b3..5a6acae53 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -59,18 +59,6 @@ class ChannelOpenSubstitutionError extends Error {} class ChannelLifecycleCancelledError extends Error {} const CHANNEL_LIFECYCLE_LOG_ID = 'mcpl/channel-lifecycle'; -/** - * A server-chosen channel id for a host log line: as it is when it's a short - * token of visible ASCII, as composite ids such as `discord:g1:c1` are, and - * otherwise only described, so an odd id can't add a line or a field. - */ -function plainIdForLog(id: unknown): string { - if (typeof id === 'string' && /^[\x21-\x7e]{1,200}$/.test(id)) return id; - return typeof id === 'string' - ? `(not a plain id: a ${[...id].length}-character string)` - : `(not a plain id: ${id === null ? 'null' : typeof id})`; -} - /** * What a channels/open answer says about which channel the server opened: * its `channel.id`, or else the `channelId` of a history item. Each is @@ -2452,9 +2440,11 @@ export class ChannelRegistry { // open it made if the host wants that channel closed. const substitute = open ? substitutedChannel(result, channelId) : undefined; if (substitute !== undefined) { - const answered = plainIdForLog(substitute.kind === 'other' ? substitute.id : substitute.value); + // Each id prints as every id this module logs does (logValue), so + // an odd one can't add a line or a field. + const answered = logValue(substitute.kind === 'other' ? substitute.id : substitute.value); console.error( - `[channel-open-substituted] server=${serverId} requested=${plainIdForLog(channelId)} answered=${answered}`, + `[channel-open-substituted] server=${serverId} requested=${logValue(channelId)} answered=${answered}`, ); this.emitTraceFn({ type: 'mcpl:channel-open-substituted', serverId, channelId, answeredFor: answered }); // A malformed id names nothing registered here, so there is diff --git a/test/channel-open-substitution.test.ts b/test/channel-open-substitution.test.ts index 34fa8b12e..2e92b1054 100644 --- a/test/channel-open-substitution.test.ts +++ b/test/channel-open-substitution.test.ts @@ -1,6 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import { logValue } from '../src/mcpl/log-value.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; import type { McplServerRegistry } from '../src/mcpl/server-registry.js'; import type { FeatureSetManager } from '../src/mcpl/feature-set-manager.js'; @@ -193,25 +194,16 @@ test('a substituted answer on a receipt made stale by retargeting is retried, no 'and the channel it opened instead is still closed again'); }); -test('odd ids are described in the log line, never printed raw', async () => { +test("the substitution line and its trace print each id as the module's other log lines do, an odd one escaped", async () => { const forged = 'y\n[channel-open-substituted] server=other requested=x answered=x'; const f = fixture(() => ({ channel: { ...descriptor('y'), id: forged } })); await f.registry.handleChanged('test', { added: [descriptor('x')] }); const { lines } = await capturingErrors(() => f.openX()); const logged = lines.filter((l) => l.includes('channel-open-substituted')); - assert.deepEqual(logged, [ - `[channel-open-substituted] server=test requested=x answered=(not a plain id: a ${forged.length}-character string)`, - ]); - assert.equal(f.substituted()[0]?.answeredFor, `(not a plain id: a ${forged.length}-character string)`, - 'the trace records the same bounded form, not the raw value'); -}); - -test('an odd id is measured in characters, not UTF-16 units', async () => { - const f = fixture(() => ({ channel: { ...descriptor('y'), id: 'y\u{1F600}' } })); - await f.registry.handleChanged('test', { added: [descriptor('x')] }); - const { lines } = await capturingErrors(() => f.openX()); - assert.deepEqual(lines.filter((l) => l.includes('channel-open-substituted')), - ['[channel-open-substituted] server=test requested=x answered=(not a plain id: a 2-character string)']); + assert.deepEqual(logged, [`[channel-open-substituted] server=test requested=x answered=${logValue(forged)}`]); + const answered = logged[0]!.slice(logged[0]!.indexOf(' answered=') + ' answered='.length); + assert.doesNotMatch(answered, /[\n =]/, 'the forged id adds no line, field or value of its own'); + assert.equal(f.substituted()[0]?.answeredFor, logValue(forged), 'the trace records the same escaped form'); }); test('an answer whose channel id is not a string fails the open, and closes nothing', async () => { @@ -227,7 +219,7 @@ test('an answer whose channel id is not a string fails the open, and closes noth assert.equal(result.success, false); assert.match(String(result.error), /with a channel id that is not a string/); assert.equal(f.isOpen('x'), false); - assert.match(String(f.substituted()[0]?.answeredFor), /^\(not a plain id: (number|boolean)\)$/); + assert.match(String(f.substituted()[0]?.answeredFor), /^(5|false)$/, 'logValue prints a non-string id as its JSON text'); assert.deepEqual(f.calls, [{ kind: 'open', id: 'x' }], 'nothing registered by that id to close'); } }); From 4b60771efece5786cdf3bacfc6911e2fdf11d925 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sat, 17 Oct 2026 16:04:58 -0400 Subject: [PATCH 23/26] docs(mcpl): openChannelNow's doc says where an open is recorded and who else queues lifecycle work Its doc called it the single open executor that marks the live entry open, with every open funnelling through it. Since the lifecycle queue, the entry is marked open by the queued work (applyDesiredChannelState) on a receipt for its current target, and reconciliation queues that work directly, as channel_close, a decline and the close of a substituted channel do. Greta-2425's read of the merge pointed it out; #350 corrected the same sentence for its own code (15a15fc9). Comment only. --- src/mcpl/channel-registry.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 5a6acae53..4d1adc99e 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -2736,11 +2736,13 @@ export class ChannelRegistry { } /** - * The single open executor: record durable desired-open intent, tell the - * server to subscribe, mark the live entry open. Every path that opens a - * channel — the agent's channel_open tool, delivery into a closed locus, - * an explicit send into a closed channel — funnels here, so lifecycle - * events, desired state, and live state can never diverge by path. + * Record durable desired-open intent, then queue the channel's lifecycle + * work (applyDesiredChannelState), which tells the server to subscribe and + * marks the live entry open on a receipt for its current target. Every + * open the agent or its speech asks for — the channel_open tool, delivery + * into a closed locus, an explicit send into a closed channel — comes + * here. Reconciliation queues that work directly, as the closes do + * (channel_close, a decline, and the close of a substituted channel). * Desired state is recorded BEFORE the server round-trip: intent sticks * even if the subscribe fails, and reconciliation retries later. * Automatic delivery needs open transport, not a change of attention: From c11b559607737461d8700b5edac252d0d9f7a013 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sat, 17 Oct 2026 16:15:58 -0400 Subject: [PATCH 24/26] docs(mcpl): the substitution line's comment and fragment say which lines share logValue They said every id the channel registry logs goes through logValue, but routeSpeech's failure lines print a channel id raw (its fail() messages for an unregistered locus, a delivered:false report and a convergence error), as on main. Imogen-2490's check of #210's texts found it. The comment now names the rejection lines' ids as the ones printed the same way, and the fragment says answeredFor is escaped by logValue. Comment and changelog only. --- changelog.d/210-channel-lifecycle-order.fixed.md | 2 +- src/mcpl/channel-registry.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/changelog.d/210-channel-lifecycle-order.fixed.md b/changelog.d/210-channel-lifecycle-order.fixed.md index 077b1af7f..d7d2a1556 100644 --- a/changelog.d/210-channel-lifecycle-order.fixed.md +++ b/changelog.d/210-channel-lifecycle-order.fixed.md @@ -6,4 +6,4 @@ - Lifecycle convergence makes at most five transport attempts per operation. Continued descriptor/intent supersession fails with a diagnostic, releases the queue, and gives joined speech or reply preparation a visible failure instead of hanging. Later stable operations can retry. -- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, compared exactly as the server's traffic is, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace, whose `answeredFor` is the answered id as the log line prints it, escaped as every id the channel registry logs is. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel, by a null id or none, is accepted as before; one whose id isn't a string fails the open the same way, with nothing to close. Either rollout order is safe: on a server that still falls back, the resident sees a failed open for an id that server no longer knows (a deleted channel), rather than another channel's traffic. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. +- A `channels/open` answered for a different channel than the one requested, by the answer's `channel.id` or a history item's `channelId`, compared exactly as the server's traffic is, fails the open. The requested channel isn't marked open (its open state becomes unconfirmed), nothing in the answer is used, its history included, and the host logs `[channel-open-substituted]` and emits an `mcpl:channel-open-substituted` trace, whose `answeredFor` is the answered id as the log line prints it, escaped by `logValue`. When the channel the server opened instead is registered and its desired state is closed, the host closes it again, so its traffic doesn't reach the resident. A substituted answer on a receipt already made stale by retargeting is retried like any stale receipt. An answer that names no channel, by a null id or none, is accepted as before; one whose id isn't a string fails the open the same way, with nothing to close. Either rollout order is safe: on a server that still falls back, the resident sees a failed open for an id that server no longer knows (a deleted channel), rather than another channel's traffic. `ChannelsOpenParams.channelId` is documented as exact: a server that doesn't know the id should refuse. diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 4d1adc99e..d0af44164 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -2440,8 +2440,8 @@ export class ChannelRegistry { // open it made if the host wants that channel closed. const substitute = open ? substitutedChannel(result, channelId) : undefined; if (substitute !== undefined) { - // Each id prints as every id this module logs does (logValue), so - // an odd one can't add a line or a field. + // Each id prints through logValue, as the rejection lines' ids do, + // so an odd one can't add a line or a field. const answered = logValue(substitute.kind === 'other' ? substitute.id : substitute.value); console.error( `[channel-open-substituted] server=${serverId} requested=${logValue(channelId)} answered=${answered}`, From 8637d55f80a7386b3db1c310b3191085dbecded6 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 12:00:17 -0400 Subject: [PATCH 25/26] test(channels): a draft published into a channel with a close pending waits for it and posts nothing, as speech does Since the merge of main 0a7fd161, a draft's publish() meets the channel's queued lifecycle work the way routed speech and a reply do. This pins it: with a channel_close held in flight, a draft into that channel waits for the close, fails as "lifecycle work prevented delivery: ...", leaves the channel closed and decided closed, publishes nothing and posts no notice. Co-Authored-By: Claude Opus 5.5 --- test/channel-lifecycle-order.test.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 6796a6f4e..6623aece8 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -297,6 +297,26 @@ test('speech does not replace a pending explicit close with a new open decision' assert.equal(f.notices.length, 0); }); +test('a draft published into a channel with a close pending waits for it and posts nothing, as speech does', async () => { + // Drafts publish through the same open step as routed speech, so they meet + // the channel's queued lifecycle work as speech and replies do. + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + f.held.add('x'); + const close = f.tool('close'); + await tick(); + const draft = f.registry.publish('resident', 'a draft', { serverId: 'test', channelId: 'x' }); + await f.drain(close, draft); + assert.equal((await close).success, true); + const outcome = await draft; + assert.equal(outcome.status, 'failed'); + assert.match(outcome.reason ?? '', /^lifecycle work prevented delivery: /); + assert.equal(f.registry.getDesiredState('test', 'x'), 'closed'); + assert.equal(f.isOpen(), false); + assert.deepEqual(f.publishedWhileOpen, [], 'nothing was published'); + assert.equal(f.notices.length, 0); +}); + test('waiting for a backscroll does not issue new opens or auto-open notices for each speech', async () => { const f = fixture(); await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); From 94d2ad38c2e0df554e8c6d9edd51e36cc2d17145 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 17:37:00 -0400 Subject: [PATCH 26/26] test(channels): a channel registered while a disconnect closes its server is removed with the rest The disconnect removes a server's channel entries before its closes start, and again after the reap. The second removal is for a connect that was still setting up when the disconnect began: a channel change it had received can be handled while its connection closes, after the first removal. The test's stub registry handles such a change during the close, checks the channel was registered then, and checks that no channel of the server outlives the disconnect. Co-Authored-By: Claude Opus 5.5 --- test/channel-lifecycle-order.test.ts | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/channel-lifecycle-order.test.ts b/test/channel-lifecycle-order.test.ts index 6623aece8..b6c80e178 100644 --- a/test/channel-lifecycle-order.test.ts +++ b/test/channel-lifecycle-order.test.ts @@ -1204,3 +1204,37 @@ test('disconnecting a server removes its channel entries before its connection c rmSync(dir, { recursive: true, force: true }); } }); + +test('a channel its server registers while a disconnect closes it is removed with the rest', async () => { + const dir = mkdtempSync(join(tmpdir(), 'disconnect-late-register-')); + const framework = await AgentFramework.create({ + storePath: join(dir, 'store'), membrane: new MockMembrane().asMembrane(), + agents: [{ name: 'resident', model: 'test-model', systemPrompt: 'Test.' }], + modules: [], syncIntervalMs: 0, + }); + try { + const f = fixture(); + await f.registry.handleChanged('test', { added: [descriptor('x', true)] }); + const internals = framework as unknown as { channelRegistry: ChannelRegistry | null; mcplServerRegistry: unknown }; + internals.channelRegistry = f.registry; + let lateRegistered = false; + internals.mcplServerRegistry = { + // A connect still setting up when the disconnect began: a channel + // change it had received is handled while its connection closes, after + // the disconnect removed the entries it found. + removeServer: async () => { + if (lateRegistered) return; + await f.registry.handleChanged('test', { added: [descriptor('y')] }); + lateRegistered = f.registry.listChannelsRaw().some((c) => c.descriptor.id === 'y'); + }, + getAllServers: () => [], + closeAll: async () => {}, + }; + await framework.disconnectMcplServer('test'); + assert.equal(lateRegistered, true, 'the channel was registered while the connection closed'); + assert.deepEqual(f.registry.listChannelsRaw(), [], 'no channel of the disconnected server outlives its disconnect'); + } finally { + await framework.stop(); + rmSync(dir, { recursive: true, force: true }); + } +});