Repository navigation
Add opt-in required admission for inbound MCPL traffic - #906
theaspirational wants to merge 39 commits into
Conversation
There was a problem hiding this comment.
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.inboundAdmissiongate 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.mdand 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:
- Medium severity. A peer can crash the host during the handshake by sending a
nullline to the newrefuseEarlylistener. - Medium severity. Coalesced work already accepted on a required connection is silently revoked by any reconnect or restart.
- Medium severity, maintainability. New test freezes exact counts of unrelated Discord identifiers in framework.ts and index.ts.
- Medium severity, test coverage. No test covers the handshake-time
refuseEarlyrefusal.
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.handleRequiredPushEventrefuses on feature-set failure, hints,check()and every verdict (denied, threw, timeout, invalid, overload) only throughresponder?.respond(...). For push/event notifications there is no responder, so the event vanishes with no console line and no trace. The existing path inhandlePushEvent(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 undocumentedcapability_deniedmessage.refuseEarlyanswers channels/incoming, push/event and inference/request sent beforeinitializedwith code -32002 and the message 'capability_denied'. That string is not in the closedRefusalCodevocabulary. 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 inhandleIncomingemitsmcpl:channel-incoming-rejectedand/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 anisRenderingcheck. A shared helper such asrequiredIngressFor(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 exampleenvelopeFor(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 combinedsignaland its ownanswerStop(trace plushostStoppedWhileHandlingplus a guardedrespondError), 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 howstreamedis reported. - Low severity, maintainability,
src/index.ts:220-221.AdmissionConnectErroris not exported, though it is the fatal startup error hosts must handle. Binding and attestation failures now abortAgentFramework.createwithAdmissionConnectError, but index.ts exports only types from inbound-admission.ts. Host applications cannot useinstanceofto tell this fatal configuration error apart from other startup failures, and have to match onnameor message text.MAX_ADMISSION_TIMEOUT_MSandMAX_PENDING_ADMISSIONSare 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.
| 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' } })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "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})`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: request.id, error: { code: CAPABILITY_DISABLED, message: 'capability_denied' } })); | ||
| } | ||
| }; | ||
| if (admission) transport.on('line', refuseEarly); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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()andmcplHostCapabilitiesFor()never consultinboundAdmission. A required legacy server is offeredtoolHints: trueby 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.tsinhandleRequiredPushEvent). It also skipstapResponseentirely (server-connection.ts:2489). - RFC-014 §6.1 says a host that advertised
toolHintsapplies hints "as soon as it has each one, whatever it then does with the message that carried it … it applies thehintsof a push it refuses". It also says "A push carrying hints is never retried". A conforming peer spends therevand 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
tapResponsecosts more thanmcpl/hints.toolset-host.ts:226-268uses that path to adopttoolsets/list(readToolsetList→presence.adopt,:317-341) and to recordtoolsets/open/toolsets/close(noteOpened/noteClosed).ownToolsetList()returnstakeChange(r)(:504-507), which is empty when the tap never ran. On a required connection, then, the post-reconnecttoolsets/listsync (connected(),:521-530) and the PR's own "list recovery" adopt nothing. Opens and closes made through tools don't update presence either. Onlytools/listsurvives, becausesendToolsListtaps through its ownonRead(server-connection.ts:1821-1840). The new lifecycle test checks that recovery senttoolsets/list, not that its result was adopted.
Fix: havehintsFor()returnfalsewhenconfig.inboundAdmission !== undefined, or rejecttoolHints: truetogether withinboundAdmissioninserverConfigProblems. 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/hintsrefusal, push-tap suppression,tapResponseskip) then become unnecessary. If hints must stay advertised, strip only_meta["mcpl/hints"]and keep applying thetoolsets/*result fields. Either way, add a test that a required connection'stoolsets/listresult 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, untilresume()(: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,…}afterresume(), 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:
revokedaudit atpush-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
- 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. - Vocabulary. It's fine, but the handshake path answers outside it (R1-6).
- Hint policy. Stop advertising
toolHintson required connections (R1-2). RFC-014 §6.3 then makes peer hints a protocol violation instead of state the host silently drops, and thetoolsets/*bookkeeping stops breaking. - Startup. The split between non-fatal outages and fatal binding/attestation failures is sound, apart from R1-8.
- Baseline.
@animalabs/mcpl-core0.4.0 has the method and result shapes used here: push{accepted, reason}, itemwise channel results, and-32002for 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
pretestfixture 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,admissionor an envelope.wakeForbiddenis false without an envelope, andenvelope?.mayWake !== falseis true when absent. TherefuseCapabilityextraction behaves the same as the code it replaced. RequiredAdmission.run():- the slot, timer and abort listener are released in
finallyon 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.
- the slot, timer and abort listener are released in
- Attestation
read/matchesare 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/listinventory andmcpl/revsurvive on required connections, becausesendToolsListtaps viaonRead, nottapResponse. - Startup rethrows a direct
AdmissionConnectError, and reconnect marks it permanent.serverConfigProblemsrejects 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.
119cb86 to
66c9db3
Compare
66c9db3 to
c848977
Compare
|
Thanks for the R1 review. All findings are addressed on the updated head
|
c848977 to
869bb3a
Compare
869bb3a to
4df24aa
Compare
|
@theaspirational I think required admission should decide single items carrying The test sent synthetic content through a local WebSocket, once per fresh connection: an open channel's one-item The docs at My review preference is to let required admission decide single items carrying Morgan-3569 built and ran the local check. Beatrice-3571 independently checked its result. Violet-3595 wrote this comment. — Violet-3595 |
|
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. |
|
@theaspirational Following up on my earlier comment: the specific single-item admission and replacement point it raised is resolved in our local test at On Linux with Node 24.22.1, a loopback WebSocket test sent synthetic items matching the released Your Morgan-3569 ran the local check. Beatrice-3571 independently checked its result. Violet-3595 wrote this follow-up. — Violet-3595 |
Anarchid
left a comment
There was a problem hiding this comment.
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(...)atsrc/mcpl/channel-registry.ts:1989, thenhandleCoalescedIncomingat:2140; - push lane:
await admission.admit(...)atsrc/mcpl/push-handler.ts:191, thenhandlePushEvent→handleCoalescedat: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*andwake-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.considerWakeOnboardingneeds a gate decision. AmayWake: falseitem reachesadmitInboundwithvetoed: true, andadmit()returns beforegate.evaluate(src/admission/admit.ts:105), so no onboarding wake is queued. UndermayWake: 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 themembrane.complete/streamResponsecall (inference-router.ts:277-427).dispatched?.()fires after the provider promise is created. - The unconfigured path is unchanged. The
coalesceFailureextraction breaks exactly where the old inline code did.finishCoalescedIncomingis the old tail, verbatim.accept()withoutcheckis the old body. Unconfiguredadmit()never returnsaccepted: false, so the new early return is dead there.triggerInferencestill defaults totrue. Thecompletioncapture incontainedInboundHandlerkeeps the old.catch. The required-mode hold guard and drain are gated onrequiredAdmission. RequiredAdmission.run()slot accounting. Exactly one controller is added and always removed infinally, 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
replyIntentvalidator 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.
|
Thanks for the review. All five R2 findings are addressed in one commit on top of
|
|
/review |
|
Automatic review of Findings, most severe first:
Minor findings 9 to 21 (of 21)
|
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>
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>
6d2e173 to
4960139
Compare
|
Thanks for the automatic review of
Head: 9862 pass / 0 fail / 63 skip; upstream baseline on the same setup 9574 / 0 / 63. No live provider was exercised. |
Anarchid
left a comment
There was a problem hiding this comment.
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-168refusesadmission_denied, and replay only comes at:220; - channel:
src/mcpl/channel-registry.ts:1926-1930refuses, 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 fromsrc/framework.ts:1083, not from the package entry (src/index.ts:221-223), so hosts can't import it. Export it, or say it equalsMAX_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 --checkflags a new blank line at EOF intest/mcpl-required-module-backlog.test.ts:112.
Verified fine
- Freshness, merge and tests. The head is directly on
main074ac60(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 --noEmitpasses (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).acceptCoalescedWithModuleCapacitywithoutcheckis plainpushCoalescer.accept. The channel-registry refactor is atry/finallyaround the old loop, withcoalesceFailureextracted verbatim.triggerInferencestill defaults totrue. Both passes diffed these againstmainindependently. - Ticket accounting doesn't leak or strand. Counts retire through the subject tail (
framework.ts:18988-18993). Batch reservations sit inside thetry, andfinallyretires all of them, including after a reservation throw (channel-registry.ts:1915-1922,:2201-2205; testmcpl-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). Everydelivercall passes a shallow copy of the occurrence (push-coalescer.ts:913, 936, 953, 1223), so theocc.event.eventlookup key thatscheduleRequiredCoalescedModulesuses holds on every delivery path. - Main's event suppression (#975) still holds for deferred module work.
eventSuppressionis aWeakMapkeyed by the event (framework.ts:17550), andkeepDecisioncopies it onto the occurrence before scheduling. A suppressed event's laterrequestInferencestill finds its rule inapplyProcessResponse. - Direct factories reject
inboundAdmission. Both public factories,connectandconnectWithReconnect, callrequireFrameworkAdmissionbefore launch (server-connection.ts:1405-1412,:1752-1761), and the registry routes only through them. Modern-family configs are refused inserverConfigProblems. 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.
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>
|
Thanks for round 3. Updated to
One more fix, found by our own review of this round: required module dispatch was pumped from the acceptance helper's 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 AI-assisted implementation with GPT-6.1-Sol |
Anarchid
left a comment
There was a problem hiding this comment.
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,sendResponsesends 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 getsaccepted: truefor the retry. There is no content effect. Awaitingticket.readybefore callingreplayReceipt(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.
mainis074ac60, 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 --noEmitandgit diff --check(full PR and the round-4 delta) pass in both passes. - The unconfigured path is unchanged by this round. The new
authorityparameters are optional and unused there.accept()with nochecktakes the original branch (push-coalescer.ts:668). The containment wrapper'sresponseWorklist stays empty for every non-required handler..finally(responseComplete)only adds a tick to thecompletionpromise, which onlytrackHeldHandlerreads, 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
retractat capacity reserves nothing. A pure withdrawal releases an unused reservation (framework.ts:19366-19371). A dropped slot cannot be revived by a lateafterResponse(: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_capacitytoadmission_denied(push-handler.ts:375,channel-registry.ts:2178), and the ticket refusal sendsadmission_denieddirectly. 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).
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>
|
Thanks for round 4. I've updated the PR to
Locally (Node 22.23.3, concurrency 4): typecheck and build pass. #931 is stacked on this branch and still sits on 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>
|
One more docs commit ( |
|
Automatic context view of What a context view runs and what its checks judge: CHECKS.md. This head couldn't be compared: it conflicts with |
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
inboundAdmissionmust behave exactly as before.Changes
addServertoconnect, so the caller's config object keeps its identity and server-text redaction of dialled credentials still applies.MAX_PENDING_REQUIRED_MODULE_WORKis exported from the package entry.074ac60; conflict resolutions keep upstream's suppression bookkeeping, envelope/null-ID routing, generic response tap, initialize decoding andTraceEventInputtyping.Limits
AgentFramework. DirectMcplServerConnectionfactories reject a configuration that carriesinboundAdmission; a shared authorized-dispatch API for direct consumers could be a separate PR.admission_deniedand 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 withmcpl:module-work-dropped.onProcessinside that chain (as onmain), so a hung module there can stall required coalesced effects too.docs/required-inbound-admission.md).Tests
074ac60on the same setup: 9,574 / 0 / 63.AI-assisted implementation with GPT-6.1-Sol