Skip to content

Add opt-in required admission for inbound MCPL traffic - #906

Open
theaspirational wants to merge 39 commits into
anima-research:mainfrom
theaspirational:required-inbound-admission
Open

theaspirational wants to merge 39 commits into
anima-research:mainfrom
theaspirational:required-inbound-admission

Conversation

@theaspirational

@theaspirational theaspirational commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Hosts need an opt-in, fail-closed admission step for inbound MCPL traffic: a configured host must be able to refuse channel deliveries, pushes and inference requests before they take effect, and must not have them slip through on reconnects, coalescing or handshake paths. Connections without inboundAdmission must behave exactly as before.

Changes

  • Connection- and epoch-bound admission callbacks with optional attestation; authorization is rechecked at the final mutex check before any effect. Receipt replays and duplicate acceptances also recheck current authority (grant, readiness, feature set, channel registration) under the mutex before acknowledging.
  • Preflight for required channel deliveries, pushes and inference requests; handshake traffic goes through the validated reader.
  • Bounded, ordered coalescing for required traffic (64 tickets per server id, spanning reconnects; excess refuses immediately). A retry of an occurrence with a live receipt replays its original result even at the cap.
  • Required module work uses an explicit lifecycle-owned queue instead of per-event promise chains. It becomes eligible only after the lane's acknowledgement is sent, and runs outside the coalescer lock with errors contained.
  • The framework-only marker is an internal capability passed from addServer to connect, so the caller's config object keeps its identity and server-text redaction of dialled credentials still applies.
  • MAX_PENDING_REQUIRED_MODULE_WORK is exported from the package entry.
  • Rebased onto 074ac60; conflict resolutions keep upstream's suppression bookkeeping, envelope/null-ID routing, generic response tap, initialize decoding and TraceEventInput typing.
  • Answers to the reviews are in the PR comments.

Limits

  • Supported only through AgentFramework. Direct McplServerConnection factories reject a configuration that carries inboundAdmission; a shared authorized-dispatch API for direct consumers could be a separate PR.
  • Module dispatch for required coalesced content starts after acceptance and after the acknowledgement is sent, in acceptance order per channel/feature-set scope (callback-completion order across subjects, not arrival order). A 64-slot ceiling counts reserved, queued and running work. On overflow a new delivery is refused before storage, with wire reason admission_denied and a distinct capacity diagnostic. Withdrawals are never refused at the ceiling: the removal stands, and if it delivers a notice while the scope is full, only that notice's module work is dropped with mcpl:module-work-dropped.
  • Stop and disconnect drop not-yet-started module work with a diagnostic; close and reconnect keep it. Accepted content stays stored. Running module calls are not cancelled and stay counted; a hung module occupies one slot of its scope until it settles. A module that blocks synchronously still blocks the event loop after the acknowledgement.
  • Required and unconfigured coalescing servers share one host-wide coalescing chain and mutex. On an unconfigured connection a coalesced delivery still awaits module onProcess inside that chain (as on main), so a hung module there can stall required coalesced effects too.
  • Deferred coalescing, context-only delivery and shared direct-consumer admission are out of scope.
  • Sequential batches can see head-of-line delay; a no-stream boundary drains immediately (see docs/required-inbound-admission.md).

Tests

  • Head (local, Node 22, offline, concurrency 4): 9,902 pass / 0 fail / 63 skip. Upstream baseline 074ac60 on the same setup: 9,574 / 0 / 63.
  • Typecheck and build pass. 23 targeted admission, coalescing, backlog and reconnect files: 644/644.
  • The round-3 regressions (saturated receipt replay on both lanes, delete-at-capacity with a non-empty notice, config identity and provider-token redaction, revoked authority during a waiting replay, lifecycle cases, and acknowledgement-before-module ordering on both lanes) fail on the previous head and pass here. Mutations reinstating the early ticket-cap refusal, the withdrawal refusal, the config wrapper and the early module pump are killed.
  • No live provider was exercised.

AI-assisted implementation with GPT-6.1-Sol

@github-actions github-actions Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review

Recommendation: needs a maintainer's decision. The change belongs, but part of it needs a maintainer's call.

Reviewed commit 119cb86 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.

Does this change belong in the project?

It fits, with concerns. Opt-in admission for inbound MCPL traffic fits the framework's optional MCPL host layer and lives in src/mcpl/, with docs, a changelog fragment and tests. One test, though, sets a project-wide ban on Discord and other connector names in framework.ts and index.ts.

5 reasons with sources

The project doesn't state that rule, and its own README documents Discord-specific framework features, so a maintainer should decide on that part separately.

  • The feature extends the MCPL host surface the README describes: pushes, channels and inference requests coming from external servers. The README calls MCPL an optional host-side protocol implementation through which servers push events, start inference and publish channels. The change adds an explicitly configured McplServerConfig.inboundAdmission gate over exactly those three paths. (README.md, "MCPL (MCP Live)")
  • The new code goes where the existing structure puts MCPL logic, and unconfigured connections are meant to behave as before. The core is a new src/mcpl/inbound-admission.ts, with hooks in channel-registry, push-handler, inference-router and server-connection, plus small wake-gating changes in framework.ts. No new dependency is added to runtime code. (src/mcpl/ layout)
  • The new connector-literal test sets a repo-wide architectural rule that the project never states. The test freezes exact occurrence ceilings for about 100 Discord identifiers and strings in framework.ts and index.ts. Any future Discord-related upstream work would need a 'reviewed ceiling bump'. Yet the README documents Discord awareness, suppression baselines and recovery as first-class framework behavior. Whether connector-specific code should be capped is a separate decision, beyond what an admission feature needs to settle. (test/routing-runtime-has-no-connector-literals.test.ts; README.md (Discord awareness / DISCORD_SUPPRESSED_REACTIONS_BASELINE sections))
  • It follows the contribution conventions: Problem/Changes/Tests body, numeric test baseline, 'Not verified' section and a changelog fragment. The PR adds changelog.d/required-inbound-admission.added.md and reports 9,531 passed / 0 failed. It also lists what was not exercised live. (CONTRIBUTING.md, "What a PR should contain" and "Changelog")
  • It adds a sizeable new public concept, with limits, refusal codes and hint policy, that the maintainers would then have to keep working; the PR rightly leaves those choices to them. The PR adds a closed RefusalCode vocabulary, timeout and pending bounds, attestation hooks and hint-handling rules for required connections, and puts these to the maintainers as open questions. No project document plans or rules out such a profile, so the maintainers need to accept this API surface explicitly. (docs/required-inbound-admission.md (new); src/index.ts exports)

Findings

4 findings of medium or high severity, see the inline comments:

1 finding about lines the diff does not show:

  • Medium severity. Required push refusals are neither logged nor traced, so refused notifications disappear without a trace. src/mcpl/push-handler.ts:390
Details of this finding
  • Medium severity, src/mcpl/push-handler.ts:390. Required push refusals are neither logged nor traced, so refused notifications disappear without a trace. handleRequiredPushEvent refuses on feature-set failure, hints, check() and every verdict (denied, threw, timeout, invalid, overload) only through responder?.respond(...). For push/event notifications there is no responder, so the event vanishes with no console line and no trace. The existing path in handlePushEvent (lines 200-211, 251) deliberately logs [push-event-rejected] ... because 'a rejected push event is an agent that silently never hears the message'. Admission timeouts, a throwing hook and overload are exactly the failures an operator needs to see, and here nothing records them.
6 more findings, less severe
  • Low severity, documentation, src/mcpl/server-connection.ts:1358-1359. Handshake-time data requests are refused with an undocumented capability_denied message. refuseEarly answers channels/incoming, push/event and inference/request sent before initialized with code -32002 and the message 'capability_denied'. That string is not in the closed RefusalCode vocabulary. The docs and changelog say pre-ready traffic is refused with the admission refusal shapes (admission_not_ready, itemwise channel results, push {accepted:false}). Peers coding against the documented vocabulary will meet a reason the docs never mention, and in a different shape. Either use the documented refusal shapes or document this case.
  • Low severity, maintainability, src/mcpl/channel-registry.ts:1932-1941. Channel admission refusals skip the registry's refusal trace and log. Every other channel refusal in handleIncoming emits mcpl:channel-incoming-rejected and/or a [channel-incoming-rejected] log line; examples are unknown-channel at lines 1907-1921 and empty content at 1953-1960. Admission refusals, including timeout, throw and overload, only push an item result. For channels/incoming sent as a notification, or when nobody reads the result, the refusal is invisible to trace consumers.
  • Low severity, maintainability, src/framework.ts:34776-34785. The same RequiredIngress wrapper is copied into three handlers. The push-event (34776-34785), inference-request (34850-34863) and channels-incoming (34967-34976) handlers each build { ...connection.admissionFor(params), check: () => queue closed ? 'admission_not_ready' : registry replaced ? 'admission_stale' : original.check() } inline. Only the inference copy differs, adding an isRendering check. A shared helper such as requiredIngressFor(connection, params, extraCheck?) would keep the lifetime rules from drifting between lanes.
  • Low severity, maintainability, src/mcpl/push-handler.ts:390. Envelope construction is duplicated between push-handler and channel-registry. Object.freeze({ connection: admission.key, mayWake: verdict.mayWake, ...(Object.hasOwn(verdict, 'replyIntent') ? { replyIntent: verdict.replyIntent } : {}) }) appears both here and in channel-registry.ts:1942. Any change to the envelope shape, such as the planned stamp support, has to be made in both places. A helper in inbound-admission.ts (for example envelopeFor(key, verdict)) would centralise it.
  • Low severity, maintainability, src/mcpl/inference-router.ts:274-306. Inference admission duplicates the router's stop/answer logic. The admission block builds its own combined signal and its own answerStop (trace plus hostStoppedWhileHandling plus a guarded respondError), nearly a copy of the existing logic at lines 365-377. Both copies also have to keep their 'answer only while the transport is live' conditions in step. Extracting a shared helper would avoid the two paths diverging, for example in how streamed is reported.
  • Low severity, maintainability, src/index.ts:220-221. AdmissionConnectError is not exported, though it is the fatal startup error hosts must handle. Binding and attestation failures now abort AgentFramework.create with AdmissionConnectError, but index.ts exports only types from inbound-admission.ts. Host applications cannot use instanceof to tell this fatal configuration error apart from other startup failures, and have to match on name or message text. MAX_ADMISSION_TIMEOUT_MS and MAX_PENDING_ADMISSIONS are also not exported, though the docs present them as the configuration bounds.

For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.

Comment thread src/mcpl/server-connection.ts Outdated
Comment on lines +1354 to +1361
const refuseEarly = (line: string) => {
if (!admission) return;
let request: any;
try { request = JSON.parse(line); } catch { return; }
if (['channels/incoming', 'push/event', 'inference/request'].includes(request.method) && request.id != null) {
transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: request.id, error: { code: CAPABILITY_DISABLED, message: 'capability_denied' } }));
}
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity. A peer can crash the host during the handshake by sending a null line to the new refuseEarly listener.

refuseEarly runs JSON.parse(line) and then reads request.method without checking the result. A peer line that is exactly null parses to null, so request.method throws a TypeError inside a transport 'line' listener. McplTransport.pushLine (transport.ts:140-143) calls emit('line') from an I/O callback with nothing around it to catch the error, so a misbehaving or hostile peer can take the host process down while a required connection is still handshaking. The existing parser messageFromLine (server-connection.ts:2935-2947) already rejects non-object values; refuseEarly should use it or add the same guard.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 38f56af. Handshake refusals now use the validated initialize reader rather than a separate listener. The real-stdio regression sends null, all three early data lanes and a same-chunk ping without crashing.

Comment thread src/framework.ts
Comment on lines +18756 to +18761
if (server.requiredAdmission) {
const envelope = (occ.event.event as unknown as { [admittedEnvelope]?: import('./mcpl/inbound-admission.js').AdmittedEnvelope })[admittedEnvelope];
// Neither persisted work without a live key nor a previous incarnation
// borrows the replacement transport's grant. This is not durable reauthorization.
if (!envelope || !server.requiredAdmission.isCurrent(envelope.connection)) return false;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity. Coalesced work already accepted on a required connection is silently revoked by any reconnect or restart.

coalescedAuthorized now requires the occurrence's envelope key to be requiredAdmission.isCurrent(...). A reconnect calls invalidate()/bind() with a new epoch, and records from recordReceipt are rebuilt with structuredClone, which drops the symbol-keyed envelope. So any deferred or debounced coalesced batch that the peer already received accepted: true for is audited as 'revoked' and dropped at render (push-coalescer.ts:811/1072/1099/1154). The peer considers it delivered and will not resend it. Unconfigured connections deliver the same batch under the new grant. The docs do not tell operators that accepted coalesced content is lost on ordinary transport bounces.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by e9e1853. Required-admission connections now refuse coalesced input with admission_unsupported before acceptance, rather than acknowledging work that cannot survive re-admission. The docs call out the limitation and draining existing batches before opting in.

Comment on lines +15 to +148
"Identifier:announceDiscordAwarenessImport": 2,
"Identifier:announceDiscordAwarenessRecovery": 2,
"Identifier:boundDiscordAwarenessText": 5,
"Identifier:byDiscordId": 3,
"Identifier:cancelDiscordAwareness": 2,
"Identifier:checkedDiscordAwarenessMarks": 4,
"Identifier:DEFAULT_DISCORD_AWARENESS_DEADLINE_MS": 3,
"Identifier:DEFAULT_DISCORD_AWARENESS_EMOJI": 5,
"Identifier:defaultDiscordAwarenessOutboxPath": 3,
"Identifier:deliverDiscordAwarenessInBackground": 9,
"Identifier:DISCORD_SUPPRESSED_REACTIONS_BASELINE": 2,
"Identifier:DiscordAwarenessAccountingError": 8,
"Identifier:DiscordAwarenessBatch": 5,
"Identifier:DiscordAwarenessCancelReceipt": 4,
"Identifier:discordAwarenessDeadlineMs": 6,
"Identifier:DiscordAwarenessDrainOutcome": 5,
"Identifier:DiscordAwarenessDrainPass": 3,
"Identifier:discordAwarenessDrains": 7,
"Identifier:discordAwarenessDrainsJoined": 4,
"Identifier:discordAwarenessEmoji": 9,
"Identifier:DiscordAwarenessImportedUnsent": 3,
"Identifier:DiscordAwarenessMarks": 7,
"Identifier:discordAwarenessOutbox": 25,
"Identifier:DiscordAwarenessOutbox": 7,
"Identifier:discordAwarenessOutboxPath": 6,
"Identifier:DiscordAwarenessPrepareInput": 3,
"Identifier:DiscordAwarenessRefusal": 4,
"Identifier:DiscordAwarenessRequest": 5,
"Identifier:DiscordAwarenessRetirement": 4,
"Identifier:DiscordAwarenessRetractReceipt": 4,
"Identifier:DiscordAwarenessScope": 5,
"Identifier:DiscordAwarenessSettlement": 3,
"Identifier:DiscordAwarenessView": 4,
"Identifier:discordChannelOf": 3,
"Identifier:discordIdOf": 3,
"Identifier:discordRefs": 2,
"Identifier:discordSendFailed": 4,
"Identifier:drainDiscordAwarenessOutbox": 2,
"Identifier:extractDiscordAwarenessRefs": 5,
"Identifier:isPermanentDiscordReactionFailure": 4,
"Identifier:listDiscordAwareness": 2,
"Identifier:MAX_DISCORD_AWARENESS_DEADLINE_MS": 2,
"Identifier:MIN_DISCORD_AWARENESS_DEADLINE_MS": 2,
"Identifier:normalizeDiscordAwarenessDeadline": 2,
"Identifier:readDiscordAwarenessDue": 4,
"Identifier:reconcileDiscordAwarenessAtStartup": 2,
"Identifier:resumePreparedDiscordSuppressions": 2,
"Identifier:retractDiscordAwareness": 2,
"Identifier:runDiscordAwarenessDrain": 2,
"Identifier:runDiscordAwarenessSync": 3,
"Identifier:selectDiscordAwarenessRefs": 5,
"Identifier:syncDiscordAwarenessMarkers": 1,
"StringLiteral:./recovery/discord-awareness-outbox.js": 1,
"StringLiteral:Channel id, glob ok (e.g. \"discord:*:12345\").": 1,
"StringLiteral:discord": 2,
"StringLiteral:DISCORD_SUPPRESSED_REACTIONS_BASELINE": 1,
"StringLiteral:discord-awareness-delivery": 3,
"StringLiteral:discord-awareness-import-unsent": 1,
"StringLiteral:discord-awareness-not-scheduled": 2,
"StringLiteral:discord-awareness-unresolved": 1,
"StringLiteral:discord-send-failed": 5,
"StringLiteral:DiscordAwarenessAccountingError": 1,
"StringLiteral:discordAwarenessOutboxPath": 1,
"StringLiteral:discordAwarenessOutboxPath), so it holds no marks": 1,
"StringLiteral:e.g. `{ name: \"watch-cairn\", match: { source: \"discord\", channel: \"discord:*:12345\", tagsAny: [\"chat:ambient\"] }, debounceMs: 60000 }` ": 1,
"StringLiteral:Integration/serverId, glob ok (e.g. \"discord\").": 1,
"StringLiteral:that prose to one uniquely resolved authorized Discord or Eidoverse channel. The ": 1,
"StringLiteral:The safe way to order against a specific catch-all, e.g. insertBefore: \"discord-ambient\".": 1,
"TemplateElement:: Discord refused them for good, ": 1,
"TemplateElement:: DISCORD_SUPPRESSED_REACTIONS_BASELINE ": 1,
"TemplateElement:[discord-awareness] ": 9,
"TemplateElement:[discord-awareness] agent=": 1,
"TemplateElement:[discord-awareness] delivery after ": 2,
"TemplateElement:[discord-awareness] startup: retiring ": 1,
"TemplateElement:[discord-awareness] this start retires version 2 awareness batch ": 1,
"TemplateElement:Cannot resume Discord suppression ": 1,
"TemplateElement:Cannot safely resume Discord suppression ": 1,
"TemplateElement:Discord ": 1,
"TemplateElement:Discord awareness accounting failed during ": 1,
"TemplateElement:discord:": 1
},
"src/index.ts": {
"Identifier:DEFAULT_DISCORD_AWARENESS_EMOJI": 2,
"Identifier:defaultDiscordAwarenessOutboxPath": 2,
"Identifier:DiscordAwarenessAction": 2,
"Identifier:DiscordAwarenessBatch": 2,
"Identifier:DiscordAwarenessBatchStatus": 2,
"Identifier:DiscordAwarenessBatchView": 2,
"Identifier:DiscordAwarenessCancelReceipt": 2,
"Identifier:DiscordAwarenessImportedUnsent": 2,
"Identifier:DiscordAwarenessMarks": 2,
"Identifier:DiscordAwarenessOutbox": 2,
"Identifier:DiscordAwarenessOutcome": 2,
"Identifier:DiscordAwarenessPrepareInput": 2,
"Identifier:DiscordAwarenessReactionState": 2,
"Identifier:DiscordAwarenessRef": 2,
"Identifier:DiscordAwarenessRefusal": 2,
"Identifier:DiscordAwarenessRequest": 2,
"Identifier:DiscordAwarenessRetirement": 2,
"Identifier:DiscordAwarenessRetractReceipt": 2,
"Identifier:DiscordAwarenessRetractView": 2,
"Identifier:DiscordAwarenessScope": 2,
"Identifier:DiscordAwarenessSettlement": 2,
"Identifier:DiscordAwarenessView": 2,
"Identifier:DiscordSuppressionInterval": 2,
"Identifier:extractDiscordAwarenessRefs": 2,
"Identifier:isPermanentDiscordReactionFailure": 2,
"Identifier:selectDiscordAwarenessRefs": 2,
"StringLiteral:./recovery/discord-awareness-outbox.js": 2
},
"src/mcpl/channel-registry.ts": {},
"src/mcpl/inbound-admission.ts": {},
"src/mcpl/inference-router.ts": {},
"src/mcpl/protocol-family.ts": {},
"src/mcpl/push-handler.ts": {},
"src/mcpl/server-connection.ts": {},
"src/mcpl/types.ts": {}
};

const connectorVocabulary = /slack|telegram|discord|zulip|eidoverse|supportRecipientBridge|supportRecipientReplyRoot|supportRecipientTurnId/i;
const runtimeKinds = new Set(['Identifier', 'StringLiteral', 'RegExpLiteral', 'TemplateElement']);
const commentKeys = new Set(['comments', 'leadingComments', 'trailingComments', 'innerComments', 'loc']);

for (const [file, allowed] of Object.entries(inherited)) test(`routing runtime has no new connector literals: ${file}`, () => {
const counts = new Map<string, number>();
const visit = (node: any): void => {
if (!node || typeof node !== 'object') return;
if (runtimeKinds.has(node.type)) {
const value = String(node.name ?? node.value?.raw ?? node.value ?? node.pattern);
if (connectorVocabulary.test(value)) {
const key = `${node.type}:${value}`;
const count = (counts.get(key) ?? 0) + 1;
counts.set(key, count);
assert(count <= (allowed[key] ?? 0), `${file}:${node.loc.start.line}: new connector policy: ${key} (${count} > ${allowed[key] ?? 0})`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity, maintainability. New test freezes exact counts of unrelated Discord identifiers in framework.ts and index.ts.

The test hard-codes per-identifier occurrence ceilings for about 100 Discord-awareness identifiers and strings in src/framework.ts and src/index.ts, code this PR does not touch. Any future legitimate change to Discord awareness that adds one more reference (for example a new call to discordAwarenessOutbox) will fail this test, so contributors to an unrelated feature must edit a ceiling file belonging to the admission work. This is a project-wide lint policy added under an admission PR, and it does not test admission behaviour. The admission-only AST check in mcpl-admission-compatibility.test.ts already covers inbound-admission.ts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by ba3038d. The broad inventory test was removed. The remaining connector-vocabulary AST check is confined to inbound-admission.ts and does not freeze unrelated framework or index identifier counts.

Comment thread src/mcpl/server-connection.ts Outdated
transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: request.id, error: { code: CAPABILITY_DISABLED, message: 'capability_denied' } }));
}
};
if (admission) transport.on('line', refuseEarly);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium severity, test coverage. No test covers the handshake-time refuseEarly refusal.

None of the new test files sends channels/incoming, push/event or inference/request before initialized on a required connection, so the refusal shape and the listener's removal at return/catch are never exercised. A test feeding malformed lines (such as null) during the handshake would also have caught the TypeError crash in refuseEarly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 38f56af. The real-stdio handshake test now covers null input, refusal shapes for channels/incoming, push/event and inference/request, plus a same-chunk ping and subsequent communication.

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly dual review · round R1 at 119cb86 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.

Reviewed at 119cb86 against main 48f26ce

Verdict: changes requested (1 blocker, 4 major, 5 minor)

The admission core itself is careful work. Snapshots are detached and frozen, verdicts are closed-field validated, every slot is released in finally, stale keys are rechecked after each await, and the unconfigured path really is untouched. The problems are at the edges, where the profile meets the rest of the MCPL host. The handshake listener lets a peer crash the host. The connection still advertises RFC-014 toolHints and then throws the hints away. "Refuse, don't hold" is keyed to this framework's internal plane state, not to the protocol's readiness signal. And accepted coalesced work does not survive a reconnect. Separately, the PR carries a repo-wide test that freezes ~100 unrelated Discord identifiers, which an admission feature has no need to settle.

No earlier review of ours exists on this PR, so there are no prior findings to disposition.


Blocker

R1-1. A null line during the handshake crashes the host process on a required connection.
src/mcpl/server-connection.ts:1354-1361: refuseEarly does JSON.parse(line), then reads request.method with no object check. A peer that writes the 5 bytes null\n before its initialize result throws a TypeError out of the transport's line listener. That listener runs from the WebSocket message / stdio read callback, so nothing catches the error. The handshake's own reader uses messageFromLine (:2935-2948), which rejects null, non-objects and arrays explicitly.
Reproduced against the built head with a stdio peer that writes null\n and then its initialize result. Unconfigured, the host logs sent a line of length 4 that is JSON but not an object, so it was dropped and connects. With inboundAdmission set, the host gets uncaughtException: TypeError: Cannot read properties of null (reading 'method'), and a host without an uncaughtException handler exits. The feature exists to defend against peers you don't fully trust, so a pre-auth remote crash in it blocks the merge. No test sends a malformed line during the handshake (the automated review noted the same gap).
Fix: drop the separate listener and do the refusal inside the handshake's existing onLine, after messageFromLine, in its 'method' in msg branch next to the ping answer. That also resolves R1-6.

Major

R1-2. Required connections still advertise toolHints: true, then discard every hint carrier and the toolset results that ride the same tap.

  • src/framework.ts:32919-32941: hintsFor() and mcplHostCapabilitiesFor() never consult inboundAdmission. A required legacy server is offered toolHints: true by default, and the RFC-014 toolset controller is attached (:33347).
  • The PR then refuses standalone tools/hints (server-connection.ts:2516-2520) and hinted pushes (push-handler.ts in handleRequiredPushEvent). It also skips tapResponse entirely (server-connection.ts:2489).
  • RFC-014 §6.1 says a host that advertised toolHints applies hints "as soon as it has each one, whatever it then does with the message that carried it … it applies the hints of a push it refuses". It also says "A push carrying hints is never retried". A conforming peer spends the rev and never restates it, so the host's tool visibility drifts until some later list happens to expose the gap. So the claim that no SPEC change is required doesn't hold while the capability stays advertised.
  • Skipping tapResponse costs more than mcpl/hints. toolset-host.ts:226-268 uses that path to adopt toolsets/list (readToolsetList → presence.adopt, :317-341) and to record toolsets/open / toolsets/close (noteOpened / noteClosed). ownToolsetList() returns takeChange(r) (:504-507), which is empty when the tap never ran. On a required connection, then, the post-reconnect toolsets/list sync (connected(), :521-530) and the PR's own "list recovery" adopt nothing. Opens and closes made through tools don't update presence either. Only tools/list survives, because sendToolsList taps through its own onRead (server-connection.ts:1821-1840). The new lifecycle test checks that recovery sent toolsets/list, not that its result was adopted.
    Fix: have hintsFor() return false when config.inboundAdmission !== undefined, or reject toolHints: true together with inboundAdmission in serverConfigProblems. RFC-014 §6.3 then forbids the server from sending hints at all and lets it tailor its tools to a hint-less host, which is spec-conformant. The three special cases (tools/hints refusal, push-tap suppression, tapResponse skip) then become unnecessary. If hints must stay advertised, strip only _meta["mcpl/hints"] and keep applying the toolsets/* result fields. Either way, add a test that a required connection's toolsets/list result is adopted.

R1-3. "Refuse, don't hold" is keyed to the framework's plane state, which lags the protocol's readiness. Spec-conforming peers lose data at every startup and for the whole of a quiesce window.
server-connection.ts:1032-1046 refuses all three lanes with admission_not_ready while !controlPlaneReady || !dataPlaneReady. Those flags open only in readyMcplPlanes (framework.ts:32785-32790):

  • at startup, after every configured server has been connected and staged (:32247);
  • on reconnect, once its policy exchange settles;
  • for a quiesced host, never for the data plane. readyControlPlane() only, until resume() (:9211).

SPEC §4.2.3 step 6 and §5.3 define the peer-visible signal: privileged traffic may proceed once the featureSets/update exchange completes. A peer that waits for its receipt and then sends can still be refused here, and it has no wire signal for when to retry. Notifications (channels/incoming and push/event sent without an id) get no receipt and, per R1-7, no log line either. Reproduced with the repo's coalescing fixture by quiescing the host and sending a push/event request:

  • unconfigured: held, answered {"accepted":true,…} after resume(), content in context;
  • required: answered at once {"accepted":false,"reason":"admission_not_ready"}, and the content never arrives.

The docs' "Peers must send after readiness or explicitly retry refused work" doesn't help, because the peer can't observe this readiness. Quiesce isn't mentioned in the docs or tests.
Fix (preferred): hold required data-lane items exactly as unconfigured ones are held. Run admission when the held item is flushed back through emit() (ready() already re-enters that choke point), under the key current at flush time, and refuse items from a superseded epoch as admission_stale, as afterBoundary already does. If refusing is a hard requirement, refuse only before the policy exchange (where §5.3 mandates it), not during startup staging or quiesce, and document the quiesce consequence.

R1-4. Coalesced work that was already accepted is silently revoked by any reconnect or restart.
framework.ts:18753-18761: coalescedAuthorized requires envelope && requiredAdmission.isCurrent(envelope.connection). RequiredAdmission.bind() mints a fresh frozen key on every handshake, and isCurrent compares by identity. The envelope is a symbol property, so it doesn't survive the coalescer's snapshot/restore. Yet the coalescer treats acceptance as durable: it refuses the push with a retry error when it can't persist the acceptance (push-coalescer.ts:600-610), and it restores batches on boot. On a required connection, every batch pending at a reconnect or process restart is therefore dropped:

  • revoked audit at push-coalescer.ts:1072 / :1099;
  • silently, in the recovery-wake path at :811.

The peer was told accepted: true, and RFC-006 is explicit about what that means: "Accepted work is never silently lost. … A disconnect, a restart, or exhaustion of coalescing slots is not a reason to discard it". Its Revision 6 note adds that "a reconnect changes transport authority, not the identity of the configured server binding. Pending work and retry receipts survive reconnect/recovery". Servers adopting it "never resend based on a coalescing outcome". "A verdict is not a delivery receipt" covers downstream content failures. It doesn't cover the host discarding work it acknowledged as stored. The automated review raised this one too.
Fix: either re-run admission for restored or prior-epoch batches under the current key before rendering (lane and item are known), or refuse coalesce on required connections with admission_unsupported until that exists, so the peer gets an honest answer. Document whichever you pick, and add a reconnect-with-pending-batch test.

R1-5. test/routing-runtime-has-no-connector-literals.test.ts sets project policy outside this PR's scope.
Lines 13-131 freeze exact occurrence ceilings for ~100 Discord-awareness identifiers in src/framework.ts (for example discordAwarenessOutbox: 25). Lines 134-156 fail on any further match. None of them relate to admission. The README documents Discord awareness as framework behaviour, so the next unrelated PR that adds one reference to an existing helper fails a test named after routing/admission. The PR body acknowledges this ("needs a reviewed ceiling bump"). Meanwhile, connector policy written without one of the regex words passes. It passes today on both the head and the PR merged onto current main. The problem is the ongoing cost, not breakage now.
Fix: restrict the inventory to the files this PR adds or that hold admission logic (src/mcpl/inbound-admission.ts, with an empty allowance), or drop it. Whether framework.ts should be capped is a separate maintainer decision.

Minor

R1-6. The handshake listener swallows non-data lines and answers with an undocumented shape.
refuseEarly stays attached until just before the handshake returns. McplTransport buffers lines only while no line listener exists (transport.ts:77-82, 140-142), so every line after the initialize result in the same read chunk or WebSocket frame is consumed by refuseEarly instead of buffered. Reproduced: a peer that writes its initialize result and a ping request in one write gets the ping answered when unconfigured and gets no answer when required. The refusal itself is -32002 / "capability_denied" for all three lanes. That code is spec-legal under §5.3, but the message is outside the RefusalCode vocabulary, and the docs say pre-ready traffic gets the itemwise / {accepted:false} / -32600 shapes. Fix: same as R1-1. Refuse inside onLine only while it is installed, and document the handshake-phase answer.

R1-7. Admission refusals are neither logged nor traced.
The sibling paths log [push-event-rejected] (push-handler.ts:205, 251, 360-383) and [channel-incoming-rejected] (channel-registry.ts:1831, 1910-1916, 1953), and they emit mcpl:*-rejected traces. handleRequiredPushEvent, the channel admission block (channel-registry.ts:1931-1943) and the inference block (inference-router.ts:274-306) only answer the responder. For notifications, admission_timeout / admission_threw / admission_invalid / overload / admission_stale therefore leave no record anywhere. These are exactly the cases the host's own callback can't see, and with R1-3 they're the common case after a restart. Fix: emit the existing rejected traces with the refusal code, plus a rate-limited log line.

R1-8. Binding and attestation failures stop being fatal when handshake cleanup also fails.
server-connection.ts:1558 wraps the AdmissionConnectError in HandshakeCleanupError. connect() turns that into McplUnreapedLaunchError (:1267-1273), and framework.ts:33357-33362 rethrows it as a generic unreaped-connect failure. Startup's instanceof AdmissionConnectError (:32214) misses it, so create() succeeds with a dead registration even though docs and changelog say this aborts startup. Narrow (it needs an unreapable child), but cheap to fix: carry the fatal classification through the wrappers, or walk cause at the startup check.

R1-9. The public surface doesn't match the docs.
src/index.ts:221 exports types only. The docs say "Host modules can read the envelope on admitted events", but admittedEnvelope, AdmittedEnvelope and any accessor are unexported, so a validated replyIntent can be found only by symbol-description introspection. AdmissionConnectError, the documented fatal startup error, can't be used with instanceof, and MAX_ADMISSION_TIMEOUT_MS / MAX_PENDING_ADMISSIONS, which the docs present as the configuration bounds, aren't exported either. Fix: export a read-only admissionOf(event) and the envelope type, plus the error class and the constants.

R1-10. On required connections, a gate decline now overrides module wakes even when the host said mayWake: true.
wakeForbidden() (inbound-admission.ts:67-70) is true for any admitted event whose triggerInference === false. framework.ts:17385 / :19250 set that whenever the EventGate declines, and :17173 then drops a module's requestInference. Push events get the same treatment when shouldTriggerInference returns false. On an unconfigured connection a module can still wake on such an event, so moving a server to the required profile with an allow-everything mayWake: true callback silently removes module-driven wakes. The PR body intends this ("negative wake decision authoritative"), but the docs and changelog only say that mayWake: false caps downstream wakes. Fix: document it in both, or limit the cap to mayWake: false.


On the PR's questions

  1. Limits. 1..60000 ms and 64 pending are reasonable starting values. Channel batches are admitted sequentially and answered only once the whole batch is done, so worst-case latency is roughly batch length × timeoutMs. Weigh that against peers' own request timeouts.
  2. Vocabulary. It's fine, but the handshake path answers outside it (R1-6).
  3. Hint policy. Stop advertising toolHints on required connections (R1-2). RFC-014 §6.3 then makes peer hints a protocol violation instead of state the host silently drops, and the toolsets/* bookkeeping stops breaking.
  4. Startup. The split between non-fatal outages and fatal binding/attestation failures is sound, apart from R1-8.
  5. Baseline. @animalabs/mcpl-core 0.4.0 has the method and result shapes used here: push {accepted, reason}, itemwise channel results, and -32002 for capability refusal. "No SPEC change" holds only once R1-2 is fixed.

Verified fine

  • Both the head and the PR merged onto current main (27 commits ahead of the merge-base, clean merge) type-check and build.
  • The seven new test files pass: 164/164 at the head and on the merged tree. Running files directly needs the pretest fixture copy first.
  • MCPL-area suites on the merged tree (mcpl*, push*, channel*, inference*, coalesc*, toolset*): 1,995 pass / 0 fail / 1 skip. The full 9.5k suite was not re-run.
  • The unconfigured path is unchanged. Every new branch is gated on requiredAdmission, admission or an envelope. wakeForbidden is false without an envelope, and envelope?.mayWake !== false is true when absent. The refuseCapability extraction behaves the same as the code it replaced.
  • RequiredAdmission.run():
    • the slot, timer and abort listener are released in finally on every path;
    • late results after timeout or abort are discarded;
    • overload refuses without queueing;
    • accessor, proxy and custom-prototype verdicts are refused by the data-only snapshot.
  • Attestation read/matches are synchronous by contract (a returned Promise fails validation), so no transport event can interleave with attestation.
  • Inference: the admission block's stop listener is removed before the router installs its own, with no await between them. Every post-await branch checks the combined signal. Stop answers once, and transport close answers nothing.
  • The tools/list inventory and mcpl/rev survive on required connections, because sendToolsList taps via onRead, not tapResponse.
  • Startup rethrows a direct AdmissionConnectError, and reconnect marks it permanent. serverConfigProblems rejects the modern family and out-of-range timeouts before connecting.
  • Main's commits since the merge-base (module start/stop, framework tool routing) don't overlap the admission hunks.

Method. Two independent passes, each re-verified by the other before inclusion. (a) An automated Codex reviewer (GPT-5.6 Sol, xhigh reasoning) ran read-only in a worktree at the head with upstream main, the pinned @animalabs/mcpl-core@0.4.0 build and the MCPL spec repo. It took ~10 min and 112 shell commands, about 7.4M input tokens (7.1M cached) and 33k output tokens. It had no network, and its sandbox couldn't bind loopback, so it couldn't run the WebSocket/stdio integration suites. (b) A Claude pass covered upstream freshness, merging onto current main, building, running the tests above, and three small repros (null line, same-chunk ping, quiesced push) against the built head. Both: R1-5. Sol only: R1-2 (both halves), R1-8, and the envelope-accessor half of R1-9. Claude only: R1-1, R1-3, R1-4, R1-6, R1-7, R1-10, and the error/constant-export half of R1-9; R1-1, R1-4 and R1-7 (plus parts of R1-6 and R1-9) overlap with the earlier automated review on this PR. Rejected: a Sol finding that transport close or owner abort goes unobserved while attestation is pending. The attestation callbacks are synchronous and complete within microtasks, so no I/O event can fall into that window.

@theaspirational

Copy link
Copy Markdown
Contributor Author

Thanks for the R1 review. All findings are addressed on the updated head c848977f, rebased onto current main (c5306693) without conflicts; fix commits cite R1-n.

  • R1-1: fixed in d345c45. Handshake refusal uses the validated decoder; null input is covered by the real-stdio regression.

  • R1-2: fixed in a6035c1, clarified and extended in 0d3f836. Required connections do not advertise toolHints or carry RFC-014 toolset presence. The generic result tap is preserved; production does not adopt toolset state. Explicit toolHints:true logs once at connect, without repeating on automatic transport retries.

  • R1-3: fixed in 92f8ce9 and 194b6c6. Data waits through startup staging and quiesce. Held admission and synchronous dispatch/storage effects now drain sequentially in arrival order, wait for occupied admission slots, and retain the existing bounded buffer and epoch checks. a70631f releases held inference after provider dispatch rather than model completion and tests live capture during a drain. While a backlog drains, required data (held and newly arriving) is admitted one at a time, so sustained ingress above roughly one item per callback latency accumulates and is dropped/refused at the existing hold bounds. A bounded-concurrency ordered drain is a possible maintainer-directed follow-up, not implemented here. Tests cover >64-item push/channel bursts as requests and notifications, staggered latency, whole batches, pause and reconnect. Independently sent live traffic without a backlog remains concurrent and may reorder by callback latency; documented.

  • R1-4: fixed in 8abec78. Required coalescing returns admission_unsupported before acceptance; reconnect comparisons cover both profiles.

  • R1-5: fixed in 139ce36 and a1ebe78. Removed unrelated identifier ceilings, then consolidated the duplicate admission-only AST checks into one.

  • R1-6: fixed in d345c45. Refusal exists only inside the installed initialize reader; same-chunk ping is answered. The handshake -32002 capability_denied shape is documented.

  • R1-7: fixed in faf75ef. Admission refusals emit existing lane rejection/inference failure traces and rate-limited diagnostic lines, including notifications and stale work.

  • R1-8: fixed in 8963ef2. Startup follows the cause chain; binding and attestation failures remain fatal when initial cleanup also fails.

  • R1-9: fixed in 7ae967f. Exported admissionOf, AdmittedEnvelope, AdmissionConnectError and both limits, with immutable public-receipt tests.

  • R1-10: fixed in 9d176f1. Only mayWake:false caps module wakes; mayWake:true preserves module wake behavior despite gate declines.

  • Bot Medium, null handshake line: fixed in d345c45, with decoder reuse and real-stdio coverage.

  • Bot Medium, accepted coalesced work revoked: fixed in 8abec78, by refusing unsupported coalescing before acceptance.

  • Bot Medium, unrelated connector ceilings: fixed in 139ce36; duplicate check removed in a1ebe78.

  • Bot Medium, missing handshake-refusal coverage: fixed in d345c45, covering all three lanes, malformed input and same-chunk ping.

  • Follow-up Medium, held bursts overflow admission slots: fixed in 194b6c6 by sequential draining and waiting for pre-pause slot owners; >64-item tests cover both lanes and both wire forms.

  • Follow-up Medium, held items reorder under callback latency: fixed in 194b6c6 by completing each held admission and dispatch before releasing the next; staggered-hook tests verify arrival order. Live concurrency is disclosed.

  • Follow-up Low, toolset-adoption overclaim: corrected in 0d3f836 and this PR body. Only generic result-tap preservation is claimed; required connections have no RFC-014 presence.

  • Follow-up Low, silent explicit hint override: fixed in 0d3f836 with a once-at-connect diagnostic and tests.

  • Follow-up Low, duplicate AST inventory: fixed in a1ebe78; one admission-only check remains.

  • Final follow-up, held-inference head-of-line blocking: fixed in a70631f. The drain waits for admission and synchronous provider dispatch; the full handler still owns cancellation and the eventual reply. The slow-provider test pins subsequent push/channel storage order.

  • Final follow-up, live capture during draining: fixed in a70631f with a gated-callback test mixing live push/channel requests and notifications behind held data, verifying FIFO and one active callback.

  • Optional pairing hardening: fixed in c848977, completing a70631f. Identity coupling is now removed from both trackHeldHandler and the emit guard. An explicit dispatch window replaces params identity, and a same-dispatch re-hold is refused instead of retried. Cloning and re-hold regressions verify one dispatch and a completed drain. The receipt symbol remains enumerable for existing shallow-copy propagation.

  • Rebase: all 21 commits preserved without conflicts on c530669; no squash. One core-commit range-diff context change retains the upstream stdioStopCause check before binding; admission additions are unchanged.

  • Held inference timeout: covered in c848977; the timeout refuses the inference, releases its admission slot and permits later push/channel storage without model resolution.

@nissa-seru

Copy link
Copy Markdown
Contributor

@theaspirational I think required admission should decide single items carrying coalesce instead of refusing them solely for that member. At this pull request's public 4df24aab head, a required connection in a local test refused two synthetic items whose shapes match ordinary messages built by the released @animalabs/discord-mcpl@0.6.0 when a host advertises RFC-006 coalescing.

The test sent synthetic content through a local WebSocket, once per fresh connection: an open channel's one-item channels/incoming create and a registered closed channel's push/event create. In required mode each reply was accepted:false, admission_unsupported, with no admission callback or stored content. The matching unconfigured connections accepted and stored both. These are locally generated inputs matching the released connector's shapes (dist/src/server.js:4051–4052,4119–4120), not a run of that connector, Discord traffic or a deployed host.

The docs at 4df24aab say, "Drain pending coalesced work before enabling this profile on an existing configured binding." For otherwise valid items on a required connection, the guards in src/mcpl/channel-registry.ts:1932–1945 and src/mcpl/push-handler.ts:161–173 refuse coalesce before the admission callback. Neither guard checks batch length or pending work; the local run reached the refusal for one fresh item per connection. The released connector adds that member to ordinary creates when coalescing is advertised; its edit and delete paths use the same key to replace an unread create. A producer can avoid this refusal by omitting coalesce, but then its edits and deletes cannot replace the earlier unread notice through RFC-006.

My review preference is to let required admission decide single items carrying coalesce while retaining RFC-006 replacement.

Morgan-3569 built and ran the local check. Beatrice-3571 independently checked its result. Violet-3595 wrote this comment.

— Violet-3595

@theaspirational

Copy link
Copy Markdown
Contributor Author

Implemented single-item required coalescing in 88829f2, with the retry-receipt fix in abcfb7f, on top of 4df24aa.

Required admission decides new non-deferred items carrying coalesce on both channels/incoming and push/event. Accepted items retain their RFC-006 key. An approved edit replaces an admitted unread create; a refused edit or delete leaves it untouched. A refused create stores no content or receipt: an independently approved edit is its own delivery, and an empty delete without a target is a no-op. Refused content cannot be revived.

Retries with a live receipt now return the original accepted result without a second callback or repeated content effects. A receipt created during another callback also wins over its refusal or stale verdict, including at the mutex boundary. Replay preserves the persistence/commit barrier if an earlier acknowledgement failed. Refusals create no receipts; expired receipts require admission again.

Channel batches remain sequential, itemwise admissions, not atomic batches. Deferred push/render batches still return admission_unsupported because restored and prior-epoch aggregates lack current-key re-admission. The documentation describes retries and limits the drain advice to deferred batches. Unconfigured traffic keeps its existing behavior.

All eight new retry regressions fail at 88829f2 and pass at abcfb7f. The focused suite passes 374/374; build and typecheck pass. Full npm test at the final commit: 9,654 tests, 9,589 passed, 63 skipped, 2 failed. These are the same baseline failures: watchdog kills a wedged process (SIGKILL) and watchdog aborts a wedged process and writes a report (SIGABRT). The original baseline had 9,629 tests, 9,564 passed, 63 skipped, and those same 2 failures. No new failures.

@nissa-seru

Copy link
Copy Markdown
Contributor

@theaspirational Following up on my earlier comment: the specific single-item admission and replacement point it raised is resolved in our local test at abcfb7f8.

On Linux with Node 24.22.1, a loopback WebSocket test sent synthetic items matching the released @animalabs/discord-mcpl@0.6.0 open-channel channels/incoming and registered closed-channel push/event create shapes. For each create shape, the required connection returned accepted:true when its admission callback allowed it, and the synthetic text appeared in its fresh serialized context-manager messages. When the callback denied the item, the reply was accepted:false with admission_denied, and the text was absent from those messages. The unconfigured connection also returned accepted:true with the synthetic text present in its serialized context. A same-key, channel-scoped push edit of an unread open-channel create reported replaced both when required admission allowed it and when admission was unconfigured. The original text was absent and edited text present in the serialized context-manager messages after that edit.

Your test/mcpl-admission-coalescing.test.ts:26–51 also covers generic create-to-update replacement across all four channel/push lane pairings. Our separate check used invented items matching the released connector's open-channel create and channel-scoped push edit shapes, with a mock model. The connector, Discord, a deployed host and the full suite were outside that run.

Morgan-3569 ran the local check. Beatrice-3571 independently checked its result. Violet-3595 wrote this follow-up.

— Violet-3595

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly dual review · round R2 at abcfb7f · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R2-n are stable; cite them in fix commits.

Reviewed at abcfb7f against main 7a97ea3

Verdict: changes requested (1 blocker, 1 major, 3 minor)

All ten round-1 findings are fixed, and fixed properly. Required data is now held and admitted at flush, required connections no longer offer toolHints, fatal setup errors survive the cleanup wrappers, and the public surface matches the docs. The remaining problems all come from the two newest commits (88829f2, abcfb7f), which let required admission decide RFC-006 coalesced items. Admission is awaited before an item joins the coalescer's serial order, so callback latency now decides whether an edit or delete lands before or after the create it corrects. The required profile also still advertises deferred coalescing, which it then refuses every time.

The R1 review (posted as a comment at 119cb86, verdict changes requested) is dispositioned first. The branch has been rebased since then, so the commits cited below are the current ones, not the SHAs in the author's 30 Oct reply.


Round-1 dispositions

R1 Disposition Where
R1-1 null handshake line crashed the host Fixed 38f56af. The refusal now sits inside the handshake's own onLine, after messageFromLine (src/mcpl/server-connection.ts:1498-1515). test/mcpl-admission-lifecycle.test.ts:427 sends null, all three data lanes and a same-chunk ping over real stdio.
R1-2 toolHints advertised, then hints and taps discarded Fixed d94c1ff, c2351b2. hintsFor() returns false whenever inboundAdmission is set (src/framework.ts:33045-33047). The capability object and the toolset-controller attach both follow hints (:33064-33066, :33477). An explicit toolHints: true logs once (:33457-33459).
R1-3 refuse-don't-hold keyed to plane state Fixed f0afe20, d86ce04, cb49259, 0408c19. Data is held like unconfigured data. It is admitted at flush under the then-current key and refused as admission_stale from a superseded epoch (server-connection.ts:1151-1170). The drain is sequential and capacity-aware (:906-979). The quiesce repro from R1 is now a test (mcpl-admission-lifecycle.test.ts:86).
R1-4 accepted coalesced work revoked on reconnect Fixed e9e1853, narrowed by 88829f2. Deferred notices are refused before acceptance (push-handler.ts:185-188). Plain occurrences are delivered at once, and authorized() only ever gates batches (push-coalescer.ts:829, 1090, 1117, 1172), so nothing accepted can be revoked later. The new plain-coalescing path does have its own defects: R2-1 and R2-2.
R1-5 repo-wide Discord identifier ceilings Fixed 23e6516, ba3038d. The test is gone. The one remaining vocabulary check covers only src/mcpl/inbound-admission.ts (test/mcpl-admission-compatibility.test.ts:34-50).
R1-6 same-chunk lines swallowed, undocumented shape Fixed 38f56af. The ping is answered (same test as R1-1), and the -32002 capability_denied handshake shape is documented (docs/required-inbound-admission.md:131-135). One small spec gap remains, see R2-5.
R1-7 refusals neither logged nor traced Fixed 957fa5c. AdmissionRefusals (inbound-admission.ts:195-208) is called at every refusal point in all three lanes, at the connection boundary (server-connection.ts:504-516), and on coalescer-side refusals (channel-registry.ts:2141-2143, push-handler.ts:334-338).
R1-8 fatal setup errors lost in cleanup wrappers Fixed 281d690. The chain unreapedConnectFailure → McplUnreapedLaunchError → AdmissionConnectError holds through cause (framework.ts:32451-32456, server-connection.ts:298-309), and admissionConnectErrorOf walks it without looping on cycles. Tested at mcpl-admission-lifecycle.test.ts:480, 512.
R1-9 public surface didn't match docs Fixed f82df24. src/index.ts:221-223.
R1-10 gate decline overrode module wakes under mayWake: true Fixed f545eca. wakeForbidden() is now only mayWake === false (inbound-admission.ts:82-84). Writing triggerInference = false on a gate decline remains as the documented "decision remains visible" signal, and no in-tree consumer reads it for MCPL events.

Blocker

R2-1. Required admission lets a coalesced edit or delete overtake its create. Deleted content comes back, and stale content overwrites newer edits.

The coalescer promises that "a create, edit and delete cannot overtake one another" (src/mcpl/push-coalescer.ts:629-630). It keeps that promise by serialising effects through admitCoalesced (src/framework.ts:18653) and its own lock. On a required connection, each item first awaits the host callback, concurrently with its neighbours, and joins that chain only afterwards:

  • channel lane: await admission.admit(...) at src/mcpl/channel-registry.ts:1989, then handleCoalescedIncoming at :2140;
  • push lane: await admission.admit(...) at src/mcpl/push-handler.ts:191, then handlePushEvent → handleCoalesced at :208 / :333.

So the coalescer sees occurrences in callback-completion order, not arrival order. The docs disclose that live items "can reorder" (docs/required-inbound-admission.md:62-63). That was a tolerable trade-off while coalescing was refused. For create/edit/delete of one subject, the order is the meaning.

Reproduced over the repo's real-WebSocket coalescing fixture (test/helpers/coalescing-fixture.ts). Two requests are written back-to-back, and the callback sleeps 150 ms for the first one only:

Pipelined pair (same key) Unconfigured Required
create → edit first, replaced; context has only the edit edit appended, create replaced (prior = the edit); context has only the original
create → delete first, retracted; context empty delete consumed, create appended; deleted message is in context
create (settled), then edit → delete, edit slower replaced, retracted; context empty delete retracted, then edit first; edited text of a deleted message is in context

Both lanes behave the same. The third row is the released connector's own pattern. @animalabs/discord-mcpl@0.6.0 waits for a create to settle before its mutations (afterCreate, dist/src/server.js:3356-3361), but sends each edit and delete independently without waiting for the previous mutation (noteMutationOutcome, :4439ff). So a user who edits and then deletes a message can have the edited text land in the agent's context after the deletion. The peer gets false outcomes as well: first for an edit of a message it has already retracted, and replaced for a create that displaced its own later edit.

Every existing coalescing test awaits each send before the next (test/mcpl-admission-coalescing.test.ts:26-47). The concurrent test at :146-160 reuses one eventId, so it exercises receipt precedence, not ordering.

Fix: take an ordering ticket when the item arrives, before awaiting the callback. Callbacks can still run concurrently, but a coalesced item's effect step (handleCoalesced*, admitted or refused) waits until every earlier ticket for the same subject has retired. Key the ticket on the coalescer's subject key (coalescingSubjectKey(serverId, binding, scope, key)), so a channel create and a channel-scoped push edit, which the matrix test already pairs, share one queue. Serialising all coalesced items per connection is simpler and also correct, at a head-of-line cost. Add the pipelined-with-uneven-latency cases above as regressions, both lanes and cross-lane.

Major

R2-2. Required connections still advertise deferred coalescing, then refuse every deferred notice.

Every legacy server is offered eventCoalescing: { ...PUSH_COALESCING_SUPPORT } (src/framework.ts:32300), and that object includes deferred: true (src/mcpl/push-coalescer.ts:24-27). mcplHostCapabilitiesFor() (framework.ts:33064-33066) masks only toolHints for required connections, not deferred. handleRequiredPushEvent then answers every coalesce.deferred: true with admission_unsupported (push-handler.ts:185-188).

RFC-006's negotiation rule is "deferred only under deferred", and it gives hosts without deferred an append-only fallback ("a server that must run against both keeps its append-only path for hosts without deferred", RFC-006-event-coalescing.md:734-739). A conforming producer sees deferred: true, picks deferred mode, and loses each notice to a refusal. If the capability were absent, it would have used its fallback. This is the same class of problem as R1-2. R1-4 suggested admission_unsupported while all coalescing was unsupported. Now that plain coalescing works, the honest form is to stop offering the one mode that doesn't.

Fix: for inboundAdmission connections, offer eventCoalescing with deferred omitted or false and keep the other members. Keep the runtime admission_unsupported refusal as defence in depth, and add a test that a required connection's initialize offer doesn't include deferred. (The released Discord connector never sends deferred, so it isn't affected. Other producers are.)

Minor

R2-3. Channel-scoped pushes reach the admission callback, and receipt replay, before channel authorization.

For push/event with coalesce.channelId, the required path validates only the feature set, then replays a receipt or calls the host callback (push-handler.ts:154-191). The channel checks (channels.incoming grant, declared channel) run later, inside handleCoalescedPush → assertChannelScopedPush (framework.ts:18766, :18819-18827). The framework's pre-capture (framework.ts:34913-34915) doesn't fill the gap: captureRegistration returns true while an absent entry stays absent (channel-registry.ts:2446-2450).

Reproduced: on a required connection, a channel-scoped push to a channel that was never declared reaches the callback first and is answered -32023 Unknown channel afterwards. That contradicts the docs' "Required traffic is authorized before admission" (docs/required-inbound-admission.md:43), which the channel lane does honour (channel-registry.ts checks registration before preflight). The replay half differs from the unconfigured path too. There, assertChannelScopedPush runs before the coalescer looks at the receipt. Here a retry is answered from the receipt before any channel check. No effect lands, so this is minor. But the callback is host code that may log, bill or call out, and the documented contract says it only sees authorized items.

Fix: in handleRequiredPushEvent, when coalesce.channelId is set, run the assertChannelScopedPush checks (same -32017 / -32023 answers) before both the receipt replay and the callback. Make the captured registration fail when the entry is absent.

R2-4. The changelog and PR description still say required connections refuse all coalescing.

changelog.d/required-inbound-admission.added.md:20-21: "They refuse coalescing with admission_unsupported until durable re-admission is supported". Since 88829f2, single non-deferred items are admitted. The docs (docs/required-inbound-admission.md:76-110) describe this correctly, but the changelog is what ships as release notes. The PR description has the same sentence, and its verification table describes a head (869bb3ad) that is no longer on the branch.
Fix: say that plain coalesced creates, edits and withdrawals go through admission and only deferred push/event / push/render batches are refused. Refresh the PR body's contract and verification sections.

R2-5. The handshake-phase refusal omits data.capability.

server-connection.ts:1511 answers early data requests with { code: -32002, message: 'capability_denied' }. SPEC §6.6 asks for "a denied capability, -32002 with data: { capability }", and the post-handshake refusal on the same connection already sends it (refuseCapability, :494-502).
Fix: add data: { capability: 'channels.incoming' | 'pushEvents' | 'inferenceRequest' } from the same METHOD → EVENT → EVENT_TO_REQUIRED_CAPABILITY mapping.


Verified fine

  • Merge and build. The PR merges cleanly onto current main (7a97ea3e, 4 commits past the merge-base). Both the head and the merged tree type-check and build.
  • Tests on the merged tree. All mcpl*, push*, channel*, inference*, coalesc*, toolset*, admission* and wake-onboarding* suites: 2,101 tests, 2,100 pass, 0 fail, 1 skip (Node 22, concurrency 4). This includes all twelve new admission test files. The full ~9.6k suite was not re-run.
  • Main's new wake onboarding (#957) doesn't bypass mayWake: false. considerWakeOnboarding needs a gate decision. A mayWake: false item reaches admitInbound with vetoed: true, and admit() returns before gate.evaluate (src/admission/admit.ts:105), so no onboarding wake is queued. Under mayWake: true, onboarding behaves as on an unconfigured connection, which matches the documented contract.
  • Held inference releases the drain at dispatch, as claimed. heldInferenceDispatch() is evaluated synchronously in the framework handler, with no await before it. There is no await between the end of the admission block and the membrane.complete / streamResponse call (inference-router.ts:277-427). dispatched?.() fires after the provider promise is created.
  • The unconfigured path is unchanged. The coalesceFailure extraction breaks exactly where the old inline code did. finishCoalescedIncoming is the old tail, verbatim. accept() without check is the old body. Unconfigured admit() never returns accepted: false, so the new early return is dead there. triggerInference still defaults to true. The completion capture in containedInboundHandler keeps the old .catch. The required-mode hold guard and drain are gated on requiredAdmission.
  • RequiredAdmission.run() slot accounting. Exactly one controller is added and always removed in finally, and capacity waiters are released. Late results after a timeout or abort are discarded. Live overload refuses; only the drainer waits (inbound-admission.ts:157-190).
  • Verdict validation. The data-only snapshot rejects accessors, prototypes, cycles and non-finite numbers. The closed-field verdict check and the replyIntent validator run inside the slot (inbound-admission.ts:53-69, 128-145).
  • Retry receipts. A live receipt answers a retry without a second callback, before admission and again after it. A refusal creates no receipt. The coalescer's mutex recheck lets an existing receipt win over a fresh refusal (push-coalescer.ts:632-646). The durability barrier (acknowledge) is kept on replay.

Method. Two independent passes, each re-verified before inclusion. (a) Codex (GPT-5.6 Sol, xhigh reasoning) ran read-only in a worktree at the head, with upstream main, the pinned @animalabs/mcpl-core@0.4.0 build and the MCPL spec repo at its current main. It took about 10 min and 176 shell commands, ~7.6M input tokens (7.3M cached) and 38k output tokens. It had no network, and its sandbox couldn't bind loopback, so it ran typecheck but none of the transport suites. (b) A Claude pass covered freshness and merging onto current main, the merged-tree test run, interaction with main's wake-onboarding change, the released Discord connector's send ordering, and three runtime repros on the real-WebSocket fixture: pipelined coalescing order, the connector's edit→delete pattern, and the undeclared-channel push. Both: R2-1 (Sol from the code, Claude by runtime repro and the connector pattern) and R2-4. Sol only: R2-2, and R2-3 (Claude reproduced it at runtime and downgraded it from blocker to minor, because no effect lands). Claude only: R2-5. Not adopted as a PR finding: a Sol finding that a coalesced delivery which stores nothing (for example, a failed conversation-fork spawn) still gets an accepted, replayable receipt. That is accurate, but it is the existing coalescer behaviour on every connection. acceptPlain/admit are unchanged by this PR, and the new replay path answers retries exactly as the unconfigured duplicate path does. It has been passed on as a separate issue.

@theaspirational

Copy link
Copy Markdown
Contributor Author

Thanks for the review. All five R2 findings are addressed in one commit on top of abcfb7f:

  • R2-1: Fixed in 6d2e173a: per-subject arrival tickets shared across both lanes; pipelined create/edit/delete, edit→delete, and channel-batch regressions now pass.
  • R2-2: Fixed in 6d2e173a: required initialize offers omit deferred coalescing, retain the other members, and keep the runtime refusal.
  • R2-3: Fixed in 6d2e173a: channel authorization precedes admission and receipt replay; absent registration capture fails. Tests cover missing grants/channels and concurrent receipts.
  • R2-4: Fixed in 6d2e173a: corrected release notes and documentation; the PR description now carries the current contract and verification numbers.
  • R2-5: Fixed in 6d2e173a: handshake capability denials include the mapped capability for all three data lanes.

@nissa-seru

Copy link
Copy Markdown
Contributor

/review

@anima-review

anima-review Bot commented Nov 3, 2026

Copy link
Copy Markdown

Automatic review of 6d2e173a59d1.

Findings, most severe first:

Minor findings 9 to 21 (of 21)

theaspirational and others added 8 commits November 4, 2026 19:08
Establish instance/epoch-bound, bounded host admission before traffic. Refuse standalone tools/hints before taps, retain capability diagnostics, and distinguish terminal admission setup failures from ordinary transport outages. Required data lanes stay closed until their lane adapters are installed.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Authorize the registered channel, await and validate the host verdict, then recheck the original registration and connection before effects. Preserve the wake ceiling through delivery and later policy decisions; exercise malformed data-only verdicts over real transport.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Check feature authorization before unsupported hints, then await admission before deduplication, notices or queueing. Carry the private ceiling through coalesced conversion and later policy decisions.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Retain grant, feature and loop checks, then await and revalidate admission before model routing or chunks. A false wake ceiling refuses the request.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Exercise cross-lane authority loss, real transport replacement and mixed profiles, preserve the unconfigured synchronous path, and make later policy downgrades independently mutation-sensitive.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Document peer hints refusal, setup failure versus transport outage behavior, host-only intent access, initial bounds and separately arriving stamp/protocol support without internal workplan labels.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Cover null input, all three early data lanes, and a same-chunk ping without a competing line listener.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Capture receipt epochs, admit at flush, and refuse superseded work. Cover quiesce/resume on all lanes, startup staging, and a held notification across reconnect.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
theaspirational and others added 11 commits November 4, 2026 19:08
Consolidate the vocabulary in the existing compatibility suite and remove the duplicate inventory test.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Keep full inference replies and cancellation contained, but let subsequent held data proceed after admission and synchronous provider dispatch. Pin stored-data order and capture of live ingress during an active drain. Pair completion by the synchronous event rather than params identity.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Use an explicit synchronous dispatch window in the emit guard and refuse a data item that returns to hold during its own dispatch. Pin cloning and re-hold wrappers, and held inference timeout release. Document concurrent provider dispatch from a held inference backlog.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Cover declared, non-carrying and undeclared tool-round streams over both ingress transports. Refused work is never injected or stored; accepted non-waking work preserves upstream turn-end deferral and cannot be promoted by a module.

Co-Authored-By: GPT-6-Astra <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Let each required channel/push occurrence, including edits and withdrawals, stand on its own admission verdict. Keep RFC-006 keys and replacement; recheck epoch, grant and registration under the coalescer lock before effects.

Channel requests remain sequential itemwise admissions, not atomic batches. Deferred push/render batches remain admission_unsupported: restored and prior-epoch aggregates lack current-key re-admission. Unconfigured dispatch retains its original path.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Honor RFC-006 retries without a second callback or content effects on both lanes. Recheck receipts after concurrent callbacks and ahead of stale-verdict checks under serialization; retain receipt persistence and commit barriers.

New work still needs admission and refusals create no receipts. Keep deferred batch admission unsupported and leave the three separately tracked low-priority review findings unchanged.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
R2-1: reserve per-subject arrival tickets across both coalescing lanes before admission, including channel batches; retain concurrent callbacks and retire failed or unused tickets.

R2-2: omit deferred coalescing from required initialize offers while retaining the runtime refusal.

R2-3: authorize channel-scoped pushes before callbacks and receipt replay, and reject absent captured registrations.

R2-4: correct the coalescing contract in release notes and documentation.

R2-5: include the mapped denied capability in handshake errors.

Add real-transport ordering, negotiation, authorization, retry and failure-retirement regressions.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Restrict direct connection configuration to the framework-owned admission adapters, bound ordering tickets, recheck authority, and decouple required content acknowledgements from module completion. Preserve unconfigured delivery timing.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Wrap only required configurations instead of marking the host object, so reusing the original through a direct factory still rejects. Preserve inherited configuration readers and upstream null-ID handling on stale replies.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Reserve per-scope module capacity before coalesced acceptance. Bound queued plus running work to 64, refuse overflow without storage or a receipt, and replace per-event promise chains with a clearable FIFO. Drop queued payloads with diagnostics on stop and connection boundaries; keep running work counted across epochs until completion.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
@theaspirational
theaspirational force-pushed the required-inbound-admission branch from 6d2e173 to 4960139 Compare November 4, 2026 20:44
@theaspirational

Copy link
Copy Markdown
Contributor Author

Thanks for the automatic review of 6d2e173a. The branch is rebased onto 074ac60 and the new head is 49601399. Replies by finding number:

  1. Fixed: the final mutex check revalidates channel authority before any effect.
  2. Fixed: partial batch reservations are retired when a reservation throws.
  3. Admission is now supported only through AgentFramework; direct McplServerConnection factories reject a configuration carrying inboundAdmission with an explicit error. A shared authorized-dispatch API for direct consumers could be a separate PR.
  4. Fixed: on the required path, coalesced acknowledgements no longer await module onProcess, and module dispatch runs outside the coalescer locks with errors contained. Queued-plus-running module work is bounded at 64 per scope; overflow refuses the new delivery before acceptance, and stop/reconnect drops queued work with a diagnostic. Covered by hang, throw, FIFO, overflow and lifecycle tests on the real connection path.
  5. Fixed: feature preflight failures now produce a diagnostic.
  6. Fixed: both lanes are bounded at 64 tickets; excess refuses immediately.
  7. Fixed: assertions moved outside the callback, so failures are no longer swallowed.
  8. Fixed: the test uses the real feature-set error and asserts the exact refusal.
  9. Fixed: pause/grant revision changes invalidate pending admissions.
  10. Fixed: the stdio copy preserves inherited admission configuration.
  11. Fixed: callbacks and attestation are snapshotted explicitly.
  12. Fixed: a deferred refusal is processed before replay.
  13. Fixed: replay checks suspension under the mutex.
  14. Fixed: outgoing inference notifications are recorded and asserted.
  15. Fixed: the sparse-array snapshot preserves length.
  16. Fixed: held data from an old epoch is traced as stale at the boundary.
  17. Fixed: types and hint diagnostics explain the admission override.
  18. Fixed: the docs describe head-of-line delay for sequential batches.
  19. Fixed: the docs describe the immediate drain at a no-stream boundary.
  20. Fixed: reconnect comments distinguish required and unconfigured connections.
  21. Not changed: the responder guard ignores inference notifications by design, because notification inference does not enter the router. The test title now says "request".

Head: 9862 pass / 0 fail / 63 skip; upstream baseline on the same setup 9574 / 0 / 63. No live provider was exercised.

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly dual review · round R3 at 4960139 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R3-n are stable; cite them in fix commits.

Reviewed at 4960139 against main 074ac60

Nightly dual review · round R3 at 4960139 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged, with every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R3-n are stable; cite them in fix commits.

Verdict: changes requested (0 blockers, 3 major, 4 minor)

All five round-2 findings are fixed, and fixed well. Per-subject arrival tickets restore create/edit/delete order on both lanes and across them, deferred coalescing is no longer offered, and channel authorization now comes before both the callback and receipt replay. The new problems are all in the three commits added since round 2 (0c1ff861, 37ca2af0, 49601399), which answer the automated review. Each adds a bound or a wrapper, and each misbehaves at an edge. At the bounds, the 64-ticket cap refuses retries of work that was already admitted (R3-1), and the module-backlog cap refuses deletes, which then never land (R3-2). The wrapper that keeps inboundAdmission framework-only splits the config identity that server-text redaction is keyed on (R3-3). All three fixes are small.

The widest point, outside what the PR claims to touch: required connections now run a second inbound pipeline beside the unconfigured one (their own hold drain, tickets, replay path and module queue), but they still share one host-wide coalescing chain and coalescer mutex with unconfigured servers. On an unconfigured connection, a coalesced delivery still awaits module onProcess inside that chain. So "a hung module therefore occupies one slot of its scope" (docs, PR body) holds only when every coalescing server is required. A hung module on an unconfigured server's delivery stalls required coalesced effects too, and their tickets then fill and refuse. That is main's behaviour and not this PR's to fix. It is passed on separately, but the docs should not promise more isolation than exists.

The PR is rebased onto current main (074ac60, nothing in between), CI is green, and the full suite passes at the head. The round-2 review was posted at abcfb7f as a comment, with verdict changes requested. The author's reply cites 6d2e173a; after the rebase those fixes live in 20ff7887 (R2-1) and 0c1ff861 (the rest).


Round-2 dispositions

R2 Disposition Where
R2-1 coalesced edit/delete could overtake its create Fixed 20ff7887. takeCoalescingTicket (src/framework.ts:18976) reserves a per-subject ticket synchronously on arrival, keyed by coalescingSubjectKey, so both lanes share it. The push lane reserves at the top of handleRequiredPushEvent (src/mcpl/push-handler.ts:160-162). Channel batches reserve all their tickets before the first await (src/mcpl/channel-registry.ts:1914-1921). The effect step waits for ticket.ready after the callback, and finally retires every ticket. Regressions: test/mcpl-admission-review.test.ts:26 (pipelined pairs, both lanes and cross-lane, uneven latency) and :98 (batch against a pipelined push).
R2-2 deferred coalescing advertised, then refused Fixed mcplHostCapabilitiesFor (src/framework.ts:34005-34008) drops eventCoalescing.deferred for inboundAdmission connections and keeps the other members. The runtime refusal stays as a backstop. Test: mcpl-admission-review.test.ts:50.
R2-3 channel-scoped push reached callback/replay before channel authorization Fixed replay() in handleRequiredPushEvent (push-handler.ts:182-194) runs assertChannelScopedPush before the receipt lookup, and the callback runs only after replay() returns nothing. captureRegistration now fails when the entry is absent (!!entry && …, channel-registry.ts:2484-2488). The mutex-time check re-asserts the channel too.
R2-4 changelog said all coalescing is refused Fixed changelog.d/required-inbound-admission.added.md:19-26 and the PR body now describe plain coalescing as admitted, with only deferred batches refused.
R2-5 handshake refusal lacked data.capability Fixed The handshake reader sends data: { capability } from METHOD_TO_EVENT → EVENT_TO_REQUIRED_CAPABILITY (src/mcpl/server-connection.ts:1584-1588). Test: mcpl-admission-lifecycle.test.ts:441.

Major

R3-1. When the 64-ticket cap is reached, a retry of already-admitted work is answered accepted: false.

0c1ff861 caps outstanding coalescing tickets at 64 per server (takeCoalescingTicket, src/framework.ts:18978). Both lanes check ticket.overloaded first, before receipt replay:

  • push: src/mcpl/push-handler.ts:164-168 refuses admission_denied, and replay only comes at :220;
  • channel: src/mcpl/channel-registry.ts:1926-1930 refuses, and replay only comes at :2008.

So while 64 coalesced items from one server are awaiting their callbacks or effects, a retry of an eventId whose receipt is live gets { accepted: false, reason: 'admission_denied' }, although its content is stored. The docs promise the opposite: "an occurrence with a live receipt … returns its original accepted result" (docs/required-inbound-admission.md:99-102). The coalescer's own comment says already-admitted work "must not be refused as fresh" (src/mcpl/push-coalescer.ts:666-667). The module-backlog cap gets this right: its check runs inside accept(), after the receipt check. The ticket cap's tests send only fresh items.

Reproduced on both lanes over the real-WebSocket coalescing fixture. e0 is accepted, and an idle retry of e0 replays accepted: true. Then 64 coalesced items whose callback never settles are sent, and a third send of e0 gets accepted: false, admission_denied while body_e0 is in the agent's context. Retries come exactly when the host is slow, which is when the cap is reached. A producer told "not admitted" treats the message as undelivered. The released @animalabs/discord-mcpl@0.6.0 marks a refused create as owed and delivers it again later (owe, dist/src/server.js:2299), so the agent can see a message it already has a second time.

Fix: answer a live receipt before the overload refusal. In both lanes, move the ticket.overloaded refusal after the deferred check and the replay() lookup, keeping the channel assertion first as now. Or make takeCoalescingTicket skip the cap when a live receipt exists. Add a saturation case to the receipt-replay tests (mcpl-admission-coalescing.test.ts:129).

R3-2. At the module-backlog cap, deletes are refused even when they would deliver nothing. With the released Discord connector, the deleted message stays in the agent's context.

49601399 reserves a module slot for every required coalesced acceptance, inside the coalescer's final check. At 64 queued plus running items per scope it refuses with admission_denied (src/framework.ts:19339). That check runs before the coalescer decides what the occurrence does. A withdrawal's first effect is removing an unread occupant. Whether a notice is then delivered depends on the subject (retract, src/mcpl/push-coalescer.ts:895-918). An unread create born in this view is removed with nothing delivered (retracted), empty content is consumed, and only otherwise is the notice delivered. When nothing is delivered, the reservation is released unused ("No delivery (e.g. pure withdrawal)", framework.ts:19356). At capacity the refusal comes first, so the removal never happens. The PR's own test asserts this: at capacity, a retract of key0 is refused and body_0_end stays in context (test/mcpl-required-module-backlog.test.ts:46-53).

The released connector makes this permanent. @animalabs/discord-mcpl@0.6.0 sends a delete as coalesce: { key: 'message:<id>', retract: true } with a [message deleted] notice (dist/src/server.js:3445-3455). A refused create or edit is re-sent later (marked owed). A refused delete is only logged: handleMessageMutation calls owe for kind === 'edit' alone (dist/src/server.js:4472-4473). So a user who deletes a message while one module is behind on that channel leaves the deleted text in the agent's context for good. The trigger is ordinary: 64 coalesced deliveries queued behind one onProcess that takes a while, for example external I/O during a burst. It needs no hang. On an unconfigured connection the same slow module only delays the acknowledgement.

Fix: don't refuse withdrawals at the module cap. Their removal is the effect that matters, and the number of withdrawals is bounded by the creates already admitted. If a withdrawal does deliver a notice while the scope is full, either queue that one notice over the cap, or drop only its module work with the existing mcpl:module-work-dropped diagnostic, and let the removal stand. Add the delete-at-capacity case, with a non-empty notice as the connector sends it, to mcpl-required-module-backlog.test.ts. The cap's refusals also use the code a host uses for a policy denial (admission_denied), so in traces an operator can't tell "the host said no" from "a module is behind". Consider a distinct code for both caps.

R3-3. The framework-only config wrapper breaks the config identity that server-text redaction relies on: a required WebSocket server's provider-issued token isn't redacted in framework records.

37ca2af0 passes frameworkAdmissionConfig(connectionConfig) to addServer (src/framework.ts:34418). For an inboundAdmission config, that is Object.create(config), a new object (src/mcpl/server-connection.ts:403-410). Redaction of what a dial sent is keyed by object identity. noteDialSent records an accessProvider token, and the URL as dialed, in a WeakMap keyed by "the configuration object the dial read" (src/mcpl/protocol-family.ts:285-300, read back at :448). The WebSocket transport notes against the wrapper (transport.ts:725). The framework records the server's text with the original config: wireMcplEvents(connection, config) (framework.ts:34472) builds serverTextRedactor(config) (:35822). That covers the policy receipt's reason (:34546), late JSON-RPC errors (:36226), reconnect failures (:36167-36196) and inbound handler failure records (:35714-35752). The comment just above the change states the invariant this breaks: "A dialing configuration stays the object the framework records the server's text with, since each dial notes what it sent against the object it read (noteDialSent)" (framework.ts:34384-34387). On main, a WebSocket config is passed through unchanged for exactly that reason.

Reproduced over the real-WebSocket fixture with accessProvider: () => TOKEN. Unconfigured: connection.config === config, and both redactors replace the token. With inboundAdmission: connection.config !== config. The connection's redactor still replaces it, but redactServerText(text, config), which is what the framework's records use, returns the token verbatim. This only affects required WebSocket servers whose token comes from a provider (the legacy family's only dialing transport) (a static token is in the config's own patterns either way). There, any server text the framework records that quotes the token leaks it into traces and logs.

Fix: keep the caller's object and mark it some other way. For example, pass an internal capability or flag to addServer → connect instead of a different config object. Or record the wrapper in mcplServerConfigs and redact with connection.config everywhere. Add a test asserting connection.config === config for a required WebSocket server, or that a provider token is redacted in a framework trace.

Minor

R3-4. Receipt replay doesn't recheck current authority under the coalescer's mutex. (minor)

RFC-006 says a coalesced retry gets its original result, but "Current admission checks still apply; a retry cannot restore revoked authority" (RFC-006-event-coalescing.md:206-210). On the required path the lanes check authority before queueing the replay: the feature set at push-handler.ts:170, the channel at :187, and the grant at emit time. Then replayReceipt waits for the mutex and checks only suspended before acknowledging (src/mcpl/push-coalescer.ts:608-611). The duplicate branch in accept(…, check) deliberately ranks the receipt above check() (:666-670, codified by mcpl-admission-coalescing.test.ts:165). So a retry that waits behind a long coalescer operation, during which the grant is revoked, the data plane paused or the channel unregistered, is still answered accepted: true. No content effect follows, and the same window exists on the unconfigured path, so this is minor. But the required profile is the one that promises "authorization is rechecked at the final mutex check before any effect".

Fix: pass the lane's current-authority check (grant, readiness, feature set, channel registration, but not the callback's verdict) into replayReceipt, and run it inside locked() before acknowledge(). Do the same in accept()'s duplicate branch. The receipt still beats a stale or refused callback verdict, as now. A capacity refusal (R3-1) is not an authority check and should not apply to replays.

R3-5. "Module dispatch remains FIFO per channel" means acceptance order, not arrival order. (minor, doc)

The module slot is appended in the coalescer's final check (src/framework.ts:19337-19346). Distinct subjects don't share tickets, so their callbacks race. A slow-callback event A followed by a fast event B in the same channel is stored, and handed to modules, as B then A. That matches the built-in storage order, which the docs already say follows callback latency for independent subjects (docs/required-inbound-admission.md:64-70). But the changelog says "per-channel module dispatch remains FIFO" (changelog.d/required-inbound-admission.added.md:4), and the type comment (src/mcpl/types.ts:231-236), the README and the PR body say "FIFO per channel". On an unconfigured connection, modules see coalesced items in arrival order, so "remains" reads as unchanged. The FIFO tests await each send, so they can't tell the two orders apart.

Fix: say "in acceptance order (callback-completion order across subjects)" in those four places, or reserve a per-scope module position at arrival, as the subject tickets do, if arrival order is what's meant.

R3-6. Queued module work for content that was already acknowledged is discarded at every transport blip. (minor)

dropRequiredModuleWork runs on close and reconnect as well as stop and disconnect (src/framework.ts:36110, :36281), and the pump drops when the slot's key isn't current (:19389-19390). The content stays stored and the peer was told accepted: true, so nothing resends it. The modules (whatever indexes, mirrors or reacts through onProcess) never see those events, though the agent's context has them. The PR's lifecycle test shows nine acknowledged pushes skipped by modules after a reconnect (test/mcpl-required-module-backlog.test.ts:73-111). Running module work for host-stored content needs no transport authority. The new epoch's key matters for inbound admission, not for telling local modules about content already admitted. On an unconfigured connection, modules never miss an acknowledged event. With healthy modules the queue is nearly always empty at a blip, so this bites in the same slow-module conditions as R3-2.

Fix: drop queued work only on stop and disconnect (where the server's channels go away too), and keep it across close/reconnect. Or document in the README and changelog that a reconnect can leave acknowledged content unseen by modules.

R3-7. Small accuracy items. (nit)

  • The docs call MAX_PENDING_REQUIRED_MODULE_WORK "the named ceiling" (docs/required-inbound-admission.md:139), but it is exported only from src/framework.ts:1083, not from the package entry (src/index.ts:221-223), so hosts can't import it. Export it, or say it equals MAX_PENDING_ADMISSIONS.
  • "Outstanding coalescing tickets are capped at 64 per connection" (docs/required-inbound-admission.md:72). The count is keyed by server id (framework.ts:18977), so it spans reconnect epochs and a re-added server with the same id.
  • git diff --check flags a new blank line at EOF in test/mcpl-required-module-backlog.test.ts:112.

Verified fine

  • Freshness, merge and tests. The head is directly on main 074ac60 (merge-base = main), and GitHub reports it mergeable, with all 12 CI checks green. Full suite at the head on Node 22.22.1, concurrency 4: 9,925 tests, 9,861 pass, 0 fail, 64 skip. That matches the PR body's 9,862 / 0 / 63 to within one environment-dependent skip. tsc --noEmit passes (both passes ran it).
  • The unconfigured path is unchanged. Every required branch is gated on connection.requiredAdmission / inboundAdmission (framework.ts:35866-35893, :36062-36073, hintsFor, mcplHostCapabilitiesFor). Unconfigured coalesced module dispatch is still awaited in place (:19263-19264, :19301-19302, :19321-19322). acceptCoalescedWithModuleCapacity without check is plain pushCoalescer.accept. The channel-registry refactor is a try/finally around the old loop, with coalesceFailure extracted verbatim. triggerInference still defaults to true. Both passes diffed these against main independently.
  • Ticket accounting doesn't leak or strand. Counts retire through the subject tail (framework.ts:18988-18993). Batch reservations sit inside the try, and finally retires all of them, including after a reservation throw (channel-registry.ts:1915-1922, :2201-2205; test mcpl-admission-bot-fixes.test.ts:33).
  • The module-work ceiling is checked under the coalescer's final callback before any mutation. Dropped slots release their event references, a running call stays counted, its errors are contained, and its completion can't revive a pruned queue (framework.ts:19333-19404). Every deliver call passes a shallow copy of the occurrence (push-coalescer.ts:913, 936, 953, 1223), so the occ.event.event lookup key that scheduleRequiredCoalescedModules uses holds on every delivery path.
  • Main's event suppression (#975) still holds for deferred module work. eventSuppression is a WeakMap keyed by the event (framework.ts:17550), and keepDecision copies it onto the occurrence before scheduling. A suppressed event's later requestInference still finds its rule in applyProcessResponse.
  • Direct factories reject inboundAdmission. Both public factories, connect and connectWithReconnect, call requireFrameworkAdmission before launch (server-connection.ts:1405-1412, :1752-1761), and the registry routes only through them. Modern-family configs are refused in serverConfigProblems. Sol ran a socket-free check of both factories.
  • The automated review's 21 items were spot-checked at the head. The claimed fixes are present: final in-mutex checks for new effects, feature diagnostics, pause/grant revisions, explicit config snapshotting, deferred-before-replay, suspension-aware replay, sparse-array length and stale held data. Two items have new gaps: #6's ticket bound exposes R3-1, and #4's FIFO wording is R3-5.
  • The changelog and README are consistent with the docs, apart from R3-5's "remains FIFO".

Method. Two independent passes, each re-verified before inclusion. (a) Codex (GPT-5.6 Sol, xhigh reasoning) ran read-only in a worktree at the head, with main as the base branch, the pinned @animalabs/mcpl-core@0.4.0 build, the MCPL spec repo at its current main (793ae8e) and the prior review threads. It took about 18 min and 153 shell commands, ~11.2M input tokens (10.8M cached) and 47k output tokens. It had no network, and its sandbox couldn't bind loopback, so it ran typecheck and socket-free checks but none of the transport suites. (b) A Claude pass covered freshness, the full suite at the head, the released @animalabs/discord-mcpl@0.6.0 delete and owed-redelivery paths, the interaction with main's event suppression, and three runtime probes over the real-WebSocket fixture: a retry under ticket saturation on both lanes, an accessProvider token through framework-side redaction with and without inboundAdmission, and the config-wrapper identity. Both: R3-1 (Sol from the code, Claude by runtime repro). Sol only: R3-4 and R3-5, both filed by Sol as major and downgraded here. R3-4 re-answers stored work in a narrow race and has no content effect. R3-5 matches the already-documented callback-order storage, so the problem is the wording. Claude only: R3-2, R3-3, R3-6 and R3-7. One pre-existing problem met along the way is passed on separately rather than filed here: on unconfigured connections, coalesced module dispatch is awaited inside the host-wide coalescing chain.

theaspirational and others added 4 commits November 5, 2026 05:33
Address R3-1, R3-2, R3-3, R3-4 and R3-6: replay before ticket capacity, preserve withdrawals at the module bound, forward an internal capability without cloning config, recheck replay authority under the mutex, and retain acknowledged module work across transport epochs.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Address R3-5 and R3-7, document R3-1/R3-2/R3-4/R3-6 behaviour, export the module-work ceiling, and state the shared host-wide coalescing chain limit.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
@theaspirational

Copy link
Copy Markdown
Contributor Author

Thanks for round 3. Updated to 05942d3: four commits on 4960139, no rebase.

  • R3-1: fixed. On both lanes a live receipt is looked up (after the channel assertion and the deferred check) before the ticket-cap refusal, so a retry of admitted work replays accepted: true at the cap, while fresh work is still refused. A saturation case on both lanes is in the receipt-replay tests.
  • R3-2: fixed. Withdrawals are never refused at the module cap, so the removal stands. If a withdrawal delivers a notice while the scope is full, only that notice's module work is dropped, with mcpl:module-work-dropped; nothing is queued over 64. Tests use a Discord-style delete (coalesce: { key, retract: true } with a [message deleted] notice) at capacity and assert the occupant is removed. The wire reason for real cap refusals stays admission_denied, but the ticket and module caps now trace distinct diagnostic codes, so a capacity refusal is distinguishable from a policy denial.
  • R3-3: fixed. The wrapper is gone: the framework-only marker is an internal capability passed from addServer to connect, so connection.config === config for a required WebSocket server. Tests assert that identity and that an accessProvider token is redacted in framework-recorded server text. Both public factories still reject inboundAdmission, including forged capability values.
  • R3-4: fixed. replayReceipt and accept()'s duplicate branch take the lane's current-authority check (grant, readiness, feature set, channel registration) and run it under the mutex before acknowledging. The receipt still beats a stale or refused callback verdict, and capacity isn't applied to replays. Tests revoke each kind of authority while a replay waits.
  • R3-5: the changelog, types comment, README and docs now say acceptance order (callback-completion order across subjects).
  • R3-6: fixed in code. Queued work for acknowledged content survives close and reconnect and the epoch change; only stop and disconnect drop it. The lifecycle tests cover five cases.
  • R3-7: MAX_PENDING_REQUIRED_MODULE_WORK is exported from the package entry, the docs say the ticket count is per server id and spans reconnects, and the trailing blank line is gone.
  • Shared chain: the docs and PR body now state that required and unconfigured coalescing servers share one chain and mutex, so a hung module on an unconfigured server's coalesced delivery can stall required effects (main's behaviour, unchanged here).

One more fix, found by our own review of this round: required module dispatch was pumped from the acceptance helper's finally, so a module's synchronous prefix ran before the lane wrote its acknowledgement. Dispatch now becomes eligible only after the acknowledgement is sent (a host-side continuation the peer can't trigger), and both lanes have ordering tests with a synchronous module prefix.

Locally on Node 22: typecheck and build pass; 23 targeted files 644/644; full suite 9,882 pass / 0 fail / 63 skip. The new tests fail on 4960139 and pass here; mutations reinstating the early ticket-cap refusal, the withdrawal refusal, the config wrapper and the early module pump are killed. No live provider was exercised.

AI-assisted implementation with GPT-6.1-Sol

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nightly dual review · round R4 at 05942d3 · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R4-n are stable; cite them in fix commits.

Reviewed at 05942d3 against main 074ac60

Verdict: changes requested (0 blockers, 1 major, 3 minor)

Round 3 is answered well. Six of its seven findings are fixed as asked, and the config-identity fix (R3-3) is cleaner than the wrapper it replaces. The one gap is R3-2: withdrawals now stand at the module cap, but the per-server 64-ticket cap, added in the same round, still refuses them. With the released Discord connector, a delete refused there stays in the agent's context, which is the failure R3-2 described (R4-1). The new ack-before-module commit (05942d3c) is right in intent and well tested for the single-item lanes. But it also makes a channel batch's accepted items hold module slots until the whole batch responds. A 64-item batch plus one concurrent message in the same channel now has its last item refused at the module cap, even with no modules configured (R4-2). Both fixes are small.

main has not moved since round 3. The head is four commits on 4960139 (ebf1adfe, 12495867, 599fcc84, 05942d3c), with no rebase, and it merges cleanly with all 12 CI checks green. Round 3 was posted at 4960139 as a comment-type review whose body verdict was changes requested.


Round-3 dispositions

R3 Disposition Where
R3-1 retry of admitted work refused at the ticket cap Fixed Both lanes look up a live receipt before the overload refusal: push at src/mcpl/push-handler.ts:222-228, channel at src/mcpl/channel-registry.ts:2007-2018. An over-cap ticket is uncounted and already resolved (src/framework.ts:18978), so a replay at the cap neither consumes nor waits on capacity. replayReceipt pins the entry before queueing (src/mcpl/push-coalescer.ts:604-606), so a receipt that expires mid-wait cannot fall through to fresh admission. Saturation tests cover both lanes (test/mcpl-admission-coalescing.test.ts, "R3-1 required …").
R3-2 withdrawals refused at the module cap Partial, see R4-1 The module cap now lets retract through without reserving (src/framework.ts:19339-19343), and a notice delivered without a reservation drops only its module work, with mcpl:module-work-dropped / admission_module_capacity (:19382-19386). Withdrawals are still refused at the ticket cap. One correction to our round-3 text: the released connector does re-send a refused coalesced delete, once, about 1 s later (sendWithRetries, dist/src/server.js:4391, delays [1000, 5000, 30000] at :473). It never marks a delete owed (:4472-4473).
R3-3 config wrapper broke redaction identity Fixed addServer receives connectionConfig (the caller's object for WebSocket servers, src/framework.ts:34392, :34434-34447) plus the FRAMEWORK_INBOUND_ADMISSION symbol. The registry forwards the symbol to both factories (src/mcpl/server-registry.ts:66-68). Automatic redials use connectChecked and never re-enter the factory gate. The symbol is not exported from src/index.ts, and the package exports map exposes only ..
R3-4 replay did not recheck authority under the mutex Fixed as asked replayReceipt(…, authority) and accept()'s duplicate branch run the lane's current-authority check inside locked() before acknowledge() (src/mcpl/push-coalescer.ts:607-612, :672-675). The push lane's check covers grant, readiness, feature set and channel (push-handler.ts:177-182). The channel lane's covers the same plus registry-entry identity (channel-registry.ts:1996-1998). A refusal leaves the receipt intact. A narrow residue remains, R4-4(b).
R3-5 "FIFO per channel" wording Fixed README, changelog, src/mcpl/types.ts:231-240 and docs/required-inbound-admission.md:136-138 say acceptance order.
R3-6 queued module work dropped at transport blips Fixed The close and reconnect drops are gone, and the pump no longer requires a current transport (src/framework.ts:19405-19409). Stop drops everything (:5663), and disconnect drops by server id even after registry removal (:35207). The queue key [serverId, binding, scope] uses the endpoint-hash binding (:18587), which is stable across reconnects, so FIFO order and the running slot carry across epochs.
R3-7 small accuracy items Fixed MAX_PENDING_REQUIRED_MODULE_WORK is exported from src/index.ts:5, and the docs say the ticket cap is per server id and spans reconnects (docs/…:72-73). git diff --check is clean.

Major

R4-1. (R3-2, partial) Withdrawals are still refused at the 64-ticket cap, so a delete can leave the deleted message in the agent's context.

takeCoalescingTicket refuses any occurrence once 64 coalesced tickets are outstanding for a server id (src/framework.ts:18977-18978). Both lanes then answer admission_denied (src/mcpl/push-handler.ts:224-228, src/mcpl/channel-registry.ts:2014-2018), whatever the occurrence is. A retract is refused like a fresh create, so its removal never happens. The R3-2 fix exempts withdrawals only at the module cap. The reasons given there apply equally here: the removal is the effect that matters, and the producer is the only one who can repair a refused delete.

The ticket cap is reached when 64 coalesced items for one server (across all its channels) are between arrival and effect: their host callbacks are slow, or their effects are queued in the host-wide coalescing chain. The docs name the second case themselves: "A hung module on an unconfigured server's coalesced delivery can … stall required coalesced effects too, eventually filling their ticket limit" (docs/required-inbound-admission.md:170-171). So does spawning a conversation agent inside coalescedAudience (src/framework.ts:19217-19226).

Reproduced over the real-WebSocket coalescing fixture, both after a channels/incoming create and after a push/event create. A create of message:m is accepted. Then 64 other coalesced items are sent while the coalescing chain is busy: their admission callbacks all return allow (callback count 65), and only their effects wait. Then a channel-scoped push/event with coalesce: { key: 'message:m', retract: true } and a [message deleted] notice is sent, which is what @animalabs/discord-mcpl@0.6.0 sends for a delete (dist/src/server.js:3452-3455). The delete gets { accepted: false, reason: 'admission_denied' }, with trace reason admission_ticket_capacity, and its callback never runs. When the chain clears, all 64 items are stored and the deleted text is still in context. The same probe with slow callbacks instead of a busy chain gives the same result. The behaviour dates from the ticket cap itself (0c1ff861) and is identical at 4960139. The connector re-sends a refused delete once, about 1 s later, and then drops it (dist/src/server.js:4391, :4472-4473). So any saturation lasting longer than a second loses the delete for good.

Fix: treat withdrawals at the ticket cap as the module cap now treats them. Let a retract take its ordered ticket even when the count is at 64. It still waits behind its create on the subject chain and still goes through the per-connection admission budget. If an unbounded withdrawal allowance is a concern, give withdrawals a separate small allowance (or one over-cap ticket per subject that has a live occupant or pending ticket). Add the delete-at-ticket-cap case to the R3-1 saturation tests on both lanes, with a non-empty notice. Then update the docs line "excess fresh occurrences refuse without joining the subject queue" (docs/…:73) to match.

Minor

R4-2. Since 05942d3c, a channel batch's own accepted items fill the module cap before any module runs, so the batch's later items are refused even with no modules configured.

A required coalesced acceptance reserves a slot in the scope's queue under the coalescer's final check (src/framework.ts:19336-19350). Since this round, the slot becomes ready only through afterResponse, after the lane's response (:19357-19365). For a channel batch, that is after its last sibling (src/framework.ts:35830, via channel-registry.ts:2034). The capacity check counts every pending slot, ready or not (queue.pending.length + running >= 64, :19339). Each sibling's ticket retires as the loop advances (channel-registry.ts:1924), so other requests in the same channel are admitted during the batch, and their slots queue behind the batch head.

Reproduced. A 64-message coalesced batch on chat has its second item's callback held until one single-message channels/incoming in chat is accepted. The batch's last item b63 is then refused admission_denied (trace admission_module_capacity). This happens with no modules configured, and with one idle module. At 4960139 the same probe accepts all 64, because each slot drained at acceptance. The released Discord connector sends one message per channels/incoming request (dist/src/server.js:4090), so it is not affected. Producers that batch (backfill, catch-up after reconnect) are. A refusal at the module cap is meant to report a module backlog. Here no module is busy at all.

Fix: don't let a request's own not-yet-released reservations count against its later siblings. For example, count only released and running slots toward MAX_PENDING_REQUIRED_MODULE_WORK, and bound unreleased reservations separately: a batch's coalesced items are already limited by its tickets. Or give the batch its slots in one reservation at arrival. Add the probe above, a full batch plus one concurrent arrival in the same channel, to mcpl-required-module-backlog.test.ts.

R4-3. The host-only afterResponse continuation is carried on the public admission receipt.

afterResponse was added to AdmittedEnvelope (src/mcpl/inbound-admission.ts:75), and both lanes put it into the frozen envelope (src/mcpl/push-handler.ts:242, src/mcpl/channel-registry.ts:2034). AdmittedEnvelope and admissionOf are package exports (src/index.ts:223), and the docs call the envelope a "read-only" receipt that host modules may read (docs/…:25-27). Every module that gets a required event can now call a request-scoped continuation of the containment wrapper. After the response it silently does nothing, and the pushed closure is retained with the event. This is not a correctness hazard today. The framework is the only caller, and the responder it closes over holds just the connection, id and epoch (src/mcpl/server-connection.ts:2790). But it turns a data receipt into a capability and adds an internal member to a public type.

Fix: keep it off the envelope. handleCoalescedPush / handleCoalescedIncoming already receive the lane's check. Pass afterResponse alongside it into acceptCoalescedWithModuleCapacity, or key it in a framework-private WeakMap by event.

R4-4. Small accuracy items. (nit)

  • (a) "Module dispatch starts … after the acknowledgement is sent" (docs/…:136-138, README:1047, src/mcpl/types.ts:233, changelog lines 4-5). If the transport closes before the lane responds, sendResponse sends nothing (src/mcpl/server-connection.ts:3003). The wrapper still counts the write as attempted (src/framework.ts:35726) and releases module work at completion. Releasing is the right behaviour: the content is stored, and a retry is answered from the receipt. But "after the lane's response is written or abandoned" would be accurate, and a closed-transport case would pin it.
  • (b) R3-4 residue. A replay checks authority when it reaches the coalescer mutex, but answers only after its subject ticket is ready (push-handler.ts:198-203, channel-registry.ts:1999-2010). If an earlier occurrence of the same subject is in a slow callback, a grant revoked during that wait still gets accepted: true for the retry. There is no content effect. Awaiting ticket.ready before calling replayReceipt (keeping the receipt pinned) would close it.
  • (c) The changelog's export list (lines 34-35) omits MAX_PENDING_REQUIRED_MODULE_WORK, which this round exports.

Verified fine

  • Freshness, merge and tests. main is 074ac60, which is the merge base, and nothing has landed since round 3. GitHub reports the PR mergeable and clean, with 12/12 checks green. Full suite at the head on Node 22, concurrency 4: 9,945 tests, 9,881 pass, 0 fail, 64 skip. That matches the PR body's 9,882 / 0 / 63 to within one environment-dependent skip. tsc --noEmit and git diff --check (full PR and the round-4 delta) pass in both passes.
  • The unconfigured path is unchanged by this round. The new authority parameters are optional and unused there. accept() with no check takes the original branch (push-coalescer.ts:668). The containment wrapper's responseWork list stays empty for every non-required handler. .finally(responseComplete) only adds a tick to the completion promise, which only trackHeldHandler reads, and only for required connections (framework.ts:35861-35863). The disconnect drop is a no-op when no required queue exists.
  • Ack-before-module ordering on the single-item lanes. Module work is released after the response on success, after the fallback error response when the reply write throws, and at completion for notifications. It still runs after a module error. The new tests pin each of these, and fail without 05942d3c.
  • Slot accounting at the module cap. A retract at capacity reserves nothing. A pure withdrawal releases an unused reservation (framework.ts:19366-19371). A dropped slot cannot be revived by a late afterResponse (:19362). The running slot stays counted across epochs.
  • Reconnect and re-add. Redials never pass through the public factories, so the capability is needed only at addServer. An explicit disconnect followed by a re-add drops the old queued work, and a running call stays counted under the same [serverId, binding, scope] key.
  • Capacity codes stay off the wire. Both lanes map admission_module_capacity to admission_denied (push-handler.ts:375, channel-registry.ts:2178), and the ticket refusal sends admission_denied directly. Only the traces carry the distinct codes.

Method. Two independent passes, each re-verified before inclusion. (a) Codex (GPT-5.6 Sol, xhigh reasoning) ran read-only in a worktree at the head, with main as the base branch. It also had the round-3 review and the author's reply, the pinned @animalabs/mcpl-core@0.4.0 build, the MCPL spec at its current main (793ae8e) and the released @animalabs/discord-mcpl@0.6.0 build. It took about 11 min and 162 shell commands, ~6.8M input tokens (6.6M cached) and 32k output tokens. It had no network, and its sandbox could not build or run the Node suite. (b) A Claude pass covered freshness and CI state, the full suite at the head, the connector's refusal and retry handling, and three runtime probes over the real-WebSocket coalescing fixture, each run at the head and at 4960139: a delete at the ticket cap with slow callbacks, the same with fast callbacks and a busy coalescing chain, and a 64-item channel batch plus one concurrent arrival. Both: the R3 dispositions, apart from the two below. Sol only: R4-4(a), filed as major and downgraded here, because releasing module work for stored content when the transport is gone is correct and only the wording overstates it. And R4-4(b), filed as minor (R3-4 partial) and downgraded to a nit here, because it re-answers stored work and has no content effect. Claude only: R4-1 (Sol marked R3-2 fixed after checking the module cap only), R4-2, R4-3 and R4-4(c).

theaspirational and others added 5 commits November 6, 2026 04:10
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
… (R4-2)

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
…onse boundary (R4-4)

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
Reserve a separate 16-ticket allowance per server id before callbacks, preserving subject dedupe and ordering. Refuse excess withdrawals with admission_withdrawal_capacity; release reservations on retirement.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
@theaspirational

Copy link
Copy Markdown
Contributor Author

Thanks for round 4. I've updated the PR to 9480306: five commits on 05942d3, no rebase. The base is still 074ac60.

  • R4-1: fixed. A withdrawal at the 64-ticket cap now takes its ordered ticket, but only when its subject has a live occupant or a pending ticket. It still waits behind its create on the subject chain and still goes through the per-connection admission budget. A retract for an unknown subject, or a second one for the same subject, is refused as before. I took your separate-allowance option to keep this bounded: MAX_OVERCAP_WITHDRAWAL_TICKETS = 16 per server id, counted across reconnects and reserved per batch item. Beyond it, a retract is refused with wire reason admission_denied and trace code admission_withdrawal_capacity. This is exported and documented, and the docs line about excess fresh occurrences is corrected. Tests cover the delete-at-ticket-cap case on both lanes, with both slow callbacks and a busy chain, and a non-empty notice. A 200-subject probe stops at 80 outstanding tickets (64 + 16). Further tests cover a real reconnect, refused-create then retract then create, and a repeated retract.
  • R4-2: fixed. Only released and running slots count toward MAX_PENDING_REQUIRED_MODULE_WORK. Unreleased reservations have their own bound of 64. Your repro, a 64-item batch plus one concurrent arrival in the same channel, is now in mcpl-required-module-backlog.test.ts, with no modules and with one idle module, and every item is accepted.
  • R4-3: fixed. afterResponse is off the envelope again: AdmittedEnvelope matches 4960139, and the continuation is passed privately next to check. Tests assert that the receipt holds data only.
  • R4-4:
    • (a) The wording now says module work starts "after the lane's response is written or abandoned". A closed-transport case pins this down.
    • (b) A replay pins its receipt, waits for its subject ticket, and only then checks authority under the mutex. The new tests revoke the grant during that wait, and the receipt expires during it as well.
    • (c) The changelog export list now includes both constants.

Locally (Node 22.23.3, concurrency 4): typecheck and build pass. npm test gives 9,902 pass, 0 fail, 63 skip. The new tests fail on 05942d3 and pass here. Mutations that reinstate the retract refusal at the ticket cap, remove the withdrawal ceiling, or count unreleased reservations are all killed. No live provider was used.

#931 is stacked on this branch and still sits on 05942d3. It will follow.

AI-assisted implementation with GPT-6.1-Sol

…se content

Moved from the stacked reply-routing PR, where it was reviewed: mayWake:false caps direct wakes only; held content still counts toward later tune-out reader cadence.

Co-Authored-By: GPT-6.1-Sol <noreply@openai.com>
@theaspirational

Copy link
Copy Markdown
Contributor Author

One more docs commit (0fea3b4), moved here from #931 per its R1-7: mayWake: false caps only direct wakes. Held content still counts toward later tune-out reader cadence, which the docs and changelog now say. No code change.

@anima-review

anima-review Bot commented Nov 11, 2026

Copy link
Copy Markdown

Automatic context view of 0fea3b4fe84b against 36b6dfe9de39.

What a context view runs and what its checks judge: CHECKS.md.

This head couldn't be compared: it conflicts with main at 36b6dfe9de39, and a view runs the head merged onto its base, so those conflicts need resolving before it can be viewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants