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..962161ca8 --- /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 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. diff --git a/src/framework.ts b/src/framework.ts index 013277a59..96afd3c60 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. @@ -14590,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' @@ -17944,9 +17947,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..bdd596938 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')); } /** @@ -153,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; @@ -261,21 +198,40 @@ 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( `[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 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)} ` + + `— it couldn't be read: ${logDetail(error instanceof Error ? error.message : error)}`, + ); } } } @@ -283,4 +239,29 @@ 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 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/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/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/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..8c4aa321f 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -18,6 +18,10 @@ 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'; +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' }; @@ -244,8 +248,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 +286,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 +312,263 @@ 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. 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 }, + 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, 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, 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 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', + ]); +}); + +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(); + 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' }] }, + // 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' } }], + 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; error?: 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'); + 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) => { 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 +583,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', 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"');