From a64f47435a9c0ff1ed3e9e5504d4ab66cae00c7c Mon Sep 17 00:00:00 2001 From: Alfred-2570 Date: Thu, 15 Oct 2026 02:26:06 -0400 Subject: [PATCH 1/4] fix: malformed MCPL blocks and a non-OK Discord attachment fetch each get a word, through one converter Places that dropped something from the agent's view without saying so, found by the errors sweep against agent-framework main 92125b07 (the bar: every error an agent can receive tells it how to fix it, or says plainly that nothing can be done), and by this change's internal reviews. MCPL text whose text isn't a string, which only a nonconformant server sends. Before: - the push and channel converters (push-handler.ts, channel-registry.ts) turned such a block into '' since 33c38d787, so beside other content it vanished from the agent's view; - the context-injection converter (hook-orchestrator.ts) passed it through. A beforeUser or afterUser injection lost it to the request builder's sanitizer (agent.ts), without a word. A system injection's content passes through context-manager 0.14.1 unchanged, and buildSystemPrompt joins block.text with no type check, so it reached the system prompt as String(text) ("[object Object]", the number, nothing for null), and an object with no string form, such as {"toString": 1}, threw a TypeError while the request was built; - a text-only MCPL tool result (framework.ts, dispatchMcplToolCall) kept only truthy text and joined it, so null, 0 and false vanished, an object read "[object Object]", and a lone such block came back as an empty success. (A mixed result already bails to JSON, which shows the block.) All four now show NON_STRING_TEXT_PLACEHOLDER, "[Text not shown: the server's text block had no string text.]", defined in visible-content.ts beside the emptiness check. The tool-result join maps before its empty-string filter, so empty strings still drop as before. One converter. The push and channel converters were identical apart from the provenance their reference stubs name, and the injection converter was a third copy that predated 33c38d787's checks (a type, data, mimeType or uri that isn't a string counts as absent). There, a type that is an object with no string form threw in the default branch's interpolation; nothing in fanOutBeforeInference catches per injection, and framework.ts catches around the whole hook with only a console line, so one such block dropped every server's injections for the turn. It also checked image data and mimeType for truthiness only, passing a non-string data on to the provider as the image's source, and showed an audio block without inline data as "[Audio: ]". convertBlock now lives once, in src/mcpl/convert-block.ts, taking the provenance as a parameter; push-handler, channel-registry, hook-orchestrator and the framework's coalesced delivery (which imported push's copy) call it. Push and channel convert exactly as before. The injection lane gains their checks and their placeholders: "[unrecognized content block: untyped]", "[Image: no data]" (was "[Image: missing data]") or "[Image: no media type]", "[Audio: no data]" (was "[Audio: ]" or "[Audio: missing data]") or "[Audio: no media type]". One injection at a time. A null block, or content that isn't an array, still throws in conversion, on every lane. On the injection lane each injection is now read in its own try: one the host can't read (such content, a null injection, a position with no string form) is left out with a console line naming its server and position, as is an answer whose contextInjections isn't an array, and every other injection still arrives. Before, any of these threw out of fanOutBeforeInference, and the framework's catch around the whole hook dropped every server's injections for the turn. On the uncoalesced push and channel lanes such content still throws, unanswered, as on main: agent-framework#297. The placeholder is the host's word, not the sender's, so it must not change what the emptiness check answers. Checked on the converted blocks, it would read the placeholder as visible text and admit a lone non-string text block on both uncoalesced paths, which 33c38d787's test pins as refused, as the coalesced paths still refuse it before conversion. Checking the wire blocks instead would change another answer: a message whose blocks aren't objects is admitted uncoalesced today as its untyped placeholders, and the wire check skips non-objects as empty. So the two uncoalesced checks (push-handler.ts, channel-registry.ts) call isVisiblyEmptyConversion(wire, converted), which leaves out exactly the blocks shown as the placeholder, block for block, and answers as isVisiblyEmptyContent did on the '' blocks. The coalescer's checks read wire content and are unchanged. The framework's two emptiness backstops read converted content, but only content one of those checks has passed, so they never see a message of placeholders alone (the coalescer's comment now says where the rule holds and why). A shouldTriggerInference filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converter's other placeholders ("[unrecognized content block: video]"). A coalesced occurrence's render result holding such a block is still refused whole by checkBlockShapes, as before, and the occurrence's own content, placeholder included, is delivered instead. Discord. convertMessageToContent fetched a small text attachment and noted "[Attachment: (failed to fetch)]" only when fetch threw. An answer that wasn't OK pushed nothing, so the attachment disappeared, and a message with nothing else in it read "[Empty message]". Both paths now push the same notice from one helper, attachmentNotFetched, so a later change to its wording changes one place. Each also writes a console line naming the file, the message and the HTTP status or the thrown error, and the non-OK branch cancels the unread body so its connection is released. The filename is printed through logValue, so one with a line break or a space stays one field of one line. Tests: - mcpl-coalesced-content-parity: - the malformed-blocks test expects the placeholder for each non-string text block, four times each (coalesced and not, on each lane), and each string text beside it four times; - the inference-filter test expects the placeholder in the filter's text; - new: a non-string text block beside blank text, and a text block with no text key, are refused as empty on all four paths (controls: main refuses them too); - new: content whose blocks aren't objects, or whose block of another type carries a text field, is still admitted uncoalesced on both lanes, as its placeholder (controls); - new: an injection's block arrives as the placeholder through HookOrchestrator.beforeInference, pinned word for word, and a system injection reaches the compiled system prompt through previewActivation; - new: one server's injection holding a type with no string form, an image with numeric data and a uri-only audio block shows each as its placeholder, and a second server's injection still arrives; - new: one server's injections holding a null block, content that isn't an array, a null injection and a position with no string form, and a second server's injections that aren't a list, are each left out with a console line, and the valid injections from the first and a third server still arrive; - new: a reference stub names the lane its block came on: a channel message, a pushed event or an injection; - new: a tool result dispatched through the framework shows the placeholder alone and beside text, and drops a blank text from the join, as before (the fixture gains tools/toolsCall); - the lone-block refusal test is main's, title and body. - discord-attachment-notice (new): a non-OK answer reads as not fetched, writes its console line and has its body cancelled; a thrown fetch reads the same and writes its line; a fetched file is read whole; a filename with a line break is printed quoted. - mcpl-push-convert imports convertBlock from its new module; its six tests are otherwise unchanged. Red-first, with main's src and these tests plus two stubs so the tests compile and fail on behaviour (the placeholder constant, and convert-block.ts re-exporting main's push converter): 9 of 37 fail (malformed blocks, injection, malformed injection, isolation, inference filter, tool result, both Discord failures, the quoted filename); the other 28 pass, the provenance test and mcpl-push-convert's six among them. Each of 15 pieces removed on its own fails its tests: the converter's placeholder, each uncoalesced emptiness check, Discord's non-OK branch, its body cancel, its console lines and their quoting, the tool-result join and its blank filter, the injection lane's own pre-round copy of the converter, each injection's own try, the answer's list check, and each lane's provenance (Linux, Node 24.21.0, on the tree of this commit). Changelog: changelog.d/fix-silent-drops-say-so.fixed.md. --- changelog.d/fix-silent-drops-say-so.fixed.md | 3 + src/framework.ts | 17 +- src/mcpl/channel-registry.ts | 93 +-------- src/mcpl/convert-block.ts | 94 +++++++++ src/mcpl/hook-orchestrator.ts | 93 ++------- src/mcpl/push-coalescer.ts | 30 +-- src/mcpl/push-handler.ts | 94 +-------- src/mcpl/visible-content.ts | 27 +++ src/modules/discord/index.ts | 19 +- test/discord-attachment-notice.test.ts | 70 +++++++ test/helpers/coalescing-fixture.ts | 5 +- test/mcpl-coalesced-content-parity.test.ts | 208 ++++++++++++++++++- test/mcpl-push-convert.test.ts | 4 +- 13 files changed, 477 insertions(+), 280 deletions(-) create mode 100644 changelog.d/fix-silent-drops-say-so.fixed.md create mode 100644 src/mcpl/convert-block.ts create mode 100644 test/discord-attachment-notice.test.ts diff --git a/changelog.d/fix-silent-drops-say-so.fixed.md b/changelog.d/fix-silent-drops-say-so.fixed.md new file mode 100644 index 000000000..eea489655 --- /dev/null +++ b/changelog.d/fix-silent-drops-say-so.fixed.md @@ -0,0 +1,3 @@ +- An MCPL text block whose text isn't a string, which only a nonconformant server sends, now reaches the agent as `[Text not shown: the server's text block had no string text.]` instead of vanishing from its view or reading as `[object Object]`. That holds on the push and channel lanes, coalesced or not, in a `context/beforeInference` injection, and in a text-only tool result. A `system` injection's text used to be joined into the system prompt as it was, which for an object with no string form threw while the request was built; a text-only tool result whose only block was such a text came back as an empty success. The placeholder still counts as no text when a message is checked for visible content, so a message holding only such blocks (or blank text) is still refused as empty, with the same answers as before, and a message whose blocks are numbers, strings or booleans is still admitted uncoalesced as its untyped placeholders. A `shouldTriggerInference` filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converters' other placeholders. +- A `context/beforeInference` injection's content now converts as a pushed event's and a channel message's does: one converter serves all three. Before, an injection block whose `type` wasn't a string read as that value (`[unrecognized content block: 5]`, `[unrecognized content block: [object Object]]`), and one whose `type` was an object with no string form threw while it was converted, so the framework dropped every server's injections for that turn, with only a console line. An image's `data`, `mimeType` or `uri`, or an audio block's `data` or `mimeType`, that wasn't a string was passed on to the provider as the block's source, and a `uri` with a `disposition` was taken as a reference. An audio block without inline data showed its uri in its text: one that wasn't a string read as itself, and one with no string form threw, as a `type` did. Such fields now count as absent, and such blocks show the placeholders the other lanes show: `[unrecognized content block: untyped]`; `[Image: no data]` (where it read `[Image: missing data]`) or `[Image: no media type]`; and `[Audio: no data]` (where it read `[Audio: ]` or `[Audio: missing data]`) or `[Audio: no media type]`. An injection the host can't read is now left out with a console line naming its server, and the turn keeps every other injection: one holding a null block or content that isn't an array, a null injection, or one whose position has no string form, and likewise an answer whose injections aren't a list. Before, any of these dropped them all. +- The Discord module now gives the agent `[Attachment: (failed to fetch)]` for a text attachment whose download is answered with a status that isn't OK, as it already did when the download threw. Before, such an attachment was left out without a word, and a message with nothing else in it read `[Empty message]`. Either failure now also writes a console line naming the file, the message and the status or error, and an answer that isn't OK has its body cancelled, so its connection is released. diff --git a/src/framework.ts b/src/framework.ts index 013277a59..d27c2a65d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -74,14 +74,15 @@ import { HookOrchestrator } from './mcpl/hook-orchestrator.js'; import { ToolLifecycleEmitter, parseToolObserveParams, type ScriptCallOrigin } from './mcpl/tool-lifecycle.js'; import { checkToolPattern, describeUnmatchedPattern, type PatternServer } from './mcpl/tool-pattern-check.js'; import { BUILTIN_TOOL_CLASSES, normalizeClassTable, parseDeclaredClasses, resolveToolClass, type EffectiveToolClass, type ToolClass, type ToolClassSource } from './mcpl/tool-classes.js'; -import { PushHandler, convertBlock as convertPushBlock, type McplPushEvent } from './mcpl/push-handler.js'; +import { PushHandler, type McplPushEvent } from './mcpl/push-handler.js'; +import { convertBlock } from './mcpl/convert-block.js'; import { PushCoalescer, CoalesceError, PUSH_COALESCING_SUPPORT, validateCoalesceMember, validateCoalescedContent, coalescingSubjectKey, type AssemblyResult, type CoalescedOccurrence, type CoalescingPlacement, type CoalescingSnapshot, type CoalescingReceiptRecord, } from './mcpl/push-coalescer.js'; import type { ChannelIncomingMessage, ChannelIncomingMessageResult, McplContentBlock, PushEventResult } from './mcpl/types.js'; -import { isSilentHeartbeatMarker, isVisiblyEmptyContent } from './mcpl/visible-content.js'; +import { isSilentHeartbeatMarker, isVisiblyEmptyContent, NON_STRING_TEXT_PLACEHOLDER } from './mcpl/visible-content.js'; import { parseProsePrefix, parseHybridProsePrefix } from './mcpl/prose-grammar.js'; import { ProseStreamRouter } from './mcpl/prose-stream-router.js'; import { detectKnownToolWrapperProse } from './tool-wrapper-prose-guard.js'; @@ -9875,7 +9876,7 @@ export class AgentFramework { messageId: identity.messageId ?? str(origin.messageId) ?? occ.eventId, threadId: identity.threadId ?? str(origin.threadId), author: identity.author ?? { id: str(origin.authorId) ?? '', name: str(origin.authorName) ?? str(origin.authorId) ?? 'Someone' }, - content: materialized ? materialized.map(convertPushBlock) : push.content, + content: materialized ? materialized.map((block) => convertBlock(block, 'attachment on push event')) : push.content, timestamp: occ.timestamp, metadata: origin, tags: push.tags, @@ -9894,7 +9895,7 @@ export class AgentFramework { ...occ.event.event, eventId: occ.eventId, timestamp: occ.timestamp, - ...(materialized ? { content: materialized.map(convertPushBlock) } : {}), + ...(materialized ? { content: materialized.map((block) => convertBlock(block, 'attachment on push event')) } : {}), coalescingSubject, // §5.2: the materialized occurrence is consumed by the request being // assembled; the wake that started it is not repeated. @@ -17944,9 +17945,13 @@ export class AgentFramework { // through so toMembraneToolResult can preserve image blocks natively. // Text-only results still collapse to a joined string for backward // compatibility with callers that expect data to be string-ish. + // A text block whose text isn't a string, which only a nonconformant + // server sends, shows as the converter's placeholder, where it used to + // vanish (null, 0, false) or read as String(text), "[object Object]". const textContent = result.content - ?.filter((c) => c.type === 'text' && c.text) - .map((c) => c.text!) + ?.filter((c) => c.type === 'text') + .map((c) => (typeof c.text === 'string' ? c.text : NON_STRING_TEXT_PLACEHOLDER)) + .filter((text) => text !== '') .join('\n'); const hasNonText = result.content?.some((c) => c.type !== 'text'); const data = result.isError diff --git a/src/mcpl/channel-registry.ts b/src/mcpl/channel-registry.ts index 082e6ed9b..5d3443d3f 100644 --- a/src/mcpl/channel-registry.ts +++ b/src/mcpl/channel-registry.ts @@ -14,7 +14,7 @@ */ import type { ContentBlock } from '@animalabs/membrane'; -import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; +import { convertBlock } from './convert-block.js'; import type { JsStore } from '@animalabs/chronicle'; import type { @@ -30,7 +30,6 @@ import type { ChannelsPublishParams, ChannelsOpenResult, ChannelHistoryRequest, - McplContentBlock, } from './types.js'; import type { McplServerRegistry } from './server-registry.js'; @@ -38,7 +37,7 @@ import type { FeatureSetManager } from './feature-set-manager.js'; import type { ToolDefinition, ToolResult, ProcessEvent } from '../types/index.js'; import { expandCoreTags } from './tags.js'; import { coalesceRejectionReason, EmptyContentError, validateCoalescedContent } from './push-coalescer.js'; -import { isVisiblyEmptyContent } from './visible-content.js'; +import { isVisiblyEmptyConversion } from './visible-content.js'; import { logDetail, logValue } from './log-value.js'; import { RefusalLines } from './refusal-lines.js'; import { CapabilityGrant } from './capability-grant.js'; @@ -241,88 +240,6 @@ interface McplChannelIncomingEvent { targetAgents?: string[]; } -// ============================================================================ -// Content Conversion: McplContentBlock → membrane ContentBlock -// ============================================================================ - -/** - * Convert a single MCPL wire-format content block to a membrane ContentBlock. - */ -function convertBlock(block: McplContentBlock): ContentBlock { - switch (block.type) { - case 'text': - // Text that isn't a string is no text, as the request sanitizer and - // the visible-content check already read it. - return { type: 'text', text: typeof block.text === 'string' ? block.text : '' }; - - case 'image': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (isFilledString(block.uri) && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on channel message') ?? '[reference]' }; - } - if (isFilledString(block.data) && isFilledString(block.mimeType)) { - return { - type: 'image', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - } as ContentBlock; - } - if (isFilledString(block.uri)) { - return { - type: 'image', - source: { type: 'url', url: block.uri }, - } as ContentBlock; - } - return { type: 'text', text: isFilledString(block.data) ? '[Image: no media type]' : '[Image: no data]' }; - - case 'audio': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (isFilledString(block.uri) && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on channel message') ?? '[reference]' }; - } - if (isFilledString(block.data) && isFilledString(block.mimeType)) { - return { - type: 'audio', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - } as ContentBlock; - } - return { type: 'text', text: isFilledString(block.data) ? '[Audio: no media type]' : '[Audio: no data]' }; - - case 'resource': - // RFC-005: reference blocks become bounded stubs — never raw URIs - // (a signed URL is a bearer credential that looks like a location). - return { type: 'text', text: referenceStubOrNull(block, 'attachment on channel message') ?? '[reference]' }; - - default: { - // Unknown wire block types previously fell off the exhaustive switch - // and propagated `undefined` into ContentBlock[]. Fail visibly. A type - // that isn't a string is no type, and is never converted to one: an - // object can throw when it is. - const type: unknown = (block as { type?: unknown }).type; - return { type: 'text', text: `[unrecognized content block: ${typeof type === 'string' ? type : 'untyped'}]` }; - } - } -} - -/** A string with something in it: the only data, mimeType or uri a block's - * source is built from, and the only uri that makes it a reference. Anything - * else a sender puts there leaves the block without it, so conversion never - * throws and never copies a non-string. */ -function isFilledString(value: unknown): value is string { - return typeof value === 'string' && value !== ''; -} - // ============================================================================ // Channel Tool Definitions // ============================================================================ @@ -1031,8 +948,8 @@ export class ChannelRegistry { } } if (message.tags) message.tags = expandCoreTags(message.tags); - const convertedContent: ContentBlock[] = message.content.map(convertBlock); - if (!emptyAllowed && isVisiblyEmptyContent(convertedContent)) { + const convertedContent: ContentBlock[] = message.content.map((block) => convertBlock(block, 'attachment on channel message')); + if (!emptyAllowed && isVisiblyEmptyConversion(message.content, convertedContent)) { rejectEmpty(); continue; } @@ -1060,7 +977,7 @@ export class ChannelRegistry { if (this.shouldTriggerInference) { // The text the agent's context gets, as the push lane's filter reads // it: the converted text blocks, a placeholder or reference stub - // among them, and a text block's non-string text as empty text. + // among them, NON_STRING_TEXT_PLACEHOLDER included. const textContent = convertedContent .filter((b): b is ContentBlock & { type: 'text' } => b.type === 'text') .map((b) => b.text) diff --git a/src/mcpl/convert-block.ts b/src/mcpl/convert-block.ts new file mode 100644 index 000000000..0c3b68076 --- /dev/null +++ b/src/mcpl/convert-block.ts @@ -0,0 +1,94 @@ +/** + * MCPL wire content to membrane content: the one conversion every lane uses, + * for pushed events, channel messages and context injections alike, so a rule + * about a malformed block holds on all three. + */ + +import type { ContentBlock } from '@animalabs/membrane'; +import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from './visible-content.js'; +import type { McplContentBlock } from './types.js'; + +/** + * Convert a single MCPL wire-format content block to a membrane ContentBlock. + * It takes any block but null or undefined without throwing (those two throw, + * for the caller to refuse or leave out): a type, data, mimeType or uri that + * isn't a string counts as absent, and a text block whose text isn't a string + * shows as NON_STRING_TEXT_PLACEHOLDER. `provenance` is what a reference stub + * says the reference came on, such as 'attachment on push event'. + */ +export function convertBlock(block: McplContentBlock, provenance: string): ContentBlock { + switch (block.type) { + case 'text': + // Text that isn't a string can't be shown, so the agent reads that a + // block was there; the emptiness checks still count it as no text + // (isVisiblyEmptyConversion), as the request sanitizer would. + return { type: 'text', text: typeof block.text === 'string' ? block.text : NON_STRING_TEXT_PLACEHOLDER }; + + case 'image': + if (isInlineContradiction(block)) { + // RFC-005 vector 2: inline data claiming bulk disposition — fail + // closed, withhold the data (checked BEFORE the data branch). + return { type: 'text', text: INLINE_WITHHELD_TEXT }; + } + if (isFilledString(block.uri) && block.disposition) { + // RFC-005 uri-form media with a disposition claim: stub, do not + // hand the URI to the provider or inline it. + return { type: 'text', text: referenceStubOrNull(block, provenance) ?? '[reference]' }; + } + if (isFilledString(block.data) && isFilledString(block.mimeType)) { + return { + type: 'image', + source: { type: 'base64', data: block.data, mediaType: block.mimeType }, + } as ContentBlock; + } + if (isFilledString(block.uri)) { + return { + type: 'image', + source: { type: 'url', url: block.uri }, + } as ContentBlock; + } + return { type: 'text', text: isFilledString(block.data) ? '[Image: no media type]' : '[Image: no data]' }; + + case 'audio': + if (isInlineContradiction(block)) { + // RFC-005 vector 2: inline data claiming bulk disposition — fail + // closed, withhold the data (checked BEFORE the data branch). + return { type: 'text', text: INLINE_WITHHELD_TEXT }; + } + if (isFilledString(block.uri) && block.disposition) { + // RFC-005 uri-form media with a disposition claim: stub, do not + // hand the URI to the provider or inline it. + return { type: 'text', text: referenceStubOrNull(block, provenance) ?? '[reference]' }; + } + if (isFilledString(block.data) && isFilledString(block.mimeType)) { + return { + type: 'audio', + source: { type: 'base64', data: block.data, mediaType: block.mimeType }, + } as ContentBlock; + } + return { type: 'text', text: isFilledString(block.data) ? '[Audio: no media type]' : '[Audio: no data]' }; + + case 'resource': + // RFC-005: reference blocks become bounded stubs — never raw URIs + // (a signed URL is a bearer credential that looks like a location). + return { type: 'text', text: referenceStubOrNull(block, provenance) ?? '[reference]' }; + + default: { + // Unknown wire block types previously fell off the exhaustive switch + // and propagated `undefined` into ContentBlock[]. Fail visibly. A type + // that isn't a string is no type, and is never converted to one: an + // object can throw when it is. + const type: unknown = (block as { type?: unknown }).type; + return { type: 'text', text: `[unrecognized content block: ${typeof type === 'string' ? type : 'untyped'}]` }; + } + } +} + +/** A string with something in it: the only data, mimeType or uri a block's + * source is built from, and the only uri that makes it a reference. Anything + * else a sender puts there leaves the block without it, so conversion never + * throws and never copies a non-string. */ +function isFilledString(value: unknown): value is string { + return typeof value === 'string' && value !== ''; +} diff --git a/src/mcpl/hook-orchestrator.ts b/src/mcpl/hook-orchestrator.ts index 14c4e85dc..3a6ffd905 100644 --- a/src/mcpl/hook-orchestrator.ts +++ b/src/mcpl/hook-orchestrator.ts @@ -14,7 +14,7 @@ */ import type { ContentBlock } from '@animalabs/membrane'; -import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; +import { convertBlock } from './convert-block.js'; import type { ContextInjection } from '@animalabs/context-manager'; import type { @@ -28,6 +28,7 @@ import type { McplServerRegistry } from './server-registry.js'; import type { McplServerConnection } from './server-connection.js'; import type { FeatureSetManager } from './feature-set-manager.js'; import { CapabilityGrant } from './capability-grant.js'; +import { logDetail, logValue } from './log-value.js'; /** Timeout for beforeInference per server (fail-open). */ const BEFORE_INFERENCE_TIMEOUT_MS = 5_000; @@ -57,72 +58,6 @@ function withTimeout(promise: Promise, ms: number, label: string): Promise // Content conversion: McplContentBlock / McplContextInjection → membrane types // ============================================================================ -/** - * Convert a single MCPL wire-format content block to a membrane ContentBlock. - */ -function convertBlock(block: McplContentBlock): ContentBlock { - switch (block.type) { - case 'text': - return { type: 'text', text: block.text }; - - case 'image': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (block.uri && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on context injection') ?? '[reference]' }; - } - if (block.data && block.mimeType) { - return { - type: 'image', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - }; - } - if (block.uri) { - return { - type: 'image', - source: { type: 'url', url: block.uri }, - }; - } - // Malformed image block — degrade to text - return { type: 'text', text: '[Image: missing data]' }; - - case 'audio': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (block.uri && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on context injection') ?? '[reference]' }; - } - if (block.data && block.mimeType) { - return { - type: 'audio', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - }; - } - // Audio without inline data — degrade to text - return { type: 'text', text: `[Audio: ${block.uri ?? 'missing data'}]` }; - - case 'resource': - // RFC-005: reference blocks become bounded stubs — never raw URIs - // (a signed URL is a bearer credential that looks like a location). - return { type: 'text', text: referenceStubOrNull(block, 'attachment on context injection') ?? '[reference]' }; - - default: - // Unknown wire block types previously fell off the exhaustive switch - // and propagated `undefined` into ContentBlock[]. Fail visibly. - return { type: 'text', text: `[unrecognized content block: ${(block as { type?: string }).type ?? 'untyped'}]` }; - } -} - /** * Convert MCPL injection content (string shorthand or block array) to membrane ContentBlock[]. */ @@ -130,7 +65,7 @@ function convertContent(content: string | McplContentBlock[]): ContentBlock[] { if (typeof content === 'string') { return [{ type: 'text', text: content }]; } - return content.map(convertBlock); + return content.map((block) => convertBlock(block, 'attachment on context injection')); } /** @@ -261,12 +196,22 @@ export class HookOrchestrator { continue; } + // One server's malformed answer, or one malformed injection in it, is + // that server's to fix: it's left out with a line naming the server, + // and every other injection still arrives. Unguarded, a throw here + // leaves fanOutBeforeInference, and the framework's catch around the + // whole hook drops every server's injections for the turn. + const answered: unknown = result.contextInjections; + if (answered !== undefined && answered !== null && !Array.isArray(answered)) { + console.error(`[mcpl] ${server.id}: dropped its beforeInference injections — contextInjections isn't an array`); + continue; + } // §5.4/§10.8: authorize EACH injection by its typed position against // the grant CURRENT NOW, at response receipt — not when the request // was sent. A revocation that landed while the hook was in flight is // enforced here without extra machinery. - if (result.contextInjections && result.contextInjections.length > 0) { - for (const mcplInj of result.contextInjections) { + for (const mcplInj of (answered ?? []) as McplContextInjection[]) { + try { const leaf = INJECT_LEAVES.find((l) => l.position === mcplInj.position); if (!leaf || !CapabilityGrant.of(server).has(leaf.path)) { console.error( @@ -276,6 +221,14 @@ export class HookOrchestrator { continue; } injections.push(convertMcplInjection(mcplInj)); + } catch (error) { + // Such as a null block, content that isn't an array, a null + // injection, or a position with no string form. + const position = (mcplInj as { position?: unknown } | null | undefined)?.position; + console.error( + `[mcpl] ${server.id}: dropped beforeInference injection at position ${logValue(position)} ` + + `— it couldn't be read: ${logDetail(error instanceof Error ? error.message : error)}`, + ); } } } diff --git a/src/mcpl/push-coalescer.ts b/src/mcpl/push-coalescer.ts index 7615256b1..ef3b759e2 100644 --- a/src/mcpl/push-coalescer.ts +++ b/src/mcpl/push-coalescer.ts @@ -245,15 +245,21 @@ function mergeIdentity(prior: CoalescedOccurrence['identity'], next: CoalescedOc * Validate wire content before it is stored, rendered or converted. * * Its shape and its visible emptiness, nothing else. An incoming coalesced - * occurrence reaches context through the converters an uncoalesced message + * occurrence reaches context through the converter an uncoalesced message * goes through: a plain occurrence the moment it is admitted (the coalescer * only remembers where it landed), a deferred one when its render settles or * falls back to it. Its content must be an array of objects. A block the - * converters can't render (a video, an unknown type, an image with neither + * converter can't render (a video, an unknown type, an image with neither * uri nor data) becomes a placeholder, as it does uncoalesced, and so does a - * malformed one, since the converters take any object without throwing: a - * type, text, data, mimeType or uri that isn't a string counts as absent, a - * text block's as no text. The uncoalesced lane doesn't check the array: + * malformed one, since the converter takes any object without throwing: a + * type, data, mimeType or uri that isn't a string counts as absent, and a + * text block whose text isn't a string shows as its own placeholder, which + * counts as no text where MCPL content is admitted: here, on the wire blocks, + * and at the uncoalesced boundaries, through isVisiblyEmptyConversion. The + * framework's backstops read converted content with isVisiblyEmptyContent, + * which would count the placeholder as text, but they see only content one of + * those checks has passed. The uncoalesced + * lane doesn't check the array: * there a member that isn't an object becomes the untyped placeholder, and a * null member or content that isn't an array throws in conversion * (agent-framework#297 brings that lane to this rule). @@ -265,8 +271,8 @@ function mergeIdentity(prior: CoalescedOccurrence['identity'], next: CoalescedOc * * A render result, which the host delivers in the occurrence's place, gets * these checks, as RFC-006 §5.2 gives it every check a pushed payload gets, - * and one more (`checkBlockShapes`): each block must be one the converters - * show as itself or as a reference stub, not as a placeholder. A malformed + * and one more (`checkBlockShapes`): each block must be one the converter + * shows as itself or as a reference stub, not as a placeholder. A malformed * render falls back to the occurrence's own content; a pushed payload has * nothing to fall back to, and gets its placeholders. * @@ -289,12 +295,12 @@ export function validateCoalescedContent( } /** - * Whether the converters (`convertBlock` in push-handler.ts and - * channel-registry.ts) show a block as itself, its text or its image or + * Whether the converter (`convertBlock` in convert-block.ts, which every lane + * uses) shows a block as itself, its text or its image or * audio source, or as a reference stub, and not as a placeholder: an - * unrecognized block, a no-data or no-media-type one, or withheld inline - * data. A text block whose text isn't a string, which they show as no text, - * is refused too. This follows their branches in their order. + * unrecognized block, a no-data or no-media-type one, withheld inline data, + * or a text block whose text isn't a string. This follows its branches in + * their order. */ function showsAsItself(b: Record): boolean { if (b.type === 'text') return typeof b.text === 'string'; diff --git a/src/mcpl/push-handler.ts b/src/mcpl/push-handler.ts index 22dc9fb38..c86027f5e 100644 --- a/src/mcpl/push-handler.ts +++ b/src/mcpl/push-handler.ts @@ -8,10 +8,9 @@ */ import type { ContentBlock } from '@animalabs/membrane'; -import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; +import { convertBlock } from './convert-block.js'; import type { - McplContentBlock, PushEventParams, PushEventResult, } from './types.js'; @@ -19,7 +18,7 @@ import type { FeatureSetManager } from './feature-set-manager.js'; import { McplFeatureSetError } from './feature-set-manager.js'; import { expandCoreTags } from './tags.js'; import { coalesceRejectionReason, EmptyContentError, validateCoalescedContent } from './push-coalescer.js'; -import { isSilentHeartbeatMarker, isVisiblyEmptyContent } from './visible-content.js'; +import { isSilentHeartbeatMarker, isVisiblyEmptyConversion } from './visible-content.js'; import { logDetail, logValue } from './log-value.js'; import { RefusalLines } from './refusal-lines.js'; @@ -52,91 +51,6 @@ export interface McplPushEvent { assemblingFor?: string; } -// ============================================================================ -// Content conversion: McplContentBlock → membrane ContentBlock -// ============================================================================ - -/** - * Convert a single MCPL wire-format content block to a membrane ContentBlock, - * as the channel lane's convertBlock (channel-registry.ts) does, apart from - * the provenance its reference stubs name. Hook context injections convert - * through hook-orchestrator.ts's own. - */ -export function convertBlock(block: McplContentBlock): ContentBlock { - switch (block.type) { - case 'text': - // Text that isn't a string is no text, as the request sanitizer and - // the visible-content check already read it. - return { type: 'text', text: typeof block.text === 'string' ? block.text : '' }; - - case 'image': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (isFilledString(block.uri) && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on push event') ?? '[reference]' }; - } - if (isFilledString(block.data) && isFilledString(block.mimeType)) { - return { - type: 'image', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - } as ContentBlock; - } - if (isFilledString(block.uri)) { - return { - type: 'image', - source: { type: 'url', url: block.uri }, - } as ContentBlock; - } - return { type: 'text', text: isFilledString(block.data) ? '[Image: no media type]' : '[Image: no data]' }; - - case 'audio': - if (isInlineContradiction(block)) { - // RFC-005 vector 2: inline data claiming bulk disposition — fail - // closed, withhold the data (checked BEFORE the data branch). - return { type: 'text', text: INLINE_WITHHELD_TEXT }; - } - if (isFilledString(block.uri) && block.disposition) { - // RFC-005 uri-form media with a disposition claim: stub, do not - // hand the URI to the provider or inline it. - return { type: 'text', text: referenceStubOrNull(block, 'attachment on push event') ?? '[reference]' }; - } - if (isFilledString(block.data) && isFilledString(block.mimeType)) { - return { - type: 'audio', - source: { type: 'base64', data: block.data, mediaType: block.mimeType }, - } as ContentBlock; - } - return { type: 'text', text: isFilledString(block.data) ? '[Audio: no media type]' : '[Audio: no data]' }; - - case 'resource': - // RFC-005: reference blocks become bounded stubs — never raw URIs - // (a signed URL is a bearer credential that looks like a location). - return { type: 'text', text: referenceStubOrNull(block, 'attachment on push event') ?? '[reference]' }; - - default: { - // Unknown wire block types previously fell off the exhaustive switch - // and propagated `undefined` into ContentBlock[]. Fail visibly. A type - // that isn't a string is no type, and is never converted to one: an - // object can throw when it is. - const type: unknown = (block as { type?: unknown }).type; - return { type: 'text', text: `[unrecognized content block: ${typeof type === 'string' ? type : 'untyped'}]` }; - } - } -} - -/** A string with something in it: the only data, mimeType or uri a block's - * source is built from, and the only uri that makes it a reference. Anything - * else a sender puts there leaves the block without it, so conversion never - * throws and never copies a non-string. */ -function isFilledString(value: unknown): value is string { - return typeof value === 'string' && value !== ''; -} - // ============================================================================ // LRU Dedup Set // ============================================================================ @@ -275,8 +189,8 @@ export class PushHandler { // 2. Convert content blocks, and refuse content with nothing visible in // it — before dedup, so a refused eventId is not burned for a retry. - const content: ContentBlock[] = params.payload.content.map(convertBlock); - if (!emptyAllowed && isVisiblyEmptyContent(content)) { + const content: ContentBlock[] = params.payload.content.map((block) => convertBlock(block, 'attachment on push event')); + if (!emptyAllowed && isVisiblyEmptyConversion(params.payload.content, content)) { const err = new EmptyContentError(); this.traceEmptyRejection(serverId, params); if (responder?.respondError) responder.respondError(err.code, err.message, { field: err.field }); diff --git a/src/mcpl/visible-content.ts b/src/mcpl/visible-content.ts index 02eea4b7b..0f6ba545c 100644 --- a/src/mcpl/visible-content.ts +++ b/src/mcpl/visible-content.ts @@ -31,6 +31,33 @@ export function isVisiblyEmptyContent(content: readonly unknown[] | null | undef return true; } +/** + * What the converter (convertBlock, on every lane) shows for a text + * block whose text isn't a string, which only a nonconformant sender writes, + * so the agent learns that a block was there. It is the host's word, not the + * sender's, so it never makes content visible (isVisiblyEmptyConversion). + */ +export const NON_STRING_TEXT_PLACEHOLDER = "[Text not shown: the server's text block had no string text.]"; + +/** A wire text block whose text isn't a string, which the converter shows as + * NON_STRING_TEXT_PLACEHOLDER. */ +function isNonStringTextBlock(block: unknown): boolean { + if (!block || typeof block !== 'object') return false; + const b = block as { type?: unknown; text?: unknown }; + return b.type === 'text' && typeof b.text !== 'string'; +} + +/** + * isVisiblyEmptyContent for content as a converter showed it, `wire` being + * the content as it arrived, block for block: a block shown as + * NON_STRING_TEXT_PLACEHOLDER counts as the nothing it held. So content that + * is only such blocks (or blank text) is still empty, and anything else is + * judged as the converted blocks show it, placeholders included. + */ +export function isVisiblyEmptyConversion(wire: readonly unknown[], converted: readonly unknown[]): boolean { + return isVisiblyEmptyContent(converted.filter((_, i) => !isNonStringTextBlock(wire[i]))); +} + /** The server id and feature set the marker must carry. */ export const SILENT_HEARTBEAT_SERVER_ID = 'heartbeat'; export const SILENT_HEARTBEAT_FEATURE_SET = 'heartbeat'; diff --git a/src/modules/discord/index.ts b/src/modules/discord/index.ts index 665057fd7..a0eb9614e 100644 --- a/src/modules/discord/index.ts +++ b/src/modules/discord/index.ts @@ -42,6 +42,7 @@ import type { FetchHistoryInput, } from './types.js'; import { formatZonedDateTime, resolveTimeZone } from '../../timezone.js'; +import { logValue } from '../../mcpl/log-value.js'; // Re-export event types for consumers who want to handle them export type { DiscordMessageEvent, DiscordEditEvent, DiscordDeleteEvent } from './types.js'; @@ -93,6 +94,12 @@ const TEXT_FILE_EXTENSIONS = [ const CONVERSATION_TIMEOUT_MS = 30 * 60 * 1000; // 30 minutes +/** What the agent reads for a text attachment whose contents couldn't be + * fetched: by an error, or by an answer that wasn't OK. */ +function attachmentNotFetched(filename: string): ContentBlock { + return { type: 'text', text: `[Attachment: ${filename} (failed to fetch)]` }; +} + /** * Discord module for the agent framework. */ @@ -944,13 +951,15 @@ export class DiscordModule implements Module { type: 'text', text: `\n--- File: ${attachment.filename} ---\n${textContent}\n--- End of ${attachment.filename} ---\n`, }); + } else { + // An answer whose body is never read holds its connection. + await response.body?.cancel().catch(() => {}); + console.warn(`Discord: Attachment ${logValue(attachment.filename)} in message ${message.id} wasn't fetched: HTTP ${response.status}`); + blocks.push(attachmentNotFetched(attachment.filename)); } } catch (error) { - // If fetch fails, just note the attachment - blocks.push({ - type: 'text', - text: `[Attachment: ${attachment.filename} (failed to fetch)]`, - }); + console.warn(`Discord: Attachment ${logValue(attachment.filename)} in message ${message.id} wasn't fetched:`, error); + blocks.push(attachmentNotFetched(attachment.filename)); } continue; } diff --git a/test/discord-attachment-notice.test.ts b/test/discord-attachment-notice.test.ts new file mode 100644 index 000000000..637be3297 --- /dev/null +++ b/test/discord-attachment-notice.test.ts @@ -0,0 +1,70 @@ +/** + * What the agent reads for a text attachment the Discord module tries to + * fetch: its contents, or a notice that they couldn't be fetched, whether the + * fetch threw or was answered with a status that wasn't OK. + * + * Run: node --import tsx --test test/discord-attachment-notice.test.ts + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import type { ContentBlock } from '@animalabs/membrane'; +import { DiscordModule } from '../src/modules/discord/index.js'; +import type { DiscordClientInterface, DiscordMessageData } from '../src/modules/discord/types.js'; + +interface ConversionHarness { + convertMessageToContent(message: DiscordMessageData): Promise; +} + +/** A message whose only content is one small text attachment. */ +function messageWithTextFile(filename: string): DiscordMessageData { + return { + id: 'm1', channelId: 'c1', guildId: 'g1', authorId: 'u1', authorName: 'User', isBot: false, + content: '', isReply: false, mentionedUserIds: [], timestamp: new Date(0), + attachments: [{ id: 'a1', filename, url: `https://cdn.example.invalid/${filename}`, contentType: 'text/plain', size: 64 }], + }; +} + +/** Converts the message with `fetch` answering as `answer` does. */ +async function convert( + t: { after: (fn: () => void) => void }, + answer: () => Promise, + message: DiscordMessageData = messageWithTextFile('notes.txt'), +): Promise { + const original = globalThis.fetch; + globalThis.fetch = (async () => answer()) as typeof fetch; + t.after(() => { globalThis.fetch = original; }); + const module = new DiscordModule({} as unknown as DiscordClientInterface, { token: 'test-token' }); + return (module as unknown as ConversionHarness).convertMessageToContent(message); +} + +test('a text attachment whose fetch is answered with a status that isn\'t OK reads as not fetched, not as an empty message', async (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + let response: Response | undefined; + const blocks = await convert(t, async () => (response = new Response('gone', { status: 404 }))); + assert.deepEqual(blocks, [{ type: 'text', text: '[Attachment: notes.txt (failed to fetch)]' }]); + // The operator's line names the file, the message and the status, and the + // unread answer is cancelled, so its connection is released. + assert.deepEqual(warn.mock.calls.map((c) => c.arguments.join(' ')), ['Discord: Attachment notes.txt in message m1 wasn\'t fetched: HTTP 404']); + assert.equal(response!.bodyUsed, true, 'the unread answer was cancelled'); +}); + +test('a filename that could break a log line is printed quoted, as one field', async (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const message = messageWithTextFile('a\nb.txt'); + await convert(t, async () => new Response('gone', { status: 404 }), message); + assert.deepEqual(warn.mock.calls.map((c) => c.arguments.join(' ')), ['Discord: Attachment "a\\nb.txt" in message m1 wasn\'t fetched: HTTP 404']); +}); + +test('a text attachment whose fetch throws reads the same', async (t) => { + const warn = t.mock.method(console, 'warn', () => {}); + const blocks = await convert(t, async () => { throw new TypeError('fetch failed'); }); + assert.deepEqual(blocks, [{ type: 'text', text: '[Attachment: notes.txt (failed to fetch)]' }]); + assert.equal(warn.mock.calls.length, 1); + assert.match(String(warn.mock.calls[0].arguments[0]), /^Discord: Attachment notes\.txt in message m1 wasn't fetched:$/); + assert.equal((warn.mock.calls[0].arguments[1] as Error).message, 'fetch failed'); +}); + +test('a text attachment that is fetched is read whole', async (t) => { + const blocks = await convert(t, async () => new Response('line one', { status: 200 })); + assert.deepEqual(blocks, [{ type: 'text', text: '\n--- File: notes.txt ---\nline one\n--- End of notes.txt ---\n' }]); +}); diff --git a/test/helpers/coalescing-fixture.ts b/test/helpers/coalescing-fixture.ts index 4ff0038e2..7373b5ba1 100644 --- a/test/helpers/coalescing-fixture.ts +++ b/test/helpers/coalescing-fixture.ts @@ -17,7 +17,7 @@ import { MockMembrane, MockYieldingStream, createMockResponse } from './mock-mem export const TS = '2026-09-30T00:00:00Z'; export const ok = () => createMockResponse([{ type: 'text', text: 'ok' }]); -export async function fixture(options: { server?: Record; framework?: Partial; agents?: unknown[]; dir?: string; port?: number; featureSets?: Record; mcpl?: Record } = {}) { +export async function fixture(options: { server?: Record; framework?: Partial; agents?: unknown[]; dir?: string; port?: number; featureSets?: Record; mcpl?: Record; tools?: unknown[]; toolsCall?: (params: Record) => unknown } = {}) { const wss = new WebSocketServer({ port: options.port ?? 0, host: '127.0.0.1' }); await once(wss, 'listening'); let online = true; @@ -44,7 +44,8 @@ export async function fixture(options: { server?: Record; frame ...options.mcpl, } } }, serverInfo: { name: 'editor', version: '1' } }); } else if (m.method === 'featureSets/update') reply({ accepted: true }); - else if (m.method === 'tools/list') reply({ tools: [] }); + else if (m.method === 'tools/list') reply({ tools: options.tools ?? [] }); + else if (m.method === 'tools/call' && options.toolsCall) reply(options.toolsCall(m.params)); else if (m.method === 'channels/publish') { published.push(m.params); if (m.id !== undefined) reply({ delivered: true }); } else if (m.method === 'channels/close') reply({ closed: true }); else if (m.method === 'channels/open') reply({ channel: { id: m.params.channelId, type: 'discord', label: m.params.channelId } }); diff --git a/test/mcpl-coalesced-content-parity.test.ts b/test/mcpl-coalesced-content-parity.test.ts index 4b8612b51..278ee30a3 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -18,6 +18,9 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { fixture, TS } from './helpers/coalescing-fixture.js'; import { RefusalLines } from '../src/mcpl/refusal-lines.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; +import { HookOrchestrator } from '../src/mcpl/hook-orchestrator.js'; +import { CapabilityGrant } from '../src/mcpl/capability-grant.js'; /** Base64 for an image well over 1 MiB once serialized (a Retina screenshot). */ const BIG_IMAGE = { type: 'image', data: 'A'.repeat(1_600_000), mimeType: 'image/png' }; @@ -244,8 +247,10 @@ test('a block whose type, text or source isn\'t a string converts on both lanes, // Data whose media type is unusable is named as such, not as no data. ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'image', data: 'AAAA', mimeType }], shows: '[Image: no media type]' })), ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'audio', data: 'AAAA', mimeType }], shows: '[Audio: no media type]' })), + // A text block whose text isn't a string shows as its placeholder, so the + // agent reads that a block was there; the text beside it shows as itself. ...odd('array_text').map((text, i) => ({ - content: [{ type: 'text', text }, { type: 'text', text: `beside_text_without_text_${i}` }], shows: `beside_text_without_text_${i}`, + content: [{ type: 'text', text }, { type: 'text', text: `beside_text_without_text_${i}` }], shows: NON_STRING_TEXT_PLACEHOLDER, })), // Reference-shaped: a uri that isn't a non-empty string never makes a // reference, with a disposition or without. @@ -280,6 +285,12 @@ test('a block whose type, text or source isn\'t a string converts on both lanes, const expected = malformed.filter((m) => m.shows === shows).length * 4; assert.equal(occurrences(context, JSON.stringify(shows)), expected, shows); } + // And each text that was a string, four times too. + for (const { content } of malformed) { + for (const block of content as Array<{ text?: unknown }>) { + if (typeof block.text === 'string') assert.equal(occurrences(context, JSON.stringify(block.text)), 4, block.text); + } + } for (const leaked of ['toString', 'array_', '424242']) assert(!context.includes(leaked), `nothing that was not a string reaches the context (${leaked})`); }); @@ -300,14 +311,201 @@ test('a text block whose text isn\'t a string is no text, so alone it is refused assert.equal(coalesced.error?.message, plain.error?.message); }); +test('beside blank text, or with no text key at all, such a block is still no text, so the message is refused as empty on both lanes, coalesced or not', async (t) => { + const f = await fixture(); t.after(f.close); await f.register(); + const contents = [ + [{ type: 'text', text: { toString: 1 } }, { type: 'text', text: ' ' }], + [{ type: 'text' }], + ]; + for (const [i, content] of contents.entries()) { + const r = await f.send('channels/incoming', { messages: [ + { ...f.channel(`e${i}`, '', { initial: true }, 'chat', `coalesced${i}`), content }, + { channelId: 'chat', messageId: `plain${i}`, timestamp: TS, author: { id: 'u', name: 'User' }, content }, + ] }); + assert.deepEqual(r.result.results.map((x: { accepted: boolean; reason?: string }) => [x.accepted, x.reason]), + [[false, 'empty_content'], [false, 'empty_content']], JSON.stringify(content)); + const coalesced = await f.send('push/event', { ...f.params(`p${i}`, '', { initial: true }), payload: { content } }); + const { coalesce: _coalesce, ...uncoalesced } = f.params(`q${i}`, ''); + const plain = await f.send('push/event', { ...uncoalesced, payload: { content } }); + assert.equal(coalesced.error?.code, -32602, JSON.stringify(content)); + assert.equal(plain.error?.code, -32602, JSON.stringify(content)); + } +}); + +test('content whose blocks aren\'t objects, or whose block of another type carries a text field, is still admitted uncoalesced on both lanes, as its placeholder', async (t) => { + const f = await fixture(); t.after(f.close); await f.register(); + f.alwaysRespond(); + // Only a text block shown as the non-string text placeholder counts as + // nothing; any other placeholder still counts as something. + for (const [i, content, shows] of [[0, [424242], 'untyped'], [1, [{ type: 'video', text: 5 }], 'video']] as const) { + const r = await f.send('channels/incoming', { messages: [ + { channelId: 'chat', messageId: `plain${i}`, timestamp: TS, author: { id: 'u', name: 'User' }, content }, + ] }); + assert.deepEqual(r.result.results.map((x: { accepted: boolean }) => x.accepted), [true]); + const { coalesce: _coalesce, ...uncoalesced } = f.params(`q${i}`, ''); + const plain = await f.send('push/event', { ...uncoalesced, payload: { content } }); + assert.equal(plain.error, undefined); + assert.equal(plain.result.accepted, true); + await f.framework.runUntilIdle(); + assert.equal(occurrences(f.context(), JSON.stringify(`[unrecognized content block: ${shows}]`)), 2); + } +}); + +test('a context injection\'s text block whose text isn\'t a string shows as the same placeholder, in the system prompt too', async (t) => { + const server = { + id: 'editor', + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system']), []), + sendBeforeInference: async () => ({ + featureSet: 'doc', + contextInjections: [{ + namespace: 'notes', position: 'system', + content: [{ type: 'text', text: { toString: 1 } }, { type: 'text', text: 'injected_text' }], + }], + }), + }; + const hooks = new HookOrchestrator({ getAllServers: () => [server] } as never, { validateInbound: () => {} } as never); + const injections = await hooks.beforeInference({} as never); + // The words themselves, which every other test takes from the constant. + const placeholder = "[Text not shown: the server's text block had no string text.]"; + assert.equal(NON_STRING_TEXT_PLACEHOLDER, placeholder); + assert.deepEqual(injections.map((injection) => injection.content), [ + [{ type: 'text', text: placeholder }, { type: 'text', text: 'injected_text' }], + ]); + // A system injection's text is joined into the system prompt as it + // arrives, so a block the converter passed through would read there as + // String(text); { toString: 1 } has no string form at all. + const f = await fixture(); t.after(f.close); + (f.framework as unknown as { hookOrchestrator: HookOrchestrator }).hookOrchestrator = hooks; + const request = await f.framework.previewActivation('agent', { injections: true }); + const system = (request as { system?: unknown }).system; + assert.equal(typeof system, 'string'); + assert.ok((system as string).endsWith(`test\n${placeholder}\ninjected_text`), system as string); +}); + +test('a malformed block in one server\'s injection shows as its placeholder, as it would pushed, and every server\'s injections still arrive', async () => { + const server = (id: string, content: unknown[]) => ({ + id, + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system']), []), + sendBeforeInference: async () => ({ + featureSet: 'doc', + contextInjections: [{ namespace: 'notes', position: 'system', content }], + }), + }); + // A type with no string form threw while the injection was converted, and + // the framework's catch then dropped every server's injections for the + // turn. Data that isn't a string went to the provider as an image's base64, + // and an audio block's uri was written into its text. + const bad = server('bad', [ + { type: { toString: 1 } }, + { type: 'image', data: 123, mimeType: 'image/png' }, + { type: 'audio', uri: 'https://example.com/a.mp3' }, + ]); + const good = server('good', [{ type: 'text', text: 'good_text' }]); + const hooks = new HookOrchestrator({ getAllServers: () => [bad, good] } as never, { validateInbound: () => {} } as never); + const injections = await hooks.beforeInference({} as never); + assert.deepEqual(injections.map((injection) => injection.content), [ + [ + { type: 'text', text: '[unrecognized content block: untyped]' }, + { type: 'text', text: '[Image: no data]' }, + { type: 'text', text: '[Audio: no data]' }, + ], + [{ type: 'text', text: 'good_text' }], + ]); +}); + +test('an injection the host can\'t read is left out, with a console line, and every other injection still arrives', async (t) => { + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const server = (id: string, contextInjections: unknown) => ({ + id, + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system']), []), + sendBeforeInference: async () => ({ featureSet: 'doc', contextInjections }), + }); + // Each of these used to throw out of the hook, and the framework's catch + // around the whole hook then dropped every server's injections for the + // turn: a null block, content that isn't an array, a null injection, a + // position with no string form, and an answer whose injections aren't a + // list. + const bad = server('bad', [ + { namespace: 'a', position: 'system', content: [null, { type: 'text', text: 'lost_beside_null' }] }, + { namespace: 'b', position: 'system', content: 5 }, + null, + { namespace: 'e', position: { toString: 1 }, content: [{ type: 'text', text: 'lost_with_its_position' }] }, + { namespace: 'c', position: 'system', content: [{ type: 'text', text: 'kept_from_bad' }] }, + ]); + const odd = server('odd', { length: 1 }); + const good = server('good', [{ namespace: 'd', position: 'system', content: [{ type: 'text', text: 'kept_from_good' }] }]); + const hooks = new HookOrchestrator({ getAllServers: () => [bad, odd, good] } as never, { validateInbound: () => {} } as never); + const injections = await hooks.beforeInference({} as never); + assert.deepEqual(injections.map((injection) => injection.content), [ + [{ type: 'text', text: 'kept_from_bad' }], + [{ type: 'text', text: 'kept_from_good' }], + ]); + const dropped = '[mcpl] bad: dropped beforeInference injection at position '; + assert.equal(lines.filter((line) => line.startsWith(dropped) && line.includes(' — it couldn\'t be read: ')).length, 4, lines.join('\n')); + assert.deepEqual(lines.filter((line) => line.startsWith('[mcpl] odd: ')), ['[mcpl] odd: dropped its beforeInference injections — contextInjections isn\'t an array']); +}); + +test('a reference stub names the lane its block came on: a channel message, a pushed event or an injection', async (t) => { + const f = await fixture(); t.after(f.close); await f.register(); + f.alwaysRespond(); + const content = [{ type: 'resource', uri: 'https://host/files?path=x.wav', mimeType: 'audio/wav', sizeBytes: 1000, name: 'x.wav', disposition: 'never' }]; + const r = await f.send('channels/incoming', { messages: [ + { channelId: 'chat', messageId: 'plain', timestamp: TS, author: { id: 'u', name: 'User' }, content }, + ] }); + assert.deepEqual(r.result.results.map((x: { accepted: boolean }) => x.accepted), [true]); + const { coalesce: _coalesce, ...uncoalesced } = f.params('q1', ''); + const plain = await f.send('push/event', { ...uncoalesced, payload: { content } }); + assert.equal(plain.result.accepted, true); + await f.framework.runUntilIdle(); + assert.equal(occurrences(f.context(), 'attachment on channel message'), 1); + assert.equal(occurrences(f.context(), 'attachment on push event'), 1); + const server = { + id: 'editor', + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system']), []), + sendBeforeInference: async () => ({ featureSet: 'doc', contextInjections: [{ namespace: 'notes', position: 'system', content }] }), + }; + const hooks = new HookOrchestrator({ getAllServers: () => [server] } as never, { validateInbound: () => {} } as never); + const [injection] = await hooks.beforeInference({} as never); + assert.equal(occurrences(JSON.stringify(injection?.content), 'attachment on context injection'), 1); +}); + +test('a tool result\'s text block whose text isn\'t a string shows as the same placeholder, alone or beside text', { timeout: 10_000 }, async (t) => { + const results: Record = { + beside: { content: [{ type: 'text', text: null }, { type: 'text', text: 'found_text' }] }, + alone: { content: [{ type: 'text', text: { a: 1 } }] }, + // An empty string is still dropped from the join, as before. + blank: { content: [{ type: 'text', text: '' }, { type: 'text', text: 'found_text' }] }, + }; + const f = await fixture({ + tools: [{ name: 'lookup', description: 'Look something up.', inputSchema: { type: 'object' } }], + toolsCall: (params) => results[(params.arguments as { case: string }).case], + }); + t.after(f.close); + const internals = f.framework as unknown as { pushEvent(event: unknown): void; dispatchToolCall(agent: string, call: unknown): void }; + const call = (id: string) => new Promise<{ data?: unknown }>((resolve) => { + const original = internals.pushEvent; + internals.pushEvent = (event: unknown) => { + const e = event as { type: string; callId?: string; result?: { data?: unknown } }; + if (e.type === 'tool-result' && e.callId === id) { internals.pushEvent = original; resolve(e.result!); } + else original.call(internals, event); + }; + internals.dispatchToolCall('agent', { id, name: 'mcpl--editor--lookup', input: { case: id } }); + }); + assert.equal((await call('beside')).data, `${NON_STRING_TEXT_PLACEHOLDER}\nfound_text`); + assert.equal((await call('alone')).data, NON_STRING_TEXT_PLACEHOLDER); + assert.equal((await call('blank')).data, 'found_text'); +}); + test('an inference filter reads the text conversion stores, a placeholder included, on both lanes, coalesced or not', async (t) => { const seen: string[] = []; const f = await fixture({ server: { shouldTriggerInference: (text: string) => { seen.push(text); return false; } } }); t.after(f.close); await f.register(); - // A text block whose text isn't a string, which reads as no text; a block - // that converts to a placeholder; and visible text. + // A text block whose text isn't a string, and a block of a type the host + // doesn't handle, each of which converts to its placeholder; and visible + // text. const content = [{ type: 'text', text: { toString: 1 } }, { type: 'video', uri: 'https://example.invalid/v.mp4' }, { type: 'text', text: 'visible_text' }]; - const shown = '\n[unrecognized content block: video]\nvisible_text'; + const shown = `${NON_STRING_TEXT_PLACEHOLDER}\n[unrecognized content block: video]\nvisible_text`; const r = await f.send('channels/incoming', { messages: [ { ...f.channel('e1', '', { initial: true }, 'chat', 'coalesced'), content }, { channelId: 'chat', messageId: 'plain', timestamp: TS, author: { id: 'u', name: 'User' }, content }, @@ -322,7 +520,7 @@ test('an inference filter reads the text conversion stores, a placeholder includ assert.equal(coalesced.result?.accepted, true); assert.equal(plain.result?.accepted, true); // Each lane joins its converted text blocks with a line break, the - // non-string one as empty text and the video as its placeholder. + // non-string text and the video each as its placeholder. assert.deepEqual(seen, [shown, shown, 'sibling_text', shown, shown], 'the filter gets the same text on both lanes, and is reached for every message'); }); diff --git a/test/mcpl-push-convert.test.ts b/test/mcpl-push-convert.test.ts index 9f7b592b4..519fb3c6e 100644 --- a/test/mcpl-push-convert.test.ts +++ b/test/mcpl-push-convert.test.ts @@ -6,10 +6,10 @@ */ import test from 'node:test'; import assert from 'node:assert/strict'; -import { convertBlock } from '../src/mcpl/push-handler.js'; +import { convertBlock } from '../src/mcpl/convert-block.js'; import type { McplContentBlock } from '../src/mcpl/types.js'; -const t = (b: unknown) => convertBlock(b as McplContentBlock) as { type: string; text?: string; source?: { type: string } }; +const t = (b: unknown) => convertBlock(b as McplContentBlock, 'attachment on push event') as { type: string; text?: string; source?: { type: string } }; test('resource block converts to a bounded stub, never the raw uri', () => { const out = t({ type: 'resource', uri: 'https://host/files?path=x.wav', mimeType: 'audio/wav', From 8ae6036c8ae0ac1495d26b15070b56b79c24f219 Mon Sep 17 00:00:00 2001 From: Alfred-2570 Date: Sat, 17 Oct 2026 00:28:23 -0400 Subject: [PATCH 2/4] fix(mcpl): an injection's namespace, a refused position's line, and a non-string text beside other blocks - R1-1: an injection whose namespace isn't a string is tried as text inside the per-injection catch. context-manager writes a beforeUser or afterUser injection's namespace into a participant's name as its text as it builds the request, after that catch, so one with no text form failed the whole request, every server's injections with it. Such an injection is now left out with the catch's console line, whatever its position. Any other namespace that isn't a string is kept as it came, as before, since it reached the agent; the first such namespace from each server gets one console line saying that section 10.4 makes the namespace a required string and naming the text it was read as. - R1-2: the line for an injection outside the server's grant prints its position through logValue, so a position holding a line break prints as one field of one line. A position with no string form is refused there now, where it reached the catch only because printing it threw. - R1-3: a tool result's text block whose text isn't a string shows as the converters' placeholder in both array serializers, the history string and the native blocks beside an image, instead of sending the whole array to the JSON fallback, a reference's uri included. - The changelog says that an injection no longer writes an audio block's uri into the agent's text, and that an MCPL tool error whose only text isn't a string reports the placeholder, which a test now pins. It gives the namespace its own before. Tests: namespaces with no text form at system and afterUser, a number at afterUser and a missing one at system, through previewActivation over two requests, beside a server whose injection still arrives, with the warning printed once; a forged position on the grant line; the isolation test's count, three unreadable and one refused; both serializers with a non-string text beside a reference and an image; and the error case. --- changelog.d/fix-silent-drops-say-so.fixed.md | 4 +- src/framework.ts | 6 +- src/mcpl/hook-orchestrator.ts | 31 ++++++++- src/tool-result-history.ts | 7 +- test/mcpl-coalesced-content-parity.test.ts | 68 +++++++++++++++++++- test/tool-result-content.test.ts | 17 +++++ test/tool-result-history.test.ts | 13 ++++ 7 files changed, 135 insertions(+), 11 deletions(-) diff --git a/changelog.d/fix-silent-drops-say-so.fixed.md b/changelog.d/fix-silent-drops-say-so.fixed.md index eea489655..c8b42f65d 100644 --- a/changelog.d/fix-silent-drops-say-so.fixed.md +++ b/changelog.d/fix-silent-drops-say-so.fixed.md @@ -1,3 +1,3 @@ -- An MCPL text block whose text isn't a string, which only a nonconformant server sends, now reaches the agent as `[Text not shown: the server's text block had no string text.]` instead of vanishing from its view or reading as `[object Object]`. That holds on the push and channel lanes, coalesced or not, in a `context/beforeInference` injection, and in a text-only tool result. A `system` injection's text used to be joined into the system prompt as it was, which for an object with no string form threw while the request was built; a text-only tool result whose only block was such a text came back as an empty success. The placeholder still counts as no text when a message is checked for visible content, so a message holding only such blocks (or blank text) is still refused as empty, with the same answers as before, and a message whose blocks are numbers, strings or booleans is still admitted uncoalesced as its untyped placeholders. A `shouldTriggerInference` filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converters' other placeholders. -- A `context/beforeInference` injection's content now converts as a pushed event's and a channel message's does: one converter serves all three. Before, an injection block whose `type` wasn't a string read as that value (`[unrecognized content block: 5]`, `[unrecognized content block: [object Object]]`), and one whose `type` was an object with no string form threw while it was converted, so the framework dropped every server's injections for that turn, with only a console line. An image's `data`, `mimeType` or `uri`, or an audio block's `data` or `mimeType`, that wasn't a string was passed on to the provider as the block's source, and a `uri` with a `disposition` was taken as a reference. An audio block without inline data showed its uri in its text: one that wasn't a string read as itself, and one with no string form threw, as a `type` did. Such fields now count as absent, and such blocks show the placeholders the other lanes show: `[unrecognized content block: untyped]`; `[Image: no data]` (where it read `[Image: missing data]`) or `[Image: no media type]`; and `[Audio: no data]` (where it read `[Audio: ]` or `[Audio: missing data]`) or `[Audio: no media type]`. An injection the host can't read is now left out with a console line naming its server, and the turn keeps every other injection: one holding a null block or content that isn't an array, a null injection, or one whose position has no string form, and likewise an answer whose injections aren't a list. Before, any of these dropped them all. +- An MCPL text block whose text isn't a string, which only a nonconformant server sends, now reaches the agent as `[Text not shown: the server's text block had no string text.]` instead of vanishing from its view or reading as `[object Object]`. That holds on the push and channel lanes, coalesced or not, in a `context/beforeInference` injection, and in a tool result, text-only or beside an image or a reference, where such a block sent the whole result to the agent as JSON, a reference's uri included. An MCPL tool error whose only text isn't a string now reports the placeholder as its error, where it read `[object Object]` for an object, the value itself for a number or `true`, and `Tool call failed` for `null` or `0`. A `system` injection's text used to be joined into the system prompt as it was, which for an object with no string form threw while the request was built; a text-only tool result whose only block was such a text came back as an empty success. The placeholder still counts as no text when a message is checked for visible content, so a message holding only such blocks (or blank text) is still refused as empty, with the same answers as before, and a message whose blocks are numbers, strings or booleans is still admitted uncoalesced as its untyped placeholders. A `shouldTriggerInference` filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converters' other placeholders. +- A `context/beforeInference` injection's content now converts as a pushed event's and a channel message's does: one converter serves all three. Before, an injection block whose `type` wasn't a string read as that value (`[unrecognized content block: 5]`, `[unrecognized content block: [object Object]]`), and one whose `type` was an object with no string form threw while it was converted, so the framework dropped every server's injections for that turn, with only a console line. An image's `data`, `mimeType` or `uri`, or an audio block's `data` or `mimeType`, that wasn't a string was passed on to the provider as the block's source, and a `uri` with a `disposition` was taken as a reference. An audio block without inline data showed its uri in its text: one that wasn't a string read as itself, and one with no string form threw, as a `type` did. Such fields now count as absent, and such blocks show the placeholders the other lanes show: `[unrecognized content block: untyped]`; `[Image: no data]` (where it read `[Image: missing data]`) or `[Image: no media type]`; and `[Audio: no data]` (where it read `[Audio: ]` or `[Audio: missing data]`) or `[Audio: no media type]`. So an injection no longer writes an audio block's uri into the agent's text. An injection the host can't read is now left out with a console line naming its server, and the turn keeps every other injection: one holding a null block or content that isn't an array, a null injection, or one whose position has no string form, and likewise an answer whose injections aren't a list. Before, any of these dropped them all. An injection whose namespace can't be read as text, such as `{"toString": 1}`, is left out the same way. Before, a `beforeUser` or `afterUser` one failed the whole request, since context-manager writes the namespace into a participant's name as it builds the request, and a `system` one arrived, as context-manager never reads its namespace. Any other namespace that isn't a string, which §10.4 doesn't allow, still reaches the agent as it did: a `beforeUser` or `afterUser` injection's is written into its participant's name as its text, such as `system_context:5`, or `system_context:undefined` for an injection without one. The first such namespace from each server now gets a console line saying so. The line for an injection outside the server's grant now prints its position as one field, so a position holding a line break can't add a line of its own. - The Discord module now gives the agent `[Attachment: (failed to fetch)]` for a text attachment whose download is answered with a status that isn't OK, as it already did when the download threw. Before, such an attachment was left out without a word, and a message with nothing else in it read `[Empty message]`. Either failure now also writes a console line naming the file, the message and the status or error, and an answer that isn't OK has its body cancelled, so its connection is released. diff --git a/src/framework.ts b/src/framework.ts index d27c2a65d..96afd3c60 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -14591,8 +14591,10 @@ export class AgentFramework { blocks.push({ type: 'text', text: INLINE_WITHHELD_TEXT }); continue; } - if (b.type === 'text' && typeof b.text === 'string') { - let text = b.text; + if (b.type === 'text') { + // A text that isn't a string shows as the converters' placeholder, + // rather than sending the whole array to the JSON fallback. + let text = typeof b.text === 'string' ? b.text : NON_STRING_TEXT_PLACEHOLDER; if (maxChars && text.length > maxChars) { text = safeSlice(text, 0, maxChars) + '\n\n[truncated — original was ' + text.length + ' chars' diff --git a/src/mcpl/hook-orchestrator.ts b/src/mcpl/hook-orchestrator.ts index 3a6ffd905..ff80582a0 100644 --- a/src/mcpl/hook-orchestrator.ts +++ b/src/mcpl/hook-orchestrator.ts @@ -88,6 +88,8 @@ export class HookOrchestrator { private registry: McplServerRegistry; private featureSetManager: FeatureSetManager; private _isInHook = false; + /** Servers already told that an injection's namespace wasn't a string. */ + private readonly namespaceWarned = new Set(); constructor(registry: McplServerRegistry, featureSetManager: FeatureSetManager) { this.registry = registry; @@ -216,14 +218,15 @@ export class HookOrchestrator { if (!leaf || !CapabilityGrant.of(server).has(leaf.path)) { console.error( `[mcpl] ${server.id}: dropped beforeInference injection at position ` + - `"${mcplInj.position}" — not in the effective grant (§10.8)`, + `${logValue(mcplInj.position)} — not in the effective grant (§10.8)`, ); continue; } + if (typeof mcplInj.namespace !== 'string') this.readNamespace(server.id, mcplInj.namespace); injections.push(convertMcplInjection(mcplInj)); } catch (error) { // Such as a null block, content that isn't an array, a null - // injection, or a position with no string form. + // injection, or a namespace that can't be read as text. const position = (mcplInj as { position?: unknown } | null | undefined)?.position; console.error( `[mcpl] ${server.id}: dropped beforeInference injection at position ${logValue(position)} ` + @@ -236,4 +239,28 @@ export class HookOrchestrator { return injections; } + /** + * Checks a namespace that isn't a string, which §10.4 doesn't allow. One + * that can be read as text is kept as it is, and its server's operator is + * told once: context-manager reads it as its text when it writes a + * beforeUser or afterUser injection's namespace into a participant's name, + * as it always has. One with no text form would throw there, after the + * catch around each injection, and fail the turn, so it throws here, and + * its injection alone is left out, whatever its position. + */ + private readNamespace(serverId: string, namespace: unknown): void { + let text: string; + try { + text = String(namespace); + } catch { + throw new Error("its namespace isn't a string and can't be read as text"); + } + if (this.namespaceWarned.has(serverId)) return; + this.namespaceWarned.add(serverId); + console.error( + `[mcpl] ${serverId}: a beforeInference injection's namespace isn't a string, which §10.4 requires, ` + + `so it was read as its text, ${logValue(text)}; this line prints once per server while the host runs`, + ); + } + } diff --git a/src/tool-result-history.ts b/src/tool-result-history.ts index 73ddf431e..bbba8ea6c 100644 --- a/src/tool-result-history.ts +++ b/src/tool-result-history.ts @@ -15,6 +15,7 @@ */ import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './mcpl/references.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from './mcpl/visible-content.js'; import { formatPreservedImageSlots, formatToolImagePlaceholder, splitPreservingImageSlots } from './tool-image-ledger.js'; export interface HistorySerializeOptions { @@ -101,8 +102,10 @@ function tryHistoryStringFromContentArray(data: unknown, options?: HistorySerial for (const [blockIndex, raw] of data.entries()) { if (!raw || typeof raw !== 'object') return null; const b = raw as { type?: unknown; text?: unknown; data?: unknown; mimeType?: unknown }; - if (b.type === 'text' && typeof b.text === 'string') { - parts.push(b.text); + if (b.type === 'text') { + // A text that isn't a string shows as the converters' placeholder, + // rather than sending the whole array to the JSON fallback. + parts.push(typeof b.text === 'string' ? b.text : NON_STRING_TEXT_PLACEHOLDER); } else if (isInlineContradiction(b)) { // RFC-005 vector 2: checked before the data branch so inline data // claiming bulk disposition is withheld, not rendered. diff --git a/test/mcpl-coalesced-content-parity.test.ts b/test/mcpl-coalesced-content-parity.test.ts index 278ee30a3..782a3ac8d 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -21,6 +21,7 @@ import { RefusalLines } from '../src/mcpl/refusal-lines.js'; import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; import { HookOrchestrator } from '../src/mcpl/hook-orchestrator.js'; import { CapabilityGrant } from '../src/mcpl/capability-grant.js'; +import { logDetail, logValue } from '../src/mcpl/log-value.js'; /** Base64 for an image well over 1 MiB once serialized (a Retina screenshot). */ const BIG_IMAGE = { type: 'image', data: 'A'.repeat(1_600_000), mimeType: 'image/png' }; @@ -425,7 +426,8 @@ test('an injection the host can\'t read is left out, with a console line, and ev // around the whole hook then dropped every server's injections for the // turn: a null block, content that isn't an array, a null injection, a // position with no string form, and an answer whose injections aren't a - // list. + // list. The position is refused as one outside the grant, and the rest + // as unreadable. const bad = server('bad', [ { namespace: 'a', position: 'system', content: [null, { type: 'text', text: 'lost_beside_null' }] }, { namespace: 'b', position: 'system', content: 5 }, @@ -442,10 +444,67 @@ test('an injection the host can\'t read is left out, with a console line, and ev [{ type: 'text', text: 'kept_from_good' }], ]); const dropped = '[mcpl] bad: dropped beforeInference injection at position '; - assert.equal(lines.filter((line) => line.startsWith(dropped) && line.includes(' — it couldn\'t be read: ')).length, 4, lines.join('\n')); + assert.equal(lines.filter((line) => line.startsWith(dropped) && line.includes(' — it couldn\'t be read: ')).length, 3, lines.join('\n')); + assert.equal(lines.filter((line) => line.startsWith(dropped) && line.endsWith(' — not in the effective grant (§10.8)')).length, 1, lines.join('\n')); assert.deepEqual(lines.filter((line) => line.startsWith('[mcpl] odd: ')), ['[mcpl] odd: dropped its beforeInference injections — contextInjections isn\'t an array']); }); +test('an injection whose namespace can\'t be read as text is left out, and any other namespace that isn\'t a string reaches the agent as before, named once per server', async (t) => { + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const server = (id: string, contextInjections: unknown[]) => ({ + id, + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system', 'contextHooks.beforeInference.inject.afterUser']), []), + sendBeforeInference: async () => ({ featureSet: 'doc', contextInjections }), + }); + // context-manager writes an afterUser or beforeUser injection's namespace + // into a participant's name, as its text, as it builds the request, and + // never reads a system injection's. One with no text form threw there, + // after the hook's catch, and failed the whole request; any other reached + // the agent, and still does. + const bad = server('bad', [ + { namespace: { toString: 1 }, position: 'afterUser', content: [{ type: 'text', text: 'lost_no_text_form' }] }, + { namespace: { toString: 1 }, position: 'system', content: [{ type: 'text', text: 'lost_system_no_text_form' }] }, + { namespace: 5, position: 'afterUser', content: [{ type: 'text', text: 'kept_number' }] }, + { position: 'system', content: [{ type: 'text', text: 'kept_missing' }] }, + ]); + const good = server('good', [{ namespace: 'notes', position: 'afterUser', content: [{ type: 'text', text: 'kept_from_good' }] }]); + const hooks = new HookOrchestrator({ getAllServers: () => [bad, good] } as never, { validateInbound: () => {} } as never); + const f = await fixture(); t.after(f.close); + (f.framework as unknown as { hookOrchestrator: HookOrchestrator }).hookOrchestrator = hooks; + const unreadable = (position: string) => `[mcpl] bad: dropped beforeInference injection at position ${position} — it couldn't be read: ${logDetail("its namespace isn't a string and can't be read as text")}`; + for (const turn of [1, 2]) { + const request = await f.framework.previewActivation('agent', { injections: true }) as unknown as { system?: unknown; messages: Array<{ participant: string; content: unknown }> }; + const whole = JSON.stringify(request); + assert.ok(whole.includes('kept_from_good'), whole); + assert.ok(!whole.includes('lost_no_text_form') && !whole.includes('lost_system_no_text_form'), whole); + assert.equal(request.messages.find((m) => JSON.stringify(m.content).includes('kept_number'))?.participant, 'system_context:5', whole); + assert.ok(JSON.stringify(request.system).includes('kept_missing'), whole); + assert.equal(lines.filter((line) => line === unreadable('afterUser')).length, turn, lines.join('\n')); + assert.equal(lines.filter((line) => line === unreadable('system')).length, turn, lines.join('\n')); + } + // A server's first namespace that isn't a string, and only that one, is + // named, as the text it was read as. + assert.deepEqual(lines.filter((line) => line.includes('namespace isn\'t a string, which')), [ + '[mcpl] bad: a beforeInference injection\'s namespace isn\'t a string, which §10.4 requires, so it was read as its text, 5; this line prints once per server while the host runs', + ]); +}); + +test('a position the grant refuses prints as one field of one line, whatever it holds', async (t) => { + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const forged = 'bogus" — x\n[mcpl] other: forged line'; + const server = { + id: 'bad', + grant: new CapabilityGrant(new Set(['contextHooks.beforeInference.inject.system']), []), + sendBeforeInference: async () => ({ featureSet: 'doc', contextInjections: [{ namespace: 'n', position: forged, content: [{ type: 'text', text: 'x' }] }] }), + }; + const hooks = new HookOrchestrator({ getAllServers: () => [server] } as never, { validateInbound: () => {} } as never); + assert.deepEqual(await hooks.beforeInference({} as never), []); + assert.deepEqual(lines, [`[mcpl] bad: dropped beforeInference injection at position ${logValue(forged)} — not in the effective grant (§10.8)`]); + assert.ok(!lines[0]!.includes('\n'), lines[0]); +}); + test('a reference stub names the lane its block came on: a channel message, a pushed event or an injection', async (t) => { const f = await fixture(); t.after(f.close); await f.register(); f.alwaysRespond(); @@ -476,6 +535,8 @@ test('a tool result\'s text block whose text isn\'t a string shows as the same p alone: { content: [{ type: 'text', text: { a: 1 } }] }, // An empty string is still dropped from the join, as before. blank: { content: [{ type: 'text', text: '' }, { type: 'text', text: 'found_text' }] }, + // An error's text is its message, so such a text is the error's words. + error: { isError: true, content: [{ type: 'text', text: { a: 1 } }] }, }; const f = await fixture({ tools: [{ name: 'lookup', description: 'Look something up.', inputSchema: { type: 'object' } }], @@ -483,7 +544,7 @@ test('a tool result\'s text block whose text isn\'t a string shows as the same p }); t.after(f.close); const internals = f.framework as unknown as { pushEvent(event: unknown): void; dispatchToolCall(agent: string, call: unknown): void }; - const call = (id: string) => new Promise<{ data?: unknown }>((resolve) => { + const call = (id: string) => new Promise<{ data?: unknown; error?: unknown }>((resolve) => { const original = internals.pushEvent; internals.pushEvent = (event: unknown) => { const e = event as { type: string; callId?: string; result?: { data?: unknown } }; @@ -495,6 +556,7 @@ test('a tool result\'s text block whose text isn\'t a string shows as the same p assert.equal((await call('beside')).data, `${NON_STRING_TEXT_PLACEHOLDER}\nfound_text`); assert.equal((await call('alone')).data, NON_STRING_TEXT_PLACEHOLDER); assert.equal((await call('blank')).data, 'found_text'); + assert.equal((await call('error')).error, NON_STRING_TEXT_PLACEHOLDER); }); test('an inference filter reads the text conversion stores, a placeholder included, on both lanes, coalesced or not', async (t) => { diff --git a/test/tool-result-content.test.ts b/test/tool-result-content.test.ts index f9a0eacf7..b5d56f275 100644 --- a/test/tool-result-content.test.ts +++ b/test/tool-result-content.test.ts @@ -7,6 +7,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { AgentFramework } from '../src/framework.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; function makeFw() { return Object.create(AgentFramework.prototype) as any; @@ -39,3 +40,19 @@ test('error results pass through unchanged', () => { assert.equal(result.isError, true); assert.equal(result.content, 'boom'); }); + +test('beside an image, a text block whose text is not a string shows the placeholder, and a reference is still a stub', () => { + // The native blocks used to give way to the JSON fallback, which sent the + // image as base64 text and the reference's uri as it was. + const fw = makeFw(); + const result = fw.toMembraneToolResult('call-5', { success: true, data: [ + { type: 'text', text: { a: 1 } }, + { type: 'image', data: 'AAAA', mimeType: 'image/png' }, + { type: 'resource', uri: 'https://signed.example/f?token=RAW_TOKEN', mimeType: 'text/plain', sizeBytes: 10, name: 'f.txt', disposition: 'never' }, + ] }); + assert.ok(Array.isArray(result.content), String(result.content).slice(0, 200)); + assert.deepEqual(result.content[0], { type: 'text', text: NON_STRING_TEXT_PLACEHOLDER }); + assert.equal(result.content[1].type, 'image'); + assert.ok(result.content[2].text.startsWith('[ref_'), result.content[2].text); + assert.ok(!JSON.stringify(result.content).includes('RAW_TOKEN')); +}); diff --git a/test/tool-result-history.test.ts b/test/tool-result-history.test.ts index 9fe34494a..9cddad25f 100644 --- a/test/tool-result-history.test.ts +++ b/test/tool-result-history.test.ts @@ -7,6 +7,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { toolResultDataToHistoryString } from '../src/tool-result-history.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; test('text-only MCP content array: blocks join with newlines', () => { const data = [ @@ -52,6 +53,18 @@ test('mixed known + unknown block shape falls back to JSON (all-or-nothing)', () assert.equal(toolResultDataToHistoryString(data), JSON.stringify(data)); }); +test('a text block whose text is not a string shows the placeholder, so a reference beside it is still a stub', () => { + // It used to send the whole array to the JSON fallback, the reference's + // uri included. + const data = [ + { type: 'text', text: null }, + { type: 'resource', uri: 'https://signed.example/f?token=RAW_TOKEN', mimeType: 'text/plain', sizeBytes: 10, name: 'f.txt', disposition: 'never' }, + ]; + const out = toolResultDataToHistoryString(data); + assert.ok(out.startsWith(`${NON_STRING_TEXT_PLACEHOLDER}\n[ref_`), out); + assert.ok(!out.includes('RAW_TOKEN'), out); +}); + test('non-array data: JSON stringify', () => { assert.equal(toolResultDataToHistoryString({ ok: true }), '{"ok":true}'); assert.equal(toolResultDataToHistoryString('hello'), '"hello"'); From 5c0cb2e1efbd72e4530689351a39d24fd9eec86f Mon Sep 17 00:00:00 2001 From: Alfred-2570 Date: Sat, 17 Oct 2026 03:02:30 -0400 Subject: [PATCH 3/4] docs(changelog): every value a tool error with no string text read before On main, an MCPL tool error whose only text isn't a string read "Tool call failed" for false as well as for null and 0, and an array as its items joined by commas. The fragment named neither. --- changelog.d/fix-silent-drops-say-so.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/fix-silent-drops-say-so.fixed.md b/changelog.d/fix-silent-drops-say-so.fixed.md index c8b42f65d..962161ca8 100644 --- a/changelog.d/fix-silent-drops-say-so.fixed.md +++ b/changelog.d/fix-silent-drops-say-so.fixed.md @@ -1,3 +1,3 @@ -- An MCPL text block whose text isn't a string, which only a nonconformant server sends, now reaches the agent as `[Text not shown: the server's text block had no string text.]` instead of vanishing from its view or reading as `[object Object]`. That holds on the push and channel lanes, coalesced or not, in a `context/beforeInference` injection, and in a tool result, text-only or beside an image or a reference, where such a block sent the whole result to the agent as JSON, a reference's uri included. An MCPL tool error whose only text isn't a string now reports the placeholder as its error, where it read `[object Object]` for an object, the value itself for a number or `true`, and `Tool call failed` for `null` or `0`. A `system` injection's text used to be joined into the system prompt as it was, which for an object with no string form threw while the request was built; a text-only tool result whose only block was such a text came back as an empty success. The placeholder still counts as no text when a message is checked for visible content, so a message holding only such blocks (or blank text) is still refused as empty, with the same answers as before, and a message whose blocks are numbers, strings or booleans is still admitted uncoalesced as its untyped placeholders. A `shouldTriggerInference` filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converters' other placeholders. +- An MCPL text block whose text isn't a string, which only a nonconformant server sends, now reaches the agent as `[Text not shown: the server's text block had no string text.]` instead of vanishing from its view or reading as `[object Object]`. That holds on the push and channel lanes, coalesced or not, in a `context/beforeInference` injection, and in a tool result, text-only or beside an image or a reference, where such a block sent the whole result to the agent as JSON, a reference's uri included. An MCPL tool error whose only text isn't a string now reports the placeholder as its error, where it read `[object Object]` for an object, the value itself for a number or `true`, its items joined by commas for an array, and `Tool call failed` for `null`, `false` or `0`. A `system` injection's text used to be joined into the system prompt as it was, which for an object with no string form threw while the request was built; a text-only tool result whose only block was such a text came back as an empty success. The placeholder still counts as no text when a message is checked for visible content, so a message holding only such blocks (or blank text) is still refused as empty, with the same answers as before, and a message whose blocks are numbers, strings or booleans is still admitted uncoalesced as its untyped placeholders. A `shouldTriggerInference` filter, and so the EventGate's content filter, now reads the placeholder where it read an empty line, as it already reads the converters' other placeholders. - A `context/beforeInference` injection's content now converts as a pushed event's and a channel message's does: one converter serves all three. Before, an injection block whose `type` wasn't a string read as that value (`[unrecognized content block: 5]`, `[unrecognized content block: [object Object]]`), and one whose `type` was an object with no string form threw while it was converted, so the framework dropped every server's injections for that turn, with only a console line. An image's `data`, `mimeType` or `uri`, or an audio block's `data` or `mimeType`, that wasn't a string was passed on to the provider as the block's source, and a `uri` with a `disposition` was taken as a reference. An audio block without inline data showed its uri in its text: one that wasn't a string read as itself, and one with no string form threw, as a `type` did. Such fields now count as absent, and such blocks show the placeholders the other lanes show: `[unrecognized content block: untyped]`; `[Image: no data]` (where it read `[Image: missing data]`) or `[Image: no media type]`; and `[Audio: no data]` (where it read `[Audio: ]` or `[Audio: missing data]`) or `[Audio: no media type]`. So an injection no longer writes an audio block's uri into the agent's text. An injection the host can't read is now left out with a console line naming its server, and the turn keeps every other injection: one holding a null block or content that isn't an array, a null injection, or one whose position has no string form, and likewise an answer whose injections aren't a list. Before, any of these dropped them all. An injection whose namespace can't be read as text, such as `{"toString": 1}`, is left out the same way. Before, a `beforeUser` or `afterUser` one failed the whole request, since context-manager writes the namespace into a participant's name as it builds the request, and a `system` one arrived, as context-manager never reads its namespace. Any other namespace that isn't a string, which §10.4 doesn't allow, still reaches the agent as it did: a `beforeUser` or `afterUser` injection's is written into its participant's name as its text, such as `system_context:5`, or `system_context:undefined` for an injection without one. The first such namespace from each server now gets a console line saying so. The line for an injection outside the server's grant now prints its position as one field, so a position holding a line break can't add a line of its own. - The Discord module now gives the agent `[Attachment: (failed to fetch)]` for a text attachment whose download is answered with a status that isn't OK, as it already did when the download threw. Before, such an attachment was left out without a word, and a message with nothing else in it read `[Empty message]`. Either failure now also writes a console line naming the file, the message and the status or error, and an answer that isn't OK has its body cancelled, so its connection is released. From 3168a0dbc7a3efbd16fea86ea458b24d757599ff Mon Sep 17 00:00:00 2001 From: Alfred-2570 Date: Sat, 17 Oct 2026 22:05:40 -0400 Subject: [PATCH 4/4] fix(mcpl): the namespace warning says what a namespace that isn't a string does (R2-3) The line said such a namespace "was read as its text". context-manager reads it only where it names a participant, at beforeUser and afterUser; at system nothing reads it. And the framework's channel scoping compares the value with the open channels' string ids, so it keeps the injection to no channel and every agent gets it, as before. The line now says both, which holds at every position and every use. --- src/mcpl/hook-orchestrator.ts | 5 +++-- test/mcpl-coalesced-content-parity.test.ts | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/mcpl/hook-orchestrator.ts b/src/mcpl/hook-orchestrator.ts index ff80582a0..bdd596938 100644 --- a/src/mcpl/hook-orchestrator.ts +++ b/src/mcpl/hook-orchestrator.ts @@ -258,8 +258,9 @@ export class HookOrchestrator { if (this.namespaceWarned.has(serverId)) return; this.namespaceWarned.add(serverId); console.error( - `[mcpl] ${serverId}: a beforeInference injection's namespace isn't a string, which §10.4 requires, ` + - `so it was read as its text, ${logValue(text)}; this line prints once per server while the host runs`, + `[mcpl] ${serverId}: a beforeInference injection's namespace isn't a string, which §10.4 requires, so it keeps the ` + + `injection to no channel and every agent gets it; where it names a participant, it's read as its text, ${logValue(text)}; ` + + 'this line prints once per server while the host runs', ); } diff --git a/test/mcpl-coalesced-content-parity.test.ts b/test/mcpl-coalesced-content-parity.test.ts index 782a3ac8d..8c4aa321f 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -484,9 +484,10 @@ test('an injection whose namespace can\'t be read as text is left out, and any o assert.equal(lines.filter((line) => line === unreadable('system')).length, turn, lines.join('\n')); } // A server's first namespace that isn't a string, and only that one, is - // named, as the text it was read as. + // named, with what it does: it keeps the injection to no channel, and it's + // read as its text where it names a participant. assert.deepEqual(lines.filter((line) => line.includes('namespace isn\'t a string, which')), [ - '[mcpl] bad: a beforeInference injection\'s namespace isn\'t a string, which §10.4 requires, so it was read as its text, 5; this line prints once per server while the host runs', + '[mcpl] bad: a beforeInference injection\'s namespace isn\'t a string, which §10.4 requires, so it keeps the injection to no channel and every agent gets it; where it names a participant, it\'s read as its text, 5; this line prints once per server while the host runs', ]); });