From 771eea1be15bad0e371bb40671eeab717670bb87 Mon Sep 17 00:00:00 2001 From: Pippa-2571 Date: Sun, 18 Oct 2026 06:34:59 -0400 Subject: [PATCH 1/3] fix(mcpl): a reference from a server with no address says so when refused A server the host doesn't connect to over a network, as a stdio server is not, delivers references that are bound to it, so their stubs offer fetch_reference. The framework's resolver returned nothing for a server without a url, so the fetcher answered "unknown server: " for a server the host does know, and save_recent_image's refusal for such an image said "Fetch it with fetch_reference and save the materialized file instead." The resolver gave the fetcher any other server's url, whatever its transport, so a reference on that url's origin was fetched, with any token the config holds, though the host never connects there for a server it spawns, nor for a WebSocket server whose url isn't ws:// or wss://. The resolver now answers three ways: a server's address and credential; 'no-address' for a server the host doesn't connect to over a network; or nothing for a server the host doesn't know. It decides as the connection does: through isWebSocketTransport, so a server spawned from a command has no address whatever else its config holds, and then through buildWebSocketUrl's own check of the url, called with no token, so a WebSocket server whose url the transport won't dial has none either. The fetcher refuses a server with no address in a phrase of its own, from noAddressReason in references.ts beside UNBOUND_REASON: "no address: this host doesn't connect to server over a network, so there's nowhere to fetch it from". It names the server, as "unknown server: " does, and like that reason it isn't one of a reference's own phrases, so fetch_reference gives it without the "reference " prefix. An unknown server still reads "unknown server: ", and any falsy answer from the resolver is still an unknown server. Every check the fetcher makes before it dials now sits in one function, in its order: the server (none, unknown, no address, off the uri's origin), the expiry, a spent budget, a claimed size over the ceiling (refused for the budget when it is also over what's left of it). The fetch asks it; so does the fetcher's new refusal(record), which answers what a fetch_reference call naming no max_bytes would be refused with before dialing, or null where the fetch would serve a saved copy, join a fetch of it already under way, or dial; and so does the eager check, which then adds only the server's eager limit, so a record the fetch would refuse (a spent budget, a claim over the fetch's ceiling) takes no eager slot. save_recent_image asks refusal(record) and gives its reason in its own sentence, as it does an unbound or expired reference's, and points at fetch_reference only where the fetch would serve, join or dial. A stub still offers the fetch for every bound reference: it is built where only the record is known, and which servers the host can dial is the framework's to know. unfetchableReason, which gives the stub its reason from the record alone, now serves the stub only. The comments on convertBlock and on executeMcplToolCall's registration now say a reference they register can be fetched through its server's address, where it has one, and so does the unreleased changelog fragment for that registration. Tests: the fetcher refuses a reference from a server with no address in the phrase naming it, gives it no eager slot though its claim is small, and passes over such a server for one on the uri's origin; a server with no address ahead of one off the origin is the refusal, and the other way round the origin check's is; refusal() answers, case by case, what the fetch then answers, for no server, an unknown server, no address, an off-origin uri, an expiry, a spent budget, a claim over the ceiling, and one over the ceiling and over what's left of the budget (both from a server whose eager limit is past them), with no eager slot for any of them, and nothing for a saved copy or for one it would dial, which is eager-eligible, while a saved copy of one it would dial takes no eager slot; a reference with two faults is refused for the first the fetch checks; and refusal() answers nothing while a fetch of the record is under way. fetch_reference through the framework's own fetcher fetches a push event's reference through a WebSocket server's ws:// url, and through one whose transport is websocket though its config holds a command; it refuses one from a stdio server, from a server spawned from a command though its config also holds a url, from a server whose transport is stdio though its config holds only a url, and from a WebSocket server whose url is http://, each in the phrase naming it, and one from a server it doesn't know as unknown. save_recent_image refuses a reference from a server with no address, one off its server's origin, one from an unknown server, one claimed over the ceiling and one from a server whose budget is spent, each with the fetcher's reason. The save_recent_image tests whose bound references reach the fetch, or its expiry, now configure their server at a wss:// url, as the fetcher resolves it. Co-Authored-By: Claude Opus 5.5 --- .../converter-references-bound.fixed.md | 3 +- .../no-address-server-refusal.fixed.md | 14 ++ src/framework.ts | 45 ++++-- src/mcpl/convert-block.ts | 4 +- src/mcpl/reference-fetcher.ts | 147 ++++++++++++------ src/mcpl/references.ts | 25 ++- test/mcpl-reference-binding.test.ts | 58 +++++-- test/mcpl-reference-fetcher.test.ts | 125 ++++++++++++++- test/save-recent-image.test.ts | 73 +++++++++ 9 files changed, 407 insertions(+), 87 deletions(-) create mode 100644 changelog.d/no-address-server-refusal.fixed.md diff --git a/changelog.d/converter-references-bound.fixed.md b/changelog.d/converter-references-bound.fixed.md index 052b16e2b..8193b3ce4 100644 --- a/changelog.d/converter-references-bound.fixed.md +++ b/changelog.d/converter-references-bound.fixed.md @@ -2,7 +2,8 @@ a context injection or the successful result of an MCPL tool called in process is registered with the server that sent it, as the references in the result of an MCPL tool call the agent makes are, so `fetch_reference` - can fetch it from that server. These were registered with no server, so + can fetch it from that server, where the host has an address for it. + These were registered with no server, so `fetch_reference` refused them, unless a tool result had registered the same uri with its server. A reference now keeps every server that delivered its uri, and `fetch_reference` dials through the first of them diff --git a/changelog.d/no-address-server-refusal.fixed.md b/changelog.d/no-address-server-refusal.fixed.md new file mode 100644 index 000000000..d3dba5b2a --- /dev/null +++ b/changelog.d/no-address-server-refusal.fixed.md @@ -0,0 +1,14 @@ +- Where `fetch_reference` refused a reference from an MCPL server the host + doesn't connect to over a network, such as a stdio server, as + `unknown server: `, it now says `no address: this host doesn't + connect to server over a network, so there's nowhere to fetch it + from`. A server the host spawns from a command has no address either, + even when its config also holds a `url`, on whose origin its references + used to be fetched; nor has a WebSocket server whose `url` isn't `ws://` + or `wss://`. `save_recent_image` now gives the reason a + `fetch_reference` call naming no `max_bytes` would be refused with before + dialing, and points at `fetch_reference` only where that fetch would go + ahead. The eager fetch of the references in the result of an MCPL tool + call the agent makes no longer spends one of its slots on a reference the + fetch would refuse before dialing, such as one from a server whose fetch + budget is spent. diff --git a/src/framework.ts b/src/framework.ts index 06b698273..99b3fe551 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -1,6 +1,6 @@ import { ToolPresentation, presentationTools, isPresentationTool, renderCatalogue, type PresentationSnapshot } from "./tool-presentation.js"; import { dirname, join } from 'node:path'; -import { INLINE_WITHHELD_TEXT, REFERENCE_LIMITS, classifyBlock, isInlineContradiction, isReferenceExpired, referenceRegistry, referenceStubOrNull, sanitizeLabel, unfetchableReason } from './mcpl/references.js'; +import { INLINE_WITHHELD_TEXT, REFERENCE_LIMITS, classifyBlock, isInlineContradiction, isReferenceExpired, referenceRegistry, referenceStubOrNull, sanitizeLabel } from './mcpl/references.js'; import { ReferenceFetcher, DEFAULT_FETCH_MAX_BYTES, EAGER_FETCH_TIMEOUT_MS } from './mcpl/reference-fetcher.js'; import { appendFileSync, closeSync, existsSync, fsyncSync, mkdirSync, openSync, readFileSync, renameSync, unlinkSync, @@ -481,7 +481,7 @@ import { UsageTracker, persistedUsageProblem, type PersistedUsageState } from '. import type { SessionUsageSnapshot, UsageUpdatedEvent } from './usage/types.js'; import { stderrTailOf, withStderr, type McplConnectionCloseInfo, type McplServerConnection } from './mcpl/server-connection.js'; import { boundedForLog, describeCause } from './mcpl/errors.js'; -import { assertServerConfigBooleans, isWebSocketTransport } from './mcpl/transport.js'; +import { assertServerConfigBooleans, buildWebSocketUrl, isWebSocketTransport } from './mcpl/transport.js'; import type { McplServerConfig, McplHostCapabilities, @@ -4690,7 +4690,17 @@ export class AgentFramework { this.referenceFetcherInstance = new ReferenceFetcher( (serverId) => { const cfg = this.mcplServerConfigs.get(serverId); - if (!cfg?.url) return undefined; + if (!cfg) return undefined; + // A server's address is the url the host dials it at, as the + // transport decides: none for a server the host spawns, as a stdio + // server, nor for a WebSocket server whose url the transport won't + // dial, which buildWebSocketUrl refuses (not ws:// or wss://). + if (!isWebSocketTransport(cfg) || !cfg.url) return 'no-address'; + try { + buildWebSocketUrl(cfg, null); + } catch { + return 'no-address'; + } return { url: cfg.url, token: cfg.token, autofetch: cfg.autofetch }; }, async (fileName, data, mimeType) => { @@ -17368,11 +17378,12 @@ export class AgentFramework { if (result.isError) return mcplToolFailure(result.content); // RFC-005 §6.2: register the result's references with this server, so // a stub later made of them is bound and fetch_reference can fetch - // them. Nothing is fetched here: an in-process caller may never show - // them to an agent, and an eager fetch would spend the server's fetch - // budget, which the agent's own fetch_reference needs, on bytes nobody - // asked for. A failure keeps only its text (mcplToolFailure), so no - // stub is made of its references and they aren't registered. + // them through this server's address, where it has one. Nothing is + // fetched here: an in-process caller may never show them to an agent, + // and an eager fetch would spend the server's fetch budget, which the + // agent's own fetch_reference needs, on bytes nobody asked for. A + // failure keeps only its text (mcplToolFailure), so no stub is made of + // its references and they aren't registered. await this.autoFetchReferences(result.content, serverId, false) .catch((e) => console.error('[rfc005] reference registration error:', (e as Error).message)); return { @@ -23017,13 +23028,15 @@ export class AgentFramework { }; } if (candidate.kind === 'reference') { - // Point at fetch_reference only where it can deliver: not for a - // reference this process holds no record of (fetch_reference - // looks the id up in the same registry), nor, as the stub does, - // for one whose record shows the fetcher would refuse it as - // unbound or expired. A saved copy it serves. + // Point at fetch_reference only where its fetch would go ahead: not + // for a reference this process holds no record of (fetch_reference + // looks the id up in the same registry), nor for one the fetcher + // would refuse before dialing. Its refusal() gives the reason a + // fetch_reference call naming no max_bytes would be refused with, + // from the checks its fetch makes, and none where the fetch would + // serve a saved copy or join a fetch already under way. const record = referenceRegistry.get(candidate.refId); - const unfetchable = record ? unfetchableReason(record) : null; + const refusal = record ? this.getReferenceFetcher().refusal(record) : null; throw new Error( `save_recent_image: image at index ${position} is a reference (${candidate.refId}, ${candidate.mediaType}, ` + `from ${describeTool(candidate)}) — its bytes are not held here. Nothing written. ` + @@ -23031,8 +23044,8 @@ export class AgentFramework { ? 'This process holds no record of it (a restart or an eviction drops records, and a stub quoted ' + "from elsewhere never had one), so fetch_reference can't fetch it either. If its tool can produce " + 'it again, re-run that tool for a fresh reference.' - : unfetchable - ? `It can't be fetched either — ${unfetchable}.` + : refusal + ? `It can't be fetched either — ${refusal.reason}.` : 'Fetch it with fetch_reference and save the materialized file instead.'), ); } diff --git a/src/mcpl/convert-block.ts b/src/mcpl/convert-block.ts index 2ba60cc18..f389cb3e7 100644 --- a/src/mcpl/convert-block.ts +++ b/src/mcpl/convert-block.ts @@ -17,8 +17,8 @@ import type { McplContentBlock } from './types.js'; * shows as NON_STRING_TEXT_PLACEHOLDER. `provenance` is what a reference stub * says the reference came on, such as 'attachment on push event'. `serverId` * is the server that sent the content: a reference is registered with it, as - * a tool result's references are, so fetch_reference can fetch it from that - * server. + * a tool result's references are, so fetch_reference can fetch it through that + * server's address, where it has one. */ export function convertBlock(block: McplContentBlock, provenance: string, serverId: string): ContentBlock { switch (block.type) { diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index 8beeebd75..38e89335a 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -30,7 +30,7 @@ */ import { createHash } from 'node:crypto'; import type { ReferenceRecord } from './references.js'; -import { EXPIRY_REASON, isReferenceExpired, referenceExpiry, REFERENCE_LIMITS, sanitizeLabel, UNBOUND_REASON } from './references.js'; +import { EXPIRY_REASON, noAddressReason, referenceExpiry, REFERENCE_LIMITS, sanitizeLabel, UNBOUND_REASON } from './references.js'; /** Per-fetch hard ceiling on actual bytes (overridable per server / call). */ export const DEFAULT_FETCH_MAX_BYTES = 256 * 1024 * 1024; @@ -49,13 +49,35 @@ const BUDGET_LASTS = "the budget doesn't renew on its own, so calling again won' const EAGER_FETCH_TIMEOUT_MS = 10_000; export interface ResolvedServerBinding { - /** The url the connection was dialed to (ws/wss/http/https). */ + /** The url the host dials the server at. The framework's resolver gives only + * a ws:// or wss:// url its WebSocket transport dials; the origin check + * reads ws as http and wss as https. */ url: string; /** Connection credential, applied host-side as Authorization: Bearer. */ token?: string; autofetch?: { maxBytes?: number; maxTotalBytes?: number }; } +/** What the host knows of a server a reference came from: its binding, or + * 'no-address' for one the host doesn't reach over a network, such as a + * stdio server, which has no address to dial. A server the host doesn't + * know resolves to undefined. */ +export type ServerResolution = ResolvedServerBinding | 'no-address'; + +/** Why the fetcher would refuse a reference without dialing. `ofReference` + * marks one of the reference's own phrases that its stub shares (unbound, + * expired), which fetch_reference words as "reference "; it gives + * the others as they are. */ +export interface Refusal { + reason: string; + ofReference: boolean; +} + +/** fetch_reference's words for a refusal. */ +function refusalText(refusal: Refusal): string { + return refusal.ofReference ? `reference ${refusal.reason}` : refusal.reason; +} + export interface FetchOutcome { ok: boolean; /** Mount-prefixed workspace path of the saved payload (on success). */ @@ -118,41 +140,101 @@ export class ReferenceFetcher { private inFlight = new Map>(); constructor( - private readonly resolveServer: (serverId: string) => ResolvedServerBinding | undefined, + private readonly resolveServer: (serverId: string) => ServerResolution | undefined, /** Persist the payload; returns a mount-prefixed path or null when no * writable workspace exists. File name is host-generated (§7.3). */ private readonly saveFile: (fileName: string, data: Buffer, mimeType: string) => Promise, ) {} - /** Would the eager (autofetch) default fetch this record? Not one the - * fetcher refuses as expired, which would only take an eager slot. It - * takes the eager limit of the server the fetch would dial. */ + /** Would the eager (autofetch) default fetch this record? Not one the fetch + * would refuse before dialing, asked as the eager fetch asks it, with the + * default ceiling, so such a record takes no eager slot; and only one whose + * claimed size is within the eager limit of the server it dials. Whether a + * fetch of it is already under way doesn't enter: the eager fetch joins + * one that is. */ isEagerEligible(record: ReferenceRecord): boolean { if (record.fetchedPath) return false; - if (isReferenceExpired(record.testimony)) return false; - const server = this.serverFor(record); - if ('refusal' in server) return false; - const cap = server.binding.autofetch?.maxBytes ?? DEFAULT_EAGER_FETCH_MAX_BYTES; + const check = this.beforeDial(record); + if ('refusal' in check) return false; + const cap = check.binding.autofetch?.maxBytes ?? DEFAULT_EAGER_FETCH_MAX_BYTES; return record.testimony.sizeBytes !== undefined && record.testimony.sizeBytes <= cap; } /** The server to dial for a record (§6.2): the first, in the order they * delivered it, whose address is on the uri's origin. When none is, the - * first server's refusal (unknown to this host, or the origin check's), - * and for a record no server delivered, that it's unbound. */ - private serverFor(record: ReferenceRecord): { serverId: string; binding: ResolvedServerBinding } | { refusal: string } { - let refusal: string | undefined; + * first server's refusal (unknown to this host, with no address to dial, + * or the origin check's), and for a record no server delivered, that it's + * unbound. */ + private serverFor(record: ReferenceRecord): { serverId: string; binding: ResolvedServerBinding } | { refusal: Refusal } { + let refusal: Refusal | undefined; for (const serverId of record.serverIds) { const binding = this.resolveServer(serverId); if (!binding) { - refusal ??= `unknown server: ${serverId}`; + refusal ??= { reason: `unknown server: ${serverId}`, ofReference: false }; + continue; + } + if (binding === 'no-address') { + refusal ??= { reason: noAddressReason(serverId), ofReference: false }; continue; } const offOrigin = this.originCheck(record, binding); if (offOrigin === null) return { serverId, binding }; - refusal ??= offOrigin; + refusal ??= { reason: offOrigin, ofReference: false }; } - return { refusal: refusal ?? `reference ${UNBOUND_REASON}` }; + return { refusal: refusal ?? { reason: UNBOUND_REASON, ofReference: true } }; + } + + /** Every check the fetch makes before it dials, in its order: the server + * (none, unknown, no address, off the uri's origin), the expiry, the + * server's spent budget, a claimed size over the ceiling. The fetch, + * `refusal()` and the eager check all ask this, so what save_recent_image + * says is the reason the fetch would refuse with, and no eager slot goes + * to a fetch that would be refused. */ + private beforeDial(record: ReferenceRecord, opts?: { maxBytes?: number }): + { refusal: Refusal } | { serverId: string; binding: ResolvedServerBinding; ceiling: number; budget: number } { + const server = this.serverFor(record); + if ('refusal' in server) return server; + const { serverId, binding } = server; + const t = record.testimony; + const expiry = referenceExpiry(t); + if (expiry) return { refusal: { reason: EXPIRY_REASON[expiry], ofReference: true } }; + + const ceiling = opts?.maxBytes ?? DEFAULT_FETCH_MAX_BYTES; + // The budget is checked before the ceiling, here and while reading: a + // larger max_bytes lifts the ceiling, and nothing the caller does lifts + // the budget, so a fetch past both is told about the budget. + const budget = binding.autofetch?.maxTotalBytes ?? DEFAULT_MAX_TOTAL_BYTES; + const used = this.totals.get(serverId) ?? 0; + if (used >= budget) { + return { refusal: { reason: `per-server fetch budget exhausted (${used}/${budget} bytes); ${BUDGET_LASTS}`, ofReference: false } }; + } + // Vector 4: a claim already over the ceiling refuses without dialing. One + // that's also over what's left of the budget is told about the budget, + // which no max_bytes lifts. A claim over what's left but within the + // ceiling still dials, and the budget stops it at its first chunk past. + if (t.sizeBytes !== undefined && t.sizeBytes > ceiling) { + if (t.sizeBytes > budget - used) { + return { + refusal: { + reason: `claimed ${t.sizeBytes} bytes exceeds what's left of the per-server fetch budget (${used}/${budget} bytes); ${BUDGET_LASTS}`, + ofReference: false, + }, + }; + } + return { refusal: { reason: `claimed ${t.sizeBytes} bytes exceeds fetch ceiling ${ceiling}`, ofReference: false } }; + } + return { serverId, binding, ceiling, budget }; + } + + /** Why a fetch of this record would be refused before dialing, with the + * default ceiling, which a fetch_reference call naming no max_bytes has. + * Null where the fetch would serve a saved copy, join a fetch of it + * already under way, or dial (where it can still fail), the same three + * steps fetch() takes. */ + refusal(record: ReferenceRecord): Refusal | null { + if (record.fetchedPath || this.inFlight.has(record.refId)) return null; + const check = this.beforeDial(record); + return 'refusal' in check ? check.refusal : null; } /** §6.1/§7.1 policy check; returns a refusal reason or null. */ @@ -188,34 +270,9 @@ export class ReferenceFetcher { private async doFetch(record: ReferenceRecord, opts?: { maxBytes?: number; timeoutMs?: number }): Promise { const t = record.testimony; - const server = this.serverFor(record); - if ('refusal' in server) return { ok: false, error: server.refusal }; - const { serverId, binding } = server; - const expiry = referenceExpiry(t); - if (expiry) return { ok: false, error: `reference ${EXPIRY_REASON[expiry]}` }; - - const ceiling = opts?.maxBytes ?? DEFAULT_FETCH_MAX_BYTES; - // The budget is checked before the ceiling, here and while reading: a - // larger max_bytes lifts the ceiling, and nothing the caller does lifts - // the budget, so a fetch past both is told about the budget. - const budget = binding.autofetch?.maxTotalBytes ?? DEFAULT_MAX_TOTAL_BYTES; - const used = this.totals.get(serverId) ?? 0; - if (used >= budget) { - return { ok: false, error: `per-server fetch budget exhausted (${used}/${budget} bytes); ${BUDGET_LASTS}` }; - } - // Vector 4: a claim already over the ceiling refuses without dialing. One - // that's also over what's left of the budget is told about the budget, - // which no max_bytes lifts. A claim over what's left but within the - // ceiling still dials, and the budget stops it at its first chunk past. - if (t.sizeBytes !== undefined && t.sizeBytes > ceiling) { - if (t.sizeBytes > budget - used) { - return { - ok: false, - error: `claimed ${t.sizeBytes} bytes exceeds what's left of the per-server fetch budget (${used}/${budget} bytes); ${BUDGET_LASTS}`, - }; - } - return { ok: false, error: `claimed ${t.sizeBytes} bytes exceeds fetch ceiling ${ceiling}` }; - } + const check = this.beforeDial(record, opts); + if ('refusal' in check) return { ok: false, error: refusalText(check.refusal) }; + const { serverId, binding, ceiling, budget } = check; const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), opts?.timeoutMs ?? FETCH_TIMEOUT_MS); diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index a6fcb300f..862009b0d 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -189,6 +189,15 @@ export const EXPIRY_REASON = { * server that delivered the reference. */ export const UNBOUND_REASON = 'unbound: no server is recorded to fetch it from'; +/** The fetcher's refusal for a server it has no address to dial, one the + * host doesn't connect to over a network, such as a stdio server; the + * fetcher dials only through a server's address. save_recent_image gives it + * too. A stub can't know it, since only the framework holds its servers' + * configs. */ +export function noAddressReason(serverId: string): string { + return `no address: this host doesn't connect to server ${serverId} over a network, so there's nowhere to fetch it from`; +} + /** Strip control characters (C0, DEL, C1) and every bidi control (marks, * embeddings, overrides, isolates, and the Arabic letter mark), trim the * whitespace (a BOM included) left at either end, and bound the length. @@ -371,13 +380,15 @@ export function buildReferenceStub(t: ReferenceTestimony, provenance?: string, s } /** - * Why the fetcher would refuse a reference, as far as its record shows: no - * server registered for it, which the fetcher checks first, or its expiry. - * Null for a saved copy, which the fetcher serves before any check, and when - * the record shows neither. The fetcher's other refusals rest on what a record - * doesn't hold: whether any of its servers is still known to the host and on - * the uri's origin, a claimed size over the ceiling a fetch is given, or the - * dialed server's fetch budget spent, and it gives those in its own words. + * Why the fetcher would refuse a reference, as far as its record alone shows, + * for its stub: no server registered for it, which the fetcher checks first, + * or its expiry. Null for a saved copy, which the fetcher serves before any + * check, and when the record shows neither. The fetcher's other refusals rest + * on what a record doesn't hold: whether any of its servers is known to the + * host, has an address and is on the uri's origin, the dialed server's fetch + * budget spent, or a claimed size over the ceiling a fetch is given. The + * fetcher gives those in its own words, and save_recent_image, in the + * framework where it can ask the fetcher, says them too. */ export function unfetchableReason(record: ReferenceRecord, nowMs = Date.now()): string | null { if (record.fetchedPath) return null; diff --git a/test/mcpl-reference-binding.test.ts b/test/mcpl-reference-binding.test.ts index 960c42803..370ad2ee3 100644 --- a/test/mcpl-reference-binding.test.ts +++ b/test/mcpl-reference-binding.test.ts @@ -13,7 +13,7 @@ import assert from 'node:assert/strict'; import { createServer } from 'node:http'; import type { AddressInfo } from 'node:net'; import { fixture, TS } from './helpers/coalescing-fixture.js'; -import { referenceRegistry, type ReferenceRecord } from '../src/mcpl/references.js'; +import { noAddressReason, referenceRegistry, type ReferenceRecord } from '../src/mcpl/references.js'; import { HookOrchestrator } from '../src/mcpl/hook-orchestrator.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; import { PushHandler } from '../src/mcpl/push-handler.js'; @@ -105,13 +105,15 @@ test("a context injection's reference is registered with the server whose hook s assert.deepEqual(stubRecords(text).map((record) => record?.serverIds), [['hooks'], ['hooks'], ['hooks']]); }); -test("fetch_reference reaches a push event's reference through its server's own address, and refuses one elsewhere or from a server with no address", async (t) => { +test("fetch_reference reaches a push event's reference through its server's own address, and refuses one elsewhere, one from a server with no address, one from a server spawned from a command whatever url its config holds, and one from a server the host doesn't know, reading a server's transport as the host connects to it", async (t) => { const png = Buffer.from('not really a png, but bytes'); let dialed = 0; const http = createServer((_req, res) => { dialed++; res.writeHead(200, { 'content-type': 'application/octet-stream' }); res.end(png); }); await new Promise((resolve) => http.listen(0, '127.0.0.1', resolve)); t.after(() => new Promise((resolve) => http.close(() => resolve()))); const origin = `http://127.0.0.1:${(http.address() as AddressInfo).port}`; + // What a WebSocket server on that origin is dialed at: the origin check reads ws as http. + const wsOrigin = origin.replace(/^http:/, 'ws:'); // The push path as the framework builds it, here with no coalescer. const pushed: unknown[] = []; const handler = new PushHandler({ validateInbound() {} } as unknown as FeatureSetManager, (event) => { pushed.push(event); }, () => {}); @@ -121,17 +123,39 @@ test("fetch_reference reaches a push event's reference through its server's own } as never, { respond: () => {}, respondError: () => {} } as never); await push('push-srv', 'fetch-own', [`${origin}/binding/own.png`, 'https://cdn.example.invalid/binding/elsewhere.png']); await push('stdio-srv', 'fetch-stdio', [`${origin}/binding/stdio.png`]); - const [own, elsewhere, stdio] = stubRecords(JSON.stringify(pushed)); - assert.deepEqual([own?.serverIds, elsewhere?.serverIds, stdio?.serverIds], [['push-srv'], ['push-srv'], ['stdio-srv']]); + await push('gone-srv', 'fetch-gone', [`${origin}/binding/gone.png`]); + await push('spawned-srv', 'fetch-spawned', [`${origin}/binding/spawned.png`]); + await push('ws-command-srv', 'fetch-ws-command', [`${origin}/binding/ws-command.png`]); + await push('stdio-url-srv', 'fetch-stdio-url', [`${origin}/binding/stdio-url.png`]); + await push('ws-http-srv', 'fetch-ws-http', [`${origin}/binding/ws-http.png`]); + const [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp] = stubRecords(JSON.stringify(pushed)); + assert.deepEqual( + [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp].map((record) => record?.serverIds), + [['push-srv'], ['push-srv'], ['stdio-srv'], ['gone-srv'], ['spawned-srv'], ['ws-command-srv'], ['stdio-url-srv'], ['ws-http-srv']], + ); // The framework's own fetcher, resolving each server from its MCPL config: - // push-srv has an address, and stdio-srv, as a stdio server, has none. + // push-srv has an address; stdio-srv, as a stdio server, has none, nor + // has spawned-srv, which the host spawns from its command though its + // config also holds a url on the uri's origin; and gone-srv isn't + // configured at all. An explicit transport decides as the host connects: + // ws-command-srv is a WebSocket server though its config holds a command, + // and stdio-url-srv is spawned though its config holds only a url. + // ws-http-srv is a WebSocket server whose url the transport won't dial, + // since it isn't ws:// or wss://. type Answer = { result: { success: boolean; data?: string; error?: string } }; const framework = Object.create(AgentFramework.prototype) as { pushEvent(event: unknown): void; dispatchFetchReferenceToolCall(agentName: string, call: { id: string; name: string; input: unknown }): void; }; Object.assign(framework, { - mcplServerConfigs: new Map([['push-srv', { id: 'push-srv', url: origin }], ['stdio-srv', { id: 'stdio-srv', command: 'stdio-server' }]]), + mcplServerConfigs: new Map([ + ['push-srv', { id: 'push-srv', url: wsOrigin }], + ['stdio-srv', { id: 'stdio-srv', command: 'stdio-server' }], + ['spawned-srv', { id: 'spawned-srv', command: 'stdio-server', url: wsOrigin }], + ['ws-command-srv', { id: 'ws-command-srv', transport: 'websocket', command: 'stdio-server', url: wsOrigin }], + ['stdio-url-srv', { id: 'stdio-url-srv', transport: 'stdio', url: wsOrigin }], + ['ws-http-srv', { id: 'ws-http-srv', transport: 'websocket', url: origin }], + ]), getWorkspaceModule: () => ({ writeBinary: async () => ({ success: true }) }), firstWritableMountName: () => 'scratch', emitTrace: () => {}, @@ -150,10 +174,26 @@ test("fetch_reference reaches a push event's reference through its server's own const refused = await fetchReference(elsewhere!.refId); assert.equal(refused.success, false); assert.match(refused.error ?? '', /third-party origin/); - const unknown = await fetchReference(stdio!.refId); + const noAddress = await fetchReference(stdio!.refId); + assert.equal(noAddress.success, false); + assert.equal(noAddress.error, noAddressReason('stdio-srv')); + const notDialed = await fetchReference(spawned!.refId); + assert.equal(notDialed.success, false); + assert.equal(notDialed.error, noAddressReason('spawned-srv')); + const unknown = await fetchReference(gone!.refId); assert.equal(unknown.success, false); - assert.equal(unknown.error, 'unknown server: stdio-srv'); - assert.equal(dialed, 1, 'nothing dialed for the other two'); + assert.equal(unknown.error, 'unknown server: gone-srv'); + assert.equal(dialed, 1, 'nothing dialed for the other four'); + const throughWebSocket = await fetchReference(wsCommand!.refId); + assert.equal(throughWebSocket.success, true, throughWebSocket.error); + assert.equal(dialed, 2); + const spawnedByTransport = await fetchReference(stdioUrl!.refId); + assert.equal(spawnedByTransport.success, false); + assert.equal(spawnedByTransport.error, noAddressReason('stdio-url-srv')); + const undialable = await fetchReference(wsHttp!.refId); + assert.equal(undialable.success, false); + assert.equal(undialable.error, noAddressReason('ws-http-srv')); + assert.equal(dialed, 2, 'nothing dialed for the last two'); }); /** The admin puppet's route to an MCPL tool, executeMcplToolCall, with a diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index de8f2ee55..5a2c22775 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -8,8 +8,8 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { createServer } from 'node:http'; import { createHash } from 'node:crypto'; -import { ReferenceFetcher, normalizedOrigin } from '../src/mcpl/reference-fetcher.js'; -import { ReferenceRegistry, UNBOUND_REASON, classifyBlock, referenceExpiry, referenceRegistry } from '../src/mcpl/references.js'; +import { ReferenceFetcher, normalizedOrigin, type ServerResolution } from '../src/mcpl/reference-fetcher.js'; +import { EXPIRY_REASON, noAddressReason, ReferenceRegistry, UNBOUND_REASON, classifyBlock, referenceExpiry, referenceRegistry } from '../src/mcpl/references.js'; import { AgentFramework } from '../src/framework.js'; const PAYLOAD = Buffer.from('RIFF-not-really-a-wav-'.repeat(100)); @@ -63,7 +63,7 @@ const port = (server.address() as { port: number }).port; const ORIGIN = `http://127.0.0.1:${port}`; const saved: Array<{ name: string; bytes: number; mime: string }> = []; -function makeFetcher(bindings: Record) { +function makeFetcher(bindings: Record) { return new ReferenceFetcher( (id) => bindings[id], async (name, data, mime) => { saved.push({ name, bytes: data.length, mime }); return `scratch/refs/${name}`; }, @@ -678,18 +678,129 @@ test('of two servers on the uri\'s origin, the first to deliver it is dialed, wi } }); -test('a server the host holds no address for is its refusal while it is the only one, and passed over for one on the origin', async () => { +test('a server unknown to the host is its refusal while it is the only one, and passed over for one on the origin', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ owner: { url: `ws://127.0.0.1:${port}` } }); const uri = `${ORIGIN}/ok?unknown-first`; - const record = makeRecord(reg, uri, {}, 'stdio-srv'); - assert.equal((await f.fetch(record)).error, 'unknown server: stdio-srv'); + const record = makeRecord(reg, uri, {}, 'gone'); + assert.equal((await f.fetch(record)).error, 'unknown server: gone'); makeRecord(reg, uri, {}, 'owner'); - assert.deepEqual(record.serverIds, ['stdio-srv', 'owner']); + assert.deepEqual(record.serverIds, ['gone', 'owner']); const out = await f.fetch(record); assert.equal(out.ok, true, out.error); }); +test('a server with no address is refused in a phrase naming it, and passed over for one on the origin', async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ 'stdio-srv': 'no-address', owner: { url: `ws://127.0.0.1:${port}` } }); + const uri = `${ORIGIN}/ok?no-address`; + // Small enough for an eager slot, so it takes none only for the server it came from. + const record = makeRecord(reg, uri, { sizeBytes: PAYLOAD.length }, 'stdio-srv'); + assert.deepEqual(f.refusal(record), { reason: noAddressReason('stdio-srv'), ofReference: false }); + assert.equal((await f.fetch(record)).error, "no address: this host doesn't connect to server stdio-srv over a network, so there's nowhere to fetch it from"); + assert.equal(f.isEagerEligible(record), false); + assert.equal(hits.get('/ok?no-address') ?? 0, 0, 'nothing dialed'); + makeRecord(reg, uri, { sizeBytes: PAYLOAD.length }, 'owner'); + assert.equal(f.refusal(record), null); + assert.equal(f.isEagerEligible(record), true); + const out = await f.fetch(record); + assert.equal(out.ok, true, out.error); +}); + +test('a server with no address ahead of one off the origin is the refusal, and the other way round the origin check\'s is', async () => { + for (const [order, refusal] of [ + [['stdio-srv', 'a'], /^no address: this host doesn't connect to server stdio-srv over a network/], + [['a', 'stdio-srv'], /^third-party origin \(http:\/\/127\.0\.0\.1:\d+ != dialed https:\/\/a\.example\.invalid\)/], + ] as const) { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ 'stdio-srv': 'no-address', a: { url: 'https://a.example.invalid' } }); + const uri = `${ORIGIN}/ok?no-address-and-off-origin=${order[0]}`; + const record = makeRecord(reg, uri, {}, order[0]); + makeRecord(reg, uri, {}, order[1]); + assert.match(f.refusal(record)?.reason ?? '', refusal); + assert.match((await f.fetch(record)).error ?? '', refusal); + assert.equal(hits.get(`/ok?no-address-and-off-origin=${order[0]}`) ?? 0, 0, 'nothing dialed'); + } +}); + +test('refusal() answers what the fetch would refuse before dialing, case by case, the eager check gives none of them a slot, and null where it would serve or dial', async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ + srv: { url: `ws://127.0.0.1:${port}` }, + 'stdio-srv': 'no-address', + spent: { url: `ws://127.0.0.1:${port}`, autofetch: { maxTotalBytes: 0 } }, + // An eager limit past the fetch's own ceiling and its budget: those still decide. + roomy: { url: `ws://127.0.0.1:${port}`, autofetch: { maxBytes: 4 * 1024 * 1024 * 1024 } }, + }); + const small = { sizeBytes: PAYLOAD.length }; + const cases: Array<[string, ReturnType, string | null]> = [ + // makeRecord's server defaults when given undefined, so this one registers itself. + ['unbound', reg.register((classifyBlock({ type: 'resource', uri: `${ORIGIN}/ok?case=unbound`, ...small }) as Extract, { kind: 'reference' }>).testimony), `reference ${UNBOUND_REASON}`], + ['unknown server', makeRecord(reg, `${ORIGIN}/ok?case=unknown`, small, 'gone'), 'unknown server: gone'], + ['no address', makeRecord(reg, `${ORIGIN}/ok?case=no-address`, small, 'stdio-srv'), noAddressReason('stdio-srv')], + ['off the origin', makeRecord(reg, 'https://cdn.example.invalid/case-origin', small, 'srv'), null], + ['expired', makeRecord(reg, `${ORIGIN}/ok?case=expired`, { ...small, expiresAt: '2001-01-01T00:00:00Z' }, 'srv'), `reference ${EXPIRY_REASON.passed}`], + ['budget spent', makeRecord(reg, `${ORIGIN}/ok?case=budget`, small, 'spent'), `per-server fetch budget exhausted (0/0 bytes); ${BUDGET_LASTS}`], + ['over the ceiling', makeRecord(reg, `${ORIGIN}/ok?case=ceiling`, { sizeBytes: 512 * 1024 * 1024 }, 'roomy'), 'claimed 536870912 bytes exceeds fetch ceiling 268435456'], + ['over the ceiling and what is left of the budget', makeRecord(reg, `${ORIGIN}/ok?case=both`, { sizeBytes: 2 * 1024 * 1024 * 1024 }, 'roomy'), + `claimed 2147483648 bytes exceeds what's left of the per-server fetch budget (0/1073741824 bytes); ${BUDGET_LASTS}`], + ]; + for (const [what, record, error] of cases) { + const refusal = f.refusal(record); + assert.ok(refusal, `${what}: a refusal`); + assert.equal(f.isEagerEligible(record), false, `${what}: no eager slot`); + const said = refusal.ofReference ? `reference ${refusal.reason}` : refusal.reason; + const out = await f.fetch(record); + assert.equal(out.ok, false, what); + assert.equal(out.error, said, `${what}: the fetch answers what refusal() said`); + if (error !== null) assert.equal(said, error, what); + else assert.match(said, /^third-party origin \(https:\/\/cdn\.example\.invalid != dialed http:\/\/127\.0\.0\.1:\d+\); not fetched by this host$/, what); + } + assert.equal([...hits.keys()].filter((k) => k.startsWith('/ok?case=')).length, 0, 'nothing dialed'); + // Null where the fetch serves a saved copy or dials, and the one it would dial takes an eager slot. + const savedCopy = makeRecord(reg, `${ORIGIN}/ok?case=saved`, { expiresAt: '2001-01-01T00:00:00Z' }, 'stdio-srv'); + savedCopy.fetchedPath = 'scratch/refs/saved.bin'; + assert.equal(f.refusal(savedCopy), null); + const dial = makeRecord(reg, `${ORIGIN}/ok?case=dial`, small, 'srv'); + assert.equal(f.refusal(dial), null); + assert.equal(f.isEagerEligible(dial), true); + // A saved copy of one the fetch would otherwise dial takes no eager slot either: the fetch serves it. + const keptCopy = makeRecord(reg, `${ORIGIN}/ok?case=kept`, small, 'srv'); + keptCopy.fetchedPath = 'scratch/refs/kept.bin'; + assert.equal(f.refusal(keptCopy), null); + assert.equal(f.isEagerEligible(keptCopy), false); +}); + +test('a reference with two faults is refused for the first the fetch checks: the server, then the expiry, the spent budget, the claimed size', async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ 'stdio-srv': 'no-address', spent: { url: `ws://127.0.0.1:${port}`, autofetch: { maxTotalBytes: 0 } } }); + const past = '2001-01-01T00:00:00Z'; + const huge = 512 * 1024 * 1024; + const cases: Array<[string, ReturnType, string]> = [ + ['no address, expired', makeRecord(reg, `${ORIGIN}/ok?two=1`, { expiresAt: past }, 'stdio-srv'), noAddressReason('stdio-srv')], + ['expired, budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=2`, { expiresAt: past, sizeBytes: huge }, 'spent'), `reference ${EXPIRY_REASON.passed}`], + ['budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=3`, { sizeBytes: huge }, 'spent'), `per-server fetch budget exhausted (0/0 bytes); ${BUDGET_LASTS}`], + ]; + for (const [what, record, error] of cases) { + const refusal = f.refusal(record); + assert.ok(refusal, what); + assert.equal(refusal.ofReference ? `reference ${refusal.reason}` : refusal.reason, error, what); + assert.equal((await f.fetch(record)).error, error, what); + } +}); + +test('refusal() answers nothing while a fetch of the record is under way, which the fetch would join', async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ srv: { url: ORIGIN } }); + const record = makeRecord(reg, `${ORIGIN}/ok?in-flight`, { sizeBytes: PAYLOAD.length }, 'srv'); + const fetching = f.fetch(record); + // The fetch has made its checks; the record's claims change while it is under way. + record.testimony = { ...record.testimony, expiresAt: '2001-01-01T00:00:00Z' }; + assert.equal(f.refusal(record), null, 'joined, not refused'); + const out = await fetching; + assert.equal(out.ok, true, out.error); +}); + test('a fetch is charged to the budget of the server it dials, not of the first to deliver the uri', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ diff --git a/test/save-recent-image.test.ts b/test/save-recent-image.test.ts index b0fc6ac05..4013197cb 100644 --- a/test/save-recent-image.test.ts +++ b/test/save-recent-image.test.ts @@ -62,11 +62,20 @@ import { UNBOUND_REASON, buildReferenceStub, classifyBlock, + noAddressReason, referenceRegistry, type ReferenceTestimony, } from '../src/mcpl/references.js'; import { createMockResponse, MockMembrane } from './helpers/mock-membrane.js'; +/** Give the framework MCPL server configs for its fetcher to resolve, without + * connecting to them: a ws:// or wss:// url for one the host connects to over + * a network, a command alone for a stdio server. */ +function configureServers(framework: AgentFramework, configs: Array<{ id: string; url?: string; command?: string; autofetch?: { maxTotalBytes?: number } }>): void { + const known = (framework as unknown as { mcplServerConfigs: Map }).mcplServerConfigs; + for (const config of configs) known.set(config.id, config); +} + const PNG = Buffer.from( 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4////fwAJ+wP9KobjigAAAABJRU5ErkJggg==', 'base64', @@ -558,6 +567,7 @@ describe('save_recent_image provenance (issue #104)', () => { done, ], seedHistory: (framework) => { + configureServers(framework, [{ id: 'vst', url: 'wss://example.invalid' }]); framework.getAgent('prime')!.getContextManager().addMessage('user', [{ type: 'tool_result', toolUseId: 'call_cam', @@ -602,6 +612,7 @@ describe('save_recent_image provenance (issue #104)', () => { done, ], seedHistory: (framework) => { + configureServers(framework, [{ id: 'vst', url: 'wss://example.invalid' }]); framework.getAgent('prime')!.getContextManager().addMessage('user', [{ type: 'tool_result', toolUseId: 'call_render', @@ -630,6 +641,68 @@ describe('save_recent_image provenance (issue #104)', () => { } }); + it("a bound reference the fetcher would refuse before dialing refuses with the fetcher's own reason", async () => { + // Each bound to a server: one with no address (a stdio server), one whose + // address is on another origin, one the host doesn't know, one claimed + // over a fetch's ceiling, and one from a server whose budget is spent. + const register = (uri: string, serverId: string, sizeBytes?: number) => { + const c = classifyBlock({ type: 'image', uri, mimeType: 'image/png', ...(sizeBytes === undefined ? {} : { sizeBytes }) }); + assert.strictEqual(c.kind, 'reference'); + return referenceRegistry.register((c as { testimony: ReferenceTestimony }).testimony, serverId); + }; + const spent = register('https://example.invalid/sri-spent.png', 'spent'); + const big = register('https://example.invalid/sri-big.png', 'vst', 512 * 1024 * 1024); + const gone = register('https://example.invalid/sri-gone.png', 'gone'); + const offOrigin = register('https://cdn.example.invalid/sri-off-origin.png', 'vst'); + const noAddress = register('https://example.invalid/sri-no-address.png', 'stdio'); + const h = await startTurn({ + prefix: 'sri-reference-refused-', + responses: [ + [saveCall('call_save0', { path: 'files/no-address.png', index: 0 })], + [saveCall('call_save1', { path: 'files/off-origin.png', index: 1 })], + [saveCall('call_save2', { path: 'files/gone.png', index: 2 })], + [saveCall('call_save3', { path: 'files/big.png', index: 3 })], + [saveCall('call_save4', { path: 'files/spent.png', index: 4 })], + done, + ], + seedHistory: (framework) => { + configureServers(framework, [ + { id: 'vst', url: 'wss://example.invalid' }, + { id: 'stdio', command: 'stdio-server' }, + { id: 'spent', url: 'wss://example.invalid', autofetch: { maxTotalBytes: 0 } }, + ]); + framework.getAgent('prime')!.getContextManager().addMessage('user', [{ + type: 'tool_result', + toolUseId: 'call_refs', + toolName: 'mcpl--vst--render', + // Index 0 is the newest slot, the last line. + content: [spent, big, gone, offOrigin, noAddress].map((r) => `[${r.refId}] image/png — from tool result — fetch with fetch_reference`).join('\n'), + isError: false, + }] as ContentBlock[]); + }, + }); + try { + const budgetLasts = "the budget doesn't renew on its own, so calling again won't change that unless whoever runs this host raises or renews it"; + const expected = [ + ['call_save0', noAddress.refId, `It can't be fetched either — ${noAddressReason('stdio')}.`], + ['call_save1', offOrigin.refId, "It can't be fetched either — third-party origin (https://cdn.example.invalid != dialed https://example.invalid); not fetched by this host."], + ['call_save2', gone.refId, "It can't be fetched either — unknown server: gone."], + ['call_save3', big.refId, "It can't be fetched either — claimed 536870912 bytes exceeds fetch ceiling 268435456."], + ['call_save4', spent.refId, `It can't be fetched either — per-server fetch budget exhausted (0/0 bytes); ${budgetLasts}.`], + ] as const; + for (const [call, refId, reason] of expected) { + const refused = await waitForToolResult(h.framework, call); + assert.strictEqual(refused.isError, true); + assert.ok(refused.content.includes(`a reference (${refId}, image/png, from`), refused.content); + assert.ok(refused.content.includes(reason), refused.content); + assert.ok(!refused.content.includes('fetch_reference'), refused.content); + } + } finally { + await h.framework.stop(); + rmSync(h.tempDir, { recursive: true, force: true }); + } + }); + it("an attachment's media type is shown and stored as a label, and one with nothing left counts as absent", async () => { // The store keeps a declared type only for bytes it can't sniff as a // raster image, so these aren't images it knows. Oldest first: a type From f100e0a6a4f138fd230ee2d581300760e0c5163b Mon Sep 17 00:00:00 2001 From: Pippa-2571 Date: Sun, 18 Oct 2026 09:20:36 -0400 Subject: [PATCH 2/3] fix(mcpl): the eager pass joins a fetch under way whatever the budget The eager check made every check the fetch makes before dialing, so a reference whose fetch was already under way was refused once that fetch had spent its server's budget, though the eager fetch would only join it, as fetch() does. A second tool result carrying the reference while that fetch's save was still pending could then get a stub offering fetch_reference instead of the saved path. For a reference whose fetch is under way, the eager check now asks only for the server and its eager limit: the expiry, the budget and the ceiling were checked when that fetch began, and refusal() doesn't ask them there either. The pre-dial check's doc says which records it serves, and the changelog fragment says the eager fetch still joins a fetch already under way. Test: with a server's budget of exactly one payload and the save held open, the reference whose fetch is under way stays eager-eligible, even once its record claims an expiry, while another reference from that server is refused for the spent budget. The framework's eager pass for a later tool result carrying the reference then joins the fetch under way: it gets that fetch's promise, the reference is requested once, no eager failure is logged, and the saved copy, once it lands, takes no eager slot. The wait for the save fails at once if the fetch ends first. Co-Authored-By: Claude Opus 5.5 --- .../no-address-server-refusal.fixed.md | 3 +- src/mcpl/reference-fetcher.ts | 18 ++++--- test/mcpl-reference-fetcher.test.ts | 52 +++++++++++++++++-- 3 files changed, 59 insertions(+), 14 deletions(-) diff --git a/changelog.d/no-address-server-refusal.fixed.md b/changelog.d/no-address-server-refusal.fixed.md index d3dba5b2a..3b528e6da 100644 --- a/changelog.d/no-address-server-refusal.fixed.md +++ b/changelog.d/no-address-server-refusal.fixed.md @@ -11,4 +11,5 @@ ahead. The eager fetch of the references in the result of an MCPL tool call the agent makes no longer spends one of its slots on a reference the fetch would refuse before dialing, such as one from a server whose fetch - budget is spent. + budget is spent, though it still joins a fetch of a reference already + under way. diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index 38e89335a..3d9a9261e 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -149,12 +149,13 @@ export class ReferenceFetcher { /** Would the eager (autofetch) default fetch this record? Not one the fetch * would refuse before dialing, asked as the eager fetch asks it, with the * default ceiling, so such a record takes no eager slot; and only one whose - * claimed size is within the eager limit of the server it dials. Whether a - * fetch of it is already under way doesn't enter: the eager fetch joins - * one that is. */ + * claimed size is within the eager limit of the server it dials. For one + * whose fetch is already under way, which the eager fetch would join as + * fetch() does, only the server and its eager limit enter: the expiry, + * the budget and the ceiling were checked when that fetch began. */ isEagerEligible(record: ReferenceRecord): boolean { if (record.fetchedPath) return false; - const check = this.beforeDial(record); + const check = this.inFlight.has(record.refId) ? this.serverFor(record) : this.beforeDial(record); if ('refusal' in check) return false; const cap = check.binding.autofetch?.maxBytes ?? DEFAULT_EAGER_FETCH_MAX_BYTES; return record.testimony.sizeBytes !== undefined && record.testimony.sizeBytes <= cap; @@ -186,10 +187,11 @@ export class ReferenceFetcher { /** Every check the fetch makes before it dials, in its order: the server * (none, unknown, no address, off the uri's origin), the expiry, the - * server's spent budget, a claimed size over the ceiling. The fetch, - * `refusal()` and the eager check all ask this, so what save_recent_image - * says is the reason the fetch would refuse with, and no eager slot goes - * to a fetch that would be refused. */ + * server's spent budget, a claimed size over the ceiling. For a record + * with no saved copy and no fetch under way, the fetch, `refusal()` and + * the eager check all ask this, so what save_recent_image says is the + * reason the fetch would refuse with, and no eager slot goes to a fetch + * that would be refused. */ private beforeDial(record: ReferenceRecord, opts?: { maxBytes?: number }): { refusal: Refusal } | { serverId: string; binding: ResolvedServerBinding; ceiling: number; budget: number } { const server = this.serverFor(record); diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index 5a2c22775..d69d3216a 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -276,11 +276,10 @@ test('a copy saved earlier is still served once its expiry has passed or reads a }); /** The framework's eager default, autoFetchReferences, run over `content` - * as a tool result from server `srv`, with this file's fetcher. It - * registers in the process-wide referenceRegistry, so each test's uris are - * its own. */ -async function eagerFetch(content: unknown[]): Promise { - const fetcher = makeFetcher({ srv: { url: ORIGIN } }); + * as a tool result from server `srv`, with this file's fetcher or the one + * given. It registers in the process-wide referenceRegistry, so each test's + * uris are its own. */ +async function eagerFetch(content: unknown[], fetcher: ReferenceFetcher = makeFetcher({ srv: { url: ORIGIN } })): Promise { const framework = Object.create(AgentFramework.prototype) as { getReferenceFetcher(): ReferenceFetcher; autoFetchReferences(content: unknown[], serverId: string, eager: boolean): Promise; @@ -801,6 +800,49 @@ test('refusal() answers nothing while a fetch of the record is under way, which assert.equal(out.ok, true, out.error); }); +test("a reference whose fetch is under way stays eager-eligible once that fetch has spent its server's budget, so the eager fetch joins it", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + let saveStarted!: () => void; + const saveCalled = new Promise((resolve) => { saveStarted = resolve; }); + let releaseSave!: () => void; + const saving = new Promise((resolve) => { releaseSave = resolve; }); + // A budget of exactly one payload, and a save held open until released. + const f = new ReferenceFetcher( + (id) => (id === 'srv' ? { url: ORIGIN, autofetch: { maxTotalBytes: PAYLOAD.length } } : undefined), + async (name) => { saveStarted(); await saving; return `scratch/refs/${name}`; }, + ); + // In the process-wide registry, where the eager pass registers a result's references. + const uri = `${ORIGIN}/ok?join-after-budget`; + const record = makeRecord(referenceRegistry, uri, { sizeBytes: PAYLOAD.length }, 'srv'); + assert.equal(f.isEagerEligible(record), true); + const fetching = f.fetch(record); + // Bounded by the fetch itself: one that ends before its save fails here, at once. + await Promise.race([saveCalled, fetching.then((out) => Promise.reject(new Error(`the fetch ended before its save: ${JSON.stringify(out)}`)))]); + // Its bytes are read, so the budget is spent, and its save hasn't landed. + const another = makeRecord(referenceRegistry, `${ORIGIN}/ok?join-after-budget-other`, { sizeBytes: PAYLOAD.length }, 'srv'); + assert.equal(f.isEagerEligible(another), false, 'a new dial is refused for the spent budget'); + assert.equal(f.isEagerEligible(record), true, 'the fetch under way is joined'); + // An expiry the record now claims doesn't enter either: that fetch passed the expiry check when it began. + const claimed = record.testimony; + record.testimony = { ...claimed, expiresAt: '2001-01-01T00:00:00Z' }; + assert.equal(f.isEagerEligible(record), true, 'still joined'); + assert.equal(f.refusal(record), null); + record.testimony = claimed; + // A later tool result carrying the reference: the eager pass asks fetch() for it, and gets the fetch under way. + const asked = t.mock.method(f, 'fetch'); + const eager = eagerFetch([{ type: 'resource', uri, sizeBytes: PAYLOAD.length }], f); + assert.equal(asked.mock.callCount(), 1, 'the eager pass asked for the reference'); + assert.equal(asked.mock.calls[0].arguments[0], record); + assert.equal(asked.mock.calls[0].result, fetching, 'and joined the fetch under way'); + releaseSave(); + const [out] = await Promise.all([fetching, eager]); + assert.equal(out.ok, true, out.error); + assert.equal(record.fetchedPath, out.path); + assert.equal(hits.get('/ok?join-after-budget'), 1, 'dialed once'); + assert.equal(errors.mock.callCount(), 0, 'no eager failure logged'); + assert.equal(f.isEagerEligible(record), false, 'a saved copy takes no slot'); +}); + test('a fetch is charged to the budget of the server it dials, not of the first to deliver the uri', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ From d206b05a61efe178768a23e35874b1844ed4de2b Mon Sep 17 00:00:00 2001 From: Pippa-2571 Date: Sun, 18 Oct 2026 09:20:36 -0400 Subject: [PATCH 3/3] fix(mcpl): the transport's url check refuses a fragment, as ws does The resolver gives a WebSocket server an address only where buildWebSocketUrl, the transport's check of a url, accepts it. The ws library the transport dials with also refuses a url with a fragment, in its constructor, before any connection, so the resolver gave an address, and the server's token, to fetches on the origin of a url the host never dials. buildWebSocketUrl now refuses a url with a fragment too, naming it, so the resolver gives such a server no address, and the transport fails its connect with that error where it failed with ws's "The URL contains a fragment identifier". An empty fragment, which ws dials, is still accepted. The resolver's comment and the changelog fragment say the resolver gives no address for a url that isn't a ws:// or wss:// url, or has a fragment. Tests: buildWebSocketUrl refuses a url with a fragment and accepts an empty one, and ws's own constructor refuses the same url; fetch_reference through the framework's own fetcher refuses a reference from a WebSocket server whose url, on the reference's origin, has a fragment, as no address, with nothing dialed. Co-Authored-By: Claude Opus 5.5 --- changelog.d/no-address-server-refusal.fixed.md | 7 +++++-- src/framework.ts | 3 ++- src/mcpl/transport.ts | 8 ++++++-- test/mcpl-reference-binding.test.ts | 18 ++++++++++++------ test/mcpl-ws-transport.test.ts | 9 ++++++++- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/changelog.d/no-address-server-refusal.fixed.md b/changelog.d/no-address-server-refusal.fixed.md index 3b528e6da..d78b3e834 100644 --- a/changelog.d/no-address-server-refusal.fixed.md +++ b/changelog.d/no-address-server-refusal.fixed.md @@ -4,8 +4,11 @@ connect to server over a network, so there's nowhere to fetch it from`. A server the host spawns from a command has no address either, even when its config also holds a `url`, on whose origin its references - used to be fetched; nor has a WebSocket server whose `url` isn't `ws://` - or `wss://`. `save_recent_image` now gives the reason a + used to be fetched; nor has a WebSocket server whose `url` the transport + won't dial: one that isn't a `ws://` or `wss://` url, or that has a + fragment. A WebSocket server whose `url` has a fragment, which the `ws` + library refuses to dial, now fails to connect with the transport's own + error naming the fragment. `save_recent_image` now gives the reason a `fetch_reference` call naming no `max_bytes` would be refused with before dialing, and points at `fetch_reference` only where that fetch would go ahead. The eager fetch of the references in the result of an MCPL tool diff --git a/src/framework.ts b/src/framework.ts index 99b3fe551..48cbb7c43 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -4694,7 +4694,8 @@ export class AgentFramework { // A server's address is the url the host dials it at, as the // transport decides: none for a server the host spawns, as a stdio // server, nor for a WebSocket server whose url the transport won't - // dial, which buildWebSocketUrl refuses (not ws:// or wss://). + // dial, which buildWebSocketUrl refuses: one that isn't a ws:// or + // wss:// url, or has a fragment. if (!isWebSocketTransport(cfg) || !cfg.url) return 'no-address'; try { buildWebSocketUrl(cfg, null); diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 5b6fbc50c..680a2068e 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -514,8 +514,9 @@ export class WebSocketTransport extends McplTransport { } /** - * Build the dial URL: validate it's a ws(s):// URL and append the auth token as - * a `token` query param when provided (preserving any existing query). + * Build the dial URL: validate it's a ws(s):// URL the ws library will dial, + * which refuses one with a fragment, and append the auth token as a `token` + * query param when provided (preserving any existing query). */ export function buildWebSocketUrl(config: McplServerConfig, tokenOverride?: string | null): string { if (!config.url) { @@ -532,6 +533,9 @@ export function buildWebSocketUrl(config: McplServerConfig, tokenOverride?: stri `MCPL server "${config.id}": websocket url must be ws:// or wss://, got "${parsed.protocol}"`, ); } + if (parsed.hash) { + throw new Error(`MCPL server "${config.id}": websocket url must not have a fragment, got "${parsed.hash}"`); + } const token = tokenOverride !== undefined ? tokenOverride : config.token; if (token) { parsed.searchParams.set('token', token); diff --git a/test/mcpl-reference-binding.test.ts b/test/mcpl-reference-binding.test.ts index 370ad2ee3..c12dc95a6 100644 --- a/test/mcpl-reference-binding.test.ts +++ b/test/mcpl-reference-binding.test.ts @@ -128,10 +128,11 @@ test("fetch_reference reaches a push event's reference through its server's own await push('ws-command-srv', 'fetch-ws-command', [`${origin}/binding/ws-command.png`]); await push('stdio-url-srv', 'fetch-stdio-url', [`${origin}/binding/stdio-url.png`]); await push('ws-http-srv', 'fetch-ws-http', [`${origin}/binding/ws-http.png`]); - const [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp] = stubRecords(JSON.stringify(pushed)); + await push('ws-fragment-srv', 'fetch-ws-fragment', [`${origin}/binding/ws-fragment.png`]); + const [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp, wsFragment] = stubRecords(JSON.stringify(pushed)); assert.deepEqual( - [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp].map((record) => record?.serverIds), - [['push-srv'], ['push-srv'], ['stdio-srv'], ['gone-srv'], ['spawned-srv'], ['ws-command-srv'], ['stdio-url-srv'], ['ws-http-srv']], + [own, elsewhere, stdio, gone, spawned, wsCommand, stdioUrl, wsHttp, wsFragment].map((record) => record?.serverIds), + [['push-srv'], ['push-srv'], ['stdio-srv'], ['gone-srv'], ['spawned-srv'], ['ws-command-srv'], ['stdio-url-srv'], ['ws-http-srv'], ['ws-fragment-srv']], ); // The framework's own fetcher, resolving each server from its MCPL config: // push-srv has an address; stdio-srv, as a stdio server, has none, nor @@ -140,8 +141,9 @@ test("fetch_reference reaches a push event's reference through its server's own // configured at all. An explicit transport decides as the host connects: // ws-command-srv is a WebSocket server though its config holds a command, // and stdio-url-srv is spawned though its config holds only a url. - // ws-http-srv is a WebSocket server whose url the transport won't dial, - // since it isn't ws:// or wss://. + // ws-http-srv and ws-fragment-srv are WebSocket servers whose url the + // transport won't dial: the first isn't ws:// or wss://, and the second, + // on the uri's origin, has a fragment. type Answer = { result: { success: boolean; data?: string; error?: string } }; const framework = Object.create(AgentFramework.prototype) as { pushEvent(event: unknown): void; @@ -155,6 +157,7 @@ test("fetch_reference reaches a push event's reference through its server's own ['ws-command-srv', { id: 'ws-command-srv', transport: 'websocket', command: 'stdio-server', url: wsOrigin }], ['stdio-url-srv', { id: 'stdio-url-srv', transport: 'stdio', url: wsOrigin }], ['ws-http-srv', { id: 'ws-http-srv', transport: 'websocket', url: origin }], + ['ws-fragment-srv', { id: 'ws-fragment-srv', url: `${wsOrigin}/mcpl#frag` }], ]), getWorkspaceModule: () => ({ writeBinary: async () => ({ success: true }) }), firstWritableMountName: () => 'scratch', @@ -193,7 +196,10 @@ test("fetch_reference reaches a push event's reference through its server's own const undialable = await fetchReference(wsHttp!.refId); assert.equal(undialable.success, false); assert.equal(undialable.error, noAddressReason('ws-http-srv')); - assert.equal(dialed, 2, 'nothing dialed for the last two'); + const fragment = await fetchReference(wsFragment!.refId); + assert.equal(fragment.success, false); + assert.equal(fragment.error, noAddressReason('ws-fragment-srv')); + assert.equal(dialed, 2, 'nothing dialed for the last three'); }); /** The admin puppet's route to an MCPL tool, executeMcplToolCall, with a diff --git a/test/mcpl-ws-transport.test.ts b/test/mcpl-ws-transport.test.ts index 8e99befa7..75f407d9c 100644 --- a/test/mcpl-ws-transport.test.ts +++ b/test/mcpl-ws-transport.test.ts @@ -1,6 +1,6 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { WebSocketServer, type WebSocket } from 'ws'; +import { WebSocketServer, WebSocket } from 'ws'; import type { AddressInfo } from 'node:net'; import { McplServerConnection, type McplConnectionCloseInfo } from '../src/mcpl/server-connection.js'; import { buildWebSocketUrl, isWebSocketTransport } from '../src/mcpl/transport.js'; @@ -84,6 +84,13 @@ test('buildWebSocketUrl validates protocol and appends the token as a query para assert.throws(() => buildWebSocketUrl({ id: 's', command: 'x' }), /requires "url"/); }); +test('buildWebSocketUrl refuses a url with a fragment, which the ws library refuses to dial', () => { + assert.throws(() => buildWebSocketUrl({ id: 's', url: 'wss://example.com/mcpl#x' }), /must not have a fragment, got "#x"/); + assert.throws(() => new WebSocket('wss://example.com/mcpl#x'), /fragment identifier/); + // An empty fragment is no fragment to either. + assert.equal(buildWebSocketUrl({ id: 's', url: 'wss://example.com/mcpl#' }), 'wss://example.com/mcpl#'); +}); + test('isWebSocketTransport selects the transport from config shape', () => { assert.equal(isWebSocketTransport({ id: 's', url: 'wss://x/y' }), true); assert.equal(isWebSocketTransport({ id: 's', url: 'wss://x/y', transport: 'websocket' }), true);